diff mbox series

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

Message ID 20240807081813.735158-4-xin@zytor.com (mailing list archive)
State New, archived
Headers show
Series target/i386: Add nested FRED support | expand

Commit Message

Xin Li Aug. 7, 2024, 8:18 a.m. UTC 
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(-)

Comments

Zhao Liu Aug. 7, 2024, 3:39 p.m. UTC | #1 
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
Xin Li Aug. 9, 2024, 6:27 a.m. UTC | #2 
>> +    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?

For 16-bit, 32-bit, and natural-width fields, they must be read/write
as a whole, thus the lowest bit of their encoding must be 0.

A 64-bit VMCS field can be accessed as two 32-bit fields, with the
higher order half using an odd VMCS encoding.  But conceptually they
should be treated as a whole 64-bit field.

Better to refer to Intel SDM 25.11.2 VMREAD, VMWRITE, and Encodings of
VMCS Fields.

> 
> 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.

Yeah, that sounds a good idea.  But the code hasn't gone wild...
Xin Li Aug. 9, 2024, 7:38 a.m. UTC | #3 
On 8/8/2024 11:27 PM, Xin Li wrote:
>>> +    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?

Okay, found it in the Intel SDM:

Index. Bits 9:1 distinguish components with the same field width and type.

Bit 0 is not included in the index field.
Zhao Liu Aug. 9, 2024, 9:01 a.m. UTC | #4 
On Fri, Aug 09, 2024 at 12:38:02AM -0700, Xin Li wrote:
> Date: Fri, 9 Aug 2024 00:38:02 -0700
> From: Xin Li <xin@zytor.com>
> Subject: Re: [PATCH v1 3/3] target/i386: Raise the highest index value used
>  for any VMCS encoding
> 
> On 8/8/2024 11:27 PM, Xin Li wrote:
> > > > +    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?
> 
> Okay, found it in the Intel SDM:
> 
> Index. Bits 9:1 distinguish components with the same field width and type.
> 
> Bit 0 is not included in the index field.

Thanks for your education and explanation! I see, for
IA32_VMX_VMCS_ENUM, bit 0 is reserved and only index field is enough.

Regards,
Zhao
diff mbox series

Patch

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
 
 #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);
+    } 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 {