Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Paolo Bonzini]

On 29/09/2016 18:56, Radim Krčmář wrote:
> 2016-09-29 18:06+0200, Igor Mammedov:
>> On Thu, 29 Sep 2016 15:18:36 +0200
>> Paolo Bonzini <address@hidden> wrote:
>>> On 29/09/2016 13:23, Radim Krčmář wrote:
>>>> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
>>>> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
>>>> forbid other APICs and also the old KVM case with less than 9, to
>>>> simplify the code.
>>>>
>>>> There is no point in enabling EIM in forbidden APICs, so we keep it
>>>> enabled only for the KVM APIC;  unconditionally, because making the
>>>> option depend on KVM version would be a maintanance burden.
>>>>
>>>> Signed-off-by: Radim Krčmář <address@hidden>
>>>> ---
>>>> v2:
>>>>  * adapt to new intr_eim parameter
>>>>  * provide first linux version that has x2apic api
>>>>  * disable QEMU's LAPIC
>>>> ---
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>          s->intr_eim = ON_OFF_AUTO_OFF;
>>>>      }
>>>>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>>>> -        s->intr_eim = ON_OFF_AUTO_ON;
>>>> +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>>>> +                                              : ON_OFF_AUTO_OFF;
>>>> +    }
>>>> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
>>>> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>>>> +            error_report("intel-iommu,eim=on requires support on the KVM 
>>>> side "
>>>> +                         "(X2APIC_API, first shipped in v4.7).");
>>>> +            exit(1);  
>>>
>>> Please use error_setg and return instead (same in patches 4 and 5).
>> Radim's version is consistent with error handling used throughout the file.
>> If we are to use preferred error_setg() here that preceding cleanup
>> patch is in order to convert error_reports to error_setg.
> 
> There is one more error_report() in the file and it doesn't have
> "Error **" -- I'll leave it be and change the rest.
> It amounts to one extra patch that before [4/7] (could be squashed too).

No problem then, keep using error_report I guess.

Paolo


>>>> +        }
>>>> +        if (!kvm_irqchip_in_kernel()) {
>>>> +            error_report("intel-iommu,eim=on requires "
>>>> +                         "accel=kvm,kernel-irqchip=split.");
>> this prints:
>>  -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires 
>> accel=kvm,kernel-irqchip=split
>> so 'intel-iommu,' not really needed, the same would happen with error_setg()
> 
> Yeah, really unreadable, thanks.
>