| Message ID | 20230721080800.2329-3-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: add support for LAM | expand |
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; > } >
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; >> } >
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.
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; }
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(-)