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@cloud.com>
---
v2:
  * Comment macros more heavily to show their performance characteristics.
  * Addressed various nits in the macro comments.
  * Macro names to uppercase.
---
 xen/arch/x86/include/asm/xstate.h | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index 07017cc4edfd..6b0daff0aeec 100644
--- 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) ({ (x) = NULL; })
+
 #endif /* __ASM_XSTATE_H */
-- 
2.47.0

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 ...

> +/*
> + * 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?

Jan

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