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

[Radim Krčmář]

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).

>> > +        }
>> > +        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.