Re: [Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Sergey Dyasli]

On Mon, 2017-10-16 at 15:01 +0100, Andrew Cooper wrote:
> On 16/10/17 08:42, Sergey Dyasli wrote:
> > +
> > +secondary_available =
> > +dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls;
> > +
> > +switch (msr)
> > +{
> > +case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMCS_ENUM:
> > +return true;
> > +
> > +case MSR_IA32_VMX_PROCBASED_CTLS2:
> > +return secondary_available;
> > +
> > +case MSR_IA32_VMX_EPT_VPID_CAP:
> > +return ( secondary_available &&
> > + (dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
> > +  dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid) );
> 
> This check can be made more efficient in two ways.  First, use a bitwise
> rather than logical or, which allows both _ept and _vpid to be tested
> with a single instruction, rather than a conditional branch.

But it's compiler's job to optimize conditions like that.
I'm getting the following asm:

if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
82d08027bc3d:   48 c1 e8 20 shr$0x20,%rax
82d08027bc41:   a8 22   test   $0x22,%al
82d08027bc43:   74 0d   je 82d08027bc52 


And "test   $0x22" is exactly the test for "enable_ept || enable_vpid"
with a single instruction.

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

Re: [Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Andrew Cooper]

On 16/10/17 08:42, Sergey Dyasli wrote:
> On Fri, 2017-10-13 at 16:16 +0100, Andrew Cooper wrote:
>> On 13/10/17 13:35, Sergey Dyasli wrote:
>>> @@ -210,6 +375,255 @@ struct msr_domain_policy
>>>  bool available; /* This MSR is non-architectural */
>>>  bool cpuid_faulting;
>>>  } plaform_info;
>>> +
>>> +/* 0x0480  MSR_IA32_VMX_BASIC */
>>> +struct {
>>> +bool available;
>> We don't need available bits for any of these MSRs.  Their availability
>> is cpuid->basic.vmx, and we don't want (let alone need) to duplicate
>> information like this.
> Andrew,
>
> What do you think about the following way of checking the availability?

Preferably not.  You are duplicating a lot of information already
available in the guest_{rd,wr}msr(), and visually separating the
availability check from the data returned.  Worst however, is that you
risk having a mismatch between the MSR ranges which fall into this
check, and those which are calculated by it.

>
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 2527fdd1d1..828f1bb503 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -33,6 +33,43 @@ struct msr_domain_policy __read_mostly 
> raw_msr_domain_policy,
>  struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
> __read_mostly  pv_max_msr_vcpu_policy;
>  
> +bool msr_vmx_available(const struct domain *d, uint32_t msr)
> +{
> +const struct msr_domain_policy *dp = d->arch.msr;
> +bool secondary_available;
> +
> +if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
> +return false;

For now, we do need to double up the d->arch.cpuid->basic.vmx with
nestedhvm_enabled(d), but rest assured that nestedhvm_enabled(d) will be
disappearing in due course.  (It exists only because we don't have fine
grain toolstack control of CPUID/MSR values yet).

> +
> +secondary_available =
> +dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls;
> +
> +switch (msr)
> +{
> +case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMCS_ENUM:
> +return true;
> +
> +case MSR_IA32_VMX_PROCBASED_CTLS2:
> +return secondary_available;
> +
> +case MSR_IA32_VMX_EPT_VPID_CAP:
> +return ( secondary_available &&
> + (dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
> +  dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid) );

This check can be made more efficient in two ways.  First, use a bitwise
rather than logical or, which allows both _ept and _vpid to be tested
with a single instruction, rather than a conditional branch.

Secondly, the CPUID infrastructure has logic to flatten dependency
trees, so we don't need to encode logic paths like this.  In practice
however, you only read into the policy for details which match the
dependency tree, so you can drop the secondary_available check here, as
you know that if secondary_available is clear,
dp->vmx_procbased_ctls2.raw will be 0.

~Andrew

> +
> +case MSR_IA32_VMX_TRUE_PINBASED_CTLS ... MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> +return dp->vmx_basic.u.default1_zero;
> +
> +case MSR_IA32_VMX_VMFUNC:
> +return ( secondary_available &&
> + dp->vmx_procbased_ctls2.u.allowed_1.enable_vm_functions );
> +
> +default: break;
> +}
> +
> +return false;
> +}
> +
>  static void __init calculate_raw_vmx_policy(struct msr_domain_policy *dp)
>  {
>  if ( !cpu_has_vmx )
>


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

Re: [Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Sergey Dyasli]

On Fri, 2017-10-13 at 16:16 +0100, Andrew Cooper wrote:
> On 13/10/17 13:35, Sergey Dyasli wrote:
> > @@ -210,6 +375,255 @@ struct msr_domain_policy
> >  bool available; /* This MSR is non-architectural */
> >  bool cpuid_faulting;
> >  } plaform_info;
> > +
> > +/* 0x0480  MSR_IA32_VMX_BASIC */
> > +struct {
> > +bool available;
> 
> We don't need available bits for any of these MSRs.  Their availability
> is cpuid->basic.vmx, and we don't want (let alone need) to duplicate
> information like this.

Andrew,

What do you think about the following way of checking the availability?

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 2527fdd1d1..828f1bb503 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -33,6 +33,43 @@ struct msr_domain_policy __read_mostly 
raw_msr_domain_policy,
 struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
__read_mostly  pv_max_msr_vcpu_policy;
 
+bool msr_vmx_available(const struct domain *d, uint32_t msr)
+{
+const struct msr_domain_policy *dp = d->arch.msr;
+bool secondary_available;
+
+if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
+return false;
+
+secondary_available =
+dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls;
+
+switch (msr)
+{
+case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMCS_ENUM:
+return true;
+
+case MSR_IA32_VMX_PROCBASED_CTLS2:
+return secondary_available;
+
+case MSR_IA32_VMX_EPT_VPID_CAP:
+return ( secondary_available &&
+ (dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
+  dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid) );
+
+case MSR_IA32_VMX_TRUE_PINBASED_CTLS ... MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+return dp->vmx_basic.u.default1_zero;
+
+case MSR_IA32_VMX_VMFUNC:
+return ( secondary_available &&
+ dp->vmx_procbased_ctls2.u.allowed_1.enable_vm_functions );
+
+default: break;
+}
+
+return false;
+}
+
 static void __init calculate_raw_vmx_policy(struct msr_domain_policy *dp)
 {
 if ( !cpu_has_vmx )

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

Re: [Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Andrew Cooper]

On 13/10/17 13:35, Sergey Dyasli wrote:
> @@ -210,6 +375,255 @@ struct msr_domain_policy
>  bool available; /* This MSR is non-architectural */
>  bool cpuid_faulting;
>  } plaform_info;
> +
> +/* 0x0480  MSR_IA32_VMX_BASIC */
> +struct {
> +bool available;

We don't need available bits for any of these MSRs.  Their availability
is cpuid->basic.vmx, and we don't want (let alone need) to duplicate
information like this.

The PLATFORM_INFO and MISC_FEATURES_ENABLE are special, because they
have no architecturally defined indication of availability.

> +union {
> +uint64_t raw;
> +struct {
> +uint32_t vmcs_revision_id:31;
> +bool  mbz:1;  /* 31 always zero */
> +uint32_t vmcs_region_size:13;
> +uint32_t :3;  /* 45:47 reserved */
> +bool  addresses_32bit:1;
> +bool dual_monitor:1;
> +uint32_t  memory_type:4;
> +bool ins_out_info:1;
> +booldefault1_zero:1;
> +uint32_t :8;  /* 56:63 reserved */
> +};
> +} u;

The code will be rather shorter if you drop this .u and make the union
anonymous.

~Andrew

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

[Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Sergey Dyasli]

New definitions provide a convenient way of accessing contents of
VMX MSRs: every bit value is accessible by its name and there is a
"raw" 64-bit msr value. Bit names match existing Xen's definitions
as close as possible.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/msr.c|  42 +
 xen/include/asm-x86/msr.h | 414 ++
 2 files changed, 456 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9737ed706e..24029a2ac1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -216,6 +216,48 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 return X86EMUL_EXCEPTION;
 }
 
+static void __init __maybe_unused build_assertions(void)
+{
+struct msr_domain_policy p;
+
+BUILD_BUG_ON(sizeof(p.vmx_basic.u) !=
+ sizeof(p.vmx_basic.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_pinbased_ctls.u) !=
+ sizeof(p.vmx_pinbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_procbased_ctls.u) !=
+ sizeof(p.vmx_procbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_exit_ctls.u) !=
+ sizeof(p.vmx_exit_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_entry_ctls.u) !=
+ sizeof(p.vmx_entry_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_misc.u) !=
+ sizeof(p.vmx_misc.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr0_fixed0.u) !=
+ sizeof(p.vmx_cr0_fixed0.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr0_fixed1.u) !=
+ sizeof(p.vmx_cr0_fixed1.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr4_fixed0.u) !=
+ sizeof(p.vmx_cr4_fixed0.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr4_fixed1.u) !=
+ sizeof(p.vmx_cr4_fixed1.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_vmcs_enum.u) !=
+ sizeof(p.vmx_vmcs_enum.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_procbased_ctls2.u) !=
+ sizeof(p.vmx_procbased_ctls2.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_ept_vpid_cap.u) !=
+ sizeof(p.vmx_ept_vpid_cap.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_pinbased_ctls.u) !=
+ sizeof(p.vmx_true_pinbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_procbased_ctls.u) !=
+ sizeof(p.vmx_true_procbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_exit_ctls.u) !=
+ sizeof(p.vmx_true_exit_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_entry_ctls.u) !=
+ sizeof(p.vmx_true_entry_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_vmfunc.u) !=
+ sizeof(p.vmx_vmfunc.u.raw));
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 751fa25a36..fc99612cca 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -202,6 +202,171 @@ void write_efer(u64 val);
 
 DECLARE_PER_CPU(u32, ler_msr);
 
+union vmx_pin_based_exec_control_bits {
+uint32_t raw;
+struct {
+bool ext_intr_exiting:1;
+uint32_t :2;  /* 1:2 reserved */
+bool  nmi_exiting:1;
+uint32_t :1;  /* 4 reserved */
+bool virtual_nmis:1;
+boolpreempt_timer:1;
+bool posted_interrupt:1;
+uint32_t :24; /* 8:31 reserved */
+};
+};
+
+union vmx_cpu_based_exec_control_bits {
+uint32_t raw;
+struct {
+uint32_t:2;  /* 0:1 reserved */
+boolvirtual_intr_pending:1;
+bool   use_tsc_offseting:1;
+uint32_t:3;  /* 4:6 reserved */
+bool hlt_exiting:1;
+uint32_t:1;  /* 8 reserved */
+bool  invlpg_exiting:1;
+bool   mwait_exiting:1;
+bool   rdpmc_exiting:1;
+bool   rdtsc_exiting:1;
+uint32_t:2;  /* 13:14 reserved */
+boolcr3_load_exiting:1;
+bool   cr3_store_exiting:1;
+uint32_t:2;  /* 17:18 reserved */
+boolcr8_load_exiting:1;
+bool   cr8_store_exiting:1;
+bool  tpr_shadow:1;
+bool virtual_nmi_pending:1;
+bool  mov_dr_exiting:1;
+bool   uncond_io_exiting:1;
+bool  activate_io_bitmap:1;
+uint32_t:1;  /* 26 reserved */
+bool   monitor_trap_flag:1;
+bool activate_msr_bitmap:1;
+bool monitor_exiting:1;
+bool   pause_exiting:1;
+bool activate_secondary_controls:1;
+};
+};
+
+union vmx_vmexit_control_bits {
+uint32_t raw;
+struct {
+uint32_t:2;  /* 0:1 reserved */
+bool   save_debug_cntrls:1;
+uint32_t:6;  /* 3:8 reserved */
+bool