| Message ID | 20180209130133.28387-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 :-)
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 <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? :-)
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
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 :-)
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.
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); }
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(-)