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

[Eduardo Habkost]

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

Additional comment below:

> 
> Signed-off-by: Radim Krčmář <address@hidden>
> ---
> 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 <address@hidden>


>      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