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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 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@xxxxxxxxxx>
>
> Thanks, Roger.

Thanks.  I'll put it alongside opt_pv32 in pv/domain.c which is indeed
pretty well suited.

~Andrew