Re: [Xen-devel] [PATCH 1/6] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.01.19 at 13:02, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -308,11 +308,16 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>      return 1;
>  }
>  
> -bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val)
> +bool hvm_set_guest_bndcfgs(struct vcpu *v, uint64_t val)
>  {
> -    if ( !hvm_funcs.set_guest_bndcfgs ||
> -         !is_canonical_address(val) ||
> -         (val & IA32_BNDCFGS_RESERVED) )
> +    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> +
> +    if ( !cp->feat.mpx )

Is the local variable really worth it?

> +        return false;
> +
> +    ASSERT(hvm_funcs.set_guest_bndcfgs);

I think it would be better (more safe for release builds) if this was
left as part of the if(). Otherwise, ...

> @@ -342,7 +347,22 @@ bool hvm_set_guest_bndcfgs(struct vcpu *v, u64 val)
>              /* nothing, best effort only */;
>      }
>  
> -    return hvm_funcs.set_guest_bndcfgs(v, val);
> +    hvm_funcs.set_guest_bndcfgs(v, val);

... you risk calling NULL here.

> @@ -3472,12 +3494,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t 
> *msr_content)
>          *msr_content = v->arch.hvm.msr_xss;
>          break;
>  
> -    case MSR_IA32_BNDCFGS:
> -        if ( !d->arch.cpuid->feat.mpx ||
> -             !hvm_get_guest_bndcfgs(v, msr_content) )
> -            goto gp_fault;
> -        break;

Note how this already checks the CPUID policy, somewhat contrary to
what you say in the commit message. I think this wants to remain this
way in guest_{rd,wr}msr() - see all the other CPUID policy checks there.
That way hvm_get_guest_bndcfgs() can be made consistent with the
hook pointer type again, and it can remain an inline function as before.

Jan



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