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>
---
 xen/arch/x86/include/asm/xstate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

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

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". This may then also help
better see the further outlook, as Andrew was asking for.

> + * @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.

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

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

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?

Having a macro-that-reads-like-a-function mutating x by name, rather
than by pointer, is somewhat rude.  This is why we capitalise
XFREE()/etc which have a similar pattern; to make it clear it's a macro
and potentially doing weird things with scopes.

~Andrew

Hi,

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);
  }

Where:
  1. v->arch.xsave_area is a pointer to the XSAVE area on non-ASI domains.
  2. v->arch.xsave_area_pg an mfn (or a pointer to a page_info, converted)
  3. percpu_fixmap(v, PCPU_FIX_XSAVE_AREA) is a slot in a per-vCPU fixmap, that
     changes as we context switch from vCPU to vCPU.

  /*
   * NOTE: Being a function this doesn't nullify the xsave_area pointer, but
   * it would in a macro. It's unimportant for the overall logic though.
   */
  void vcpu_unmap_xsave_area(struct vcpu *v, struct xsave_struct *xsave_area)
  {
      /* Catch mismatched areas when ASI is disabled */
      ASSERT(v->domain->asi || xsave_area == v->arch.xsave_area);

      /* Likely some new vunmap-like abstraction after AMX */
      if ( v->domain->asi && v != current )
          unmap_domain_page(xsave_area);
  }

Of course, many of these details hang in the balance of what happens to the ASI
series from Roger. In any case, the takeaway is that map/unmap must have
fastpaths for "current" that don't involve mapping. The assumption is that
non-current vCPUs are cold paths. In particular, context switches will undergo
some refactoring in order to make save/restore not require additional
map/unmaps besides the page table switch and yet another change to further
align "current" with the currently running page tables. Paths like the
instruction emulator go through these wrappers later on for ease of
auditability, but are early-returns that cause no major overhead.

My expectation is that these macros are general enough to be tweakable in
whatever way is most suitable, thus allowing the refactor of the codebase at
large to make it ASI-friendly before the details of the ASI infra are merged,
or even finalised.

>
> Having a macro-that-reads-like-a-function mutating x by name, rather
> than by pointer, is somewhat rude.  This is why we capitalise
> XFREE()/etc which have a similar pattern; to make it clear it's a macro
> and potentially doing weird things with scopes.
>
> ~Andrew

That magic trick on unmap warrants uppercase, agreed. Initially it was all
function calls and after macrofying them I was lazy to change their users.

Cheers,
Alejandro

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

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,

On Tue, Oct 29, 2024 at 11:58 AM Alejandro Vallejo
<alejandro.vallejo@cloud.com> wrote:
>
> Hi,
>
> 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);
>   }
>
> Where:
>   1. v->arch.xsave_area is a pointer to the XSAVE area on non-ASI domains.
>   2. v->arch.xsave_area_pg an mfn (or a pointer to a page_info, converted)
>   3. percpu_fixmap(v, PCPU_FIX_XSAVE_AREA) is a slot in a per-vCPU fixmap, that
>      changes as we context switch from vCPU to vCPU.
>
>   /*
>    * NOTE: Being a function this doesn't nullify the xsave_area pointer, but
>    * it would in a macro. It's unimportant for the overall logic though.
>    */
>   void vcpu_unmap_xsave_area(struct vcpu *v, struct xsave_struct *xsave_area)
>   {
>       /* Catch mismatched areas when ASI is disabled */
>       ASSERT(v->domain->asi || xsave_area == v->arch.xsave_area);
>
>       /* Likely some new vunmap-like abstraction after AMX */
>       if ( v->domain->asi && v != current )
>           unmap_domain_page(xsave_area);
>   }
>
> Of course, many of these details hang in the balance of what happens to the ASI
> series from Roger. In any case, the takeaway is that map/unmap must have
> fastpaths for "current" that don't involve mapping. The assumption is that
> non-current vCPUs are cold paths. In particular, context switches will undergo
> some refactoring in order to make save/restore not require additional
> map/unmaps besides the page table switch and yet another change to further
> align "current" with the currently running page tables. Paths like the
> instruction emulator go through these wrappers later on for ease of
> auditability, but are early-returns that cause no major overhead.
>
> My expectation is that these macros are general enough to be tweakable in
> whatever way is most suitable, thus allowing the refactor of the codebase at
> large to make it ASI-friendly before the details of the ASI infra are merged,
> or even finalised.
>
> >
> > Having a macro-that-reads-like-a-function mutating x by name, rather
> > than by pointer, is somewhat rude.  This is why we capitalise
> > XFREE()/etc which have a similar pattern; to make it clear it's a macro
> > and potentially doing weird things with scopes.
> >
> > ~Andrew
>
> That magic trick on unmap warrants uppercase, agreed. Initially it was all
> function calls and after macrofying them I was lazy to change their users.
>
> Cheers,
> Alejandro
>

