Re: [PATCH 02/14] x86/xstate: Create map/unmap primitives for xsave areas

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Oct 29, 2024 at 11:58 AM Alejandro Vallejo
<alejandro.vallejo@xxxxxxxxx> wrote:
>
> Hi,
>
> On Mon Oct 28, 2024 at 5:20 PM GMT, Andrew Cooper wrote:
> > On 28/10/2024 3:49 pm, Alejandro Vallejo wrote:
> > > diff --git a/xen/arch/x86/include/asm/xstate.h 
> > > b/xen/arch/x86/include/asm/xstate.h
> > > index 07017cc4edfd..36260459667c 100644
> > > --- a/xen/arch/x86/include/asm/xstate.h
> > > +++ b/xen/arch/x86/include/asm/xstate.h
> > > @@ -143,4 +143,24 @@ static inline bool xstate_all(const struct vcpu *v)
> > >             (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
> > >  }
> > >
> > > +/*
> > > + * Fetch a pointer to the XSAVE area of a vCPU
> > > + *
> > > + * If ASI is enabled for the domain, this mapping is pCPU-local.
> > > + *
> > > + * @param v Owner of the XSAVE area
> > > + */
> > > +#define vcpu_map_xsave_area(v) ((v)->arch.xsave_area)
> > > +
> > > +/*
> > > + * Drops the XSAVE area of a vCPU and nullifies its pointer on exit.
> > > + *
> > > + * If ASI is enabled and v is not the currently scheduled vCPU then the
> > > + * per-pCPU mapping is removed from the address space.
> > > + *
> > > + * @param v           vCPU logically owning xsave_area
> > > + * @param xsave_area  XSAVE blob of v
> > > + */
> > > +#define vcpu_unmap_xsave_area(v, x) ({ (x) = NULL; })
> > > +
> >
> > Is there a preview of how these will end up looking with the real ASI
> > bits in place?
>
> I expect the contents to be something along these lines (in function form for
> clarity):
>
>   struct xsave_struct *vcpu_map_xsave_area(struct vcpu *v)
>   {
>       if ( !v->domain->asi )
>           return v->arch.xsave_area;
>
>       if ( likely(v == current) )
>           return percpu_fixmap(v, PCPU_FIX_XSAVE_AREA);
>
>       /* Likely some new vmap-like abstraction after AMX */
>       return map_domain_page(v->arch.xsave_area_pg);
>   }
>
> Where:
>   1. v->arch.xsave_area is a pointer to the XSAVE area on non-ASI domains.
>   2. v->arch.xsave_area_pg an mfn (or a pointer to a page_info, converted)
>   3. percpu_fixmap(v, PCPU_FIX_XSAVE_AREA) is a slot in a per-vCPU fixmap, 
> that
>      changes as we context switch from vCPU to vCPU.
>
>   /*
>    * NOTE: Being a function this doesn't nullify the xsave_area pointer, but
>    * it would in a macro. It's unimportant for the overall logic though.
>    */
>   void vcpu_unmap_xsave_area(struct vcpu *v, struct xsave_struct *xsave_area)
>   {
>       /* Catch mismatched areas when ASI is disabled */
>       ASSERT(v->domain->asi || xsave_area == v->arch.xsave_area);
>
>       /* Likely some new vunmap-like abstraction after AMX */
>       if ( v->domain->asi && v != current )
>           unmap_domain_page(xsave_area);
>   }
>
> Of course, many of these details hang in the balance of what happens to the 
> ASI
> series from Roger. In any case, the takeaway is that map/unmap must have
> fastpaths for "current" that don't involve mapping. The assumption is that
> non-current vCPUs are cold paths. In particular, context switches will undergo
> some refactoring in order to make save/restore not require additional
> map/unmaps besides the page table switch and yet another change to further
> align "current" with the currently running page tables. Paths like the
> instruction emulator go through these wrappers later on for ease of
> auditability, but are early-returns that cause no major overhead.
>
> My expectation is that these macros are general enough to be tweakable in
> whatever way is most suitable, thus allowing the refactor of the codebase at
> large to make it ASI-friendly before the details of the ASI infra are merged,
> or even finalised.
>
> >
> > Having a macro-that-reads-like-a-function mutating x by name, rather
> > than by pointer, is somewhat rude.  This is why we capitalise
> > XFREE()/etc which have a similar pattern; to make it clear it's a macro
> > and potentially doing weird things with scopes.
> >
> > ~Andrew
>
> That magic trick on unmap warrants uppercase, agreed. Initially it was all
> function calls and after macrofying them I was lazy to change their users.
>
> Cheers,
> Alejandro
>

Why not using static inline functions?

On the documentation, I found weird that "v" is described quite
differently for the 2 macros:
1) @param v Owner of the XSAVE area;
2) @param v           vCPU logically owning xsave_area

For "x" the documentation is "@param xsave_area  XSAVE blob of v", but
there's no "xsave_area" parameter.

(very minors, you can ignore)

Frediano