Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 06, 2016 at 11:51:42AM -0300, Eduardo Habkost wrote:
> On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
> > QEMU 2.7 allowed EIM even in configurations that were forbidden in the
> > last patch because they were not working, like old KVM or userspace
> > APIC.  In order to keep backward compatibility, we again allow guests to
> > misbehave in non-obvious ways, and make it the default for old machine
> > types.
> > 
> > A user can enable the buggy mode it with "buggy_eim=on", which is weird,
> > but I don't know how to add a private property.
> 
> Properties et by compat_props are always user-visible. I believe
> that's a feature (this way, it will be possible to let management
> software and other tools know what exactly is behind a
> machine-type).

There's a rule that any name beginning with x- is deemed
experimental. See docs/qmp-spec.txt.
It is a good idea to always use this for compat properties as
we want to be able to drop them when we drop the old
machine type.


> Additional comment below:
> 
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > v4:
> >  * use a device property [Igor]
> >  * clarify the last sentence of the commit message
> > v3: shorten the code [Peter]
> > ---
> >  hw/i386/intel_iommu.c         | 7 ++++---
> >  include/hw/compat.h           | 4 ++++
> >  include/hw/i386/intel_iommu.h | 1 +
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index efb018b85544..fe257e8357b4 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> >                              ON_OFF_AUTO_AUTO),
> > +    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),
> 
> I suggest "buggy-eim", to follow the usual style for QOM property
> names.
> 
> Assuming the name gets changed:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -2473,11 +2474,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >      }
> >  
> >      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> > -        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
> > +        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
> > +                      && x86_iommu->intr_supported ?
> >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> >      }
> > -
> > -    if (s->intr_eim == ON_OFF_AUTO_ON) {
> > +    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> >          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)");
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 46412b229a70..43b50065e082 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -10,6 +10,10 @@
> >          .driver   = "ioapic",\
> >          .property = "version",\
> >          .value    = "0x11",\
> > +    },{\
> > +        .driver   = "intel-iommu",\
> > +        .property = "buggy_eim",\
> > +        .value    = "true",\
> >      },
> >  
> >  #define HW_COMPAT_2_6 \
> > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > index b5ac60927b1f..1989c1eec10a 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -290,6 +290,7 @@ struct IntelIOMMUState {
> >      uint32_t intr_size;             /* Number of IR table entries */
> >      bool intr_eime;                 /* Extended interrupt mode enabled */
> >      OnOffAuto intr_eim;             /* Toggle for EIM cabability */
> > +    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
> >  };
> >  
> >  /* Find the VTD Address space associated with the given bus pointer,
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Eduardo