| Message ID | 20240924112343.193506-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Further simplify CR4 handling in dom0_construct_pv() | expand |
On Tue, Sep 24, 2024 at 12:23:43PM +0100, Andrew Cooper wrote: > The logic would be more robust disabling SMAP based on its precense in CR4, > rather than on certain features. > > A forthcoming feature, LASS, needs the same treatment here. Introduce minimum > enumeration information, although it will take a bit more work to get LASS > fully usable in guests. Reading the ISA, doesn't LASS require SMAP to be enabled in %cr4, and hence disabling SMAP already disables LASS? (without having to toggle the LASS %cr4 bit) "A supervisor-mode data access causes a LASS violation only if supervisor-mode access protection is enabled (because CR4.SMAP = 1) and either RFLAGS.AC = 0 or the access implicitly accesses a system data structure." We can consider also disabling it, but I think it would need to be noted that such disabling is not strictly necessary, as disabling SMAP already disables LASS. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > > I know LASS can't be used with traditional PV guests, but I have some PV-lite > plans. The problem is the PV kernel, in CPL3, accessing addresses in the high > canonincal half. > --- > xen/arch/x86/include/asm/x86-defns.h | 1 + > xen/arch/x86/pv/dom0_build.c | 18 ++++++++++-------- > xen/include/public/arch-x86/cpufeatureset.h | 1 + > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h > index caa92829eaa9..8f97fb1e6a12 100644 > --- a/xen/arch/x86/include/asm/x86-defns.h > +++ b/xen/arch/x86/include/asm/x86-defns.h > @@ -75,6 +75,7 @@ > #define X86_CR4_PKE 0x00400000 /* enable PKE */ > #define X86_CR4_CET 0x00800000 /* Control-flow Enforcement Technology */ > #define X86_CR4_PKS 0x01000000 /* Protection Key Supervisor */ > +#define X86_CR4_LASS 0x08000000 /* Linear Address Space Separation */ > > /* > * XSTATE component flags in XCR0 | MSR_XSS > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c > index 262edb6bf2f0..f5c868df384f 100644 > --- a/xen/arch/x86/pv/dom0_build.c > +++ b/xen/arch/x86/pv/dom0_build.c > @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d, > module_t *initrd, > const char *cmdline) > { > + unsigned long cr4 = read_cr4(); > + unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS; const maybe? Seeing as it is read-only. Thanks, Roger.
On 24.09.2024 13:23, Andrew Cooper wrote: > --- a/xen/arch/x86/pv/dom0_build.c > +++ b/xen/arch/x86/pv/dom0_build.c > @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d, > module_t *initrd, > const char *cmdline) > { > + unsigned long cr4 = read_cr4(); > + unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS; > int rc; > > /* > - * Clear SMAP in CR4 to allow user-accesses in construct_dom0(). This > - * prevents us needing to write construct_dom0() in terms of > + * Clear SMAP/LASS in CR4 to allow user-accesses in construct_dom0(). > + * This prevents us needing to write construct_dom0() in terms of > * copy_{to,from}_user(). > */ > - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) > + if ( cr4 & mask ) > { > if ( IS_ENABLED(CONFIG_PV32) ) > - cr4_pv32_mask &= ~X86_CR4_SMAP; > + cr4_pv32_mask &= ~mask; > > - write_cr4(read_cr4() & ~X86_CR4_SMAP); > + write_cr4(cr4 & ~mask); > } > > rc = dom0_construct(d, image, image_headroom, initrd, cmdline); > > - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) > + if ( cr4 & mask ) > { > - write_cr4(read_cr4() | X86_CR4_SMAP); > + write_cr4(cr4); > > if ( IS_ENABLED(CONFIG_PV32) ) > - cr4_pv32_mask |= X86_CR4_SMAP; > + cr4_pv32_mask |= mask; You may end up setting a bit here which wasn't previously set, and which might then fault when cr4_pv32_restore tries to OR this into %cr4. Aiui you must have tested this on LASS-capable hardware, for it to have worked. Jan
On Tue, Sep 24, 2024 at 03:44:07PM +0200, Jan Beulich wrote: > On 24.09.2024 13:23, Andrew Cooper wrote: > > --- a/xen/arch/x86/pv/dom0_build.c > > +++ b/xen/arch/x86/pv/dom0_build.c > > @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d, > > module_t *initrd, > > const char *cmdline) > > { > > + unsigned long cr4 = read_cr4(); > > + unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS; > > int rc; > > > > /* > > - * Clear SMAP in CR4 to allow user-accesses in construct_dom0(). This > > - * prevents us needing to write construct_dom0() in terms of > > + * Clear SMAP/LASS in CR4 to allow user-accesses in construct_dom0(). > > + * This prevents us needing to write construct_dom0() in terms of > > * copy_{to,from}_user(). > > */ > > - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) > > + if ( cr4 & mask ) > > { > > if ( IS_ENABLED(CONFIG_PV32) ) > > - cr4_pv32_mask &= ~X86_CR4_SMAP; > > + cr4_pv32_mask &= ~mask; > > > > - write_cr4(read_cr4() & ~X86_CR4_SMAP); > > + write_cr4(cr4 & ~mask); > > } > > > > rc = dom0_construct(d, image, image_headroom, initrd, cmdline); > > > > - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) > > + if ( cr4 & mask ) > > { > > - write_cr4(read_cr4() | X86_CR4_SMAP); > > + write_cr4(cr4); > > > > if ( IS_ENABLED(CONFIG_PV32) ) > > - cr4_pv32_mask |= X86_CR4_SMAP; > > + cr4_pv32_mask |= mask; > > You may end up setting a bit here which wasn't previously set, and which > might then fault when cr4_pv32_restore tries to OR this into %cr4. Aiui > you must have tested this on LASS-capable hardware, for it to have worked. Possibly also needs X86_CR4_LASS adding to the XEN_CR4_PV32_BITS define, as otherwise it won't end up in cr4_pv32_mask in the first place AFAICT. Thanks, Roger.
On 24/09/2024 1:30 pm, Roger Pau Monné wrote: > On Tue, Sep 24, 2024 at 12:23:43PM +0100, Andrew Cooper wrote: >> The logic would be more robust disabling SMAP based on its precense in CR4, >> rather than on certain features. >> >> A forthcoming feature, LASS, needs the same treatment here. Introduce minimum >> enumeration information, although it will take a bit more work to get LASS >> fully usable in guests. > Reading the ISA, doesn't LASS require SMAP to be enabled in %cr4, and > hence disabling SMAP already disables LASS? (without having to toggle > the LASS %cr4 bit) > > "A supervisor-mode data access causes a LASS violation only if > supervisor-mode access protection is enabled (because CR4.SMAP = 1) > and either RFLAGS.AC = 0 or the access implicitly accesses a system > data structure." > > We can consider also disabling it, but I think it would need to be > noted that such disabling is not strictly necessary, as disabling SMAP > already disables LASS. Hmm. LASS looks to have no CR4 dependencies on SMAP or SMEP, and the ISE does suggest they can be used independently. However, I see no connection to paging (beyond LMA), and that is going to become a problem in due course. Anyway - I'll drop the LASS aspect for now. It can be left to whomever gets some working real hardware first. ~Andrew
diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h index caa92829eaa9..8f97fb1e6a12 100644 --- a/xen/arch/x86/include/asm/x86-defns.h +++ b/xen/arch/x86/include/asm/x86-defns.h @@ -75,6 +75,7 @@ #define X86_CR4_PKE 0x00400000 /* enable PKE */ #define X86_CR4_CET 0x00800000 /* Control-flow Enforcement Technology */ #define X86_CR4_PKS 0x01000000 /* Protection Key Supervisor */ +#define X86_CR4_LASS 0x08000000 /* Linear Address Space Separation */ /* * XSTATE component flags in XCR0 | MSR_XSS diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index 262edb6bf2f0..f5c868df384f 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d, module_t *initrd, const char *cmdline) { + unsigned long cr4 = read_cr4(); + unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS; int rc; /* - * Clear SMAP in CR4 to allow user-accesses in construct_dom0(). This - * prevents us needing to write construct_dom0() in terms of + * Clear SMAP/LASS in CR4 to allow user-accesses in construct_dom0(). + * This prevents us needing to write construct_dom0() in terms of * copy_{to,from}_user(). */ - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) + if ( cr4 & mask ) { if ( IS_ENABLED(CONFIG_PV32) ) - cr4_pv32_mask &= ~X86_CR4_SMAP; + cr4_pv32_mask &= ~mask; - write_cr4(read_cr4() & ~X86_CR4_SMAP); + write_cr4(cr4 & ~mask); } rc = dom0_construct(d, image, image_headroom, initrd, cmdline); - if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) + if ( cr4 & mask ) { - write_cr4(read_cr4() | X86_CR4_SMAP); + write_cr4(cr4); if ( IS_ENABLED(CONFIG_PV32) ) - cr4_pv32_mask |= X86_CR4_SMAP; + cr4_pv32_mask |= mask; } return rc; diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 8fa3fb711a8d..cbc0a3a8aa2b 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -303,6 +303,7 @@ XEN_CPUFEATURE(SM3, 10*32+ 1) /*A SM3 Instructions */ XEN_CPUFEATURE(SM4, 10*32+ 2) /*A SM4 Instructions */ XEN_CPUFEATURE(AVX_VNNI, 10*32+ 4) /*A AVX-VNNI Instructions */ XEN_CPUFEATURE(AVX512_BF16, 10*32+ 5) /*A AVX512 BFloat16 Instructions */ +XEN_CPUFEATURE(LASS, 10*32+ 6) /* Linear Address Space Separation */ XEN_CPUFEATURE(CMPCCXADD, 10*32+ 7) /*a CMPccXADD Instructions */ XEN_CPUFEATURE(FZRM, 10*32+10) /*A Fast Zero-length REP MOVSB */ XEN_CPUFEATURE(FSRS, 10*32+11) /*A Fast Short REP STOSB */
The logic would be more robust disabling SMAP based on its precense in CR4, rather than on certain features. A forthcoming feature, LASS, needs the same treatment here. Introduce minimum enumeration information, although it will take a bit more work to get LASS fully usable in guests. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> I know LASS can't be used with traditional PV guests, but I have some PV-lite plans. The problem is the PV kernel, in CPL3, accessing addresses in the high canonincal half. --- xen/arch/x86/include/asm/x86-defns.h | 1 + xen/arch/x86/pv/dom0_build.c | 18 ++++++++++-------- xen/include/public/arch-x86/cpufeatureset.h | 1 + 3 files changed, 12 insertions(+), 8 deletions(-)