| Message ID | 20240829183817.2807665-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pv: Make cr4_pv32_mask be PV32-only | expand |
On 29.08.2024 20:38, Andrew Cooper wrote: > The user of cr4_pv32_mask (the cr4_pv32_restore() function) only exists in a > CONFIG_PV32 build, but right now the variable is unconditionally set up. > > To start with, move the setup into set_in_cr4() and remove it from it's > somewhat ad-hoc position in __start_xen(). This means the variable will be > set up in two steps for a CONFIG_PV32=y build, but it's cleaner and more > robust logic overall. > > With that, there's no good reason for the variable to stay in setup.c. Move > it to x86/pv/traps.c (for want of any better place to live), and move the > declaration to beside set_in_cr4() and mmu_cr4_features which is a better > position than setup.h. > > Guard the reference with CONFIG_PV32, and fix up a recent typo in an adjacent > comment while at it. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with a suggestion: > --- a/xen/arch/x86/pv/traps.c > +++ b/xen/arch/x86/pv/traps.c > @@ -18,6 +18,10 @@ > #include <asm/traps.h> > #include <irq_vectors.h> > > +#ifdef CONFIG_PV32 > +unsigned long __ro_after_init cr4_pv32_mask; > +#endif To save on the number of such #ifdef-s, how about moving this into an existing one. pv/mm.c has one, albeit near the bottom of the file (which I'd be fine with, but I could imagine you or others not liking such placement), and pv/domain.c has one near the top which seems pretty well suited. Jan
On Fri, Aug 30, 2024 at 09:55:12AM +0200, Jan Beulich wrote: > On 29.08.2024 20:38, Andrew Cooper wrote: > > The user of cr4_pv32_mask (the cr4_pv32_restore() function) only exists in a > > CONFIG_PV32 build, but right now the variable is unconditionally set up. > > > > To start with, move the setup into set_in_cr4() and remove it from it's > > somewhat ad-hoc position in __start_xen(). This means the variable will be > > set up in two steps for a CONFIG_PV32=y build, but it's cleaner and more > > robust logic overall. > > > > With that, there's no good reason for the variable to stay in setup.c. Move > > it to x86/pv/traps.c (for want of any better place to live), and move the > > declaration to beside set_in_cr4() and mmu_cr4_features which is a better > > position than setup.h. > > > > Guard the reference with CONFIG_PV32, and fix up a recent typo in an adjacent > > comment while at it. > > > > No functional change. > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with a suggestion: > > > --- a/xen/arch/x86/pv/traps.c > > +++ b/xen/arch/x86/pv/traps.c > > @@ -18,6 +18,10 @@ > > #include <asm/traps.h> > > #include <irq_vectors.h> > > > > +#ifdef CONFIG_PV32 > > +unsigned long __ro_after_init cr4_pv32_mask; > > +#endif > > To save on the number of such #ifdef-s, how about moving this into an existing > one. pv/mm.c has one, albeit near the bottom of the file (which I'd be fine > with, but I could imagine you or others not liking such placement), and > pv/domain.c has one near the top which seems pretty well suited. I'm fine either way: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 30/08/2024 10:19 am, Roger Pau Monné wrote: > On Fri, Aug 30, 2024 at 09:55:12AM +0200, Jan Beulich wrote: >> On 29.08.2024 20:38, Andrew Cooper wrote: >>> The user of cr4_pv32_mask (the cr4_pv32_restore() function) only exists in a >>> CONFIG_PV32 build, but right now the variable is unconditionally set up. >>> >>> To start with, move the setup into set_in_cr4() and remove it from it's >>> somewhat ad-hoc position in __start_xen(). This means the variable will be >>> set up in two steps for a CONFIG_PV32=y build, but it's cleaner and more >>> robust logic overall. >>> >>> With that, there's no good reason for the variable to stay in setup.c. Move >>> it to x86/pv/traps.c (for want of any better place to live), and move the >>> declaration to beside set_in_cr4() and mmu_cr4_features which is a better >>> position than setup.h. >>> >>> Guard the reference with CONFIG_PV32, and fix up a recent typo in an adjacent >>> comment while at it. >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with a suggestion: >> >>> --- a/xen/arch/x86/pv/traps.c >>> +++ b/xen/arch/x86/pv/traps.c >>> @@ -18,6 +18,10 @@ >>> #include <asm/traps.h> >>> #include <irq_vectors.h> >>> >>> +#ifdef CONFIG_PV32 >>> +unsigned long __ro_after_init cr4_pv32_mask; >>> +#endif >> To save on the number of such #ifdef-s, how about moving this into an existing >> one. pv/mm.c has one, albeit near the bottom of the file (which I'd be fine >> with, but I could imagine you or others not liking such placement), and >> pv/domain.c has one near the top which seems pretty well suited. > I'm fine either way: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks, Roger. Thanks. I'll put it alongside opt_pv32 in pv/domain.c which is indeed pretty well suited. ~Andrew
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h index 66463f6a6d67..e71dbb8d3fbf 100644 --- a/xen/arch/x86/include/asm/processor.h +++ b/xen/arch/x86/include/asm/processor.h @@ -312,11 +312,15 @@ static inline void stts(void) * after us can get the correct flags. */ extern unsigned long mmu_cr4_features; +extern unsigned long cr4_pv32_mask; static always_inline void set_in_cr4 (unsigned long mask) { mmu_cr4_features |= mask; write_cr4(read_cr4() | mask); + + if ( IS_ENABLED(CONFIG_PV32) && (mask & XEN_CR4_PV32_BITS) ) + cr4_pv32_mask |= (mask & XEN_CR4_PV32_BITS); } static always_inline void __monitor(const void *eax, unsigned long ecx, diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index 8f7dfefb4dcf..d75589178b91 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -64,8 +64,6 @@ extern bool opt_dom0_verbose; extern bool opt_dom0_cpuid_faulting; extern bool opt_dom0_msr_relaxed; -extern unsigned long cr4_pv32_mask; - #define max_init_domid (0) #endif diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index 57b1834b5eaa..262edb6bf2f0 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -1061,12 +1061,14 @@ int __init dom0_construct_pv(struct domain *d, /* * Clear SMAP in CR4 to allow user-accesses in construct_dom0(). This - * prevents us needing to write rewrite construct_dom0() in terms of + * prevents us needing to write construct_dom0() in terms of * copy_{to,from}_user(). */ if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) { - cr4_pv32_mask &= ~X86_CR4_SMAP; + if ( IS_ENABLED(CONFIG_PV32) ) + cr4_pv32_mask &= ~X86_CR4_SMAP; + write_cr4(read_cr4() & ~X86_CR4_SMAP); } @@ -1075,7 +1077,9 @@ int __init dom0_construct_pv(struct domain *d, if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) { write_cr4(read_cr4() | X86_CR4_SMAP); - cr4_pv32_mask |= X86_CR4_SMAP; + + if ( IS_ENABLED(CONFIG_PV32) ) + cr4_pv32_mask |= X86_CR4_SMAP; } return rc; diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c index 5a7341abf068..3389a25acd83 100644 --- a/xen/arch/x86/pv/traps.c +++ b/xen/arch/x86/pv/traps.c @@ -18,6 +18,10 @@ #include <asm/traps.h> #include <irq_vectors.h> +#ifdef CONFIG_PV32 +unsigned long __ro_after_init cr4_pv32_mask; +#endif + void pv_inject_event(const struct x86_event *event) { struct vcpu *curr = current; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f1076c72032d..c2e0082a3020 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -79,8 +79,6 @@ bool __read_mostly use_invpcid; int8_t __initdata opt_probe_port_aliases = -1; boolean_param("probe-port-aliases", opt_probe_port_aliases); -unsigned long __ro_after_init cr4_pv32_mask; - /* **** Linux config option: propagated to domain0. */ /* "acpi=off": Sisables both ACPI table parsing and interpreter. */ /* "acpi=force": Override the disable blacklist. */ @@ -1898,8 +1896,6 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) set_in_cr4(X86_CR4_SMAP); - cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS; - if ( boot_cpu_has(X86_FEATURE_FSGSBASE) ) set_in_cr4(X86_CR4_FSGSBASE);
The user of cr4_pv32_mask (the cr4_pv32_restore() function) only exists in a CONFIG_PV32 build, but right now the variable is unconditionally set up. To start with, move the setup into set_in_cr4() and remove it from it's somewhat ad-hoc position in __start_xen(). This means the variable will be set up in two steps for a CONFIG_PV32=y build, but it's cleaner and more robust logic overall. With that, there's no good reason for the variable to stay in setup.c. Move it to x86/pv/traps.c (for want of any better place to live), and move the declaration to beside set_in_cr4() and mmu_cr4_features which is a better position than setup.h. Guard the reference with CONFIG_PV32, and fix up a recent typo in an adjacent comment while at it. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/processor.h | 4 ++++ xen/arch/x86/include/asm/setup.h | 2 -- xen/arch/x86/pv/dom0_build.c | 10 +++++++--- xen/arch/x86/pv/traps.c | 4 ++++ xen/arch/x86/setup.c | 4 ---- 5 files changed, 15 insertions(+), 9 deletions(-) base-commit: 99f942f3d410059dc223ee0a908827e928ef3592