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(-)

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);
 

base-commit: 99f942f3d410059dc223ee0a908827e928ef3592
-- 
2.39.2

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