Re: [PATCH v3 01/12] x86/xstate: Create map/unmap primitives for xsave areas

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

Hi,

On Mon Jan 27, 2025 at 10:44 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > Add infrastructure to simplify ASI handling. With ASI in the picture
> > we'll have several different means of accessing the XSAVE area of a
> > given vCPU, depending on whether a domain is covered by ASI or not and
> > whether the vCPU is question is scheduled on the current pCPU or not.
> > 
> > Having these complexities exposed at the call sites becomes unwieldy
> > very fast. These wrappers are intended to be used in a similar way to
> > map_domain_page() and unmap_domain_page(); The map operation will
> > dispatch the appropriate pointer for each case in a future patch, while
> > unmap will remain a no-op where no unmap is required (e.g: when there's
> > no ASI) and remove the transient maping if one was required.
> > 
> > Follow-up patches replace all uses of raw v->arch.xsave_area by this
> > mechanism in preparation to add the beforementioned dispatch logic to be
> > added at a later time.
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one nit below.
>
> > --- 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)
> > +
> > +/*
> > + * 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) do { (void)(v); (x) = NULL; } while(0)
>
> The "while" still wants to conform to style, despite it being a kind of
> degenerate form. The overwhelming majority of instances we've got have at
> least a blank before the opening parenthesis. Many also have the ones
> inside.
>
> Jan

Sure, makes sense. I'll adjust it.

Cheers
Alejandro