Re: [RFC PATCH v2 10/10] x86/hvm: Enable XSAVES LBR save/restore

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.01.2025 09:45, Tu Dinh wrote:
> Add a new save code type CPU_XSAVES_CODE containing a compressed XSAVES
> image.
> 
> Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>

I'm afraid this way too little of a description here. First unanswered
question would be why it is that we need a new save code in the first
place. Second question then would be what the interaction is when both
old and new save records are present. After all aiui ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1238,6 +1238,36 @@ static int cf_check hvm_save_cpu_xsave_states(
>      return 0;
>  }
>  
> +#define HVM_CPU_XSAVES_SIZE(xcr0) (offsetof(struct hvm_hw_cpu_xsave, \
> +                                            save_area) + \
> +                                   xstate_compressed_size(xcr0))
> +
> +static int cf_check hvm_save_cpu_xsaves_states(
> +    struct vcpu *v, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_cpu_xsave *ctxt;
> +    unsigned int size;
> +    int err;
> +
> +    if ( !xsave_enabled(v) )
> +        return 0;   /* do nothing */
> +
> +    size = HVM_CPU_XSAVES_SIZE(v->arch.xcr0_accum);
> +    err = _hvm_init_entry(h, CPU_XSAVES_CODE, v->vcpu_id, size);
> +    if ( err )
> +        return err;
> +
> +    ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
> +    h->cur += size;
> +    ctxt->xfeature_mask = xfeature_mask;
> +    ctxt->xcr0 = v->arch.xcr0;
> +    ctxt->xcr0_accum = v->arch.xcr0_accum;
> +
> +    memcpy(&ctxt->save_area, v->arch.xsave_area, size);
> +
> +    return 0;
> +}

... you save all states under this new code, not just the XSS-controlled
ones. Plus you're going through all of this even if there are no XSS-
controlled components, i.e. in particular also when there's no XSAVES
support in hardware. This way you then end up saving twice the exact
same data, just differently arranged.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -946,8 +946,7 @@ int validate_xstate(const struct domain *d, uint64_t 
> xcr0, uint64_t xcr0_accum,
>           !valid_xcr0(xcr0_accum) )
>          return -EINVAL;
>  
> -    if ( (xcr0_accum & ~xfeature_mask) ||
> -         hdr->xcomp_bv )
> +    if ( xcr0_accum & ~xfeature_mask )
>          return -EOPNOTSUPP;
>  
>      for ( i = 0; i < ARRAY_SIZE(hdr->reserved); ++i )

Can you really merely delete the check? Don't you need to validate
non-zero ->xcomp_bv then instead?

Jan