[PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Vitaly Kuznetsov]

Devices which use level-triggered interrupts under Windows 2016 with
Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
version which has no EOI register so EOI never happens.

The issue was discovered and discussed a while ago:
https://www.spinics.net/lists/kvm/msg148098.html

While this is a guest OS bug (it should check that IOAPIC has the required
capabilities before disabling EOI broadcast) we can workaround it in KVM:
advertising DIRECTED_EOI with in-kernel IOAPIC makes little sense anyway.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- Radim's suggestion was to disable DIRECTED_EOI unconditionally but I'm not
  that radical :-) In theory, we may have multiple IOAPICs in userspace in
  future and DIRECTED_EOI can be leveraged.
---
 arch/x86/kvm/lapic.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 924ac8ce9d50..5339287fee63 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -321,8 +321,16 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 	if (!lapic_in_kernel(vcpu))
 		return;
 
+	/*
+	 * KVM emulates 82093AA datasheet (with in-kernel IOAPIC implementation)
+	 * which doesn't have EOI register; Some buggy OSes (e.g. Windows with
+	 * Hyper-V role) disable EOI broadcast in lapic not checking for IOAPIC
+	 * version first and level-triggered interrupts never get EOIed in
+	 * IOAPIC.
+	 */
 	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
-	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))))
+	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
+	    !ioapic_in_kernel(vcpu->kvm))
 		v |= APIC_LVR_DIRECTED_EOI;
 	kvm_lapic_set_reg(apic, APIC_LVR, v);
 }
-- 
2.14.3

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Nikita Leshenko]

The patch looks correct, however I’m confused about why you consider
this to be a bug in the guest rather than a bug in KVM.

The spec for x2APIC states:
"The support for Directed EOI capability can be detected by means of
bit 24 in the Local APIC Version Register” (Intel’s x2APIC spec, 2.5.1
Directed EOI)
It seems to me that Windows did the right thing by testing for the
presence of directed EOI feature rather than implying it exists by
testing a version number. KVM did the wrong thing by advertising a
feature it doesn’t support.

Therefore I think that you should change the comment to something like
“KVM’s in-kernel IOAPIC doesn’t support Directed EOI register, so don’t
advertise this capability in the LAPIC Version Register.” instead of
talking about buggy guests, as it may confuse future readers of this
code.

Thanks,
Nikita
> On 9 Feb 2018, at 15:01, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> 
> Devices which use level-triggered interrupts under Windows 2016 with
> Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
> unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
> version which has no EOI register so EOI never happens.
> 
> The issue was discovered and discussed a while ago:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_kvm_msg148098.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=JD7W0KpKqI3xo5AglC-aIVDRz_ysy5CrQRnZ9Jb7je0&m=GWIw1X7PvyWESZaIau591RwjCXYZTi6THVNSOEcdaxU&s=5QUI6ED5i6frC8BzcF_e7hp6Kd_OqAxkg0z73R-UIDI&e=
> 
> While this is a guest OS bug (it should check that IOAPIC has the required
> capabilities before disabling EOI broadcast) we can workaround it in KVM:
> advertising DIRECTED_EOI with in-kernel IOAPIC makes little sense anyway.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - Radim's suggestion was to disable DIRECTED_EOI unconditionally but I'm not
>  that radical :-) In theory, we may have multiple IOAPICs in userspace in
>  future and DIRECTED_EOI can be leveraged.
> ---
> arch/x86/kvm/lapic.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 924ac8ce9d50..5339287fee63 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -321,8 +321,16 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
> 	if (!lapic_in_kernel(vcpu))
> 		return;
> 
> +	/*
> +	 * KVM emulates 82093AA datasheet (with in-kernel IOAPIC implementation)
> +	 * which doesn't have EOI register; Some buggy OSes (e.g. Windows with
> +	 * Hyper-V role) disable EOI broadcast in lapic not checking for IOAPIC
> +	 * version first and level-triggered interrupts never get EOIed in
> +	 * IOAPIC.
> +	 */
> 	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
> -	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))))
> +	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
> +	    !ioapic_in_kernel(vcpu->kvm))
> 		v |= APIC_LVR_DIRECTED_EOI;
> 	kvm_lapic_set_reg(apic, APIC_LVR, v);
> }
> -- 
> 2.14.3
> 

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Vitaly Kuznetsov]

Nikita Leshenko <nikita.leshchenko@oracle.com> writes:

