| Message ID | 1476830705-19269-2-git-send-email-jan.dakinevich@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-10-19 01:45+0300, Jan Dakinevich: > - Expose all invalidation types to the L1 > > - Reject invvpid instruction, if L1 passed zero vpid value to single > context invalidations > > Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> > --- > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > @@ -132,6 +132,11 @@ > > #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 > > +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ SUPPORTED instead of ALL would be a better name. > + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ > + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ > + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) > + > /* > * These 2 parameters are used to config the controls for Pause-Loop Exiting: > * ple_gap: upper bound on the amount of time between two successive > @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) > */ > if (enable_vpid) > vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | > - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | > - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; > + VMX_VPID_EXTENT_ALL_MASK; I'd still support only type 2, because it is the only one we implement, and type 1, because of buggy KVMs. Are there some OSes that can't use single or all context invalidation, so supporting more might benefit something? > else > vmx->nested.nested_vmx_vpid_caps = 0; > @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) > vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); > type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf); > > - types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7; > + types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8; > > if (!(types & (1UL << type))) { > nested_vmx_failValid(vcpu, > @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) > } > > switch (type) { > + case VMX_VPID_EXTENT_INDIVIDUAL_ADDR: > case VMX_VPID_EXTENT_SINGLE_CONTEXT: > + case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS: > + if (!vpid) { > + nested_vmx_failValid(vcpu, > + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); > + skip_emulated_instruction(vcpu); > + return 1; (Just break and share the code.) > + } > + /* fall through */ > + case VMX_VPID_EXTENT_GLOBAL_CONTEXT: > /* > - * Old versions of KVM use the single-context version so we > - * have to support it; just treat it the same as all-context. > + * Treat any invvpid type as all-context invalidation > */ > - case VMX_VPID_EXTENT_GLOBAL_CONTEXT: > __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02); > nested_vmx_succeed(vcpu); > break; > default: > - /* Trap individual address invalidation invvpid calls */ > BUG_ON(1); Yeah, this was wrong when this type was not actually supported. BUG_ON() is not a good thing to do in any case, please change it into WARN_ON_ONCE(), because we should never get into the default branch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2016-10-19 01:45+0300, Jan Dakinevich: >> - Expose all invalidation types to the L1 >> >> - Reject invvpid instruction, if L1 passed zero vpid value to single >> context invalidations >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> >> --- >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> @@ -132,6 +132,11 @@ >> >> #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 >> >> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ > > SUPPORTED instead of ALL would be a better name. > >> + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ >> + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ >> + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) >> + >> /* >> * These 2 parameters are used to config the controls for Pause-Loop Exiting: >> * ple_gap: upper bound on the amount of time between two successive >> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) >> */ >> if (enable_vpid) >> vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | >> - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | >> - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; >> + VMX_VPID_EXTENT_ALL_MASK; > > I'd still support only type 2, because it is the only one we implement, > and type 1, because of buggy KVMs. > > Are there some OSes that can't use single or all context invalidation, > so supporting more might benefit something? Windows Server 2016 with Hyper-V enabled requires all four invalidation types. The log message is not super clear, just s/allowed/required/ and s/required/available/ " Hypervisor launch failed; Processor does not support the minimum features required to run the hypervisor (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041). " I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this, although it's not the only issue preventing Hyper-V from running on KVM at the moment. I'll be happy to test the next version of this patch. Thanks! Ladi >> else >> vmx->nested.nested_vmx_vpid_caps = 0; >> @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) >> vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); >> type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf); >> >> - types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7; >> + types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8; >> >> if (!(types & (1UL << type))) { >> nested_vmx_failValid(vcpu, >> @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) >> } >> >> switch (type) { >> + case VMX_VPID_EXTENT_INDIVIDUAL_ADDR: >> case VMX_VPID_EXTENT_SINGLE_CONTEXT: >> + case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS: >> + if (!vpid) { >> + nested_vmx_failValid(vcpu, >> + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); >> + skip_emulated_instruction(vcpu); >> + return 1; > > (Just break and share the code.) > >> + } >> + /* fall through */ >> + case VMX_VPID_EXTENT_GLOBAL_CONTEXT: >> /* >> - * Old versions of KVM use the single-context version so we >> - * have to support it; just treat it the same as all-context. >> + * Treat any invvpid type as all-context invalidation >> */ >> - case VMX_VPID_EXTENT_GLOBAL_CONTEXT: >> __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02); >> nested_vmx_succeed(vcpu); >> break; >> default: >> - /* Trap individual address invalidation invvpid calls */ >> BUG_ON(1); > > Yeah, this was wrong when this type was not actually supported. > BUG_ON() is not a good thing to do in any case, please change it into > WARN_ON_ONCE(), because we should never get into the default branch. > > Thanks. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/10/2016 22:02, Radim Krčmář wrote: >> >> + case VMX_VPID_EXTENT_INDIVIDUAL_ADDR: >> case VMX_VPID_EXTENT_SINGLE_CONTEXT: >> + case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS: The comment below can be moved here, saying that Hyper-V and old versions of KVM require these cases to be supported: /* * Hyper-V and old versions of KVM require that we support these * cases, even though we cannot really do anything special about * them and treat them as all-context invalidation. */ Paolo >> + if (!vpid) { >> + nested_vmx_failValid(vcpu, >> + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); >> + skip_emulated_instruction(vcpu); >> + return 1; > > (Just break and share the code.) > >> + } >> + /* fall through */ >> + case VMX_VPID_EXTENT_GLOBAL_CONTEXT: >> /* >> - * Old versions of KVM use the single-context version so we >> - * have to support it; just treat it the same as all-context. >> + * Treat any invvpid type as all-context invalidation >> */ >> - case VMX_VPID_EXTENT_GLOBAL_CONTEXT: >> __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02); >> nested_vmx_succeed(vcpu); >> break; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-10-25 10:08+0200, Ladi Prosek: > On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: >> 2016-10-19 01:45+0300, Jan Dakinevich: >>> - Expose all invalidation types to the L1 >>> >>> - Reject invvpid instruction, if L1 passed zero vpid value to single >>> context invalidations >>> >>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> >>> --- >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> @@ -132,6 +132,11 @@ >>> >>> #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 >>> >>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ >> >> SUPPORTED instead of ALL would be a better name. >> >>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ >>> + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ >>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) >>> + >>> /* >>> * These 2 parameters are used to config the controls for Pause-Loop Exiting: >>> * ple_gap: upper bound on the amount of time between two successive >>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) >>> */ >>> if (enable_vpid) >>> vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | >>> - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | >>> - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; >>> + VMX_VPID_EXTENT_ALL_MASK; >> >> I'd still support only type 2, because it is the only one we implement, >> and type 1, because of buggy KVMs. >> >> Are there some OSes that can't use single or all context invalidation, >> so supporting more might benefit something? > > Windows Server 2016 with Hyper-V enabled requires all four > invalidation types. The log message is not super clear, just > s/allowed/required/ and s/required/available/ Yep, I would have understood it the other way around ... > " > Hypervisor launch failed; > Processor does not support the minimum features required to run the hypervisor > (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041). > " > > I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and > VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this, Great info. Jan, I take back the conservativeness in review -- we'll want to support the currently existing modes. (Invalidating unnecessary entries is sad, but not that bad ... nested is already slow.) > although > it's not the only issue preventing Hyper-V from running on KVM at the > moment. You mentioned another "allowed" bit that KVM lacks, which one was it? Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 25, 2016 at 3:40 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2016-10-25 10:08+0200, Ladi Prosek: >> On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: >>> 2016-10-19 01:45+0300, Jan Dakinevich: >>>> - Expose all invalidation types to the L1 >>>> >>>> - Reject invvpid instruction, if L1 passed zero vpid value to single >>>> context invalidations >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> >>>> --- >>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>> @@ -132,6 +132,11 @@ >>>> >>>> #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 >>>> >>>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ >>> >>> SUPPORTED instead of ALL would be a better name. >>> >>>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ >>>> + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ >>>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) >>>> + >>>> /* >>>> * These 2 parameters are used to config the controls for Pause-Loop Exiting: >>>> * ple_gap: upper bound on the amount of time between two successive >>>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) >>>> */ >>>> if (enable_vpid) >>>> vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | >>>> - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | >>>> - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; >>>> + VMX_VPID_EXTENT_ALL_MASK; >>> >>> I'd still support only type 2, because it is the only one we implement, >>> and type 1, because of buggy KVMs. >>> >>> Are there some OSes that can't use single or all context invalidation, >>> so supporting more might benefit something? >> >> Windows Server 2016 with Hyper-V enabled requires all four >> invalidation types. The log message is not super clear, just >> s/allowed/required/ and s/required/available/ > > Yep, I would have understood it the other way around ... > >> " >> Hypervisor launch failed; >> Processor does not support the minimum features required to run the hypervisor >> (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041). >> " >> >> I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and >> VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this, > > Great info. Jan, I take back the conservativeness in review -- we'll > want to support the currently existing modes. (Invalidating unnecessary > entries is sad, but not that bad ... nested is already slow.) > >> although >> it's not the only issue preventing Hyper-V from running on KVM at the >> moment. > > You mentioned another "allowed" bit that KVM lacks, which one was it? MSR 48B 'Secondary Processor-Based VM-Execution Controls' bit 2 'Descriptor-table exiting' > Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/10/2016 15:57, Ladi Prosek wrote: > On Tue, Oct 25, 2016 at 3:40 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: >> 2016-10-25 10:08+0200, Ladi Prosek: >>> On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote: >>>> 2016-10-19 01:45+0300, Jan Dakinevich: >>>>> - Expose all invalidation types to the L1 >>>>> >>>>> - Reject invvpid instruction, if L1 passed zero vpid value to single >>>>> context invalidations >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> >>>>> --- >>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>>> @@ -132,6 +132,11 @@ >>>>> >>>>> #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 >>>>> >>>>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ >>>> >>>> SUPPORTED instead of ALL would be a better name. >>>> >>>>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ >>>>> + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ >>>>> + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) >>>>> + >>>>> /* >>>>> * These 2 parameters are used to config the controls for Pause-Loop Exiting: >>>>> * ple_gap: upper bound on the amount of time between two successive >>>>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) >>>>> */ >>>>> if (enable_vpid) >>>>> vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | >>>>> - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | >>>>> - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; >>>>> + VMX_VPID_EXTENT_ALL_MASK; >>>> >>>> I'd still support only type 2, because it is the only one we implement, >>>> and type 1, because of buggy KVMs. >>>> >>>> Are there some OSes that can't use single or all context invalidation, >>>> so supporting more might benefit something? >>> >>> Windows Server 2016 with Hyper-V enabled requires all four >>> invalidation types. The log message is not super clear, just >>> s/allowed/required/ and s/required/available/ >> >> Yep, I would have understood it the other way around ... >> >>> " >>> Hypervisor launch failed; >>> Processor does not support the minimum features required to run the hypervisor >>> (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041). >>> " >>> >>> I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and >>> VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this, >> >> Great info. Jan, I take back the conservativeness in review -- we'll >> want to support the currently existing modes. (Invalidating unnecessary >> entries is sad, but not that bad ... nested is already slow.) >> >>> although >>> it's not the only issue preventing Hyper-V from running on KVM at the >>> moment. >> >> You mentioned another "allowed" bit that KVM lacks, which one was it? > > MSR 48B 'Secondary Processor-Based VM-Execution Controls' > bit 2 'Descriptor-table exiting' That's trivial to add it. Let me take a look. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index b727028..e11507d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -132,6 +132,11 @@ #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5 +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT | \ + VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | \ + VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT | \ + VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT) + /* * These 2 parameters are used to config the controls for Pause-Loop Exiting: * ple_gap: upper bound on the amount of time between two successive @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) */ if (enable_vpid) vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT | - VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT | - VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT; + VMX_VPID_EXTENT_ALL_MASK; else vmx->nested.nested_vmx_vpid_caps = 0; @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf); - types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7; + types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8; if (!(types & (1UL << type))) { nested_vmx_failValid(vcpu, @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu) } switch (type) { + case VMX_VPID_EXTENT_INDIVIDUAL_ADDR: case VMX_VPID_EXTENT_SINGLE_CONTEXT: + case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS: + if (!vpid) { + nested_vmx_failValid(vcpu, + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); + skip_emulated_instruction(vcpu); + return 1; + } + /* fall through */ + case VMX_VPID_EXTENT_GLOBAL_CONTEXT: /* - * Old versions of KVM use the single-context version so we - * have to support it; just treat it the same as all-context. + * Treat any invvpid type as all-context invalidation */ - case VMX_VPID_EXTENT_GLOBAL_CONTEXT: __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02); nested_vmx_succeed(vcpu); break; default: - /* Trap individual address invalidation invvpid calls */ BUG_ON(1); break; }
- Expose all invalidation types to the L1 - Reject invvpid instruction, if L1 passed zero vpid value to single context invalidations Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com> --- arch/x86/kvm/vmx.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)