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ář <rkrc...@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 <ehabk...@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
