Re: [Xen-devel] [PATCH v4 6/7] X86: MSR_IA32_BNDCFGS save/restore

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.12.13 at 09:54, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -580,13 +580,61 @@ static int vmx_load_vmcs_ctxt(struct vcpu *v, struct 
> hvm_hw_cpu *ctxt)
>      return 0;
>  }
>  
> -/* Temporarily NULL, could be added in the future */
> +static bool_t vmx_has_mpx(void)
> +{
> +    unsigned int eax, ebx, edx, ecx = 0;
> +
> +    hvm_cpuid(7, &eax, &ebx, &ecx, &edx);

hvm_cpuid() accepts NULL now for outputs you don't care about.

> +
> +    return !!(ebx & cpufeat_mask(X86_FEATURE_MPX));
> +}

And then this function clearly isn't VMX-specific. But with the above
I don't think you need a separate function here in the first place.

> +
> +static void vmx_save_msr_bndcfgs(struct vcpu *v, struct hvm_msr *ctxt)
> +{
> +    if ( !vmx_has_mpx() )
> +        return;
> +
> +    ctxt->msr[ctxt->count].index = MSR_IA32_BNDCFGS;
> +    vmx_vmcs_enter(v);
> +    __vmread(GUEST_BNDCFGS, &ctxt->msr[ctxt->count++].val);
> +    vmx_vmcs_exit(v);
> +}
> +
> +static int vmx_load_msr_bndcfgs(struct vcpu *v, uint64_t val)
> +{
> +    if ( !vmx_has_mpx() )
> +        return -EINVAL;
> +
> +    vmx_vmcs_enter(v);
> +    __vmwrite(GUEST_BNDCFGS, val);
> +    vmx_vmcs_exit(v);
> +
> +    return 0;
> +}

This looks inefficient - I'd assume that (almost) all MSRs that VMX
specific code would need to deal with will go into a VMCS field.
Hence entering/exiting the VMCS for each individual MSR seems
odd.

And once again, with that addressed there's hardly a reason not
to integrate these into their callers.

>  static int vmx_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
>  {
> +    int i;
> +
> +    for ( i = 0; i < ctxt->count; i++ )
> +    {
> +        switch ( ctxt->msr[i].index )
> +        {
> +        case MSR_IA32_BNDCFGS:
> +            if ( vmx_load_msr_bndcfgs(v, ctxt->msr[i].val) )
> +                return -EINVAL;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
>      return 0;
>  }

This points out a shortcoming of the interface: Generic code ought
to have a way to know which of the incoming records were
processed by the vendor specific code. Then, after processing
eventual vendor independent records, it'll know whether all records
got consumed. And if not, the restore operation _must_ fail.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel