Re: [PATCH v3 6/6] x86/msr: handle VMX MSRs with guest_rd/wrmsr()

[Sergey Dyasli <sergey.dyasli@citrix.com>]

On Fri, 2017-10-13 at 16:38 +0100, Andrew Cooper wrote:
> On 13/10/17 13:35, Sergey Dyasli wrote:
> > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > index a22e3dfaf2..2527fdd1d1 100644
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -426,6 +426,13 @@ int init_vcpu_msr_policy(struct vcpu *v)
> >      return 0;
> >  }
> >  
> > +#define vmx_guest_rdmsr(dp, name, msr)     \
> > +    case name:                             \
> > +        if ( !dp->msr.available )          \
> > +            goto gp_fault;                 \
> > +        *val = dp->msr.u.raw;              \
> > +        break;
> 
> Eww :(
> 
> For blocks of MSRs, it would be far better to go with the same structure
> as the cpuid policy.  Something like:
> 
> struct {
>     union {
>         uint64_t raw[NR_VMX_MSRS];
>         struct {
>             struct {
>                 ...
>             } basic;
>             struct {
>                 ...
>             } pinbased_ctls;
>         };
>     };
> } vmx;
> 
> This way, the guest_rdmsr() will be far more efficient.
> 
> case MSR_IA32_VMX_BASIC ... xxx:
>     if ( !cpuid->basic.vmx )
>         goto gp_fault;
>     *val = dp->vmx.raw[msr - MSR_IA32_VMX_BASIC];
>     break;
> 
> It would probably be worth splitting into a couple of different blocks
> based on the different availability checks.

I can understand an argument about removing available flags and getting
smaller msr policy's struct, but I fail to see how a big number of case
statements will make guest_rdmsr() inefficient. I expect a switch
statement to have O(log(N)) complexity which means it doesn't really
matter how many case statements there are.

Do you have some other performance concerns?

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel