| Message ID | 20240309012725.1409949-9-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu: KVM: Clean up PAT and VMX macros | expand |
On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: > Date: Fri, 8 Mar 2024 17:27:24 -0800 > From: Sean Christopherson <seanjc@google.com> > Subject: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask > in its accessor > X-Mailer: git-send-email 2.44.0.278.ge034bb2e1d-goog > > From: Xin Li <xin3.li@intel.com> > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > that the function looks like all the helpers that grab values from > VMX_BASIC and VMX_MISC MSR values. > > No functional change intended. > > Cc: Shan Kang <shan.kang@intel.com> > Cc: Kai Huang <kai.huang@intel.com> > Signed-off-by: Xin Li <xin3.li@intel.com> > [sean: split to separate patch, write changelog] > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/vmx.h | 3 +-- > arch/x86/kvm/vmx/vmx.c | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index 6ff179b11235..90ed559076d7 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -148,7 +148,6 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > return (vmx_basic & GENMASK_ULL(53, 50)) >> 50; > } > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) > #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) > #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) > #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) > { > - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; > + return vmx_misc & GENMASK_ULL(4, 0); > } I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than GENMASK_ULL(4, 0), and the former improves code readability. May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? Thanks, Zhao
On Fri, Mar 15, 2024, Zhao Liu wrote: > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > > that the function looks like all the helpers that grab values from > > VMX_BASIC and VMX_MISC MSR values. ... > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) > > #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) > > #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) > > #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) > > { > > - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; > > + return vmx_misc & GENMASK_ULL(4, 0); > > } > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than > GENMASK_ULL(4, 0), and the former improves code readability. > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? I don't necessarily disagree, but in this case I value consistency over one individual case. As called out in the changelog, the motivation is to make vmx_misc_preemption_timer_rate() look like all the surrounding helpers. _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), vmx_misc_max_msr(), etc. I don't have a super strong preference, though I think my vote would be to not add the masks and go with this patch. These helpers are intended to be the _only_ way to access the fields, i.e. they effectively _are_ the mask macros, just in function form.
On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote: > From: Xin Li <xin3.li@intel.com> > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > that the function looks like all the helpers that grab values from Is "all other helpers" better? > VMX_BASIC and VMX_MISC MSR values. > > No functional change intended. > > Cc: Shan Kang <shan.kang@intel.com> > Cc: Kai Huang <kai.huang@intel.com> > Signed-off-by: Xin Li <xin3.li@intel.com> > [sean: split to separate patch, write changelog] > Signed-off-by: Sean Christopherson <seanjc@google.com> > Reviewed-by: Kai Huang <kai.huang@intel.com>
On 3/16/2024 1:54 AM, Sean Christopherson wrote: > On Fri, Mar 15, 2024, Zhao Liu wrote: >> On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: >>> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), >>> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so >>> that the function looks like all the helpers that grab values from >>> VMX_BASIC and VMX_MISC MSR values. > > ... > >>> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) >>> #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) >>> #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) >>> #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) >>> @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) >>> >>> static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) >>> { >>> - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; >>> + return vmx_misc & GENMASK_ULL(4, 0); >>> } >> >> I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than >> GENMASK_ULL(4, 0), and the former improves code readability. >> >> May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? > > I don't necessarily disagree, but in this case I value consistency over one > individual case. As called out in the changelog, the motivation is to make > vmx_misc_preemption_timer_rate() look like all the surrounding helpers. > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), > vmx_misc_max_msr(), etc. > > I don't have a super strong preference, though I think my vote would be to not > add the masks and go with this patch. These helpers are intended to be the _only_ > way to access the fields, i.e. they effectively _are_ the mask macros, just in > function form. > +1. However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I just recommended to define the MASK. Because we already have #define VMX_BASIC_MEM_TYPE_SHIFT 50 and it has been used in vmx/nested.c, static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) { return (vmx_basic & GENMASK_ULL(53, 50)) >> VMX_BASIC_MEM_TYPE_SHIFT; } looks not intuitive than original patch.
On Mon, Apr 01, 2024, Xiaoyao Li wrote: > On 3/16/2024 1:54 AM, Sean Christopherson wrote: > > On Fri, Mar 15, 2024, Zhao Liu wrote: > > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: > > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > > > > that the function looks like all the helpers that grab values from > > > > VMX_BASIC and VMX_MISC MSR values. > > > > ... > > > > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) > > > > #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) > > > > #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) > > > > #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) > > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) > > > > { > > > > - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; > > > > + return vmx_misc & GENMASK_ULL(4, 0); > > > > } > > > > > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than > > > GENMASK_ULL(4, 0), and the former improves code readability. > > > > > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? > > > > I don't necessarily disagree, but in this case I value consistency over one > > individual case. As called out in the changelog, the motivation is to make > > vmx_misc_preemption_timer_rate() look like all the surrounding helpers. > > > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), > > vmx_misc_max_msr(), etc. > > > > I don't have a super strong preference, though I think my vote would be to not > > add the masks and go with this patch. These helpers are intended to be the _only_ > > way to access the fields, i.e. they effectively _are_ the mask macros, just in > > function form. > > > > +1. > > However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I > just recommended to define the MASK. > > Because we already have > > #define VMX_BASIC_MEM_TYPE_SHIFT 50 > > and it has been used in vmx/nested.c, > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > { > return (vmx_basic & GENMASK_ULL(53, 50)) >> > VMX_BASIC_MEM_TYPE_SHIFT; > } > > looks not intuitive than original patch. Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). Thanks!
On Tue, Apr 02, 2024, Sean Christopherson wrote: > On Mon, Apr 01, 2024, Xiaoyao Li wrote: > > On 3/16/2024 1:54 AM, Sean Christopherson wrote: > > > On Fri, Mar 15, 2024, Zhao Liu wrote: > > > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: > > > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > > > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > > > > > that the function looks like all the helpers that grab values from > > > > > VMX_BASIC and VMX_MISC MSR values. > > > > > > ... > > > > > > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) > > > > > #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) > > > > > #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) > > > > > #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) > > > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > > static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) > > > > > { > > > > > - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; > > > > > + return vmx_misc & GENMASK_ULL(4, 0); > > > > > } > > > > > > > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than > > > > GENMASK_ULL(4, 0), and the former improves code readability. > > > > > > > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? > > > > > > I don't necessarily disagree, but in this case I value consistency over one > > > individual case. As called out in the changelog, the motivation is to make > > > vmx_misc_preemption_timer_rate() look like all the surrounding helpers. > > > > > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), > > > vmx_misc_max_msr(), etc. > > > > > > I don't have a super strong preference, though I think my vote would be to not > > > add the masks and go with this patch. These helpers are intended to be the _only_ > > > way to access the fields, i.e. they effectively _are_ the mask macros, just in > > > function form. > > > > > > > +1. > > > > However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I > > just recommended to define the MASK. > > > > Because we already have > > > > #define VMX_BASIC_MEM_TYPE_SHIFT 50 > > > > and it has been used in vmx/nested.c, > > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > { > > return (vmx_basic & GENMASK_ULL(53, 50)) >> > > VMX_BASIC_MEM_TYPE_SHIFT; > > } > > > > looks not intuitive than original patch. > > Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). Drat. Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT, which is presumably why past me kept them around. I'm leaning towards keeping things as proposed in this series. I don't see us gaining a third copy, or even a third user, i.e. I don't think we are creating a future problem by open coding the shift in vmx_basic_vmcs_mem_type(). And IMO code like this return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >> VMX_BASIC_MEM_TYPE_SHIFT; is an unnecessary obfuscation when there is literally one user (the accessor). Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT, and either open code the values or use local const variables, but that also seems like a net negative, e.g. splits the effective definitions over too many locations.
On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote: > On Tue, Apr 02, 2024, Sean Christopherson wrote: > > On Mon, Apr 01, 2024, Xiaoyao Li wrote: > > > On 3/16/2024 1:54 AM, Sean Christopherson wrote: > > > > On Fri, Mar 15, 2024, Zhao Liu wrote: > > > > > On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: > > > > > > Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), > > > > > > and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so > > > > > > that the function looks like all the helpers that grab values from > > > > > > VMX_BASIC and VMX_MISC MSR values. > > > > > > > > ... > > > > > > > > > > -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) > > > > > > #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) > > > > > > #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) > > > > > > #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) > > > > > > @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > > > static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) > > > > > > { > > > > > > - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; > > > > > > + return vmx_misc & GENMASK_ULL(4, 0); > > > > > > } > > > > > > > > > > I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than > > > > > GENMASK_ULL(4, 0), and the former improves code readability. > > > > > > > > > > May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? > > > > > > > > I don't necessarily disagree, but in this case I value consistency over one > > > > individual case. As called out in the changelog, the motivation is to make > > > > vmx_misc_preemption_timer_rate() look like all the surrounding helpers. > > > > > > > > _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), > > > > vmx_misc_max_msr(), etc. > > > > > > > > I don't have a super strong preference, though I think my vote would be to not > > > > add the masks and go with this patch. These helpers are intended to be the _only_ > > > > way to access the fields, i.e. they effectively _are_ the mask macros, just in > > > > function form. > > > > > > > > > > +1. > > > > > > However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I > > > just recommended to define the MASK. > > > > > > Because we already have > > > > > > #define VMX_BASIC_MEM_TYPE_SHIFT 50 > > > > > > and it has been used in vmx/nested.c, > > > > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > { > > > return (vmx_basic & GENMASK_ULL(53, 50)) >> > > > VMX_BASIC_MEM_TYPE_SHIFT; > > > } > > > > > > looks not intuitive than original patch. > > > > Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop > > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). > > Drat. Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT, > which is presumably why past me kept them around. > > I'm leaning towards keeping things as proposed in this series. I don't see us > gaining a third copy, or even a third user, i.e. I don't think we are creating a > future problem by open coding the shift in vmx_basic_vmcs_mem_type(). And IMO > code like this > > return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >> > VMX_BASIC_MEM_TYPE_SHIFT; > > is an unnecessary obfuscation when there is literally one user (the accessor). > > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT, > and either open code the values or use local const variables, but that also seems > like a net negative, e.g. splits the effective definitions over too many locations. Alternatively, we can add macros like below to <asm/vmx.h> close to vmx_basic_vmcs_size() etc, so it's straightforward to see. +#define VMX_BSAIC_VMCS12_SIZE ((u64)VMCS12_SIZE << 32) +#define VMX_BASIC_MEM_TYPE_WB (MEM_TYPE_WB << 50)
On 4/25/2024 4:06 AM, Sean Christopherson wrote: > On Tue, Apr 02, 2024, Sean Christopherson wrote: >> On Mon, Apr 01, 2024, Xiaoyao Li wrote: >>> On 3/16/2024 1:54 AM, Sean Christopherson wrote: >>>> On Fri, Mar 15, 2024, Zhao Liu wrote: >>>>> On Fri, Mar 08, 2024 at 05:27:24PM -0800, Sean Christopherson wrote: >>>>>> Use vmx_misc_preemption_timer_rate() to get the rate in hardware_setup(), >>>>>> and open code the rate's bitmask in vmx_misc_preemption_timer_rate() so >>>>>> that the function looks like all the helpers that grab values from >>>>>> VMX_BASIC and VMX_MISC MSR values. >>>> >>>> ... >>>> >>>>>> -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) >>>>>> #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) >>>>>> #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) >>>>>> #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) >>>>>> @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) >>>>>> static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) >>>>>> { >>>>>> - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; >>>>>> + return vmx_misc & GENMASK_ULL(4, 0); >>>>>> } >>>>> >>>>> I feel keeping VMX_MISC_PREEMPTION_TIMER_RATE_MASK is clearer than >>>>> GENMASK_ULL(4, 0), and the former improves code readability. >>>>> >>>>> May not need to drop VMX_MISC_PREEMPTION_TIMER_RATE_MASK? >>>> >>>> I don't necessarily disagree, but in this case I value consistency over one >>>> individual case. As called out in the changelog, the motivation is to make >>>> vmx_misc_preemption_timer_rate() look like all the surrounding helpers. >>>> >>>> _If_ we want to preserve the mask, then we should add #defines for vmx_misc_cr3_count(), >>>> vmx_misc_max_msr(), etc. >>>> >>>> I don't have a super strong preference, though I think my vote would be to not >>>> add the masks and go with this patch. These helpers are intended to be the _only_ >>>> way to access the fields, i.e. they effectively _are_ the mask macros, just in >>>> function form. >>>> >>> >>> +1. >>> >>> However, it seems different for vmx_basic_vmcs_mem_type() in patch 5, that I >>> just recommended to define the MASK. >>> >>> Because we already have >>> >>> #define VMX_BASIC_MEM_TYPE_SHIFT 50 >>> >>> and it has been used in vmx/nested.c, >>> >>> static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) >>> { >>> return (vmx_basic & GENMASK_ULL(53, 50)) >> >>> VMX_BASIC_MEM_TYPE_SHIFT; >>> } >>> >>> looks not intuitive than original patch. >> >> Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop >> VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). > > Drat. Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT, > which is presumably why past me kept them around. > > I'm leaning towards keeping things as proposed in this series. If it goes this way, I beg for a comment above the code to explain. Otherwise, people might send patch to use defined MARCO in the future. > I don't see us > gaining a third copy, or even a third user, i.e. I don't think we are creating a > future problem by open coding the shift in vmx_basic_vmcs_mem_type(). And IMO > code like this > > return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >> > VMX_BASIC_MEM_TYPE_SHIFT; > > is an unnecessary obfuscation when there is literally one user (the accessor). > > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT, > and either open code the values or use local const variables, but that also seems > like a net negative, e.g. splits the effective definitions over too many locations.
On Thu, Apr 25, 2024, Kai Huang wrote: > On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote: > > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > { > > > > return (vmx_basic & GENMASK_ULL(53, 50)) >> > > > > VMX_BASIC_MEM_TYPE_SHIFT; > > > > } > > > > > > > > looks not intuitive than original patch. > > > > > > Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop > > > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). > > > > Drat. Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't > > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT, > > which is presumably why past me kept them around. > > > > I'm leaning towards keeping things as proposed in this series. I don't see us > > gaining a third copy, or even a third user, i.e. I don't think we are creating a > > future problem by open coding the shift in vmx_basic_vmcs_mem_type(). And IMO > > code like this > > > > return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >> > > VMX_BASIC_MEM_TYPE_SHIFT; > > > > is an unnecessary obfuscation when there is literally one user (the accessor). > > > > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT, > > and either open code the values or use local const variables, but that also seems > > like a net negative, e.g. splits the effective definitions over too many locations. > > Alternatively, we can add macros like below to <asm/vmx.h> close to > vmx_basic_vmcs_size() etc, so it's straightforward to see. > > +#define VMX_BSAIC_VMCS12_SIZE ((u64)VMCS12_SIZE << 32) > +#define VMX_BASIC_MEM_TYPE_WB (MEM_TYPE_WB << 50) Hmm, it's a bit hard to see it's specifically VMCS12 size, and given that prior to this series, VMX_BASIC_MEM_TYPE_WB = 6, I'm hesitant to re-introduce/redefine that macro with a different value. What if we add a helper in vmx.h to encode the VMCS info? Then the #defines for the shifts can go away because the open coded shifts are colocated and more obviously related. E.g. static inline u64 vmx_basic_encode_vmcs_info(u32 revision, u16 size, u8 memtype) { return revision | ((u64)size << 32) | ((u64)memtype << 50); } and static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs) { /* * This MSR reports some information about VMX support. We * should return information about the VMX we emulate for the * guest, and the VMCS structure we give it - not about the * VMX support of the underlying hardware. */ msrs->basic = vmx_basic_encode_vmcs_info(VMCS12_REVISION, VMCS12_SIZE, X86_MEMTYPE_WB); msrs->basic |= VMX_BASIC_TRUE_CTLS if (cpu_has_vmx_basic_inout()) msrs->basic |= VMX_BASIC_INOUT; }
On Thu, 2024-04-25 at 07:42 -0700, Sean Christopherson wrote: > On Thu, Apr 25, 2024, Kai Huang wrote: > > On Wed, 2024-04-24 at 13:06 -0700, Sean Christopherson wrote: > > > > > static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) > > > > > { > > > > > return (vmx_basic & GENMASK_ULL(53, 50)) >> > > > > > VMX_BASIC_MEM_TYPE_SHIFT; > > > > > } > > > > > > > > > > looks not intuitive than original patch. > > > > > > > > Yeah, agreed, that's taking the worst of both worlds. I'll update patch 5 to drop > > > > VMX_BASIC_MEM_TYPE_SHIFT when effectively "moving" it into vmx_basic_vmcs_mem_type(). > > > > > > Drat. Finally getting back to this, dropping VMX_BASIC_MEM_TYPE_SHIFT doesn't > > > work because it's used by nested_vmx_setup_basic(), as is VMX_BASIC_VMCS_SIZE_SHIFT, > > > which is presumably why past me kept them around. > > > > > > I'm leaning towards keeping things as proposed in this series. I don't see us > > > gaining a third copy, or even a third user, i.e. I don't think we are creating a > > > future problem by open coding the shift in vmx_basic_vmcs_mem_type(). And IMO > > > code like this > > > > > > return (vmx_basic & VMX_BASIC_MEM_TYPE_MASK) >> > > > VMX_BASIC_MEM_TYPE_SHIFT; > > > > > > is an unnecessary obfuscation when there is literally one user (the accessor). > > > > > > Another idea would be to delete VMX_BASIC_MEM_TYPE_SHIFT and VMX_BASIC_VMCS_SIZE_SHIFT, > > > and either open code the values or use local const variables, but that also seems > > > like a net negative, e.g. splits the effective definitions over too many locations. > > > > Alternatively, we can add macros like below to <asm/vmx.h> close to > > vmx_basic_vmcs_size() etc, so it's straightforward to see. > > > > +#define VMX_BSAIC_VMCS12_SIZE ((u64)VMCS12_SIZE << 32) > > +#define VMX_BASIC_MEM_TYPE_WB (MEM_TYPE_WB << 50) > > Hmm, it's a bit hard to see it's specifically VMCS12 size, and given that prior > to this series, VMX_BASIC_MEM_TYPE_WB = 6, I'm hesitant to re-introduce/redefine > that macro with a different value. > > What if we add a helper in vmx.h to encode the VMCS info? Then the #defines for > the shifts can go away because the open coded shifts are colocated and more > obviously related. E.g. > > static inline u64 vmx_basic_encode_vmcs_info(u32 revision, u16 size, u8 memtype) > { > return revision | ((u64)size << 32) | ((u64)memtype << 50); > } > > > and > > static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs) > { > /* > * This MSR reports some information about VMX support. We > * should return information about the VMX we emulate for the > * guest, and the VMCS structure we give it - not about the > * VMX support of the underlying hardware. > */ > msrs->basic = vmx_basic_encode_vmcs_info(VMCS12_REVISION, VMCS12_SIZE, > X86_MEMTYPE_WB); > > msrs->basic |= VMX_BASIC_TRUE_CTLS > if (cpu_has_vmx_basic_inout()) > msrs->basic |= VMX_BASIC_INOUT; > } Yeah this is better. Thanks.
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index 6ff179b11235..90ed559076d7 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -148,7 +148,6 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) return (vmx_basic & GENMASK_ULL(53, 50)) >> 50; } -#define VMX_MISC_PREEMPTION_TIMER_RATE_MASK GENMASK_ULL(4, 0) #define VMX_MISC_SAVE_EFER_LMA BIT_ULL(5) #define VMX_MISC_ACTIVITY_HLT BIT_ULL(6) #define VMX_MISC_ACTIVITY_SHUTDOWN BIT_ULL(7) @@ -162,7 +161,7 @@ static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic) static inline int vmx_misc_preemption_timer_rate(u64 vmx_misc) { - return vmx_misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; + return vmx_misc & GENMASK_ULL(4, 0); } static inline int vmx_misc_cr3_count(u64 vmx_misc) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index e312c48f542f..4fcfa1a213fa 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -8613,7 +8613,7 @@ static __init int hardware_setup(void) u64 use_timer_freq = 5000ULL * 1000 * 1000; cpu_preemption_timer_multi = - vmcs_config.misc & VMX_MISC_PREEMPTION_TIMER_RATE_MASK; + vmx_misc_preemption_timer_rate(vmcs_config.misc); if (tsc_khz) use_timer_freq = (u64)tsc_khz * 1000;