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

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

On Tue Oct 29, 2024 at 8:19 AM GMT, Jan Beulich wrote:
> On 28.10.2024 16:49, Alejandro Vallejo wrote:
> > --- 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.
>
> Taking the umap commentary into account, I think this needs to expand
> some, to also symmetrically cover what the unmap comment says regarding
> "v is [not] the currently scheduled vCPU".

Yes, that's fair.

> This may then also help
> better see the further outlook, as Andrew was asking for.

Sure, I'll answer his comment in a jiffy with a rough approximation of what I
expect them to contain.

>
> > + * @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.
>
> Nit: I expect it drops the mapping, not the area.

Yes, although even the mapping might not be dropped if we can credibly avoid
it. Regardless, yes this needs rewriting.

The particulars are murky and should become easier to see with the pseudo-code
I'm about to answer Andrew with

>
> > + * 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; })
> > +
> >  #endif /* __ASM_XSTATE_H */

Cheers,
Alejandro