> The patch looks correct, however I’m confused about why you consider
> this to be a bug in the guest rather than a bug in KVM.
>
> The spec for x2APIC states:
> "The support for Directed EOI capability can be detected by means of
> bit 24 in the Local APIC Version Register” (Intel’s x2APIC spec, 2.5.1
> Directed EOI)
> It seems to me that Windows did the right thing by testing for the
> presence of directed EOI feature rather than implying it exists by
> testing a version number. KVM did the wrong thing by advertising a
> feature it doesn’t support.
>
> Therefore I think that you should change the comment to something like
> “KVM’s in-kernel IOAPIC doesn’t support Directed EOI register, so don’t
> advertise this capability in the LAPIC Version Register.” instead of
> talking about buggy guests, as it may confuse future readers of this
> code.
>

Before disabling EOI broadcast guests should check if IOAPIC is of
proper version - the fact that you can disable EOI broadcast doesn't
automatically mean that you can later do EOI through IOAPIC... 

Anyway, I got the impression that last time conversation ended up with
'this is a guest bug' conclusion
(https://www.spinics.net/lists/kvm/msg148187.html - see Radim's and
Ladi's comments) but I'm not insisting - if now we conclude this is not a
Windows issue let it be it. But let's just fix it once and for all :-)

-- 
  Vitaly

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Peter Xu]

On Fri, Feb 09, 2018 at 02:01:33PM +0100, Vitaly Kuznetsov wrote:
> Devices which use level-triggered interrupts under Windows 2016 with
> Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
> unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
> version which has no EOI register so EOI never happens.
> 
> The issue was discovered and discussed a while ago:
> https://www.spinics.net/lists/kvm/msg148098.html
> 
> While this is a guest OS bug (it should check that IOAPIC has the required
> capabilities before disabling EOI broadcast) we can workaround it in KVM:
> advertising DIRECTED_EOI with in-kernel IOAPIC makes little sense anyway.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - Radim's suggestion was to disable DIRECTED_EOI unconditionally but I'm not
>   that radical :-) In theory, we may have multiple IOAPICs in userspace in
>   future and DIRECTED_EOI can be leveraged.

I sort of agree on this, especially considering that we already have
IOAPIC version 0x20 support in QEMU already.

> ---
>  arch/x86/kvm/lapic.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 924ac8ce9d50..5339287fee63 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -321,8 +321,16 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
>  	if (!lapic_in_kernel(vcpu))
>  		return;
>  
> +	/*
> +	 * KVM emulates 82093AA datasheet (with in-kernel IOAPIC implementation)
> +	 * which doesn't have EOI register; Some buggy OSes (e.g. Windows with
> +	 * Hyper-V role) disable EOI broadcast in lapic not checking for IOAPIC
> +	 * version first and level-triggered interrupts never get EOIed in
> +	 * IOAPIC.
> +	 */
>  	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
> -	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))))
> +	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
> +	    !ioapic_in_kernel(vcpu->kvm))
>  		v |= APIC_LVR_DIRECTED_EOI;
>  	kvm_lapic_set_reg(apic, APIC_LVR, v);
>  }
> -- 
> 2.14.3
> 

Does this mean that we can avoid the migration problem that Radim
raised in previous discussion?  Basically the OSs should only probe
this version once for each boot, if so I think it should be fine.  But
since you didn't mention that in either commit message and comment, I
would like to ask and confirm.

For the change itself, it looks sane to me.

Thanks,

-- 
Peter Xu

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Vitaly Kuznetsov]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 09, 2018 at 02:01:33PM +0100, Vitaly Kuznetsov wrote:
>> Devices which use level-triggered interrupts under Windows 2016 with
>> Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
>> unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
>> version which has no EOI register so EOI never happens.
>> 
>> The issue was discovered and discussed a while ago:
>> https://www.spinics.net/lists/kvm/msg148098.html
>> 
>> While this is a guest OS bug (it should check that IOAPIC has the required
>> capabilities before disabling EOI broadcast) we can workaround it in KVM:
>> advertising DIRECTED_EOI with in-kernel IOAPIC makes little sense anyway.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> - Radim's suggestion was to disable DIRECTED_EOI unconditionally but I'm not
>>   that radical :-) In theory, we may have multiple IOAPICs in userspace in
>>   future and DIRECTED_EOI can be leveraged.
>
> I sort of agree on this, especially considering that we already have
> IOAPIC version 0x20 support in QEMU already.
>
>> ---
>>  arch/x86/kvm/lapic.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 924ac8ce9d50..5339287fee63 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -321,8 +321,16 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
>>  	if (!lapic_in_kernel(vcpu))
>>  		return;
>>  
>> +	/*
>> +	 * KVM emulates 82093AA datasheet (with in-kernel IOAPIC implementation)
>> +	 * which doesn't have EOI register; Some buggy OSes (e.g. Windows with
>> +	 * Hyper-V role) disable EOI broadcast in lapic not checking for IOAPIC
>> +	 * version first and level-triggered interrupts never get EOIed in
>> +	 * IOAPIC.
>> +	 */
>>  	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
>> -	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))))
>> +	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
>> +	    !ioapic_in_kernel(vcpu->kvm))
>>  		v |= APIC_LVR_DIRECTED_EOI;
>>  	kvm_lapic_set_reg(apic, APIC_LVR, v);
>>  }
>> -- 
>> 2.14.3
>> 
>
> Does this mean that we can avoid the migration problem that Radim
> raised in previous discussion?  Basically the OSs should only probe
> this version once for each boot, if so I think it should be fine.  But
> since you didn't mention that in either commit message and comment, I
> would like to ask and confirm.
>

