Messages in this thread | | | Date | Mon, 1 Apr 2024 15:07:13 +0800 | Subject | Re: [PATCH v6 8/9] KVM: VMX: Open code VMX preemption timer rate mask in its accessor | From | Xiaoyao Li <> |
| |
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.
| |