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

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

On Tue Oct 29, 2024 at 1:28 PM GMT, Jan Beulich wrote:
> On 29.10.2024 12:57, Alejandro Vallejo wrote:
> > 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);
> >   }
>
> I'd like to ask that map_domain_page() be avoided here from the beginning, to
> take AMX into account right away. I've been sitting on the AMX series for
> years, and I'd consider it pretty unfair if it was me to take care of such an
> aspect, when instead the series should (imo) long have landed.
>
> Jan

Of course. This is just pseudo-code for explanation purposes, but I didn't want
to introduce imaginary functions. In the final thing we'll want to map an array
of MFNs if the XSAVE area is large enough.

I am already accounting for the XSAVE area to possibly exceed a single page (3
after AMX, I think?). Part of this abstraction stems from that want, in fact,
as otherwise I could simply stash it all away under map_domain_page() and let
that take care of everything. We'll want map_domain_pages_contig() or something
along those lines that takes an array of mfns we've previously stored in
arch_vcpu. But that's a tomorrow problem for when we do have a secret area to
create those mappings on.

For today, I'd be happy with most code to stop assuming there will be a pointer
in the vcpu.

Cheers,