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

[Jan Beulich <jbeulich@xxxxxxxx>]

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