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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.03.19 at 19:09, <paul.durrant@xxxxxxxxxx> wrote:
> v2:
>  - Addressed comments from Jan by largely removing hunks

I think you've removed more than I was expecting, but I'm fine
this way.

> @@ -158,6 +158,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          ret = guest_rdmsr_x2apic(v, msr, val);
>          break;
>  
> +    case MSR_IA32_BNDCFGS:
> +        if ( !is_hvm_domain(d) || !cp->feat.mpx ||
> +             !hvm_get_guest_bndcfgs(v, val) )
> +            goto gp_fault;
> +
> +        break;
> +
>      case 0x40000000 ... 0x400001ff:
>          if ( is_viridian_domain(d) )
>          {
> @@ -319,6 +326,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>          ret = guest_wrmsr_x2apic(v, msr, val);
>          break;
>  
> +    case MSR_IA32_BNDCFGS:
> +        if ( !is_hvm_domain(d) || !cp->feat.mpx ||
> +             !hvm_set_guest_bndcfgs(v, val) )
> +            goto gp_fault;

In both cases the is_hvm_*() check looks to be redundant, as
for PV guests cp->feat.mpx can't be set. Personally I'd prefer
this to be an ASSERT() instead, but I'd listen to Andrew (as
the main author of this code) saying otherwise.

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -328,7 +328,7 @@ int init_vcpu_msr_policy(struct vcpu *v);
>   * These functions are also used by the migration logic, so need to cope with
>   * being used outside of v's context.
>   */
> -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val);
> +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);

I find this pretty undesirable, and I'd like to at least put out
for discussion a means how to avoid it: Any entity being
passed a const struct vcpu *cv can get hold of a non-const
one by doing

    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];

Of course this shouldn't be used arbitrarily, but to hide an
implementation detail like that of vmx_vmcs_enter() I think
this could be justified. Thoughts?

Jan



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