Re: [Qemu-devel] [PATCH v4 6/8] intel_iommu: reject broken EIM

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-10-08 15:21+0800, Peter Xu:
> On Wed, Oct 05, 2016 at 03:06:55PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
>> @@ -2472,10 +2473,22 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>      }
>>  
>>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>> -        s->intr_eim = x86_iommu->intr_supported ?
>> +        s->intr_eim = x86_iommu->intr_supported && 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_setg(errp, "eim=on requires support on the KVM side"
>> +                             "(X2APIC_API, first shipped in v4.7)");
>> +            return false;
>> +        }
>> +        if (!kvm_irqchip_in_kernel()) {
>> +            error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>> +            return false;
>> +        }
> 
> I would prefer:
> 
>   if (kvm_irqchip_in_kernel()) {
>       if (!kvm_enable_x2apic()) {
>           error("enable x2apic failed");
>           return false;
>       }
>   } else {
>       error("need split irqchip");
>       return false;
>   }

Yeah, it looks better, thanks.

> But that's really a matter of taste. So:

I'll currently go for an implicit else: (because 4 levels of indentation
are getting helper-function worthy and it has less curly braces)

        if (!kvm_irqchip_in_kernel()) {
            error("need split irqchip");
            return false;
        }
        if (!kvm_enable_x2apic()) {
            error("enable x2apic failed");
            return false;
        }

> Reviewed-by: Peter Xu <peterx@redhat.com>

I squashed [7/8] into this patch in v5 and the second one didn't have
your r-b, so I made the change as I'd have to drop the r-b anyway.