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 <[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. > I'm fine either way: > > Reviewed-by: Roger Pau Monné <[email protected]> > > Thanks, Roger.
Thanks. I'll put it alongside opt_pv32 in pv/domain.c which is indeed pretty well suited. ~Andrew
