Re: [Xen-devel] [PATCH 1/2] x86/xstate: Use the CPUID policy in valid_xcr0(), rather than allowing all features

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.07.18 at 14:30, <andrew.cooper3@xxxxxxxxxx> wrote:
> Backporting notes: This is safe in the restore case, but only back as far as
> the introduction of cpuid_policy infrastructure.  Before then, a restore
> boolean needs to be pumbed down as well, and used to select between the
> hardware maximum value and calls to {hvm,pv}_cpuid() to find the
> toolstack-chosen level.

So can I take this to mean that for ...

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1170,7 +1170,7 @@ long arch_do_domctl(
>              if ( _xcr0_accum )
>              {
>                  if ( evc->size >= PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
> -                    ret = validate_xstate(_xcr0, _xcr0_accum,
> +                    ret = validate_xstate(d, _xcr0, _xcr0_accum,

... this and ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1254,7 +1254,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, 
> hvm_domain_context_t *h)
>      ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>      h->cur += desc->length;
>  
> -    err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
> +    err = validate_xstate(d, ctxt->xcr0, ctxt->xcr0_accum,

... this there's no ordering issue on master (i.e. policy arrives before
register state)?

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -645,8 +645,16 @@ void xstate_init(struct cpuinfo_x86 *c)
>          BUG();
>  }
>  
> -static bool valid_xcr0(u64 xcr0)
> +static bool valid_xcr0(const struct domain *d, uint64_t xcr0)
>  {
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    uint64_t xcr0_max =
> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
> +
> +    /* Check xcr0 against the CPUID policy. */
> +    if ( xcr0 & ~xcr0_max )
> +        return false;
> +

I don't think this belongs here - the function's purpose is really to
do consistency checks on the xcr0 value passed in.

> @@ -670,14 +678,15 @@ static bool valid_xcr0(u64 xcr0)
>      return !(xcr0 & X86_XCR0_BNDREGS) == !(xcr0 & X86_XCR0_BNDCSR);
>  }
>  
> -int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
> +int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t 
> xcr0_accum,
> +                    const struct xsave_hdr *hdr)
>  {
>      unsigned int i;
>  
>      if ( (hdr->xstate_bv & ~xcr0_accum) ||
>           (xcr0 & ~xcr0_accum) ||
> -         !valid_xcr0(xcr0) ||
> -         !valid_xcr0(xcr0_accum) )
> +         !valid_xcr0(d, xcr0) ||
> +         !valid_xcr0(d, xcr0_accum) )
>          return -EINVAL;

I think you want to move the check here instead; there's also no point
to check it twice, as xcr0 can never have more bits set than xcr0_accum.

> @@ -699,13 +708,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>          return -EOPNOTSUPP;
>  
> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
> +    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(curr->domain, new_bv) )
>          return -EINVAL;
>  
> -    /* XCR0.PKRU is disabled on PV mode. */
> -    if ( is_pv_vcpu(curr) && (new_bv & X86_XCR0_PKRU) )
> -        return -EOPNOTSUPP;
> -

And you would do the full check here instead of the partial one you
remove.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel