[v4,6/8] intel_iommu: reject broken EIM

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.

Old QEMUs would enable eim whenever intremap was on, which would trick
guests into thinking that they can enable cluster x2APIC even if any
interrupt destination would get clamped to 8 bits.
Depending on your configuration, QEMU could notice that the destination
LAPIC is not present and report it with a very non-obvious:

  KVM: injection failed, MSI lost (Operation not permitted)

Or the guest could say something about unexpected interrupts, because
clamping leads to aliasing so interrupts were being delivered to
incorrect VCPUs.

KVM_X2APIC_API is the feature that allows us to enable eim for KVM.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: be more specific in the comment [Igor]
v3:
 * use error_setg [Paolo]
 * shorten the code [Peter]
v2:
 * adapt to new intr_eim parameter
 * provide first linux version that has x2apic api
 * disable QEMU's LAPIC
---
 hw/i386/intel_iommu.c  | 15 ++++++++++++++-
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 13 +++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 33 insertions(+), 1 deletion(-)

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;
  }

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

Reviewed-by: Peter Xu <peterx@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.

On Mon, Oct 10, 2016 at 05:11:19PM +0200, Radim Krčmář wrote:

[...]

> > 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;
>         }

Good to me.

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

Sure. Thanks,

-- peterx