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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Mon Dec 9, 2024 at 4:11 PM GMT, Jan Beulich wrote:
> On 05.11.2024 15:32, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/include/asm/xstate.h
> > +++ b/xen/arch/x86/include/asm/xstate.h
> > @@ -143,4 +143,46 @@ static inline bool xstate_all(const struct vcpu *v)
> >             (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
> >  }
> >  
> > +/*
> > + * Fetch a pointer to a vCPU's XSAVE area
> > + *
> > + * TL;DR: If v == current, the mapping is guaranteed to already exist.
> > + *
> > + * Despite the name, this macro might not actually map anything. The only 
> > case
> > + * in which a mutation of page tables is strictly required is when ASI==on 
> > &&
> > + * v!=current. For everything else the mapping already exists and needs not
> > + * be created nor destroyed.
> > + *
> > + *                         +-----------------+--------------+
> > + *                         |   v == current  | v != current |
> > + *          +--------------+-----------------+--------------+
> > + *          | ASI  enabled | per-vCPU fixmap |  actual map  |
> > + *          +--------------+-----------------+--------------+
> > + *          | ASI disabled |             directmap          |
> > + *          +--------------+--------------------------------+
> > + *
> > + * There MUST NOT be outstanding maps of XSAVE areas of the non-current 
> > vCPU
> > + * at the point of context switch. Otherwise, the unmap operation will
> > + * misbehave.
> > + *
> > + * TODO: Expand the macro to the ASI cases after infra to do so is in 
> > place.
> > + *
> > + * @param v Owner of the XSAVE area
> > + */
> > +#define VCPU_MAP_XSAVE_AREA(v) ((v)->arch.xsave_area)
>
> When this is fleshed out, I expect (hope) type safety (type of "return
> value") will remain to be there. I think it would be nice ...

The return type will always be a pointer to `struct xsave_struct`, so that's
definitely type-safe.

>
> > +/*
> > + * Drops the mapping of a vCPU's XSAVE area and nullifies its pointer on 
> > exit
> > + *
> > + * See VCPU_MAP_XSAVE_AREA() for additional information on the persistence 
> > of
> > + * these mappings. This macro only tears down the mappings in the ASI=on &&
> > + * v!=current case.
> > + *
> > + * TODO: Expand the macro to the ASI cases after infra to do so is in 
> > place.
> > + *
> > + * @param v Owner of the XSAVE area
> > + * @param x XSAVE blob of v
> > + */
> > +#define VCPU_UNMAP_XSAVE_AREA(v, x) ({ (x) = NULL; })
>
> ... if this was typesafe (at least on x) from the very beginning as
> well. Thoughts?

I tentatively intend for both macros to involve a call to static inline
functions when the real infrastructure is in place. By the time everything is
fleshed out both will be definitely type-safe. Const-ness might suffer though,
as there's a tension between clarity of static inlines and flexible qualifiers
(i.e: having const outputs iff inputs are const).

>
> Jan

Cheers,
Alejandro