Re: [PATCH v1 3/3] target/i386: Raise the highest index value used for any VMCS encoding

[Zhao Liu <zhao1.liu@intel.com>]

Hi Xin,

On Wed, Aug 07, 2024 at 01:18:12AM -0700, Xin Li (Intel) wrote:
> Date: Wed,  7 Aug 2024 01:18:12 -0700
> From: "Xin Li (Intel)" <xin@zytor.com>
> Subject: [PATCH v1 3/3] target/i386: Raise the highest index value used for
>  any VMCS encoding
> X-Mailer: git-send-email 2.45.2
> 
> From: Lei Wang <lei4.wang@intel.com>
> 
> Because the index value of the VMCS field encoding of FRED injected-event
> data (one of the newly added VMCS fields for FRED transitions), 0x52, is
> larger than any existing index value, raise the highest index value used
> for any VMCS encoding to 0x52.
> 
> Because the index value of the VMCS field encoding of Secondary VM-exit
> controls, 0x44, is larger than any existing index value, raise the highest
> index value used for any VMCS encoding to 0x44.
> 
> Co-developed-by: Xin Li <xin3.li@intel.com>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> Signed-off-by: Lei Wang <lei4.wang@intel.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
>  target/i386/cpu.h     | 1 +
>  target/i386/kvm/kvm.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 118ef9cb68..62324c3dcd 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1186,6 +1186,7 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
>  #define VMX_VM_EXIT_PT_CONCEAL_PIP                  0x01000000
>  #define VMX_VM_EXIT_CLEAR_IA32_RTIT_CTL             0x02000000
>  #define VMX_VM_EXIT_LOAD_IA32_PKRS                  0x20000000
> +#define VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS     0x80000000

It's necessary to add the corresponding feat_name to FEAT_VMX_EXIT_CTLS
feat word array, which could help filter the user's settings in the -cpu.

>  #define VMX_VM_ENTRY_LOAD_DEBUG_CONTROLS            0x00000004
>  #define VMX_VM_ENTRY_IA32E_MODE                     0x00000200
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 31f149c990..fac5990274 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -3694,7 +3694,14 @@ static void kvm_msr_entry_add_vmx(X86CPU *cpu, FeatureWordArray f)
>      kvm_msr_entry_add(cpu, MSR_IA32_VMX_CR4_FIXED0,
>                        CR4_VMXE_MASK);
>  
> -    if (f[FEAT_VMX_SECONDARY_CTLS] & VMX_SECONDARY_EXEC_TSC_SCALING) {
> +    if (f[FEAT_7_1_EAX] & CPUID_7_1_EAX_FRED) {
> +        /* FRED injected-event data (0x2052).  */
> +        kvm_msr_entry_add(cpu, MSR_IA32_VMX_VMCS_ENUM, 0x52);

HMM, I have the questions when I check the FRED spec.

Section 9.3.4 said, (for injected-event data) "This field has uses the
encoding pair 2052H/2053H."

So why adjust the highest index to 0x52 other than 0x53?

And it seems FRED introduces another field "original-event data"
(0x2404/0x2405), why not consider this field here as well?

> +    } else if (f[FEAT_VMX_EXIT_CTLS] &
> +               VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
> +        /* Secondary VM-exit controls (0x2044).  */
> +        kvm_msr_entry_add(cpu, MSR_IA32_VMX_VMCS_ENUM, 0x44);
> +    } else if (f[FEAT_VMX_SECONDARY_CTLS] & VMX_SECONDARY_EXEC_TSC_SCALING) {
>          /* TSC multiplier (0x2032).  */
>          kvm_msr_entry_add(cpu, MSR_IA32_VMX_VMCS_ENUM, 0x32);
>      } else {

Maybe we could adjust the index in a cleaner way like
x86_cpu_adjust_level(), but the current case-by-case is ok for me as
well.

Regards,
Zhao