Re: [RFC PATCH v2 06/10] x86: Enable XSTATE save/restore for arch LBR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.01.2025 09:45, Tu Dinh wrote:
> --- a/xen/arch/x86/include/asm/xstate.h
> +++ b/xen/arch/x86/include/asm/xstate.h
> @@ -33,13 +33,13 @@ extern uint32_t mxcsr_mask;
>  #define XSTATE_FP_SSE  (X86_XCR0_X87 | X86_XCR0_SSE)
>  #define XCNTXT_MASK    (X86_XCR0_X87 | X86_XCR0_SSE | X86_XCR0_YMM | \
>                          X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM | \
> -                        XSTATE_NONLAZY)
> +                        XSTATE_NONLAZY | XSTATE_XSAVES_ONLY)

This is odd - in the sole pre-existing place where the #define is used you
then remove X86_XSS_STATES again. Plus please see also
https://lists.xen.org/archives/html/xen-devel/2021-04/msg01336.html.

>  #define XSTATE_ALL     (~(1ULL << 63))
>  #define XSTATE_NONLAZY (X86_XCR0_BNDREGS | X86_XCR0_BNDCSR | X86_XCR0_PKRU | 
> \
>                          X86_XCR0_TILE_CFG | X86_XCR0_TILE_DATA)
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
> -#define XSTATE_XSAVES_ONLY         0
> +#define XSTATE_XSAVES_ONLY         (X86_XSS_LBR)
>  #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
>  
>  #define XSTATE_XSS     (1U << 0)
> @@ -91,6 +91,21 @@ struct xstate_bndcsr {
>      uint64_t bndstatus;
>  };
>  
> +struct xstate_lbr_entry {
> +    uint64_t lbr_from_ip;
> +    uint64_t lbr_to_ip;
> +    uint64_t lbr_info;
> +};
> +
> +struct xstate_lbr {
> +     uint64_t lbr_ctl;
> +     uint64_t lbr_depth;
> +     uint64_t ler_from_ip;
> +     uint64_t ler_to_ip;
> +     uint64_t ler_info;
> +     struct xstate_lbr_entry entries[32];
> +};

Doesn't this 32 appear in an earlier patch as well? They need tying together
via a #define then.

Also nit: Hard tabs slipped in.

> @@ -114,6 +129,9 @@ int xstate_alloc_save_area(struct vcpu *v);
>  void xstate_init(struct cpuinfo_x86 *c);
>  unsigned int xstate_uncompressed_size(uint64_t xcr0);
>  unsigned int xstate_compressed_size(uint64_t xstates);
> +void *get_xstate_component_comp(struct xsave_struct *xstate,
> +                                unsigned int size,
> +                                uint64_t component);
>  
>  static inline uint64_t xgetbv(unsigned int index)
>  {
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 289cf10b78..68a419ac8e 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -522,8 +522,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>          if ( !cp->xstate.xsaves )
>              goto gp_fault;
>  
> -        /* No XSS features currently supported for guests */
> -        if ( val != 0 )
> +        if ( val & ~(uint64_t)XSTATE_XSAVES_ONLY )
>              goto gp_fault;

Imo we'd be better off arranging for casts like this to not be required. It's
too easy to leave one out unintentionally.

> @@ -210,7 +214,7 @@ void expand_xsave_states(const struct vcpu *v, void 
> *dest, unsigned int size)
>       * non-compacted offset.
>       */
>      src = xstate;
> -    valid = xstate_bv & ~XSTATE_FP_SSE;
> +    valid = xstate_bv & ~XSTATE_FP_SSE & ~X86_XSS_STATES;
>      while ( valid )
>      {
>          u64 feature = valid & -valid;
> @@ -276,7 +280,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>       * possibly compacted offset.
>       */
>      dest = xstate;
> -    valid = xstate_bv & ~XSTATE_FP_SSE;
> +    valid = xstate_bv & ~XSTATE_FP_SSE & ~X86_XSS_STATES;
>      while ( valid )
>      {
>          u64 feature = valid & -valid;

I can't figure why these two changes are needed, and the description isn't
of any help here either.

> @@ -1072,6 +1085,30 @@ void xstate_set_init(uint64_t mask)
>          BUG();
>  }
>  
> +void *get_xstate_component_comp(struct xsave_struct *xstate,
> +                                unsigned int size,
> +                                uint64_t component)
> +{
> +    uint16_t comp_offsets[sizeof(xfeature_mask) * 8];
> +    uint16_t offset;
> +    unsigned int i = ilog2(component);
> +
> +    ASSERT(generic_hweightl(component) == 1);
> +
> +    if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) ||
> +         i >= xstate_features ||
> +         xstate_sizes[i] == 0 ||
> +         !(xstate->xsave_hdr.xcomp_bv & component) )
> +        return NULL;
> +
> +    setup_xstate_comp(comp_offsets, xstate->xsave_hdr.xcomp_bv);
> +    offset = comp_offsets[i];
> +    if ( xstate_sizes[i] + offset > size )
> +        return NULL;
> +
> +    return (void *)xstate + offset;
> +}

The function is unused at this point. Besides this being a Misra violation
(unreachable code), it also means it's left unclear what the purpose is.
That would, for example, influence whether there should be some "const"
applied. I find it particularly worrying that the function returns a
pointer to non-const, thus permitting the caller to fiddle with the
contents. Similarly it's left unclear what the "size" parameter is for.

Jan