Why not using static inline functions?

On the documentation, I found weird that "v" is described quite
differently for the 2 macros:
1) @param v Owner of the XSAVE area;
2) @param v           vCPU logically owning xsave_area

For "x" the documentation is "@param xsave_area  XSAVE blob of v", but
there's no "xsave_area" parameter.

(very minors, you can ignore)

Frediano

On Tue Oct 29, 2024 at 1:24 PM GMT, Frediano Ziglio wrote:
> On Tue, Oct 29, 2024 at 11:58 AM Alejandro Vallejo
> <alejandro.vallejo@cloud.com> wrote:
> >
> > Hi,
> >
> > 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);
> >   }
> >
> > Where:
> >   1. v->arch.xsave_area is a pointer to the XSAVE area on non-ASI domains.
> >   2. v->arch.xsave_area_pg an mfn (or a pointer to a page_info, converted)
> >   3. percpu_fixmap(v, PCPU_FIX_XSAVE_AREA) is a slot in a per-vCPU fixmap, that
> >      changes as we context switch from vCPU to vCPU.
> >
> >   /*
> >    * NOTE: Being a function this doesn't nullify the xsave_area pointer, but
> >    * it would in a macro. It's unimportant for the overall logic though.
> >    */
> >   void vcpu_unmap_xsave_area(struct vcpu *v, struct xsave_struct *xsave_area)
> >   {
> >       /* Catch mismatched areas when ASI is disabled */
> >       ASSERT(v->domain->asi || xsave_area == v->arch.xsave_area);
> >
> >       /* Likely some new vunmap-like abstraction after AMX */
> >       if ( v->domain->asi && v != current )
> >           unmap_domain_page(xsave_area);
> >   }
> >
> > Of course, many of these details hang in the balance of what happens to the ASI
> > series from Roger. In any case, the takeaway is that map/unmap must have
> > fastpaths for "current" that don't involve mapping. The assumption is that
> > non-current vCPUs are cold paths. In particular, context switches will undergo
> > some refactoring in order to make save/restore not require additional
> > map/unmaps besides the page table switch and yet another change to further
> > align "current" with the currently running page tables. Paths like the
> > instruction emulator go through these wrappers later on for ease of
> > auditability, but are early-returns that cause no major overhead.
> >
> > My expectation is that these macros are general enough to be tweakable in
> > whatever way is most suitable, thus allowing the refactor of the codebase at
> > large to make it ASI-friendly before the details of the ASI infra are merged,
> > or even finalised.
> >
> > >
> > > Having a macro-that-reads-like-a-function mutating x by name, rather
> > > than by pointer, is somewhat rude.  This is why we capitalise
> > > XFREE()/etc which have a similar pattern; to make it clear it's a macro
> > > and potentially doing weird things with scopes.
> > >
> > > ~Andrew
> >
> > That magic trick on unmap warrants uppercase, agreed. Initially it was all
> > function calls and after macrofying them I was lazy to change their users.
> >
> > Cheers,
> > Alejandro
> >
>
> Why not using static inline functions?

I'd rather use regular function in fact for the final thing. These ones aren't
to avoid headaches with const parameters and to allow nullifying the pointer
on exit without requiring a double pointer (which is doubly fun with const
involved).

As they gain more logic it's not impossible for them to be split in most
everything to be done in a function and the rest by the macro to avoid tons of
duplicate codegen everywhere.

Anyhow, all of that can be decided later driven by bloat checkers, benchmarks
and the like. My preference is also towards type-safety where possible.

>
> On the documentation, I found weird that "v" is described quite
> differently for the 2 macros:
> 1) @param v Owner of the XSAVE area;
> 2) @param v           vCPU logically owning xsave_area

(2) should have the content of (1). Will do in v2.

>
> For "x" the documentation is "@param xsave_area  XSAVE blob of v", but
> there's no "xsave_area" parameter.

True. I created these macros from previous functions, and there the parameter
was called xsave_area. It should be "x" here.

>
> (very minors, you can ignore)
>
> Frediano