Re: [PATCH] x86/pv: Make cr4_pv32_mask be PV32-only

["Roger Pau Monné" <roger.pau@citrix.com>]

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.