[PATCH v1 0/3] target/i386: Add nested FRED support

[PATCH v1 0/3] target/i386: Add nested FRED support

[Xin Li (Intel)]

This patch set adds nested FRED support to allow KVM to run a nested
guest with FRED enabled.


Lei Wang (1):
  target/i386: Raise the highest index value used for any VMCS encoding

Xin Li (Intel) (2):
  target/i386: Delete duplicated macro definition CR4_FRED_MASK
  target/i386: Add VMX control bits for nested FRED support

 target/i386/cpu.c     | 4 ++--
 target/i386/cpu.h     | 7 +------
 target/i386/kvm/kvm.c | 9 ++++++++-
 3 files changed, 11 insertions(+), 9 deletions(-)


base-commit: 6d00c6f982562222adbd0613966285792125abe5
-- 
2.45.2

[PATCH v1 1/3] target/i386: Delete duplicated macro definition CR4_FRED_MASK

[Xin Li (Intel)]

Macro CR4_FRED_MASK is defined twice, delete one.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 target/i386/cpu.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index c6cc035df3..118ef9cb68 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -267,12 +267,6 @@ typedef enum X86Seg {
 #define CR4_FRED_MASK   0
 #endif
 
-#ifdef TARGET_X86_64
-#define CR4_FRED_MASK   (1ULL << 32)
-#else
-#define CR4_FRED_MASK   0
-#endif
-
 #define CR4_RESERVED_MASK \
 (~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
                 | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
-- 
2.45.2

[PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Xin Li (Intel)]

Add definitions of
  1) VM-exit activate secondary controls bit
  2) VM-entry load FRED bit
which are required to enable nested FRED.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 target/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 85ef7452c0..31f287cae0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "vmx-exit-save-efer", "vmx-exit-load-efer",
                 "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
             NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
-            NULL, "vmx-exit-load-pkrs", NULL, NULL,
+            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
         },
         .msr = {
             .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
@@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, "vmx-entry-ia32e-mode", NULL, NULL,
             NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
             "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
-            NULL, NULL, "vmx-entry-load-pkrs", NULL,
+            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-- 
2.45.2

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

[Xin Li (Intel)]

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
 
 #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 {
-- 
2.45.2

Re: [PATCH v1 1/3] target/i386: Delete duplicated macro definition CR4_FRED_MASK

[Zhao Liu]

On Wed, Aug 07, 2024 at 01:18:10AM -0700, Xin Li (Intel) wrote:
> Date: Wed,  7 Aug 2024 01:18:10 -0700
> From: "Xin Li (Intel)" <xin@zytor.com>
> Subject: [PATCH v1 1/3] target/i386: Delete duplicated macro definition
>  CR4_FRED_MASK
> X-Mailer: git-send-email 2.45.2
> 
> Macro CR4_FRED_MASK is defined twice, delete one.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
>  target/i386/cpu.h | 6 ------
>  1 file changed, 6 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

[Zhao Liu]

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

Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Zhao Liu]

Hi Xin,

On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
> Date: Wed,  7 Aug 2024 01:18:11 -0700
> From: "Xin Li (Intel)" <xin@zytor.com>
> Subject: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED
>  support
> X-Mailer: git-send-email 2.45.2
> 
> Add definitions of
>   1) VM-exit activate secondary controls bit
>   2) VM-entry load FRED bit
> which are required to enable nested FRED.
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
>  target/i386/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 85ef7452c0..31f287cae0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "vmx-exit-save-efer", "vmx-exit-load-efer",
>                  "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
>              NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
> -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
> +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",

Oh, the order of my reviews is mixed up.
It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.

>          },
>          .msr = {
>              .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>              NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>              "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",

Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
if they are not used.

Regards,
Zhao

>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>          },
> -- 
> 2.45.2
> 

Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Xin Li]

On 8/7/2024 8:58 AM, Zhao Liu wrote:
> On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
>> @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>               "vmx-exit-save-efer", "vmx-exit-load-efer",
>>                   "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
>>               NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
>> -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
>> +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
> 
> Oh, the order of my reviews is mixed up.
> It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.

Usually a simple definition is added in a patch where it is used, not in
qemu?

>>           },
>>           .msr = {
>>               .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
>> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>               NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>>               NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>>               "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
>> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
>> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> 
> Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> if they are not used.

I'm not sure, but why add something that is not being used (thus not
tested)?

Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Zhao Liu]

Hi Xin,

On Thu, Aug 08, 2024 at 12:04:42AM -0700, Xin Li wrote:
> Date: Thu, 8 Aug 2024 00:04:42 -0700
> From: Xin Li <xin@zytor.com>
> Subject: Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested
>  FRED support
> 
> On 8/7/2024 8:58 AM, Zhao Liu wrote:
> > On Wed, Aug 07, 2024 at 01:18:11AM -0700, Xin Li (Intel) wrote:
> > > @@ -1435,7 +1435,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > >               "vmx-exit-save-efer", "vmx-exit-load-efer",
> > >                   "vmx-exit-save-preemption-timer", "vmx-exit-clear-bndcfgs",
> > >               NULL, "vmx-exit-clear-rtit-ctl", NULL, NULL,
> > > -            NULL, "vmx-exit-load-pkrs", NULL, NULL,
> > > +            NULL, "vmx-exit-load-pkrs", NULL, "vmx-exit-secondary-ctls",
> > 
> > Oh, the order of my reviews is mixed up.
> > It's better to move VMX_VM_EXIT_ACTIVATE_SECONDARY_CONTROLS into this patch.
> 
> Usually a simple definition is added in a patch where it is used, not in
> qemu?
> 
> > >           },
> > >           .msr = {
> > >               .index = MSR_IA32_VMX_TRUE_EXIT_CTLS,
> > > @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > >               NULL, "vmx-entry-ia32e-mode", NULL, NULL,
> > >               NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
> > >               "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> > > -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> > > +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> > 
> > Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> > and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> > if they are not used.
> 
> I'm not sure, but why add something that is not being used (thus not
> tested)?

