* [PATCH v3 0/2] KVM: VMX: cleanup VMXON region allocation @ 2020-03-06 13:02 Vitaly Kuznetsov 2020-03-06 13:02 ` [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' Vitaly Kuznetsov 2020-03-06 13:02 ` [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS Vitaly Kuznetsov 0 siblings, 2 replies; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-03-06 13:02 UTC (permalink / raw) To: Paolo Bonzini Cc: Jim Mattson, Sean Christopherson, Wanpeng Li, kvm, linux-kernel Minor cleanup with no functional change (intended): - Rename 'kvm_area' to 'vmxon_region' - Simplify setting revision_id for VMXON region when eVMCS is in use Changes since v1: - Pass 'enum vmcs_type' parameter to alloc_vmcs() too [Sean Christopherson <sean.j.christopherson@intel.com>] Vitaly Kuznetsov (2): KVM: VMX: rename 'kvm_area' to 'vmxon_region' KVM: VMX: untangle VMXON revision_id setting when using eVMCS arch/x86/kvm/vmx/nested.c | 2 +- arch/x86/kvm/vmx/vmx.c | 44 ++++++++++++++++++--------------------- arch/x86/kvm/vmx/vmx.h | 12 ++++++++--- 3 files changed, 30 insertions(+), 28 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' 2020-03-06 13:02 [PATCH v3 0/2] KVM: VMX: cleanup VMXON region allocation Vitaly Kuznetsov @ 2020-03-06 13:02 ` Vitaly Kuznetsov 2020-03-06 22:05 ` Krish Sadhukhan 2020-03-06 13:02 ` [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS Vitaly Kuznetsov 1 sibling, 1 reply; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-03-06 13:02 UTC (permalink / raw) To: Paolo Bonzini Cc: Jim Mattson, Sean Christopherson, Wanpeng Li, kvm, linux-kernel The name 'kvm_area' is misleading (as we have way too many areas which are KVM related), what alloc_kvm_area()/free_kvm_area() functions really do is allocate/free VMXON region for all CPUs. Rename accordingly. No functional change intended. Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/vmx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 40b1e6138cd5..dab19e4e5f2b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2635,7 +2635,7 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) return -ENOMEM; } -static void free_kvm_area(void) +static void free_vmxon_regions(void) { int cpu; @@ -2645,7 +2645,7 @@ static void free_kvm_area(void) } } -static __init int alloc_kvm_area(void) +static __init int alloc_vmxon_regions(void) { int cpu; @@ -2654,7 +2654,7 @@ static __init int alloc_kvm_area(void) vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); if (!vmcs) { - free_kvm_area(); + free_vmxon_regions(); return -ENOMEM; } @@ -7815,7 +7815,7 @@ static __init int hardware_setup(void) return r; } - r = alloc_kvm_area(); + r = alloc_vmxon_regions(); if (r) nested_vmx_hardware_unsetup(); return r; @@ -7826,7 +7826,7 @@ static __exit void hardware_unsetup(void) if (nested) nested_vmx_hardware_unsetup(); - free_kvm_area(); + free_vmxon_regions(); } static bool vmx_check_apicv_inhibit_reasons(ulong bit) -- 2.24.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' 2020-03-06 13:02 ` [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' Vitaly Kuznetsov @ 2020-03-06 22:05 ` Krish Sadhukhan 0 siblings, 0 replies; 11+ messages in thread From: Krish Sadhukhan @ 2020-03-06 22:05 UTC (permalink / raw) To: Vitaly Kuznetsov, Paolo Bonzini Cc: Jim Mattson, Sean Christopherson, Wanpeng Li, kvm, linux-kernel On 3/6/20 5:02 AM, Vitaly Kuznetsov wrote: > The name 'kvm_area' is misleading (as we have way too many areas which are > KVM related), what alloc_kvm_area()/free_kvm_area() functions really do is > allocate/free VMXON region for all CPUs. Rename accordingly. > > No functional change intended. > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/vmx.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 40b1e6138cd5..dab19e4e5f2b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2635,7 +2635,7 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > return -ENOMEM; > } > > -static void free_kvm_area(void) > +static void free_vmxon_regions(void) > { > int cpu; > > @@ -2645,7 +2645,7 @@ static void free_kvm_area(void) > } > } > > -static __init int alloc_kvm_area(void) > +static __init int alloc_vmxon_regions(void) > { > int cpu; > > @@ -2654,7 +2654,7 @@ static __init int alloc_kvm_area(void) > > vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); > if (!vmcs) { > - free_kvm_area(); > + free_vmxon_regions(); > return -ENOMEM; > } > > @@ -7815,7 +7815,7 @@ static __init int hardware_setup(void) > return r; > } > > - r = alloc_kvm_area(); > + r = alloc_vmxon_regions(); > if (r) > nested_vmx_hardware_unsetup(); > return r; > @@ -7826,7 +7826,7 @@ static __exit void hardware_unsetup(void) > if (nested) > nested_vmx_hardware_unsetup(); > > - free_kvm_area(); > + free_vmxon_regions(); > } > > static bool vmx_check_apicv_inhibit_reasons(ulong bit) Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-06 13:02 [PATCH v3 0/2] KVM: VMX: cleanup VMXON region allocation Vitaly Kuznetsov 2020-03-06 13:02 ` [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' Vitaly Kuznetsov @ 2020-03-06 13:02 ` Vitaly Kuznetsov 2020-03-06 22:20 ` Krish Sadhukhan 1 sibling, 1 reply; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-03-06 13:02 UTC (permalink / raw) To: Paolo Bonzini Cc: Jim Mattson, Sean Christopherson, Wanpeng Li, kvm, linux-kernel As stated in alloc_vmxon_regions(), VMXON region needs to be tagged with revision id from MSR_IA32_VMX_BASIC even in case of eVMCS. The logic to do so is not very straightforward: first, we set hdr.revision_id = KVM_EVMCS_VERSION in alloc_vmcs_cpu() just to reset it back to vmcs_config.revision_id in alloc_vmxon_regions(). Simplify this by introducing 'enum vmcs_type' parameter to alloc_vmcs_cpu()/alloc_vmcs(). No functional change intended. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/nested.c | 2 +- arch/x86/kvm/vmx/vmx.c | 34 +++++++++++++++------------------- arch/x86/kvm/vmx/vmx.h | 12 +++++++++--- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index e920d7834d73..8c0ed62b29be 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4566,7 +4566,7 @@ static struct vmcs *alloc_shadow_vmcs(struct kvm_vcpu *vcpu) WARN_ON(loaded_vmcs == &vmx->vmcs01 && loaded_vmcs->shadow_vmcs); if (!loaded_vmcs->shadow_vmcs) { - loaded_vmcs->shadow_vmcs = alloc_vmcs(true); + loaded_vmcs->shadow_vmcs = alloc_vmcs(SHADOW_VMCS_REGION); if (loaded_vmcs->shadow_vmcs) vmcs_clear(loaded_vmcs->shadow_vmcs); } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index dab19e4e5f2b..a45d3721e7d7 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2554,7 +2554,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, return 0; } -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags) +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags) { int node = cpu_to_node(cpu); struct page *pages; @@ -2566,13 +2566,21 @@ struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags) vmcs = page_address(pages); memset(vmcs, 0, vmcs_config.size); - /* KVM supports Enlightened VMCS v1 only */ - if (static_branch_unlikely(&enable_evmcs)) + /* + * When eVMCS is enabled, vmcs->revision_id needs to be set to the + * supported eVMCS version (KVM_EVMCS_VERSION) instead of revision_id + * reported by MSR_IA32_VMX_BASIC. + * + * However, even though not explicitly documented by TLFS, VMXArea + * passed as VMXON argument should still be marked with revision_id + * reported by physical CPU. + */ + if (type != VMXON_REGION && static_branch_unlikely(&enable_evmcs)) vmcs->hdr.revision_id = KVM_EVMCS_VERSION; else vmcs->hdr.revision_id = vmcs_config.revision_id; - if (shadow) + if (type == SHADOW_VMCS_REGION) vmcs->hdr.shadow_vmcs = 1; return vmcs; } @@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) { - loaded_vmcs->vmcs = alloc_vmcs(false); + loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); if (!loaded_vmcs->vmcs) return -ENOMEM; @@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) for_each_possible_cpu(cpu) { struct vmcs *vmcs; - vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); + /* The VMXON region is really just a special type of VMCS. */ + vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); if (!vmcs) { free_vmxon_regions(); return -ENOMEM; } - /* - * When eVMCS is enabled, alloc_vmcs_cpu() sets - * vmcs->revision_id to KVM_EVMCS_VERSION instead of - * revision_id reported by MSR_IA32_VMX_BASIC. - * - * However, even though not explicitly documented by - * TLFS, VMXArea passed as VMXON argument should - * still be marked with revision_id reported by - * physical CPU. - */ - if (static_branch_unlikely(&enable_evmcs)) - vmcs->hdr.revision_id = vmcs_config.revision_id; - per_cpu(vmxarea, cpu) = vmcs; } return 0; diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index e64da06c7009..a5eb92638ac2 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) return &(to_vmx(vcpu)->pi_desc); } -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); +enum vmcs_type { + VMXON_REGION, + VMCS_REGION, + SHADOW_VMCS_REGION, +}; + +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); void free_vmcs(struct vmcs *vmcs); int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); -static inline struct vmcs *alloc_vmcs(bool shadow) +static inline struct vmcs *alloc_vmcs(enum vmcs_type type) { - return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), + return alloc_vmcs_cpu(type, raw_smp_processor_id(), GFP_KERNEL_ACCOUNT); } -- 2.24.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-06 13:02 ` [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS Vitaly Kuznetsov @ 2020-03-06 22:20 ` Krish Sadhukhan 2020-03-06 23:07 ` Sean Christopherson 0 siblings, 1 reply; 11+ messages in thread From: Krish Sadhukhan @ 2020-03-06 22:20 UTC (permalink / raw) To: Vitaly Kuznetsov, Paolo Bonzini Cc: Jim Mattson, Sean Christopherson, Wanpeng Li, kvm, linux-kernel On 3/6/20 5:02 AM, Vitaly Kuznetsov wrote: > As stated in alloc_vmxon_regions(), VMXON region needs to be tagged with > revision id from MSR_IA32_VMX_BASIC even in case of eVMCS. The logic to > do so is not very straightforward: first, we set > hdr.revision_id = KVM_EVMCS_VERSION in alloc_vmcs_cpu() just to reset it > back to vmcs_config.revision_id in alloc_vmxon_regions(). Simplify this by > introducing 'enum vmcs_type' parameter to alloc_vmcs_cpu()/alloc_vmcs(). > > No functional change intended. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/nested.c | 2 +- > arch/x86/kvm/vmx/vmx.c | 34 +++++++++++++++------------------- > arch/x86/kvm/vmx/vmx.h | 12 +++++++++--- > 3 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index e920d7834d73..8c0ed62b29be 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4566,7 +4566,7 @@ static struct vmcs *alloc_shadow_vmcs(struct kvm_vcpu *vcpu) > WARN_ON(loaded_vmcs == &vmx->vmcs01 && loaded_vmcs->shadow_vmcs); > > if (!loaded_vmcs->shadow_vmcs) { > - loaded_vmcs->shadow_vmcs = alloc_vmcs(true); > + loaded_vmcs->shadow_vmcs = alloc_vmcs(SHADOW_VMCS_REGION); > if (loaded_vmcs->shadow_vmcs) > vmcs_clear(loaded_vmcs->shadow_vmcs); > } > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index dab19e4e5f2b..a45d3721e7d7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2554,7 +2554,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > return 0; > } > > -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags) > +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags) > { > int node = cpu_to_node(cpu); > struct page *pages; > @@ -2566,13 +2566,21 @@ struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags) > vmcs = page_address(pages); > memset(vmcs, 0, vmcs_config.size); > > - /* KVM supports Enlightened VMCS v1 only */ > - if (static_branch_unlikely(&enable_evmcs)) > + /* > + * When eVMCS is enabled, vmcs->revision_id needs to be set to the > + * supported eVMCS version (KVM_EVMCS_VERSION) instead of revision_id > + * reported by MSR_IA32_VMX_BASIC. > + * > + * However, even though not explicitly documented by TLFS, VMXArea > + * passed as VMXON argument should still be marked with revision_id > + * reported by physical CPU. > + */ > + if (type != VMXON_REGION && static_branch_unlikely(&enable_evmcs)) > vmcs->hdr.revision_id = KVM_EVMCS_VERSION; > else > vmcs->hdr.revision_id = vmcs_config.revision_id; > > - if (shadow) > + if (type == SHADOW_VMCS_REGION) > vmcs->hdr.shadow_vmcs = 1; > return vmcs; > } > @@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > > int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > { > - loaded_vmcs->vmcs = alloc_vmcs(false); > + loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); > if (!loaded_vmcs->vmcs) > return -ENOMEM; > > @@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) > for_each_possible_cpu(cpu) { > struct vmcs *vmcs; > > - vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); > + /* The VMXON region is really just a special type of VMCS. */ Not sure if this is the right way to correlate the two. AFAIU, the SDM calls VMXON region as a memory area that holds the VMCS data structure and it calls VMCS the data structure that is used by software to switch between VMX root-mode and not-root-mode. So VMXON is a memory area whereas VMCS is the structure of the data that resides in that memory area. So if we follow this interpretation, your enum should rather look like, enum vmcs_type { + VMCS, + EVMCS, + SHADOW_VMCS > + vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); > if (!vmcs) { > free_vmxon_regions(); > return -ENOMEM; > } > > - /* > - * When eVMCS is enabled, alloc_vmcs_cpu() sets > - * vmcs->revision_id to KVM_EVMCS_VERSION instead of > - * revision_id reported by MSR_IA32_VMX_BASIC. > - * > - * However, even though not explicitly documented by > - * TLFS, VMXArea passed as VMXON argument should > - * still be marked with revision_id reported by > - * physical CPU. > - */ > - if (static_branch_unlikely(&enable_evmcs)) > - vmcs->hdr.revision_id = vmcs_config.revision_id; > - > per_cpu(vmxarea, cpu) = vmcs; > } > return 0; > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index e64da06c7009..a5eb92638ac2 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) > return &(to_vmx(vcpu)->pi_desc); > } > > -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); > +enum vmcs_type { > + VMXON_REGION, > + VMCS_REGION, > + SHADOW_VMCS_REGION, > +}; > + > +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); > void free_vmcs(struct vmcs *vmcs); > int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); > void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); > > -static inline struct vmcs *alloc_vmcs(bool shadow) > +static inline struct vmcs *alloc_vmcs(enum vmcs_type type) > { > - return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), > + return alloc_vmcs_cpu(type, raw_smp_processor_id(), > GFP_KERNEL_ACCOUNT); > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-06 22:20 ` Krish Sadhukhan @ 2020-03-06 23:07 ` Sean Christopherson 2020-03-06 23:57 ` Krish Sadhukhan 0 siblings, 1 reply; 11+ messages in thread From: Sean Christopherson @ 2020-03-06 23:07 UTC (permalink / raw) To: Krish Sadhukhan Cc: Vitaly Kuznetsov, Paolo Bonzini, Jim Mattson, Wanpeng Li, kvm, linux-kernel On Fri, Mar 06, 2020 at 02:20:13PM -0800, Krish Sadhukhan wrote: > >@@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > > int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > > { > >- loaded_vmcs->vmcs = alloc_vmcs(false); > >+ loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); > > if (!loaded_vmcs->vmcs) > > return -ENOMEM; > >@@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) > > for_each_possible_cpu(cpu) { > > struct vmcs *vmcs; > >- vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); > >+ /* The VMXON region is really just a special type of VMCS. */ > > > Not sure if this is the right way to correlate the two. > > AFAIU, the SDM calls VMXON region as a memory area that holds the VMCS data > structure and it calls VMCS the data structure that is used by software to > switch between VMX root-mode and not-root-mode. So VMXON is a memory area > whereas VMCS is the structure of the data that resides in that memory area. > > So if we follow this interpretation, your enum should rather look like, > > enum vmcs_type { > + VMCS, > + EVMCS, > + SHADOW_VMCS No (to the EVMCS suggestion), because this allocation needs to happen for !eVMCS. The SDM never explictly calls the VMXON region a VMCS, but it's just being coy. E.g. VMCLEAR doesn't fail if you point it at random memory, but point it at the VMXON region and it yells. We could call it VMXON_VMCS if that helps. The SDM does call the memory allocation for regular VMCSes a "VMCS region": A logical processor associates a region in memory with each VMCS. This region is called the VMCS region. I don't think I've ever heard anyone differentiate that two though, i.e. VMCS is used colloquially to mean both the data structure itself and the memory region containing the data structure. > >+ vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); > > if (!vmcs) { > > free_vmxon_regions(); > > return -ENOMEM; > > } > >- /* > >- * When eVMCS is enabled, alloc_vmcs_cpu() sets > >- * vmcs->revision_id to KVM_EVMCS_VERSION instead of > >- * revision_id reported by MSR_IA32_VMX_BASIC. > >- * > >- * However, even though not explicitly documented by > >- * TLFS, VMXArea passed as VMXON argument should > >- * still be marked with revision_id reported by > >- * physical CPU. > >- */ > >- if (static_branch_unlikely(&enable_evmcs)) > >- vmcs->hdr.revision_id = vmcs_config.revision_id; > >- > > per_cpu(vmxarea, cpu) = vmcs; > > } > > return 0; > >diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > >index e64da06c7009..a5eb92638ac2 100644 > >--- a/arch/x86/kvm/vmx/vmx.h > >+++ b/arch/x86/kvm/vmx/vmx.h > >@@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) > > return &(to_vmx(vcpu)->pi_desc); > > } > >-struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); > >+enum vmcs_type { > >+ VMXON_REGION, > >+ VMCS_REGION, > >+ SHADOW_VMCS_REGION, > >+}; > >+ > >+struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); > > void free_vmcs(struct vmcs *vmcs); > > int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > > void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > > void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); > > void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); > >-static inline struct vmcs *alloc_vmcs(bool shadow) > >+static inline struct vmcs *alloc_vmcs(enum vmcs_type type) > > { > >- return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), > >+ return alloc_vmcs_cpu(type, raw_smp_processor_id(), > > GFP_KERNEL_ACCOUNT); > > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-06 23:07 ` Sean Christopherson @ 2020-03-06 23:57 ` Krish Sadhukhan 2020-03-07 0:28 ` Sean Christopherson 0 siblings, 1 reply; 11+ messages in thread From: Krish Sadhukhan @ 2020-03-06 23:57 UTC (permalink / raw) To: Sean Christopherson Cc: Vitaly Kuznetsov, Paolo Bonzini, Jim Mattson, Wanpeng Li, kvm, linux-kernel On 3/6/20 3:07 PM, Sean Christopherson wrote: > On Fri, Mar 06, 2020 at 02:20:13PM -0800, Krish Sadhukhan wrote: >>> @@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) >>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) >>> { >>> - loaded_vmcs->vmcs = alloc_vmcs(false); >>> + loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); >>> if (!loaded_vmcs->vmcs) >>> return -ENOMEM; >>> @@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) >>> for_each_possible_cpu(cpu) { >>> struct vmcs *vmcs; >>> - vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); >>> + /* The VMXON region is really just a special type of VMCS. */ >> >> Not sure if this is the right way to correlate the two. >> >> AFAIU, the SDM calls VMXON region as a memory area that holds the VMCS data >> structure and it calls VMCS the data structure that is used by software to >> switch between VMX root-mode and not-root-mode. So VMXON is a memory area >> whereas VMCS is the structure of the data that resides in that memory area. >> >> So if we follow this interpretation, your enum should rather look like, >> >> enum vmcs_type { >> + VMCS, >> + EVMCS, >> + SHADOW_VMCS > No (to the EVMCS suggestion), because this allocation needs to happen for > !eVMCS. The SDM never explictly calls the VMXON region a VMCS, but it's > just being coy. E.g. VMCLEAR doesn't fail if you point it at random > memory, but point it at the VMXON region and it yells. > > We could call it VMXON_VMCS if that helps. Are you saying, + enum vmcs_type { + VMXON_REGION, + VMXON_VMCS, + SHADOW_VMCS_REGION, +}; ? In that case, "VMXON_REGION" and "VMXON_VMCS" are no different according to your explanation. > The SDM does call the memory > allocation for regular VMCSes a "VMCS region": > > A logical processor associates a region in memory with each VMCS. This > region is called the VMCS region. > > I don't think I've ever heard anyone differentiate that two though, i.e. > VMCS is used colloquially to mean both the data structure itself and the > memory region containing the data structure. > >>> + vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); >>> if (!vmcs) { >>> free_vmxon_regions(); >>> return -ENOMEM; >>> } >>> - /* >>> - * When eVMCS is enabled, alloc_vmcs_cpu() sets >>> - * vmcs->revision_id to KVM_EVMCS_VERSION instead of >>> - * revision_id reported by MSR_IA32_VMX_BASIC. >>> - * >>> - * However, even though not explicitly documented by >>> - * TLFS, VMXArea passed as VMXON argument should >>> - * still be marked with revision_id reported by >>> - * physical CPU. >>> - */ >>> - if (static_branch_unlikely(&enable_evmcs)) >>> - vmcs->hdr.revision_id = vmcs_config.revision_id; >>> - >>> per_cpu(vmxarea, cpu) = vmcs; >>> } >>> return 0; >>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h >>> index e64da06c7009..a5eb92638ac2 100644 >>> --- a/arch/x86/kvm/vmx/vmx.h >>> +++ b/arch/x86/kvm/vmx/vmx.h >>> @@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) >>> return &(to_vmx(vcpu)->pi_desc); >>> } >>> -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); >>> +enum vmcs_type { >>> + VMXON_REGION, >>> + VMCS_REGION, >>> + SHADOW_VMCS_REGION, >>> +}; >>> + >>> +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); >>> void free_vmcs(struct vmcs *vmcs); >>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); >>> void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); >>> void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); >>> void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); >>> -static inline struct vmcs *alloc_vmcs(bool shadow) >>> +static inline struct vmcs *alloc_vmcs(enum vmcs_type type) >>> { >>> - return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), >>> + return alloc_vmcs_cpu(type, raw_smp_processor_id(), >>> GFP_KERNEL_ACCOUNT); >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-06 23:57 ` Krish Sadhukhan @ 2020-03-07 0:28 ` Sean Christopherson 2020-03-07 1:34 ` Krish Sadhukhan 2020-03-09 9:31 ` Vitaly Kuznetsov 0 siblings, 2 replies; 11+ messages in thread From: Sean Christopherson @ 2020-03-07 0:28 UTC (permalink / raw) To: Krish Sadhukhan Cc: Vitaly Kuznetsov, Paolo Bonzini, Jim Mattson, Wanpeng Li, kvm, linux-kernel On Fri, Mar 06, 2020 at 03:57:25PM -0800, Krish Sadhukhan wrote: > > On 3/6/20 3:07 PM, Sean Christopherson wrote: > >On Fri, Mar 06, 2020 at 02:20:13PM -0800, Krish Sadhukhan wrote: > >>>@@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > >>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > >>> { > >>>- loaded_vmcs->vmcs = alloc_vmcs(false); > >>>+ loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); > >>> if (!loaded_vmcs->vmcs) > >>> return -ENOMEM; > >>>@@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) > >>> for_each_possible_cpu(cpu) { > >>> struct vmcs *vmcs; > >>>- vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); > >>>+ /* The VMXON region is really just a special type of VMCS. */ > >> > >>Not sure if this is the right way to correlate the two. > >> > >>AFAIU, the SDM calls VMXON region as a memory area that holds the VMCS data > >>structure and it calls VMCS the data structure that is used by software to > >>switch between VMX root-mode and not-root-mode. So VMXON is a memory area > >>whereas VMCS is the structure of the data that resides in that memory area. > >> > >>So if we follow this interpretation, your enum should rather look like, > >> > >>enum vmcs_type { > >>+ VMCS, > >>+ EVMCS, > >>+ SHADOW_VMCS > >No (to the EVMCS suggestion), because this allocation needs to happen for > >!eVMCS. The SDM never explictly calls the VMXON region a VMCS, but it's > >just being coy. E.g. VMCLEAR doesn't fail if you point it at random > >memory, but point it at the VMXON region and it yells. > > > >We could call it VMXON_VMCS if that helps. > > Are you saying, > > + enum vmcs_type { > + VMXON_REGION, > + VMXON_VMCS, > + SHADOW_VMCS_REGION, > +}; > > ? > > In that case, "VMXON_REGION" and "VMXON_VMCS" are no different according to > your explanation. enum vmcs_type { VMXON_VMCS, VMCS, SHADOW_VMCS, }; alloc_vmcs_cpu() does more than just allocate the memory, it also initializes the data structure, e.g. "allocate and initalize a VMXON VMCS", > > The SDM does call the memory > >allocation for regular VMCSes a "VMCS region": > > > > A logical processor associates a region in memory with each VMCS. This > > region is called the VMCS region. > > > >I don't think I've ever heard anyone differentiate that two though, i.e. > >VMCS is used colloquially to mean both the data structure itself and the > >memory region containing the data structure. > > > >>>+ vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); > >>> if (!vmcs) { > >>> free_vmxon_regions(); > >>> return -ENOMEM; > >>> } > >>>- /* > >>>- * When eVMCS is enabled, alloc_vmcs_cpu() sets > >>>- * vmcs->revision_id to KVM_EVMCS_VERSION instead of > >>>- * revision_id reported by MSR_IA32_VMX_BASIC. > >>>- * > >>>- * However, even though not explicitly documented by > >>>- * TLFS, VMXArea passed as VMXON argument should > >>>- * still be marked with revision_id reported by > >>>- * physical CPU. > >>>- */ > >>>- if (static_branch_unlikely(&enable_evmcs)) > >>>- vmcs->hdr.revision_id = vmcs_config.revision_id; > >>>- > >>> per_cpu(vmxarea, cpu) = vmcs; > >>> } > >>> return 0; > >>>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > >>>index e64da06c7009..a5eb92638ac2 100644 > >>>--- a/arch/x86/kvm/vmx/vmx.h > >>>+++ b/arch/x86/kvm/vmx/vmx.h > >>>@@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) > >>> return &(to_vmx(vcpu)->pi_desc); > >>> } > >>>-struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); > >>>+enum vmcs_type { > >>>+ VMXON_REGION, > >>>+ VMCS_REGION, > >>>+ SHADOW_VMCS_REGION, > >>>+}; > >>>+ > >>>+struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); > >>> void free_vmcs(struct vmcs *vmcs); > >>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > >>> void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); > >>> void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); > >>> void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); > >>>-static inline struct vmcs *alloc_vmcs(bool shadow) > >>>+static inline struct vmcs *alloc_vmcs(enum vmcs_type type) > >>> { > >>>- return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), > >>>+ return alloc_vmcs_cpu(type, raw_smp_processor_id(), > >>> GFP_KERNEL_ACCOUNT); > >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-07 0:28 ` Sean Christopherson @ 2020-03-07 1:34 ` Krish Sadhukhan 2020-03-09 9:31 ` Vitaly Kuznetsov 1 sibling, 0 replies; 11+ messages in thread From: Krish Sadhukhan @ 2020-03-07 1:34 UTC (permalink / raw) To: Sean Christopherson Cc: Vitaly Kuznetsov, Paolo Bonzini, Jim Mattson, Wanpeng Li, kvm, linux-kernel On 3/6/20 4:28 PM, Sean Christopherson wrote: > On Fri, Mar 06, 2020 at 03:57:25PM -0800, Krish Sadhukhan wrote: >> On 3/6/20 3:07 PM, Sean Christopherson wrote: >>> On Fri, Mar 06, 2020 at 02:20:13PM -0800, Krish Sadhukhan wrote: >>>>> @@ -2599,7 +2607,7 @@ void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) >>>>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) >>>>> { >>>>> - loaded_vmcs->vmcs = alloc_vmcs(false); >>>>> + loaded_vmcs->vmcs = alloc_vmcs(VMCS_REGION); >>>>> if (!loaded_vmcs->vmcs) >>>>> return -ENOMEM; >>>>> @@ -2652,25 +2660,13 @@ static __init int alloc_vmxon_regions(void) >>>>> for_each_possible_cpu(cpu) { >>>>> struct vmcs *vmcs; >>>>> - vmcs = alloc_vmcs_cpu(false, cpu, GFP_KERNEL); >>>>> + /* The VMXON region is really just a special type of VMCS. */ >>>> Not sure if this is the right way to correlate the two. >>>> >>>> AFAIU, the SDM calls VMXON region as a memory area that holds the VMCS data >>>> structure and it calls VMCS the data structure that is used by software to >>>> switch between VMX root-mode and not-root-mode. So VMXON is a memory area >>>> whereas VMCS is the structure of the data that resides in that memory area. >>>> >>>> So if we follow this interpretation, your enum should rather look like, >>>> >>>> enum vmcs_type { >>>> + VMCS, >>>> + EVMCS, >>>> + SHADOW_VMCS >>> No (to the EVMCS suggestion), because this allocation needs to happen for >>> !eVMCS. The SDM never explictly calls the VMXON region a VMCS, but it's >>> just being coy. E.g. VMCLEAR doesn't fail if you point it at random >>> memory, but point it at the VMXON region and it yells. >>> >>> We could call it VMXON_VMCS if that helps. >> Are you saying, >> >> + enum vmcs_type { >> + VMXON_REGION, >> + VMXON_VMCS, >> + SHADOW_VMCS_REGION, >> +}; >> >> ? >> >> In that case, "VMXON_REGION" and "VMXON_VMCS" are no different according to >> your explanation. > enum vmcs_type { > VMXON_VMCS, > VMCS, > SHADOW_VMCS, > }; It looks reasonable. > > alloc_vmcs_cpu() does more than just allocate the memory, it also > initializes the data structure, e.g. "allocate and initalize a VMXON VMCS", > >>> The SDM does call the memory >>> allocation for regular VMCSes a "VMCS region": >>> >>> A logical processor associates a region in memory with each VMCS. This >>> region is called the VMCS region. >>> >>> I don't think I've ever heard anyone differentiate that two though, i.e. >>> VMCS is used colloquially to mean both the data structure itself and the >>> memory region containing the data structure. >>> >>>>> + vmcs = alloc_vmcs_cpu(VMXON_REGION, cpu, GFP_KERNEL); >>>>> if (!vmcs) { >>>>> free_vmxon_regions(); >>>>> return -ENOMEM; >>>>> } >>>>> - /* >>>>> - * When eVMCS is enabled, alloc_vmcs_cpu() sets >>>>> - * vmcs->revision_id to KVM_EVMCS_VERSION instead of >>>>> - * revision_id reported by MSR_IA32_VMX_BASIC. >>>>> - * >>>>> - * However, even though not explicitly documented by >>>>> - * TLFS, VMXArea passed as VMXON argument should >>>>> - * still be marked with revision_id reported by >>>>> - * physical CPU. >>>>> - */ >>>>> - if (static_branch_unlikely(&enable_evmcs)) >>>>> - vmcs->hdr.revision_id = vmcs_config.revision_id; >>>>> - >>>>> per_cpu(vmxarea, cpu) = vmcs; >>>>> } >>>>> return 0; >>>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h >>>>> index e64da06c7009..a5eb92638ac2 100644 >>>>> --- a/arch/x86/kvm/vmx/vmx.h >>>>> +++ b/arch/x86/kvm/vmx/vmx.h >>>>> @@ -489,16 +489,22 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu) >>>>> return &(to_vmx(vcpu)->pi_desc); >>>>> } >>>>> -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags); >>>>> +enum vmcs_type { >>>>> + VMXON_REGION, >>>>> + VMCS_REGION, >>>>> + SHADOW_VMCS_REGION, >>>>> +}; >>>>> + >>>>> +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags); >>>>> void free_vmcs(struct vmcs *vmcs); >>>>> int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); >>>>> void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs); >>>>> void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs); >>>>> void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs); >>>>> -static inline struct vmcs *alloc_vmcs(bool shadow) >>>>> +static inline struct vmcs *alloc_vmcs(enum vmcs_type type) >>>>> { >>>>> - return alloc_vmcs_cpu(shadow, raw_smp_processor_id(), >>>>> + return alloc_vmcs_cpu(type, raw_smp_processor_id(), >>>>> GFP_KERNEL_ACCOUNT); >>>>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-07 0:28 ` Sean Christopherson 2020-03-07 1:34 ` Krish Sadhukhan @ 2020-03-09 9:31 ` Vitaly Kuznetsov 2020-03-18 17:17 ` Vitaly Kuznetsov 1 sibling, 1 reply; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-03-09 9:31 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: Krish Sadhukhan, Jim Mattson, Wanpeng Li, kvm, linux-kernel Sean Christopherson <sean.j.christopherson@intel.com> writes: > enum vmcs_type { > VMXON_VMCS, > VMCS, > SHADOW_VMCS, > }; > No objections from my side. v4 or would it be possible to tweak it upon commit? -- Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS 2020-03-09 9:31 ` Vitaly Kuznetsov @ 2020-03-18 17:17 ` Vitaly Kuznetsov 0 siblings, 0 replies; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-03-18 17:17 UTC (permalink / raw) To: Paolo Bonzini Cc: Krish Sadhukhan, Jim Mattson, Wanpeng Li, kvm, linux-kernel, Sean Christopherson Vitaly Kuznetsov <vkuznets@redhat.com> writes: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > >> enum vmcs_type { >> VMXON_VMCS, >> VMCS, >> SHADOW_VMCS, >> }; >> > > No objections from my side. v4 or would it be possible to tweak it upon > commit? It seems this slipped through the cracks, rebased v4 is comming to rescue! -- Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-03-18 17:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-06 13:02 [PATCH v3 0/2] KVM: VMX: cleanup VMXON region allocation Vitaly Kuznetsov 2020-03-06 13:02 ` [PATCH v3 1/2] KVM: VMX: rename 'kvm_area' to 'vmxon_region' Vitaly Kuznetsov 2020-03-06 22:05 ` Krish Sadhukhan 2020-03-06 13:02 ` [PATCH v3 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS Vitaly Kuznetsov 2020-03-06 22:20 ` Krish Sadhukhan 2020-03-06 23:07 ` Sean Christopherson 2020-03-06 23:57 ` Krish Sadhukhan 2020-03-07 0:28 ` Sean Christopherson 2020-03-07 1:34 ` Krish Sadhukhan 2020-03-09 9:31 ` Vitaly Kuznetsov 2020-03-18 17:17 ` Vitaly Kuznetsov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.