Re: [PATCH 03/22] x86/dom0: only disable SMAP for the PV dom0 build

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 29, 2024 at 04:59:09PM +0100, Andrew Cooper wrote:
> On 26/07/2024 4:21 pm, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index eee20bb1753c..bc387d96b519 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -955,26 +955,9 @@ static struct domain *__init create_dom0(const 
> > module_t *image,
> >          }
> >      }
> >  
> > -    /*
> > -     * Temporarily clear SMAP in CR4 to allow user-accesses in 
> > construct_dom0().
> > -     * This saves a large number of corner cases interactions with
> > -     * copy_from_user().
> > -     */
> > -    if ( cpu_has_smap )
> > -    {
> > -        cr4_pv32_mask &= ~X86_CR4_SMAP;
> > -        write_cr4(read_cr4() & ~X86_CR4_SMAP);
> > -    }
> > -
> >      if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
> >          panic("Could not construct domain 0\n");
> >  
> > -    if ( cpu_has_smap )
> > -    {
> > -        write_cr4(read_cr4() | X86_CR4_SMAP);
> > -        cr4_pv32_mask |= X86_CR4_SMAP;
> > -    }
> > -
> 
> Hang on.  Isn't this (preexistingly) broken given the distinction
> between cpu_has_smap and X86_FEATURE_XEN_SMAP ?

I see, looks like Xen will unconditionally enable SMAP if the user has
requested SMP for HVM only.  Forcefully disabling SMAP for both PV and
HVM will result in the CPUID bit getting cleared, and hence
cpu_has_smap == false.

> I'm very tempted to use this as a justification to remove opt_smap.

Oh, so my change fixes that bug by caching the previous cr4 instead of
using cpu_has_smap.

It seems like opt_smap is useful for the PV shim, as it caused some
unnecessary performance degradation on AMD hardware due to AMD not
allowing to selectively trap accesses to CR4, so on pvshim mode
it gets disabled:

b05ec9263e56 x86/sm{e, a}p: do not enable SMEP/SMAP in PV shim by default on AMD

Thanks, Roger.