On Mon, 2023-10-30 at 16:39 -0700, Xin Li (Intel) wrote:
> From: Xin Li <xin3.li@xxxxxxxxx>
>
> Define VMX basic information fields with BIT_ULL()/GENMASK_ULL(), and
> replace hardcoded VMX basic numbers with these field macros.
>
> Per Sean's ask, save the full/raw value of MSR_IA32_VMX_BASIC in the
> global vmcs_config as type u64 to get rid of the hi/lo crud, and then
> use VMX_BASIC helpers to extract info as needed.
>
[...]
Btw, it's better to have a cover letter even for this small series and give a
lore link for old versions so that people can easily find old discussions.
>
> +/* x86 memory types, explicitly used in VMX only */
> +#define MEM_TYPE_WB 0x6ULL
> +#define MEM_TYPE_UC 0x0ULL
The renaming of memory type macros deserves some justification in the changelog
IMHO, because it doesn't belong to what the patch title claims to do.
You can even split this part out, but will leave to Sean/Paolo.
> +
> +/* VMX_BASIC bits and bitmasks */
> +#define VMX_BASIC_32BIT_PHYS_ADDR_ONLY BIT_ULL(48)
> +#define VMX_BASIC_INOUT BIT_ULL(54)
> +
> #define VMX_MISC_PREEMPTION_TIMER_RATE_MASK 0x0000001f
> #define VMX_MISC_SAVE_EFER_LMA 0x00000020
> #define VMX_MISC_ACTIVITY_HLT 0x00000040
> @@ -143,6 +151,16 @@ static inline u32 vmx_basic_vmcs_size(u64 vmx_basic)
> return (vmx_basic & GENMASK_ULL(44, 32)) >> 32;
> }
>
> +static inline u32 vmx_basic_vmcs_basic_cap(u64 vmx_basic)
> +{
> + return (vmx_basic & GENMASK_ULL(63, 45)) >> 32;
> +}
Is this still needed?
> +
> +static inline u32 vmx_basic_vmcs_mem_type(u64 vmx_basic)
> +{
> + return (vmx_basic & GENMASK_ULL(53, 50)) >> 50;
> +}
You already have VMX_BASIC_MEM_TYPE_SHIFT defined below, so it looks a little
bit odd to still use hard-coded values here.
But per Sean I agree it's quite noisy to have all these _SHIFT defined just in
order to get rid of these hard-coded values.
How about, ...
> +#define VMX_BASIC_VMCS_SIZE_SHIFT 32
> +#define VMX_BASIC_DUAL_MONITOR_TREATMENT BIT_ULL(49)
> +#define VMX_BASIC_MEM_TYPE_SHIFT 50
> +#define VMX_BASIC_TRUE_CTLS BIT_ULL(55)
> +
>
... since, if I am reading correctly, the two _SHIFT above are only used ...
[...]
> @@ -6964,7 +6975,7 @@ static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
> VMCS12_REVISION |
> VMX_BASIC_TRUE_CTLS |
> ((u64)VMCS12_SIZE << VMX_BASIC_VMCS_SIZE_SHIFT) |
> - (VMX_BASIC_MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
> + (MEM_TYPE_WB << VMX_BASIC_MEM_TYPE_SHIFT);
>
... here, we can remove the two _SHIFT but define below instead:
#define VMX_BASIC_VMCS12_SIZE ((u64)VMCS12_SIZE << 32)
#define VMX_BASIC_MEM_TYPE_WB (MEM_TYPE_WB << 50)
And use above two macros in nested_vmx_setup_basic()?
