[PATCH v3 2/2] target/i386: add control bits support for LAM

Binbin Wu posted 2 patches 2 years, 6 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
[PATCH v3 2/2] target/i386: add control bits support for LAM
Posted by Binbin Wu 2 years, 6 months ago
LAM uses CR3[61] and CR3[62] to configure/enable LAM on user pointers.
LAM uses CR4[28] to configure/enable LAM on supervisor pointers.

For CR3 LAM bits, no additional handling needed:
- TCG
  LAM is not supported for TCG of target-i386.  helper_write_crN() and helper_vmrun()
  check max physical address bits before calling cpu_x86_update_cr3(), no change needed,
  i.e. CR3 LAM bits are not allowed to be set in TCG.
- gdbstub
  x86_cpu_gdb_write_register() will call cpu_x86_update_cr3() to update cr3. Allow gdb
  to set the LAM bit(s) to CR3, if vcpu doesn't support LAM, KVM_SET_SREGS will fail as
  other CR3 reserved bits.

For CR4 LAM bit, its reservation depends on vcpu supporting LAM feature or not.
- TCG
  LAM is not supported for TCG of target-i386.  helper_write_crN() and helper_vmrun()
  check CR4 reserved bit before calling cpu_x86_update_cr4(), i.e. CR4 LAM bit is not
  allowed to be set in TCG.
- gdbstub
  x86_cpu_gdb_write_register() will call cpu_x86_update_cr4() to update cr4. Allow gdb
  to set the LAM bit to CR4, if vcpu doesn't support LAM, KVM_SET_SREGS will fail.
- x86_cpu_reset_hold() doesn't need special handling.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 target/i386/cpu.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 4db97899fe..710fadf550 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -261,6 +261,7 @@ typedef enum X86Seg {
 #define CR4_SMAP_MASK   (1U << 21)
 #define CR4_PKE_MASK   (1U << 22)
 #define CR4_PKS_MASK   (1U << 24)
+#define CR4_LAM_SUP_MASK (1U << 28)
 
 #define CR4_RESERVED_MASK \
 (~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
@@ -269,7 +270,8 @@ typedef enum X86Seg {
                 | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK | CR4_UMIP_MASK \
                 | CR4_LA57_MASK \
                 | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
-                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
+                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK \
+                | CR4_LAM_SUP_MASK))
 
 #define DR6_BD          (1 << 13)
 #define DR6_BS          (1 << 14)
@@ -2478,6 +2480,9 @@ static inline uint64_t cr4_reserved_bits(CPUX86State *env)
     if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
         reserved_bits |= CR4_PKS_MASK;
     }
+    if (!(env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_LAM)) {
+        reserved_bits |= CR4_LAM_SUP_MASK;
+    }
     return reserved_bits;
 }
 
-- 
2.25.1
Re: [PATCH v3 2/2] target/i386: add control bits support for LAM
Posted by Xiaoyao Li 2 years, 1 month ago
On 7/21/2023 4:08 PM, Binbin Wu wrote:
> LAM uses CR3[61] and CR3[62] to configure/enable LAM on user pointers.
> LAM uses CR4[28] to configure/enable LAM on supervisor pointers.
> 
> For CR3 LAM bits, no additional handling needed:
> - TCG
>    LAM is not supported for TCG of target-i386.  helper_write_crN() and helper_vmrun()
>    check max physical address bits before calling cpu_x86_update_cr3(), no change needed,
>    i.e. CR3 LAM bits are not allowed to be set in TCG.
> - gdbstub
>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr3() to update cr3. Allow gdb
>    to set the LAM bit(s) to CR3, if vcpu doesn't support LAM, KVM_SET_SREGS will fail as
>    other CR3 reserved bits.
> 
> For CR4 LAM bit, its reservation depends on vcpu supporting LAM feature or not.
> - TCG
>    LAM is not supported for TCG of target-i386.  helper_write_crN() and helper_vmrun()
>    check CR4 reserved bit before calling cpu_x86_update_cr4(), i.e. CR4 LAM bit is not
>    allowed to be set in TCG.
> - gdbstub
>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr4() to update cr4. Allow gdb
>    to set the LAM bit to CR4, if vcpu doesn't support LAM, KVM_SET_SREGS will fail.

I would go follow the current code, to mask out LAM bit if no CPUID.

> - x86_cpu_reset_hold() doesn't need special handling.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
>   target/i386/cpu.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 4db97899fe..710fadf550 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -261,6 +261,7 @@ typedef enum X86Seg {
>   #define CR4_SMAP_MASK   (1U << 21)
>   #define CR4_PKE_MASK   (1U << 22)
>   #define CR4_PKS_MASK   (1U << 24)
> +#define CR4_LAM_SUP_MASK (1U << 28)
>   
>   #define CR4_RESERVED_MASK \
>   (~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
> @@ -269,7 +270,8 @@ typedef enum X86Seg {
>                   | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK | CR4_UMIP_MASK \
>                   | CR4_LA57_MASK \
>                   | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
> -                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
> +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK \
> +                | CR4_LAM_SUP_MASK))
>   
>   #define DR6_BD          (1 << 13)
>   #define DR6_BS          (1 << 14)
> @@ -2478,6 +2480,9 @@ static inline uint64_t cr4_reserved_bits(CPUX86State *env)
>       if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
>           reserved_bits |= CR4_PKS_MASK;
>       }
> +    if (!(env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_LAM)) {
> +        reserved_bits |= CR4_LAM_SUP_MASK;
> +    }
>       return reserved_bits;
>   }
>
Re: [PATCH v3 2/2] target/i386: add control bits support for LAM
Posted by Binbin Wu 2 years, 1 month ago

