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.d...@vates.tech>
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