Yes, the use of macros is a factor. My another consideration is the
integrity of the feature definitions. When the such feature definitions
were first introduced in commit 704798add83b (”target/i386: add VMX
definitions”), I understand thay were mainly used to enumerate and
reflect hardware support and not all defs are used directly.

The feat word name and the feature definition should essentially be
bound, and it might be possible to generate the feature definition
from the feat word via some script without having to add it manually,
but right now there is no work on this, and no additional constraints,
so we have to manually add and manually check it to make sure that the
two correspond to each other. When a feature word is added, it means
that Host supports the corresponding feature, and from an integrity
perspective, so it is natural to continue adding definition (just like
the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
control bits")), right?

Though I found that there are still some mismatches between the feature
word and the corresponding definition, but ideally they should coexist.

About the test, if it's just enumerated and not added to a specific CPU
model or involved by other logic, it's harmless?

Thanks,
Zhao

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

[Xin Li]

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

Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Xin Li]

>>>> @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>>>                NULL, "vmx-entry-ia32e-mode", NULL, NULL,
>>>>                NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
>>>>                "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
>>>> -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
>>>> +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
>>>
>>> Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
>>> and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
>>> if they are not used.
>>
>> I'm not sure, but why add something that is not being used (thus not
>> tested)?
> 
> Yes, the use of macros is a factor. My another consideration is the
> integrity of the feature definitions. When the such feature definitions
> were first introduced in commit 704798add83b (”target/i386: add VMX
> definitions”), I understand thay were mainly used to enumerate and
> reflect hardware support and not all defs are used directly.
> 
> The feat word name and the feature definition should essentially be
> bound, and it might be possible to generate the feature definition
> from the feat word via some script without having to add it manually,
> but right now there is no work on this, and no additional constraints,
> so we have to manually add and manually check it to make sure that the
> two correspond to each other. When a feature word is added, it means
> that Host supports the corresponding feature, and from an integrity
> perspective, so it is natural to continue adding definition (just like
> the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
> control bits")), right?
> 
> Though I found that there are still some mismatches between the feature
> word and the corresponding definition, but ideally they should coexist.
> 
> About the test, if it's just enumerated and not added to a specific CPU
> model or involved by other logic, it's harmless?

Unless tests are ready, such code are literally dead code, and could get
broken w/o being noticed for a long time.

I think we should add it only when tests are also added.  Otherwise we 
added burden to maintainers, hoping test will be added soon, which often
never happen.

> 
> Thanks,
> Zhao
> 
> 

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

[Xin Li]

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.

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

[Zhao Liu]

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

Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested FRED support

[Zhao Liu]

On Thu, Aug 08, 2024 at 11:38:11PM -0700, Xin Li wrote:
> Date: Thu, 8 Aug 2024 23:38:11 -0700
> From: Xin Li <xin@zytor.com>
> Subject: Re: [PATCH v1 2/3] target/i386: Add VMX control bits for nested
>  FRED support
> 
> > > > > @@ -1450,7 +1450,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> > > > >                NULL, "vmx-entry-ia32e-mode", NULL, NULL,
> > > > >                NULL, "vmx-entry-load-perf-global-ctrl", "vmx-entry-load-pat", "vmx-entry-load-efer",
> > > > >                "vmx-entry-load-bndcfgs", NULL, "vmx-entry-load-rtit-ctl", NULL,
> > > > > -            NULL, NULL, "vmx-entry-load-pkrs", NULL,
> > > > > +            NULL, NULL, "vmx-entry-load-pkrs", "vmx-entry-load-fred",
> > > > 
> > > > Should we also define VMX_VM_ENTRY_LOAD_FRED? "vmx-entry-load-rtit-ctl"
> > > > and "vmx-entry-load-pkrs" have their corresponding bit definitions, even
> > > > if they are not used.
> > > 
> > > I'm not sure, but why add something that is not being used (thus not
> > > tested)?
> > 
> > Yes, the use of macros is a factor. My another consideration is the
> > integrity of the feature definitions. When the such feature definitions
> > were first introduced in commit 704798add83b (”target/i386: add VMX
> > definitions”), I understand thay were mainly used to enumerate and
> > reflect hardware support and not all defs are used directly.
> > 
> > The feat word name and the feature definition should essentially be
> > bound, and it might be possible to generate the feature definition
> > from the feat word via some script without having to add it manually,
> > but right now there is no work on this, and no additional constraints,
> > so we have to manually add and manually check it to make sure that the
> > two correspond to each other. When a feature word is added, it means
> > that Host supports the corresponding feature, and from an integrity
> > perspective, so it is natural to continue adding definition (just like
> > the commit 52a44ad2b92b ("target/i386: Expose VMX entry/exit load pkrs
> > control bits")), right?
> > 
> > Though I found that there are still some mismatches between the feature
> > word and the corresponding definition, but ideally they should coexist.
> > 
> > About the test, if it's just enumerated and not added to a specific CPU
> > model or involved by other logic, it's harmless?
> 
> Unless tests are ready, such code are literally dead code, and could get
> broken w/o being noticed for a long time.
> 
> I think we should add it only when tests are also added.  Otherwise we added
> burden to maintainers, hoping test will be added soon, which often
> never happen.

It makes sense and can reduce the burden on maintainers. Now I totally
agree with you.

Thanks,
Zhao