My thoughts were: with in-kernel IOAPIC nobody does EOI broadcast
disabling today or he's already broken -- there's no way to EOI
level-triggered interrupts. And the patch changes nothing for QEMU
ioapic implementation.

What I'm not sure about is if we allow migration between in-kernel and
QEMU ioapic implementations. I don't think so but in case we do it is
again already broken because going down from 0x20 to 0x11 will leave you
without EOI register. Migrating from 0x11 to 0x20 should be fine, the
guest will continue without knowing there's a new feature available.

But I may have missed something. Radim? :-)

-- 
  Vitaly

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Paolo Bonzini]

On 12/02/2018 12:30, Vitaly Kuznetsov wrote:
>> - Radim's suggestion was to disable DIRECTED_EOI unconditionally but I'm not
>>   that radical :-) In theory, we may have multiple IOAPICs in userspace in
>>   future and DIRECTED_EOI can be leveraged.
> 
> I sort of agree on this, especially considering that we already have
> IOAPIC version 0x20 support in QEMU already.

Yeah, and directed EOI is faster AFAIU.

> My thoughts were: with in-kernel IOAPIC nobody does EOI broadcast
> disabling today or he's already broken -- there's no way to EOI
> level-triggered interrupts. And the patch changes nothing for QEMU
> ioapic implementation.
> 
> What I'm not sure about is if we allow migration between in-kernel and
> QEMU ioapic implementations. I don't think so but in case we do it is
> again already broken because going down from 0x20 to 0x11 will leave you
> without EOI register. Migrating from 0x11 to 0x20 should be fine, the
> guest will continue without knowing there's a new feature available.

No, we don't allow that.

Paolo

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Devices which use level-triggered interrupts under Windows 2016 with
> Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
> unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
> version which has no EOI register so EOI never happens.
>
> The issue was discovered and discussed a while ago:
> https://www.spinics.net/lists/kvm/msg148098.html
>

Radim, Paolo,

do we have a consensus here? (I wouldn't mind re-writing commit message
in case you think we shouldn't blame Windows, I just want to have the
annoying bug fixed :-)

-- 
  Vitaly

Re: [PATCH] KVM: lapic: stop advertising DIRECTED_EOI when in-kernel IOAPIC is in use

[Radim Krčmář]

2018-03-05 11:29+0100, Vitaly Kuznetsov:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Devices which use level-triggered interrupts under Windows 2016 with
> > Hyper-V role enabled don't work: Windows disables EOI broadcast in SPIV
> > unconditionally. Our in-kernel IOAPIC implementation emulates an old IOAPIC
> > version which has no EOI register so EOI never happens.
> >
> > The issue was discovered and discussed a while ago:
> > https://www.spinics.net/lists/kvm/msg148098.html
> >
> 
> Radim, Paolo,
> 
> do we have a consensus here? (I wouldn't mind re-writing commit message
> in case you think we shouldn't blame Windows, I just want to have the
> annoying bug fixed :-)

I have applied the patch, thanks.

I think the commit message is correct (this is a Windows bug):

* Current SDM has a different wording that only talks about
  EOI-broadcast suppression capability of the CPU.

* The x2APIC document mentions that "the bit is reserved to 0 if the
  processor doesn't support Directed EOI" and IOAPIC is not a part of
  the processor, so I would assume that it doesn't hold.

  Basically, I think that the document confuses "Directed EOI" CPU
  feature and "directed EOI" IOAPIC feature, which got amended in SDM.