Re: [PATCH 3/4] x86: drop ASM_{CL,ST}AC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2020 16:51, Andrew Cooper wrote:
> On 15/07/2020 11:49, Jan Beulich wrote:
> > Use ALTERNATIVE directly, such that at the use sites it is visible that
> > alternative code patching is in use. Similarly avoid hiding the fact in
> > SAVE_ALL.
> >
> > No change to generated code.
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Definitely +1 to not hiding the STAC/CLAC in SAVE_ALL.  I've been
> meaning to undo that mistake for ages.
>
> OOI, what made you change your mind?  I'm pleased that you have.
This, to me, is a direct consequence of dropping ASM_STAC / ASM_CLAC:
If we don't want the fact of a use of ALTERNATIVE be hidden there, it
also shouldn't be hidden inside SAVE_ALL.

> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -2165,9 +2165,9 @@ void activate_debugregs(const struct vcp
> >   void asm_domain_crash_synchronous(unsigned long addr)
> >   {
> >       /*
> > -     * We need clear AC bit here because in entry.S AC is set
> > -     * by ASM_STAC to temporarily allow accesses to user pages
> > -     * which is prevented by SMAP by default.
> > +     * We need to clear AC bit here because in entry.S it gets set to
> > +     * temporarily allow accesses to user pages which is prevented by
> > +     * SMAP by default.
> As you're adjusting the text, It should read "We need to clear the AC
> bit ..."
>
> But I also think it would be clearer to say that exception fixup may
> leave user access enabled, which we fix up here by unconditionally
> disabling user access.
Can do.

> Preferably with this rewritten, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>
Thanks.

Jan