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 <[email protected]>
Reviewed-by: Jan Beulich <[email protected]> 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