On 12/28/2023 4:51 PM, Xiaoyao Li wrote:
> On 7/21/2023 4:08 PM, Binbin Wu wrote:
>> LAM uses CR3[61] and CR3[62] to configure/enable LAM on user pointers.
>> LAM uses CR4[28] to configure/enable LAM on supervisor pointers.
>>
>> For CR3 LAM bits, no additional handling needed:
>> - TCG
>>    LAM is not supported for TCG of target-i386. helper_write_crN() 
>> and helper_vmrun()
>>    check max physical address bits before calling 
>> cpu_x86_update_cr3(), no change needed,
>>    i.e. CR3 LAM bits are not allowed to be set in TCG.
>> - gdbstub
>>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr3() to 
>> update cr3. Allow gdb
>>    to set the LAM bit(s) to CR3, if vcpu doesn't support LAM, 
>> KVM_SET_SREGS will fail as
>>    other CR3 reserved bits.
>>
>> For CR4 LAM bit, its reservation depends on vcpu supporting LAM 
>> feature or not.
>> - TCG
>>    LAM is not supported for TCG of target-i386. helper_write_crN() 
>> and helper_vmrun()
>>    check CR4 reserved bit before calling cpu_x86_update_cr4(), i.e. 
>> CR4 LAM bit is not
>>    allowed to be set in TCG.
>> - gdbstub
>>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr4() to 
>> update cr4. Allow gdb
>>    to set the LAM bit to CR4, if vcpu doesn't support LAM, 
>> KVM_SET_SREGS will fail.
>
> I would go follow the current code, to mask out LAM bit if no CPUID.

I can do it in the next version.

But I am curious what's the rule of masking out a CR4 bit if no CPUID
in cpu_x86_update_cr4()?
e.g. current code checks SMAP but not SMEP.


>
>> - x86_cpu_reset_hold() doesn't need special handling.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>>   target/i386/cpu.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 4db97899fe..710fadf550 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -261,6 +261,7 @@ typedef enum X86Seg {
>>   #define CR4_SMAP_MASK   (1U << 21)
>>   #define CR4_PKE_MASK   (1U << 22)
>>   #define CR4_PKS_MASK   (1U << 24)
>> +#define CR4_LAM_SUP_MASK (1U << 28)
>>     #define CR4_RESERVED_MASK \
>>   (~(target_ulong)(CR4_VME_MASK | CR4_PVI_MASK | CR4_TSD_MASK \
>> @@ -269,7 +270,8 @@ typedef enum X86Seg {
>>                   | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK | 
>> CR4_UMIP_MASK \
>>                   | CR4_LA57_MASK \
>>                   | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | 
>> CR4_OSXSAVE_MASK \
>> -                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | 
>> CR4_PKS_MASK))
>> +                | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | 
>> CR4_PKS_MASK \
>> +                | CR4_LAM_SUP_MASK))
>>     #define DR6_BD          (1 << 13)
>>   #define DR6_BS          (1 << 14)
>> @@ -2478,6 +2480,9 @@ static inline uint64_t 
>> cr4_reserved_bits(CPUX86State *env)
>>       if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS)) {
>>           reserved_bits |= CR4_PKS_MASK;
>>       }
>> +    if (!(env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_LAM)) {
>> +        reserved_bits |= CR4_LAM_SUP_MASK;
>> +    }
>>       return reserved_bits;
>>   }
>


Re: [PATCH v3 2/2] target/i386: add control bits support for LAM
Posted by Xiaoyao Li 2 years ago
On 1/3/2024 5:25 PM, Binbin Wu wrote:
> 
> 
> On 12/28/2023 4:51 PM, Xiaoyao Li wrote:
>> On 7/21/2023 4:08 PM, Binbin Wu wrote:
>>> LAM uses CR3[61] and CR3[62] to configure/enable LAM on user pointers.
>>> LAM uses CR4[28] to configure/enable LAM on supervisor pointers.
>>>
>>> For CR3 LAM bits, no additional handling needed:
>>> - TCG
>>>    LAM is not supported for TCG of target-i386. helper_write_crN() 
>>> and helper_vmrun()
>>>    check max physical address bits before calling 
>>> cpu_x86_update_cr3(), no change needed,
>>>    i.e. CR3 LAM bits are not allowed to be set in TCG.
>>> - gdbstub
>>>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr3() to 
>>> update cr3. Allow gdb
>>>    to set the LAM bit(s) to CR3, if vcpu doesn't support LAM, 
>>> KVM_SET_SREGS will fail as
>>>    other CR3 reserved bits.
>>>
>>> For CR4 LAM bit, its reservation depends on vcpu supporting LAM 
>>> feature or not.
>>> - TCG
>>>    LAM is not supported for TCG of target-i386. helper_write_crN() 
>>> and helper_vmrun()
>>>    check CR4 reserved bit before calling cpu_x86_update_cr4(), i.e. 
>>> CR4 LAM bit is not
>>>    allowed to be set in TCG.
>>> - gdbstub
>>>    x86_cpu_gdb_write_register() will call cpu_x86_update_cr4() to 
>>> update cr4. Allow gdb
>>>    to set the LAM bit to CR4, if vcpu doesn't support LAM, 
>>> KVM_SET_SREGS will fail.
>>
>> I would go follow the current code, to mask out LAM bit if no CPUID.
> 
> I can do it in the next version.
> 
> But I am curious what's the rule of masking out a CR4 bit if no CPUID
> in cpu_x86_update_cr4()?
> e.g. current code checks SMAP but not SMEP.
> 

Frankly, I don't know. As you explained in commit message, missing the 
check doesn't cause any functional issue because the function is only 
called for tcg code and LAM is not enabled for tcg.

But personally, I think adding the check does no harm and the logic is 
straightforward, while not adding the check looks not intuitive and begs 
a comment to explain.

So my preference is to add the check.