On Fri, 2021-06-18 at 14:46 -0700, Sean Christopherson wrote:
> Calculate the max VMCS index for vmcs12 by walking the array to find the
> actual max index. Hardcoding the index is prone to bitrot, and the
> calculation is only done on KVM bringup (albeit on every CPU, but there
> aren't _that_ many null entries in the array).
>
> Fixes: 3c0f99366e34 ("KVM: nVMX: Add a TSC multiplier field in VMCS12")
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
Could you give me an example on how this fails in the KVM unit tests?
I have a bug report here and it might be related so I want to save some
time triaging it.
Best regards,
Maxim Levitsky
> ---
>
> Note, the vmx test in kvm-unit-tests will still fail using stock QEMU,
> as QEMU also hardcodes and overwrites the MSR. The test passes if I
> hack KVM to ignore userspace (it was easier than rebuilding QEMU).
>
> arch/x86/kvm/vmx/nested.c | 37 +++++++++++++++++++++++++++++++++++--
> arch/x86/kvm/vmx/vmcs.h | 8 ++++++++
> arch/x86/kvm/vmx/vmcs12.h | 6 ------
> 3 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b531e08a095b..183fd9d62fc5 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6374,6 +6374,40 @@ void nested_vmx_set_vmcs_shadowing_bitmap(void)
> }
> }
>
> +/*
> + * Indexing into the vmcs12 uses the VMCS encoding rotated left by 6. Undo
> + * that madness to get the encoding for comparison.
> + */
> +#define VMCS12_IDX_TO_ENC(idx) ((u16)(((u16)(idx) >> 6) | ((u16)(idx) << 10)))
> +
> +static u64 nested_vmx_calc_vmcs_enum_msr(void)
> +{
> + /*
> + * Note these are the so called "index" of the VMCS field encoding, not
> + * the index into vmcs12.
> + */
> + unsigned int max_idx, idx;
> + int i;
> +
> + /*
> + * For better or worse, KVM allows VMREAD/VMWRITE to all fields in
> + * vmcs12, regardless of whether or not the associated feature is
> + * exposed to L1. Simply find the field with the highest index.
> + */
> + max_idx = 0;
> + for (i = 0; i < nr_vmcs12_fields; i++) {
> + /* The vmcs12 table is very, very sparsely populated. */
> + if (!vmcs_field_to_offset_table[i])
> + continue;
> +
> + idx = vmcs_field_index(VMCS12_IDX_TO_ENC(i));
> + if (idx > max_idx)
> + max_idx = idx;
> + }
> +
> + return (u64)max_idx << VMCS_FIELD_INDEX_SHIFT;
> +}
> +
> /*
> * nested_vmx_setup_ctls_msrs() sets up variables containing the values to be
> * returned for the various VMX controls MSRs when nested VMX is enabled.
> @@ -6619,8 +6653,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
> rdmsrl(MSR_IA32_VMX_CR0_FIXED1, msrs->cr0_fixed1);
> rdmsrl(MSR_IA32_VMX_CR4_FIXED1, msrs->cr4_fixed1);
>
> - /* highest index: VMX_PREEMPTION_TIMER_VALUE */
> - msrs->vmcs_enum = VMCS12_MAX_FIELD_INDEX << 1;
> + msrs->vmcs_enum = nested_vmx_calc_vmcs_enum_msr();
> }
>
> void nested_vmx_hardware_unsetup(void)
> diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
> index 1472c6c376f7..de3b04d4b587 100644
> --- a/arch/x86/kvm/vmx/vmcs.h
> +++ b/arch/x86/kvm/vmx/vmcs.h
> @@ -164,4 +164,12 @@ static inline int vmcs_field_readonly(unsigned long field)
> return (((field >> 10) & 0x3) == 1);
> }
>
> +#define VMCS_FIELD_INDEX_SHIFT (1)
> +#define VMCS_FIELD_INDEX_MASK GENMASK(9, 1)
> +
> +static inline unsigned int vmcs_field_index(unsigned long field)
> +{
> + return (field & VMCS_FIELD_INDEX_MASK) >> VMCS_FIELD_INDEX_SHIFT;
> +}
> +
> #endif /* __KVM_X86_VMX_VMCS_H */
> diff --git a/arch/x86/kvm/vmx/vmcs12.h b/arch/x86/kvm/vmx/vmcs12.h
> index bb81a23afe89..5e0e1b39f495 100644
> --- a/arch/x86/kvm/vmx/vmcs12.h
> +++ b/arch/x86/kvm/vmx/vmcs12.h
> @@ -205,12 +205,6 @@ struct __packed vmcs12 {
> */
> #define VMCS12_SIZE KVM_STATE_NESTED_VMX_VMCS_SIZE
>
> -/*
> - * VMCS12_MAX_FIELD_INDEX is the highest index value used in any
> - * supported VMCS12 field encoding.
> - */
> -#define VMCS12_MAX_FIELD_INDEX 0x17
> -
> /*
> * For save/restore compatibility, the vmcs12 field offsets must not change.
> */
