| Message ID | 20180228134401.6544-4-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 28, 2018 at 02:44:01PM +0100, Vitaly Kuznetsov wrote: > Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following > trace: > > kvm_entry: vcpu 0 > kvm_exit: reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0 > kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0 > kvm_msr: msr_write 40000090 = 0x10000 (#GP) > kvm_inj_exception: #GP (0x0) I don't remember having seen this... Does this happen with the mainline QEMU, which doesn't set the SintPollingModeAvailable (17) bit in cpuid 0x40000003:edx? > > KVM acts according to the following statement from TLFS: > > " > 11.8.4 SINTx Registers > ... > Valid values for vector are 16-255 inclusive. Specifying an invalid > vector number results in #GP. > " > > However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000 > to SINTx. I checked with Microsoft and they confirmed that if either the > Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they > ignore the value of Vector. Make KVM act accordingly. I wonder if that cpuid setting affects this behavior? Also curious what exactly the guest is trying to achieve writing this bogus value? > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/uapi/asm/hyperv.h | 1 + > arch/x86/kvm/hyperv.c | 7 ++++++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index 62c778a303a1..a492dc357bd7 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -326,6 +326,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE { > #define HV_SYNIC_SIEFP_ENABLE (1ULL << 0) > #define HV_SYNIC_SINT_MASKED (1ULL << 16) > #define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17) > +#define HV_SYNIC_SINT_POLLING (1ULL << 18) > #define HV_SYNIC_SINT_VECTOR_MASK (0xFF) > > #define HV_SYNIC_STIMER_COUNT (4) > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 6d14f808145d..d3d866c32976 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -95,9 +95,14 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > u64 data, bool host) > { > int vector, old_vector; > + bool masked, polling; > > vector = data & HV_SYNIC_SINT_VECTOR_MASK; > - if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) > + masked = data & HV_SYNIC_SINT_MASKED; > + polling = data & HV_SYNIC_SINT_POLLING; > + > + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && > + !host && !masked && !polling) > return 1; > /* > * Guest may configure multiple SINTs to use the same vector, so I'm not sure this is enough to implement the polling mode: per spec, > Setting the polling bit will have the effect of unmasking an interrupt > source, except that an actual interrupt is not generated. However, if the guest sets a valid vector and the masked bit cleared, we'll consider it a usual SINT and add to masks and inject interrupts, etc, regardless of the polling bit. I must admit I'm confused by the above quote from the spec: is the polling bit supposed to come together with the masked bit? If so, then we probably should validate it here (but your logs indicate otherwise). In general I'm missing the utility of this mode: why should an interrupt controller be involved in polling at all? Roman.
Roman Kagan <rkagan@virtuozzo.com> writes: > On Wed, Feb 28, 2018 at 02:44:01PM +0100, Vitaly Kuznetsov wrote: >> Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following >> trace: >> >> kvm_entry: vcpu 0 >> kvm_exit: reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0 >> kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0 >> kvm_msr: msr_write 40000090 = 0x10000 (#GP) >> kvm_inj_exception: #GP (0x0) > > I don't remember having seen this... Does this happen with the mainline > QEMU, which doesn't set the SintPollingModeAvailable (17) bit in cpuid > 0x40000003:edx? Yes, you need to have Hyper-V role enabled, kvm-intel modules needs to be loaded with 'nesting' support enabled. > >> >> KVM acts according to the following statement from TLFS: >> >> " >> 11.8.4 SINTx Registers >> ... >> Valid values for vector are 16-255 inclusive. Specifying an invalid >> vector number results in #GP. >> " >> >> However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000 >> to SINTx. I checked with Microsoft and they confirmed that if either the >> Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they >> ignore the value of Vector. Make KVM act accordingly. > > I wonder if that cpuid setting affects this behavior? Also curious what > exactly the guest is trying to achieve writing this bogus value? The value is actually the default value which is supposed to be there: "At virtual processor creation time, the default value of all SINTx (synthetic interrupt source) registers is 0x0000000000010000." so I guess this is just an intialization procedure. > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/include/uapi/asm/hyperv.h | 1 + >> arch/x86/kvm/hyperv.c | 7 ++++++- >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h >> index 62c778a303a1..a492dc357bd7 100644 >> --- a/arch/x86/include/uapi/asm/hyperv.h >> +++ b/arch/x86/include/uapi/asm/hyperv.h >> @@ -326,6 +326,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE { >> #define HV_SYNIC_SIEFP_ENABLE (1ULL << 0) >> #define HV_SYNIC_SINT_MASKED (1ULL << 16) >> #define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17) >> +#define HV_SYNIC_SINT_POLLING (1ULL << 18) >> #define HV_SYNIC_SINT_VECTOR_MASK (0xFF) >> >> #define HV_SYNIC_STIMER_COUNT (4) >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index 6d14f808145d..d3d866c32976 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -95,9 +95,14 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, >> u64 data, bool host) >> { >> int vector, old_vector; >> + bool masked, polling; >> >> vector = data & HV_SYNIC_SINT_VECTOR_MASK; >> - if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) >> + masked = data & HV_SYNIC_SINT_MASKED; >> + polling = data & HV_SYNIC_SINT_POLLING; >> + >> + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && >> + !host && !masked && !polling) >> return 1; >> /* >> * Guest may configure multiple SINTs to use the same vector, so > > I'm not sure this is enough to implement the polling mode: per spec, > Oh, no, I wasn't trying to -- and by the way we don't currently announce SintPollingModeAvailable so guests are not supposed to do that. This is rather a future proof to 'not forget'. >> Setting the polling bit will have the effect of unmasking an interrupt >> source, except that an actual interrupt is not generated. > > However, if the guest sets a valid vector and the masked bit cleared, > we'll consider it a usual SINT and add to masks and inject interrupts, > etc, regardless of the polling bit. > > I must admit I'm confused by the above quote from the spec: is the > polling bit supposed to come together with the masked bit? If so, then > we probably should validate it here (but your logs indicate otherwise). > In general I'm missing the utility of this mode: why should an interrupt > controller be involved in polling at all? "Setting the polling bit will have the effect of unmasking an interrupt source, except that an actual interrupt is not generated." So, as I understand it, setting polling bit makes Vector value irrelevant - the interrupt is not generated so I *assume* we may see writes with zero Vector and polling bit set. But again, we're not implementing polling mode for now, I can just drop it from the patch if you think it is confusing.
On Wed, Feb 28, 2018 at 04:35:59PM +0100, Vitaly Kuznetsov wrote: > Roman Kagan <rkagan@virtuozzo.com> writes: > > > On Wed, Feb 28, 2018 at 02:44:01PM +0100, Vitaly Kuznetsov wrote: > >> Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following > >> trace: > >> > >> kvm_entry: vcpu 0 > >> kvm_exit: reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0 > >> kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0 > >> kvm_msr: msr_write 40000090 = 0x10000 (#GP) > >> kvm_inj_exception: #GP (0x0) > > > > I don't remember having seen this... Does this happen with the mainline > > QEMU, which doesn't set the SintPollingModeAvailable (17) bit in cpuid > > 0x40000003:edx? > > Yes, you need to have Hyper-V role enabled, kvm-intel modules needs to > be loaded with 'nesting' support enabled. Thanks, reproduced now. > >> > >> KVM acts according to the following statement from TLFS: > >> > >> " > >> 11.8.4 SINTx Registers > >> ... > >> Valid values for vector are 16-255 inclusive. Specifying an invalid > >> vector number results in #GP. > >> " > >> > >> However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000 > >> to SINTx. I checked with Microsoft and they confirmed that if either the > >> Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they > >> ignore the value of Vector. Make KVM act accordingly. > > > > I wonder if that cpuid setting affects this behavior? Also curious what > > exactly the guest is trying to achieve writing this bogus value? > > The value is actually the default value which is supposed to be there: > > "At virtual processor creation time, the default value of all SINTx > (synthetic interrupt source) registers is 0x0000000000010000." so I > guess this is just an intialization procedure. Oh, sorry, I trapped on that polling thing throughout the patch so I missed that the value you observe has actually only the masked bit set, which is indeed the initial value (no idea why the guest *writes* it though, it's the hypervisor's responsibility to put it there on vCPU reset). > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> --- > >> arch/x86/include/uapi/asm/hyperv.h | 1 + > >> arch/x86/kvm/hyperv.c | 7 ++++++- > >> 2 files changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > >> index 62c778a303a1..a492dc357bd7 100644 > >> --- a/arch/x86/include/uapi/asm/hyperv.h > >> +++ b/arch/x86/include/uapi/asm/hyperv.h > >> @@ -326,6 +326,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE { > >> #define HV_SYNIC_SIEFP_ENABLE (1ULL << 0) > >> #define HV_SYNIC_SINT_MASKED (1ULL << 16) > >> #define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17) > >> +#define HV_SYNIC_SINT_POLLING (1ULL << 18) > >> #define HV_SYNIC_SINT_VECTOR_MASK (0xFF) > >> > >> #define HV_SYNIC_STIMER_COUNT (4) > >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > >> index 6d14f808145d..d3d866c32976 100644 > >> --- a/arch/x86/kvm/hyperv.c > >> +++ b/arch/x86/kvm/hyperv.c > >> @@ -95,9 +95,14 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > >> u64 data, bool host) > >> { > >> int vector, old_vector; > >> + bool masked, polling; > >> > >> vector = data & HV_SYNIC_SINT_VECTOR_MASK; > >> - if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) > >> + masked = data & HV_SYNIC_SINT_MASKED; > >> + polling = data & HV_SYNIC_SINT_POLLING; > >> + > >> + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && > >> + !host && !masked && !polling) > >> return 1; > >> /* > >> * Guest may configure multiple SINTs to use the same vector, so > > > > I'm not sure this is enough to implement the polling mode: per spec, > > > > Oh, no, I wasn't trying to -- and by the way we don't currently announce > SintPollingModeAvailable so guests are not supposed to do that. This is > rather a future proof to 'not forget'. > > >> Setting the polling bit will have the effect of unmasking an interrupt > >> source, except that an actual interrupt is not generated. > > > > However, if the guest sets a valid vector and the masked bit cleared, > > we'll consider it a usual SINT and add to masks and inject interrupts, > > etc, regardless of the polling bit. > > > > I must admit I'm confused by the above quote from the spec: is the > > polling bit supposed to come together with the masked bit? If so, then > > we probably should validate it here (but your logs indicate otherwise). > > In general I'm missing the utility of this mode: why should an interrupt > > controller be involved in polling at all? > > "Setting the polling bit will have the effect of unmasking an interrupt > source, except that an actual interrupt is not generated." > > So, as I understand it, setting polling bit makes Vector value > irrelevant - the interrupt is not generated so I *assume* we may see > writes with zero Vector and polling bit set. But again, we're not > implementing polling mode for now, I can just drop it from the patch if > you think it is confusing. I'd suggest indeed to leave polling out for now, as it seems to be a different matter from the one being resolved by this patch (unless, of course, there *are* known cases with invalid vector && !masked && polling). Thanks, Roman.
diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h index 62c778a303a1..a492dc357bd7 100644 --- a/arch/x86/include/uapi/asm/hyperv.h +++ b/arch/x86/include/uapi/asm/hyperv.h @@ -326,6 +326,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE { #define HV_SYNIC_SIEFP_ENABLE (1ULL << 0) #define HV_SYNIC_SINT_MASKED (1ULL << 16) #define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17) +#define HV_SYNIC_SINT_POLLING (1ULL << 18) #define HV_SYNIC_SINT_VECTOR_MASK (0xFF) #define HV_SYNIC_STIMER_COUNT (4) diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 6d14f808145d..d3d866c32976 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -95,9 +95,14 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, u64 data, bool host) { int vector, old_vector; + bool masked, polling; vector = data & HV_SYNIC_SINT_VECTOR_MASK; - if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) + masked = data & HV_SYNIC_SINT_MASKED; + polling = data & HV_SYNIC_SINT_POLLING; + + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && + !host && !masked && !polling) return 1; /* * Guest may configure multiple SINTs to use the same vector, so
Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following trace: kvm_entry: vcpu 0 kvm_exit: reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0 kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0 kvm_msr: msr_write 40000090 = 0x10000 (#GP) kvm_inj_exception: #GP (0x0) KVM acts according to the following statement from TLFS: " 11.8.4 SINTx Registers ... Valid values for vector are 16-255 inclusive. Specifying an invalid vector number results in #GP. " However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000 to SINTx. I checked with Microsoft and they confirmed that if either the Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they ignore the value of Vector. Make KVM act accordingly. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/uapi/asm/hyperv.h | 1 + arch/x86/kvm/hyperv.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)