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

[Sergey Dyasli]

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

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

[Andrew Cooper]

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.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Sergey Dyasli]

Now that each domain has a correct view of VMX MSRs in it's per-domain
MSR policy, it's possible to handle guest's RD/WRMSR with the new
handlers. Do it and remove the old nvmx_msr_read_intercept() and
associated bits.

There is no functional change to what a guest sees in VMX MSRs.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/hvm/vmx/vmx.c |   6 --
 xen/arch/x86/hvm/vmx/vvmx.c| 178 -
 xen/arch/x86/msr.c |  34 +++
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 -
 4 files changed, 34 insertions(+), 186 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c2148701ee..1a1cb98069 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2906,10 +2906,6 @@ static int vmx_msr_read_intercept(unsigned int msr, 
uint64_t *msr_content)
 if ( nestedhvm_enabled(curr->domain) )
 *msr_content |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
 break;
-case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
-if ( !nvmx_msr_read_intercept(msr, msr_content) )
-goto gp_fault;
-break;
 case MSR_IA32_MISC_ENABLE:
 rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
 /* Debug Trace Store is not supported. */
@@ -3133,8 +3129,6 @@ static int vmx_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 break;
 }
 case MSR_IA32_FEATURE_CONTROL:
-case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
-/* None of these MSRs are writeable. */
 goto gp_fault;
 
 case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index dde02c076b..b0474ad310 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1976,184 +1976,6 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 return X86EMUL_OKAY;
 }
 
-#define __emul_value(enable1, default1) \
-((enable1 | default1) << 32 | (default1))
-
-#define gen_vmx_msr(enable1, default1, host_value) \
-(((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
-((uint32_t)(__emul_value(enable1, default1) | host_value)))
-
-/*
- * Capability reporting
- */
-int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
-{
-struct vcpu *v = current;
-struct domain *d = v->domain;
-u64 data = 0, host_data = 0;
-int r = 1;
-
-/* VMX capablity MSRs are available only when guest supports VMX. */
-if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
-return 0;
-
-/*
- * These MSRs are only available when flags in other MSRs are set.
- * These prerequisites are listed in the Intel 64 and IA-32
- * Architectures Software Developer’s Manual, Vol 3, Appendix A.
- */
-switch ( msr )
-{
-case MSR_IA32_VMX_PROCBASED_CTLS2:
-if ( !cpu_has_vmx_secondary_exec_control )
-return 0;
-break;
-
-case MSR_IA32_VMX_EPT_VPID_CAP:
-if ( !(cpu_has_vmx_ept || cpu_has_vmx_vpid) )
-return 0;
-break;
-
-case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
-case MSR_IA32_VMX_TRUE_EXIT_CTLS:
-case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
-return 0;
-break;
-
-case MSR_IA32_VMX_VMFUNC:
-if ( !cpu_has_vmx_vmfunc )
-return 0;
-break;
-}
-
-rdmsrl(msr, host_data);
-
-/*
- * Remove unsupport features from n1 guest capability MSR
- */
-switch (msr) {
-case MSR_IA32_VMX_BASIC:
-{
-const struct vmcs_struct *vmcs =
-map_domain_page(_mfn(PFN_DOWN(v->arch.hvm_vmx.vmcs_pa)));
-
-data = (host_data & (~0ul << 32)) |
-   (vmcs->vmcs_revision_id & 0x7fff);
-unmap_domain_page(vmcs);
-break;
-}
-case MSR_IA32_VMX_PINBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-/* 1-settings */
-data = PIN_BASED_EXT_INTR_MASK |
-   PIN_BASED_NMI_EXITING |
-   PIN_BASED_PREEMPT_TIMER;
-data = gen_vmx_msr(data, VMX_PINBASED_CTLS_DEFAULT1, host_data);
-break;
-case MSR_IA32_VMX_PROCBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
-{
-u32 default1_bits = VMX_PROCBASED_CTLS_DEFAULT1;
-/* 1-settings */
-data = CPU_BASED_HLT_EXITING |
-   CPU_BASED_VIRTUAL_INTR_PENDING |
-   CPU_BASED_CR8_LOAD_EXITING |
-   CPU_BASED_CR8_STORE_EXITING |
-   CPU_BASED_INVLPG_EXITING |
-   CPU_BASED_CR3_LOAD_EXITING |
-   CPU_BASED_CR3_STORE_EXITING |
-   CPU_BASED_MONITOR_EXITING |
-   CPU_BASED_MWAIT_EXITING |
-   CPU_BASED_MOV_DR_EXITING |
-   CPU_BASED_ACTIVATE_IO_BITMAP |
-   CPU_BASED_USE_TSC_OFFSETING |
-