[PATCH v1 0/6] VMX MSRs policy for Nested Virt: part 1

[PATCH v1 0/6] VMX MSRs policy for Nested Virt: part 1

[Sergey Dyasli]

The end goal of having VMX MSRs policy is to be able to manage
L1 VMX features. This patch series is the first part of this work.
There is no functional change to what L1 sees in VMX MSRs at this
point. But each domain will have a policy object which allows to
sensibly query what VMX features the domain has. This will unblock
some other nested virt work items.

Currently, when nested virt is enabled, the set of L1 VMX features
is fixed and calculated by nvmx_msr_read_intercept() as an intersection
between the full set of Xen's supported L1 VMX features, the set of
actual H/W features and, for MSR_IA32_VMX_EPT_VPID_CAP, the set of
features that Xen uses.

The above makes L1 VMX feature set inconsistent between different H/W
and there is no ability to control what features are available to L1.
The overall set of issues has much in common with CPUID policy.

Part 1 introduces struct vmx_msr_policy and the following instances:

* Raw policy (raw_vmx_msr_policy) -- the actual contents of H/W VMX MSRs
* HVM max policy (hvm_max_vmx_msr_policy) -- the end result of
                               nvmx_msr_read_intercept() on current H/W
* Per-domain policy (d->arch.vmx_msr) -- the copy of HVM max policy
                                         (for now)

There is no "Host policy" because Xen already has a set of variables
(vmx_pin_based_exec_control and others) which represent the set of
VMX features that Xen uses.  There are features that Xen doesn't use
(i.g. CPU_BASED_PAUSE_EXITING) but they are available to L1.  This makes
it not worthy to introduce "Host policy" at this stage.

Sergey Dyasli (6):
  vmx: add struct vmx_msr_policy
  vmx: add raw_vmx_msr_policy
  vmx: refactor vmx_init_vmcs_config()
  vvmx: add hvm_max_vmx_msr_policy
  vvmx: add per domain vmx msr policy
  vmx: print H/W VMX MSRs values during startup

 xen/arch/x86/domain.c              |   6 +
 xen/arch/x86/hvm/vmx/vmcs.c        | 639 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/vmx/vmx.c         |   4 +
 xen/arch/x86/hvm/vmx/vvmx.c        | 309 +++++++++---------
 xen/include/asm-x86/domain.h       |   2 +
 xen/include/asm-x86/hvm/vmx/vmcs.h | 346 ++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vvmx.h |   3 +
 7 files changed, 1070 insertions(+), 239 deletions(-)

-- 
2.11.0


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

[PATCH v1 1/6] vmx: add struct vmx_msr_policy

[Sergey Dyasli]

This structure provides a convenient way of accessing contents of
VMX MSRs: every bit value is accessible by its name.  Bit names match
existing Xen's definitions as close as possible.

The structure also contains the bitmap of available MSRs since not all
of them may be available on a particular H/W.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        |  47 +++++
 xen/include/asm-x86/hvm/vmx/vmcs.h | 344 +++++++++++++++++++++++++++++++++++++
 2 files changed, 391 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8103b20d29..e6ea197230 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -144,6 +144,14 @@ static void __init vmx_display_features(void)
         printk(" - none\n");
 }
 
+bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
+{
+    if ( msr < MSR_IA32_VMX_BASIC || msr > MSR_IA32_VMX_VMFUNC )
+        return 0;
+
+    return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
+}
+
 static u32 adjust_vmx_controls(
     const char *name, u32 ctl_min, u32 ctl_opt, u32 msr, bool_t *mismatch)
 {
@@ -1956,6 +1964,45 @@ void __init setup_vmcs_dump(void)
     register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
 }
 
+static void __init __maybe_unused build_assertions(void)
+{
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.basic) !=
+                 sizeof(raw_vmx_msr_policy.basic.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.pinbased_ctls) !=
+                 sizeof(raw_vmx_msr_policy.pinbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.procbased_ctls) !=
+                 sizeof(raw_vmx_msr_policy.procbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.exit_ctls) !=
+                 sizeof(raw_vmx_msr_policy.exit_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.entry_ctls) !=
+                 sizeof(raw_vmx_msr_policy.entry_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.misc) !=
+                 sizeof(raw_vmx_msr_policy.misc.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.cr0_fixed_0) !=
+                 sizeof(raw_vmx_msr_policy.cr0_fixed_0.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.cr0_fixed_1) !=
+                 sizeof(raw_vmx_msr_policy.cr0_fixed_1.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.cr4_fixed_0) !=
+                 sizeof(raw_vmx_msr_policy.cr4_fixed_0.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.cr4_fixed_1) !=
+                 sizeof(raw_vmx_msr_policy.cr4_fixed_1.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.vmcs_enum) !=
+                 sizeof(raw_vmx_msr_policy.vmcs_enum.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.procbased_ctls2) !=
+                 sizeof(raw_vmx_msr_policy.procbased_ctls2.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.ept_vpid_cap) !=
+                 sizeof(raw_vmx_msr_policy.ept_vpid_cap.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.true_pinbased_ctls) !=
+                 sizeof(raw_vmx_msr_policy.true_pinbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.true_procbased_ctls) !=
+                 sizeof(raw_vmx_msr_policy.true_procbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.true_exit_ctls) !=
+                 sizeof(raw_vmx_msr_policy.true_exit_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.true_entry_ctls) !=
+                 sizeof(raw_vmx_msr_policy.true_entry_ctls.raw));
+    BUILD_BUG_ON(sizeof(raw_vmx_msr_policy.vmfunc) !=
+                 sizeof(raw_vmx_msr_policy.vmfunc.raw));
+}
 
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index e3cdfdf576..fca1e62e4c 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -562,6 +562,350 @@ void vmx_domain_flush_pml_buffers(struct domain *d);
 
 void vmx_domain_update_eptp(struct domain *d);
 
+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;
+        bool    preempt_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 */
+        bool        virtual_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 */
+        bool            cr3_load_exiting:1;
+        bool           cr3_store_exiting:1;
+        uint32_t                        :2;  /* 17:18 reserved */
+        bool            cr8_load_exiting:1;
+        bool           cr8_store_exiting:1;
+        bool                tpr_shadow_0: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              ia32e_mode:1;
+        uint32_t                    :2;  /* 10:11 reserved */
+        bool   load_perf_global_ctrl:1;
+        uint32_t                    :2;  /* 13:14 reserved */
+        bool        ack_intr_on_exit:1;
+        uint32_t                    :2;  /* 16:17 reserved */
+        bool          save_guest_pat:1;
+        bool           load_host_pat:1;
+        bool         save_guest_efer:1;
+        bool          load_host_efer:1;
+        bool      save_preempt_timer:1;
+        bool           clear_bndcfgs:1;
+        bool conceal_vmexits_from_pt:1;
+        uint32_t                    :7;  /* 25:31 reserved */
+    };
+};
+
+union vmx_vmentry_control_bits {
+    uint32_t raw;
+    struct {
+        uint32_t                        :2;  /* 0:1 reserved */
+        bool           load_debug_cntrls:1;
+        uint32_t                        :6;  /* 3:8 reserved */
+        bool                  ia32e_mode:1;
+        bool                         smm:1;
+        bool          deact_dual_monitor:1;
+        uint32_t                        :1;  /* 12 reserved */
+        bool       load_perf_global_ctrl:1;
+        bool              load_guest_pat:1;
+        bool             load_guest_efer:1;
+        bool                load_bndcfgs:1;
+        bool   conceal_vmentries_from_pt:1;
+        uint32_t                        :14; /* 18:31 reserved */
+    };
+};
+
+union vmx_secondary_exec_control_bits {
+    uint32_t raw;
+    struct {
+        bool    virtualize_apic_accesses:1;
+        bool                  enable_ept:1;
+        bool    descriptor_table_exiting:1;
+        bool               enable_rdtscp:1;
+        bool      virtualize_x2apic_mode:1;
+        bool                 enable_vpid:1;
+        bool              wbinvd_exiting:1;
+        bool          unrestricted_guest:1;
+        bool          apic_register_virt:1;
+        bool       virtual_intr_delivery:1;
+        bool          pause_loop_exiting:1;
+        bool              rdrand_exiting:1;
+        bool              enable_invpcid:1;
+        bool         enable_vm_functions:1;
+        bool       enable_vmcs_shadowing:1;
+        bool               encls_exiting:1;
+        bool              rdseed_exiting:1;
+        bool                  enable_pml:1;
+        bool      enable_virt_exceptions:1;
+        bool conceal_vmx_nonroot_from_pt:1;
+        bool                      xsaves:1;
+        uint32_t                        :1;  /* 21 reserved */
+        bool   ept_mode_based_exec_cntrl:1;
+        uint32_t                        :2;  /* 23:24 reserved */
+        bool                 tsc_scaling:1;
+        uint32_t                        :6;  /* 26:31 reserved */
+    };
+};
+
+struct vmx_msr_policy
+{
+    /*
+     * Bitmap of readable MSRs, starting from MSR_IA32_VMX_BASIC,
+     * derived from contents of MSRs in this structure.
+     */
+    uint32_t available;
+
+    union {
+        uint64_t msr[MSR_IA32_VMX_VMFUNC - MSR_IA32_VMX_BASIC + 1];
+
+        struct {
+            /* MSR 0x480 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t vmcs_revision_id:31;
+                    bool                     :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;
+                    bool        default1_zero:1;
+                    uint32_t                 :8;  /* 56:63 reserved */
+                };
+            } basic;
+
+            /* MSR 0x481 */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_pin_based_exec_control_bits allowed_0;
+                    union vmx_pin_based_exec_control_bits allowed_1;
+                };
+            } pinbased_ctls;
+
+            /* MSR 0x482 */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_cpu_based_exec_control_bits allowed_0;
+                    union vmx_cpu_based_exec_control_bits allowed_1;
+                };
+            } procbased_ctls;
+
+            /* MSR 0x483 */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmexit_control_bits allowed_0;
+                    union vmx_vmexit_control_bits allowed_1;
+                };
+            } exit_ctls;
+
+            /* MSR 0x484 */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmentry_control_bits allowed_0;
+                    union vmx_vmentry_control_bits allowed_1;
+                };
+            } entry_ctls;
+
+            /* MSR 0x485 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t      preempt_timer_scale:5;
+                    bool            vmexit_stores_lma:1;
+                    bool           hlt_activity_state:1;
+                    bool      shutdown_activity_state:1;
+                    bool wait_for_sipi_activity_state:1;
+                    uint32_t                         :5;  /* 9:13 reserved */
+                    bool                    pt_in_vmx:1;
+                    bool          ia32_smbase_support:1;
+                    uint32_t               cr3_target:9;
+                    uint32_t       max_msr_load_count:3;
+                    bool    ia32_smm_monitor_ctl_bit2:1;
+                    bool                  vmwrite_all:1;
+                    bool           inject_ilen0_event:1;
+                    uint32_t                         :1;  /* 31 reserved */
+                    uint32_t         mseg_revision_id;
+                };
+            } misc;
+
+            /* MSR 0x486 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t allowed_0;
+                    uint32_t :32;
+                };
+            } cr0_fixed_0;
+
+            /* MSR 0x487 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t allowed_1;
+                    uint32_t :32;
+                };
+            } cr0_fixed_1;
+
+            /* MSR 0x488 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t allowed_0;
+                    uint32_t :32;
+                };
+            } cr4_fixed_0;
+
+            /* MSR 0x489 */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t allowed_1;
+                    uint32_t :32;
+                };
+            } cr4_fixed_1;
+
+            /* MSR 0x48A */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t                      :1;  /* 0 reserved */
+                    uint32_t vmcs_encoding_max_idx:9;
+                    uint64_t                      :54; /* 10:63 reserved */
+                };
+            } vmcs_enum;
+
+            /* MSR 0x48B */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_secondary_exec_control_bits allowed_0;
+                    union vmx_secondary_exec_control_bits allowed_1;
+                };
+            } procbased_ctls2;
+
+            /* MSR 0x48C */
+            union {
+                uint64_t raw;
+                struct {
+                    bool     exec_only_supported:1;
+                    uint32_t                    :5;  /* 1:5 reserved */
+                    bool walk_length_4_supported:1;
+                    uint32_t                    :1;  /* 7 reserved */
+                    bool          memory_type_uc:1;
+                    uint32_t                    :5;  /* 9:13 reserved */
+                    bool          memory_type_wb:1;
+                    uint32_t                    :1;  /* 15 reserved */
+                    bool           superpage_2mb:1;
+                    bool           superpage_1gb:1;
+                    uint32_t                    :2;  /* 18:19 reserved */
+                    bool      invept_instruction:1;
+                    bool                  ad_bit:1;
+                    bool advanced_ept_violations:1;
+                    uint32_t                    :2;  /* 23:24 reserved */
+                    bool   invept_single_context:1;
+                    bool      invept_all_context:1;
+                    uint32_t                    :5;  /* 27:31 reserved */
+                    bool     invvpid_instruction:1;
+                    uint32_t                    :7;  /* 33:39 reserved */
+                    bool invvpid_individual_addr:1;
+                    bool  invvpid_single_context:1;
+                    bool     invvpid_all_context:1;
+                    bool invvpid_single_context_retaining_global:1;
+                    uint32_t                    :20; /* 44:63 reserved */
+                };
+            } ept_vpid_cap;
+
+            /* MSR 0x48D */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_pin_based_exec_control_bits allowed_0;
+                    union vmx_pin_based_exec_control_bits allowed_1;
+                };
+            } true_pinbased_ctls;
+
+            /* MSR 0x48E */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_cpu_based_exec_control_bits allowed_0;
+                    union vmx_cpu_based_exec_control_bits allowed_1;
+                };
+            } true_procbased_ctls;
+
+            /* MSR 0x48F */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmexit_control_bits allowed_0;
+                    union vmx_vmexit_control_bits allowed_1;
+                };
+            } true_exit_ctls;
+
+            /* MSR 0x490 */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmentry_control_bits allowed_0;
+                    union vmx_vmentry_control_bits allowed_1;
+                };
+            } true_entry_ctls;
+
+            /* MSR 0x491 */
+            union {
+                uint64_t raw;
+                struct {
+                    bool eptp_switching:1;
+                };
+            } vmfunc;
+        };
+    };
+};
+
+bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr);
+
 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
 
 /*
-- 
2.11.0


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

Re: [PATCH v1 1/6] vmx: add struct vmx_msr_policy

[Jan Beulich]

>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,14 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
>  
> +bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)

const

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -562,6 +562,350 @@ void vmx_domain_flush_pml_buffers(struct domain *d);
>  
>  void vmx_domain_update_eptp(struct domain *d);
>  
> +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;
> +        bool    preempt_timer:1;
> +        bool posted_interrupt:1;
> +        uint32_t             :24; /* 8:31 reserved */

This mixture of bool and uint32_t worries me - I don't think the
resulting layout is well defined. Yes, you put suitable
BUILD_BUG_ON()s in place to catch possible issues, but anyway.

> +struct vmx_msr_policy
> +{
> +    /*
> +     * Bitmap of readable MSRs, starting from MSR_IA32_VMX_BASIC,
> +     * derived from contents of MSRs in this structure.
> +     */
> +    uint32_t available;
> +
> +    union {
> +        uint64_t msr[MSR_IA32_VMX_VMFUNC - MSR_IA32_VMX_BASIC + 1];

Considering the recurring use of MSR_IA32_VMX_VMFUNC,
wouldn't it be worthwhile to have a "last" #define? You'd then
clearly want to add a BUILD_BUG_ON() to vmx_msr_available()
making sure the delta doesn't grow beyond 32.

> +        struct {
> +            /* MSR 0x480 */

Please also give the msr-index.h name in the comment, for grep-s
to match here. In fact I'm unconvinced the hex index is of much use.

> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t vmcs_revision_id:31;
> +                    bool                     :1;  /* 31 always zero */

Name it mbz then?

> +            /* MSR 0x486 */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t allowed_0;
> +                    uint32_t :32;
> +                };
> +            } cr0_fixed_0;

I can't find any indication that this and the following MSRs have an
undefined upper half. The VMCS fields they correspond to are native
width, so I think the type here should be unsigned long.

Yet then the question arises whether breaking these up into bit
fields wouldn't be useful too. Of if that's no useful, is there really
a point in having both a "raw" and a properly named field?

> +            /* MSR 0x48A */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t                      :1;  /* 0 reserved */
> +                    uint32_t vmcs_encoding_max_idx:9;
> +                    uint64_t                      :54; /* 10:63 reserved */

This pairing of uint32_t and uint64_t looks even more worrying to
me than the bool/uint32_t one further up. I'm actually surprised
this doesn't cause the respective BUILD_BUG_ON() to trigger.

> +            /* MSR 0x491 */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    bool eptp_switching:1;
> +                };

Any reason the other 63 bits don't have a placeholder here, just like
you do everywhere else?

Jan


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

Re: [PATCH v1 1/6] vmx: add struct vmx_msr_policy

[Sergey Dyasli]

On Tue, 2017-07-04 at 07:57 -0600, Jan Beulich wrote:
> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> > 
> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -562,6 +562,350 @@ void vmx_domain_flush_pml_buffers(struct domain *d);
> >  
> >  void vmx_domain_update_eptp(struct domain *d);
> >  
> > +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;
> > +        bool    preempt_timer:1;
> > +        bool posted_interrupt:1;
> > +        uint32_t             :24; /* 8:31 reserved */
> 
> This mixture of bool and uint32_t worries me - I don't think the
> resulting layout is well defined. Yes, you put suitable
> BUILD_BUG_ON()s in place to catch possible issues, but anyway.

It was Andrew's suggestion to use bool because "It avoids subtle bugs like
foo.exec_only = (a & EXEC) truncating to zero". In the end it doesn't matter
which types are being used for bitfields, the layout depends only on the width.

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

Re: [PATCH v1 1/6] vmx: add struct vmx_msr_policy

[Jan Beulich]

>>> On 06.07.17 at 11:21, <sergey.dyasli@citrix.com> wrote:
> On Tue, 2017-07-04 at 07:57 -0600, Jan Beulich wrote:
>> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
>> > 
>> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> > @@ -562,6 +562,350 @@ void vmx_domain_flush_pml_buffers(struct domain *d);
>> >  
>> >  void vmx_domain_update_eptp(struct domain *d);
>> >  
>> > +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;
>> > +        bool    preempt_timer:1;
>> > +        bool posted_interrupt:1;
>> > +        uint32_t             :24; /* 8:31 reserved */
>> 
>> This mixture of bool and uint32_t worries me - I don't think the
>> resulting layout is well defined. Yes, you put suitable
>> BUILD_BUG_ON()s in place to catch possible issues, but anyway.
> 
> It was Andrew's suggestion to use bool because "It avoids subtle bugs like
> foo.exec_only = (a & EXEC) truncating to zero". In the end it doesn't matter
> which types are being used for bitfields, the layout depends only on the 
> width.

Okay, I've read the text again, and I now agree that using bool
ought to be fine.

Jan


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

Re: [PATCH v1 1/6] vmx: add struct vmx_msr_policy

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, June 26, 2017 6:45 PM
> 
> This structure provides a convenient way of accessing contents of
> VMX MSRs: every bit value is accessible by its name.  Bit names match
> existing Xen's definitions as close as possible.
> 
> The structure also contains the bitmap of available MSRs since not all
> of them may be available on a particular H/W.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        |  47 +++++
>  xen/include/asm-x86/hvm/vmx/vmcs.h | 344
> +++++++++++++++++++++++++++++++++++++
>  2 files changed, 391 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 8103b20d29..e6ea197230 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,14 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
> 
> +bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
> +{
> +    if ( msr < MSR_IA32_VMX_BASIC || msr > MSR_IA32_VMX_VMFUNC )
> +        return 0;
> +
> +    return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));

can you add a BUILD_BUG_ON to detect size of available won't be
overloaded?

[...]
> +
> +struct vmx_msr_policy
> +{
> +    /*
> +     * Bitmap of readable MSRs, starting from MSR_IA32_VMX_BASIC,
> +     * derived from contents of MSRs in this structure.
> +     */
> +    uint32_t available;
> +
> +    union {
> +        uint64_t msr[MSR_IA32_VMX_VMFUNC - MSR_IA32_VMX_BASIC + 1];
> +
> +        struct {
> +            /* MSR 0x480 */

use actual MSR name please

Thanks
Kevin

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

[PATCH v1 2/6] vmx: add raw_vmx_msr_policy

[Sergey Dyasli]

Add calculate_raw_policy() which fills raw_vmx_msr_policy (the actual
contents of H/W VMX MSRs) on the boot CPU.  On secondary CPUs, this
function checks that contents of VMX MSRs match the boot CPU's contents.

Remove lesser version of same-contents-check from vmx_init_vmcs_config().

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        | 130 +++++++++++++++++++++----------------
 xen/arch/x86/hvm/vmx/vmx.c         |   4 ++
 xen/include/asm-x86/hvm/vmx/vmcs.h |   2 +
 3 files changed, 79 insertions(+), 57 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index e6ea197230..00fbc0ccb8 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -144,6 +144,8 @@ static void __init vmx_display_features(void)
         printk(" - none\n");
 }
 
+struct vmx_msr_policy __read_mostly raw_vmx_msr_policy;
+
 bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
 {
     if ( msr < MSR_IA32_VMX_BASIC || msr > MSR_IA32_VMX_VMFUNC )
@@ -152,6 +154,74 @@ bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
     return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
 }
 
+int calculate_raw_policy(bool bsp)
+{
+    struct vmx_msr_policy policy;
+    struct vmx_msr_policy *p = &policy;
+    int msr;
+
+    /* Raw policy is filled only on boot CPU */
+    if ( bsp )
+        p = &raw_vmx_msr_policy;
+    else
+        memset(&policy, 0, sizeof(policy));
+
+    p->available = 0x7ff;
+    for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMCS_ENUM; msr++ )
+        rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
+
+    if ( p->basic.default1_zero )
+    {
+        p->available |= 0x1e000;
+        for ( msr = MSR_IA32_VMX_TRUE_PINBASED_CTLS;
+              msr <= MSR_IA32_VMX_TRUE_ENTRY_CTLS; msr++ )
+            rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
+    }
+
+    if ( p->procbased_ctls.allowed_1.activate_secondary_controls )
+    {
+        p->available |= 0x800;
+        msr = MSR_IA32_VMX_PROCBASED_CTLS2;
+        rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
+
+        if ( p->procbased_ctls2.allowed_1.enable_ept ||
+             p->procbased_ctls2.allowed_1.enable_vpid )
+        {
+            p->available |= 0x1000;
+            msr = MSR_IA32_VMX_EPT_VPID_CAP;
+            rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
+        }
+
+        if ( p->procbased_ctls2.allowed_1.enable_vm_functions )
+        {
+            p->available |= 0x20000;
+            msr = MSR_IA32_VMX_VMFUNC;
+            rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
+        }
+    }
+
+    /* Check that secondary CPUs have exactly the same bits in VMX MSRs */
+    if ( !bsp && memcmp(p, &raw_vmx_msr_policy, sizeof(*p)) != 0 )
+    {
+        for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMFUNC; msr++ )
+        {
+            if ( p->msr[msr - MSR_IA32_VMX_BASIC] !=
+                 raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] )
+            {
+                printk("VMX msr %#x: saw 0x%016"PRIx64" expected 0x%016"PRIx64
+                        "\n", msr, p->msr[msr - MSR_IA32_VMX_BASIC],
+                        raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC]);
+            }
+        }
+
+        printk("VMX: Capabilities fatally differ between CPU%d and boot CPU\n",
+               smp_processor_id());
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static u32 adjust_vmx_controls(
     const char *name, u32 ctl_min, u32 ctl_opt, u32 msr, bool_t *mismatch)
 {
@@ -173,13 +243,6 @@ static u32 adjust_vmx_controls(
     return ctl;
 }
 
-static bool_t cap_check(const char *name, u32 expected, u32 saw)
-{
-    if ( saw != expected )
-        printk("VMX %s: saw %#x expected %#x\n", name, saw, expected);
-    return saw != expected;
-}
-
 static int vmx_init_vmcs_config(void)
 {
     u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
@@ -412,56 +475,6 @@ static int vmx_init_vmcs_config(void)
             return -EINVAL;
         }
     }
-    else
-    {
-        /* Globals are already initialised: re-check them. */
-        mismatch |= cap_check(
-            "VMCS revision ID",
-            vmcs_revision_id, vmx_basic_msr_low & VMX_BASIC_REVISION_MASK);
-        mismatch |= cap_check(
-            "Pin-Based Exec Control",
-            vmx_pin_based_exec_control, _vmx_pin_based_exec_control);
-        mismatch |= cap_check(
-            "CPU-Based Exec Control",
-            vmx_cpu_based_exec_control, _vmx_cpu_based_exec_control);
-        mismatch |= cap_check(
-            "Secondary Exec Control",
-            vmx_secondary_exec_control, _vmx_secondary_exec_control);
-        mismatch |= cap_check(
-            "VMExit Control",
-            vmx_vmexit_control, _vmx_vmexit_control);
-        mismatch |= cap_check(
-            "VMEntry Control",
-            vmx_vmentry_control, _vmx_vmentry_control);
-        mismatch |= cap_check(
-            "EPT and VPID Capability",
-            vmx_ept_vpid_cap, _vmx_ept_vpid_cap);
-        mismatch |= cap_check(
-            "VMFUNC Capability",
-            vmx_vmfunc, _vmx_vmfunc);
-        if ( cpu_has_vmx_ins_outs_instr_info !=
-             !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)) )
-        {
-            printk("VMX INS/OUTS Instruction Info: saw %d expected %d\n",
-                   !!(vmx_basic_msr_high & (VMX_BASIC_INS_OUT_INFO >> 32)),
-                   cpu_has_vmx_ins_outs_instr_info);
-            mismatch = 1;
-        }
-        if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) !=
-             ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) )
-        {
-            printk("VMX: CPU%d unexpected VMCS size %Lu\n",
-                   smp_processor_id(),
-                   vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32));
-            mismatch = 1;
-        }
-        if ( mismatch )
-        {
-            printk("VMX: Capabilities fatally differ between CPU%d and CPU0\n",
-                   smp_processor_id());
-            return -EINVAL;
-        }
-    }
 
     /* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
     if ( vmx_basic_msr_high & (VMX_BASIC_32BIT_ADDRESSES >> 32) )
@@ -611,6 +624,9 @@ int vmx_cpu_up(void)
 
     BUG_ON(!(read_cr4() & X86_CR4_VMXE));
 
+    if ( (rc = calculate_raw_policy(false)) != 0 )
+        return rc;
+
     /* 
      * Ensure the current processor operating mode meets 
      * the requred CRO fixed bits in VMX operation. 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c53b24955a..f344d6a5ea 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2432,6 +2432,8 @@ static void pi_notification_interrupt(struct cpu_user_regs *regs)
     raise_softirq(VCPU_KICK_SOFTIRQ);
 }
 
+int calculate_raw_policy(bool bsp);
+
 static void __init lbr_tsx_fixup_check(void);
 static void __init bdw_erratum_bdf14_fixup_check(void);
 
@@ -2439,6 +2441,8 @@ const struct hvm_function_table * __init start_vmx(void)
 {
     set_in_cr4(X86_CR4_VMXE);
 
+    calculate_raw_policy(true);
+
     if ( vmx_cpu_up() )
     {
         printk("VMX: failed to initialise.\n");
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index fca1e62e4c..8b97f85c46 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -904,6 +904,8 @@ struct vmx_msr_policy
     };
 };
 
+extern struct vmx_msr_policy raw_vmx_msr_policy;
+
 bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr);
 
 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
-- 
2.11.0


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

Re: [PATCH v1 2/6] vmx: add raw_vmx_msr_policy

[Jan Beulich]

>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,8 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
>  
> +struct vmx_msr_policy __read_mostly raw_vmx_msr_policy;

Does this really need to be non-static? I don't see a use outside of
this file in the patch here at least.

> @@ -152,6 +154,74 @@ bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
>      return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
>  }
>  
> +int calculate_raw_policy(bool bsp)
> +{
> +    struct vmx_msr_policy policy;
> +    struct vmx_msr_policy *p = &policy;
> +    int msr;

unsigned int

> +    /* Raw policy is filled only on boot CPU */
> +    if ( bsp )
> +        p = &raw_vmx_msr_policy;
> +    else
> +        memset(&policy, 0, sizeof(policy));
> +
> +    p->available = 0x7ff;

(1u << (MSR_IA32_VMX_VMCS_ENUM + 1 - MSR_IA32_VMX_BASIC)) - 1

> +    for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMCS_ENUM; msr++ )
> +        rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
> +
> +    if ( p->basic.default1_zero )
> +    {
> +        p->available |= 0x1e000;

Same here and further down - please calculate the values from
available constants. Maybe you want to have a helper macro or
inline function.

> +    /* Check that secondary CPUs have exactly the same bits in VMX MSRs */
> +    if ( !bsp && memcmp(p, &raw_vmx_msr_policy, sizeof(*p)) != 0 )
> +    {
> +        for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMFUNC; msr++ )
> +        {
> +            if ( p->msr[msr - MSR_IA32_VMX_BASIC] !=
> +                 raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] )
> +            {
> +                printk("VMX msr %#x: saw 0x%016"PRIx64" expected 0x%016"PRIx64
> +                        "\n", msr, p->msr[msr - MSR_IA32_VMX_BASIC],

Please keep the newline on the same line as the rest of the format
string. It being slightly longer then really wanted is okay for format
strings.

> @@ -611,6 +624,9 @@ int vmx_cpu_up(void)
>  
>      BUG_ON(!(read_cr4() & X86_CR4_VMXE));
>  
> +    if ( (rc = calculate_raw_policy(false)) != 0 )
> +        return rc;
> +
>      /* 
>       * Ensure the current processor operating mode meets 
>       * the requred CRO fixed bits in VMX operation. 

Following here are reads of MSR_IA32_VMX_CR0_FIXED{0,1} which
you should be able to drop now, instead using the raw policy you've
just checked matches this CPU.

Btw., is it intentional that the function is being invoked for the BSP a
second time here (after start_vmx() did so already), with the flag
now being passed with the wrong value?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2432,6 +2432,8 @@ static void pi_notification_interrupt(struct cpu_user_regs *regs)
>      raise_softirq(VCPU_KICK_SOFTIRQ);
>  }
>  
> +int calculate_raw_policy(bool bsp);

Declarations need to go in a header included by both producer and
consumer, so that someone changing only one of definition and
declaration will be forced to also change the other side.

Jan


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

Re: [PATCH v1 2/6] vmx: add raw_vmx_msr_policy

[Sergey Dyasli]

On Tue, 2017-07-04 at 08:15 -0600, Jan Beulich wrote:
> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> > 
> > @@ -611,6 +624,9 @@ int vmx_cpu_up(void)
> >  
> >      BUG_ON(!(read_cr4() & X86_CR4_VMXE));
> >  
> > +    if ( (rc = calculate_raw_policy(false)) != 0 )
> > +        return rc;
> > +
> >      /* 
> >       * Ensure the current processor operating mode meets 
> >       * the requred CRO fixed bits in VMX operation. 
> 
> Btw., is it intentional that the function is being invoked for the BSP a
> second time here (after start_vmx() did so already), with the flag
> now being passed with the wrong value?

Unfortunately, I couldn't find a better way of detecting if the code is running
on the boot CPU. And I decided to use the existing practice of passing a flag.

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

Re: [PATCH v1 2/6] vmx: add raw_vmx_msr_policy

[Jan Beulich]

>>> On 06.07.17 at 11:29, <sergey.dyasli@citrix.com> wrote:
> On Tue, 2017-07-04 at 08:15 -0600, Jan Beulich wrote:
>> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
>> > 
>> > @@ -611,6 +624,9 @@ int vmx_cpu_up(void)
>> >  
>> >      BUG_ON(!(read_cr4() & X86_CR4_VMXE));
>> >  
>> > +    if ( (rc = calculate_raw_policy(false)) != 0 )
>> > +        return rc;
>> > +
>> >      /* 
>> >       * Ensure the current processor operating mode meets 
>> >       * the requred CRO fixed bits in VMX operation. 
>> 
>> Btw., is it intentional that the function is being invoked for the BSP a
>> second time here (after start_vmx() did so already), with the flag
>> now being passed with the wrong value?
> 
> Unfortunately, I couldn't find a better way of detecting if the code is running
> on the boot CPU. And I decided to use the existing practice of passing a flag.

This passing of a flag is fine; what I'm uncomfortable with is that the
second invocation on the BSP will say it's not on the BSP. While this
looks to be benign, it would feel better if there wasn't such a second
invocation at all.

Jan


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

[PATCH v1 3/6] vmx: refactor vmx_init_vmcs_config()

[Sergey Dyasli]

1. Remove RDMSRs of VMX MSRs since all values are already available in
   raw_vmx_msr_policy.
2. Replace bit operations involving VMX bitmasks with accessing VMX
   features by name and using vmx_msr_available() where appropriate.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 56 +++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 00fbc0ccb8..dbf6eb7433 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -227,7 +227,8 @@ static u32 adjust_vmx_controls(
 {
     u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
 
-    rdmsr(msr, vmx_msr_low, vmx_msr_high);
+    vmx_msr_low = raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC];
+    vmx_msr_high = raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] >> 32;
 
     ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
     ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
@@ -245,19 +246,16 @@ static u32 adjust_vmx_controls(
 
 static int vmx_init_vmcs_config(void)
 {
-    u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
+    u32 min, opt;
     u32 _vmx_pin_based_exec_control;
     u32 _vmx_cpu_based_exec_control;
     u32 _vmx_secondary_exec_control = 0;
     u64 _vmx_ept_vpid_cap = 0;
-    u64 _vmx_misc_cap = 0;
     u32 _vmx_vmexit_control;
     u32 _vmx_vmentry_control;
     u64 _vmx_vmfunc = 0;
     bool_t mismatch = 0;
 
-    rdmsr(MSR_IA32_VMX_BASIC, vmx_basic_msr_low, vmx_basic_msr_high);
-
     min = (PIN_BASED_EXT_INTR_MASK |
            PIN_BASED_NMI_EXITING);
     opt = (PIN_BASED_VIRTUAL_NMIS |
@@ -291,7 +289,7 @@ static int vmx_init_vmcs_config(void)
         _vmx_cpu_based_exec_control &=
             ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
 
-    if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
+    if ( vmx_msr_available(&raw_vmx_msr_policy, MSR_IA32_VMX_PROCBASED_CTLS2) )
     {
         min = 0;
         opt = (SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
@@ -305,8 +303,7 @@ static int vmx_init_vmcs_config(void)
                SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
                SECONDARY_EXEC_XSAVES |
                SECONDARY_EXEC_TSC_SCALING);
-        rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
-        if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
+        if ( raw_vmx_msr_policy.misc.vmwrite_all )
             opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
         if ( opt_vpid_enabled )
             opt |= SECONDARY_EXEC_ENABLE_VPID;
@@ -331,10 +328,9 @@ static int vmx_init_vmcs_config(void)
     }
 
     /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
-    if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
-                                        SECONDARY_EXEC_ENABLE_VPID) )
+    if ( vmx_msr_available(&raw_vmx_msr_policy, MSR_IA32_VMX_EPT_VPID_CAP) )
     {
-        rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, _vmx_ept_vpid_cap);
+        _vmx_ept_vpid_cap = raw_vmx_msr_policy.ept_vpid_cap.raw;
 
         if ( !opt_ept_ad )
             _vmx_ept_vpid_cap &= ~VMX_EPT_AD_BIT;
@@ -379,10 +375,14 @@ static int vmx_init_vmcs_config(void)
          * To use EPT we expect to be able to clear certain intercepts.
          * We check VMX_BASIC_MSR[55] to correctly handle default controls.
          */
-        uint32_t must_be_one, must_be_zero, msr = MSR_IA32_VMX_PROCBASED_CTLS;
-        if ( vmx_basic_msr_high & (VMX_BASIC_DEFAULT1_ZERO >> 32) )
-            msr = MSR_IA32_VMX_TRUE_PROCBASED_CTLS;
-        rdmsr(msr, must_be_one, must_be_zero);
+        uint32_t must_be_one = raw_vmx_msr_policy.procbased_ctls.allowed_0.raw;
+        uint32_t must_be_zero = raw_vmx_msr_policy.procbased_ctls.allowed_1.raw;
+        if ( vmx_msr_available(&raw_vmx_msr_policy,
+                               MSR_IA32_VMX_TRUE_PROCBASED_CTLS) )
+        {
+            must_be_one = raw_vmx_msr_policy.true_procbased_ctls.allowed_0.raw;
+            must_be_zero = raw_vmx_msr_policy.true_procbased_ctls.allowed_1.raw;
+        }
         if ( must_be_one & (CPU_BASED_INVLPG_EXITING |
                             CPU_BASED_CR3_LOAD_EXITING |
                             CPU_BASED_CR3_STORE_EXITING) )
@@ -423,9 +423,9 @@ static int vmx_init_vmcs_config(void)
         _vmx_pin_based_exec_control  &= ~ PIN_BASED_POSTED_INTERRUPT;
 
     /* The IA32_VMX_VMFUNC MSR exists only when VMFUNC is available */
-    if ( _vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VM_FUNCTIONS )
+    if ( vmx_msr_available(&raw_vmx_msr_policy, MSR_IA32_VMX_VMFUNC) )
     {
-        rdmsrl(MSR_IA32_VMX_VMFUNC, _vmx_vmfunc);
+        _vmx_vmfunc = raw_vmx_msr_policy.vmfunc.raw;
 
         /*
          * VMFUNC leaf 0 (EPTP switching) must be supported.
@@ -451,33 +451,31 @@ static int vmx_init_vmcs_config(void)
     if ( !vmx_pin_based_exec_control )
     {
         /* First time through. */
-        vmcs_revision_id           = vmx_basic_msr_low & VMX_BASIC_REVISION_MASK;
+        vmcs_revision_id           = raw_vmx_msr_policy.basic.vmcs_revision_id;
         vmx_pin_based_exec_control = _vmx_pin_based_exec_control;
         vmx_cpu_based_exec_control = _vmx_cpu_based_exec_control;
         vmx_secondary_exec_control = _vmx_secondary_exec_control;
         vmx_ept_vpid_cap           = _vmx_ept_vpid_cap;
         vmx_vmexit_control         = _vmx_vmexit_control;
         vmx_vmentry_control        = _vmx_vmentry_control;
-        vmx_basic_msr              = ((u64)vmx_basic_msr_high << 32) |
-                                     vmx_basic_msr_low;
+        vmx_basic_msr              = raw_vmx_msr_policy.basic.raw;
         vmx_vmfunc                 = _vmx_vmfunc;
         vmx_virt_exception         = !!(_vmx_secondary_exec_control &
                                        SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
         vmx_display_features();
 
         /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
-        if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) >
-             PAGE_SIZE )
+        if ( raw_vmx_msr_policy.basic.vmcs_region_size > PAGE_SIZE )
         {
-            printk("VMX: CPU%d VMCS size is too big (%Lu bytes)\n",
+            printk("VMX: CPU%d VMCS size is too big (%d bytes)\n",
                    smp_processor_id(),
-                   vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32));
+                   raw_vmx_msr_policy.basic.vmcs_region_size);
             return -EINVAL;
         }
     }
 
     /* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
-    if ( vmx_basic_msr_high & (VMX_BASIC_32BIT_ADDRESSES >> 32) )
+    if ( raw_vmx_msr_policy.basic.addresses_32bit )
     {
         printk("VMX: CPU%d limits VMX structure pointers to 32 bits\n",
                smp_processor_id());
@@ -485,9 +483,7 @@ static int vmx_init_vmcs_config(void)
     }
 
     /* Require Write-Back (WB) memory type for VMCS accesses. */
-    opt = (vmx_basic_msr_high & (VMX_BASIC_MEMORY_TYPE_MASK >> 32)) /
-          ((VMX_BASIC_MEMORY_TYPE_MASK & -VMX_BASIC_MEMORY_TYPE_MASK) >> 32);
-    if ( opt != MTRR_TYPE_WRBACK )
+    if ( raw_vmx_msr_policy.basic.memory_type != MTRR_TYPE_WRBACK )
     {
         printk("VMX: CPU%d has unexpected VMCS access type %u\n",
                smp_processor_id(), opt);
@@ -632,8 +628,8 @@ int vmx_cpu_up(void)
      * the requred CRO fixed bits in VMX operation. 
      */
     cr0 = read_cr0();
-    rdmsrl(MSR_IA32_VMX_CR0_FIXED0, vmx_cr0_fixed0);
-    rdmsrl(MSR_IA32_VMX_CR0_FIXED1, vmx_cr0_fixed1);
+    vmx_cr0_fixed0 = raw_vmx_msr_policy.cr0_fixed_0.raw;
+    vmx_cr0_fixed1 = raw_vmx_msr_policy.cr0_fixed_1.raw;
     if ( (~cr0 & vmx_cr0_fixed0) || (cr0 & ~vmx_cr0_fixed1) )
     {
         printk("CPU%d: some settings of host CR0 are " 
-- 
2.11.0


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

Re: [PATCH v1 3/6] vmx: refactor vmx_init_vmcs_config()

[Jan Beulich]

>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -227,7 +227,8 @@ static u32 adjust_vmx_controls(
>  {
>      u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
>  
> -    rdmsr(msr, vmx_msr_low, vmx_msr_high);
> +    vmx_msr_low = raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC];
> +    vmx_msr_high = raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] >> 32;

Please consider adding a helper macro or inline function for the
raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] sub-expression
(which likely is going to be usable elsewhere too, e.g. in patch 2).

> @@ -632,8 +628,8 @@ int vmx_cpu_up(void)
>       * the requred CRO fixed bits in VMX operation. 
>       */
>      cr0 = read_cr0();
> -    rdmsrl(MSR_IA32_VMX_CR0_FIXED0, vmx_cr0_fixed0);
> -    rdmsrl(MSR_IA32_VMX_CR0_FIXED1, vmx_cr0_fixed1);
> +    vmx_cr0_fixed0 = raw_vmx_msr_policy.cr0_fixed_0.raw;
> +    vmx_cr0_fixed1 = raw_vmx_msr_policy.cr0_fixed_1.raw;

Ah, here comes the change I did ask for. Judging from the title I
wasn't able to guess that, and I really don't mind in which of the
patches it happens.

Jan


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

Re: [PATCH v1 3/6] vmx: refactor vmx_init_vmcs_config()

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, June 26, 2017 6:45 PM
> 
> 1. Remove RDMSRs of VMX MSRs since all values are already available in
>    raw_vmx_msr_policy.
> 2. Replace bit operations involving VMX bitmasks with accessing VMX
>    features by name and using vmx_msr_available() where appropriate.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c | 56 +++++++++++++++++++++-------------------
> -----
>  1 file changed, 26 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 00fbc0ccb8..dbf6eb7433 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -227,7 +227,8 @@ static u32 adjust_vmx_controls(
>  {
>      u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
> 
> -    rdmsr(msr, vmx_msr_low, vmx_msr_high);
> +    vmx_msr_low = raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC];
> +    vmx_msr_high = raw_vmx_msr_policy.msr[msr -
> MSR_IA32_VMX_BASIC] >> 32;

also need check vmx_msr_available() here?

Thanks
Kevin

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

[PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Sergey Dyasli]

Currently, when nested virt is enabled, the set of L1 VMX features
is fixed and calculated by nvmx_msr_read_intercept() as an intersection
between the full set of Xen's supported L1 VMX features, the set of
actual H/W features and, for MSR_IA32_VMX_EPT_VPID_CAP, the set of
features that Xen uses.

Add hvm_max_vmx_msr_policy object which represents the end result of
nvmx_msr_read_intercept() on current H/W.  Most of the code is moved
from nvmx_msr_read_intercept() to calculate_hvm_max_policy() which is
called only once during the startup.

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

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c |   3 +
 xen/arch/x86/hvm/vmx/vvmx.c | 297 +++++++++++++++++++++-----------------------
 2 files changed, 147 insertions(+), 153 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index dbf6eb7433..da6ddf52f1 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -244,6 +244,8 @@ static u32 adjust_vmx_controls(
     return ctl;
 }
 
+void calculate_hvm_max_policy(void);
+
 static int vmx_init_vmcs_config(void)
 {
     u32 min, opt;
@@ -463,6 +465,7 @@ static int vmx_init_vmcs_config(void)
         vmx_virt_exception         = !!(_vmx_secondary_exec_control &
                                        SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
         vmx_display_features();
+        calculate_hvm_max_policy();
 
         /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
         if ( raw_vmx_msr_policy.basic.vmcs_region_size > PAGE_SIZE )
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 3560faec6d..657371ec69 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1941,6 +1941,8 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     return X86EMUL_OKAY;
 }
 
+struct vmx_msr_policy __read_mostly hvm_max_vmx_msr_policy;
+
 #define __emul_value(enable1, default1) \
     ((enable1 | default1) << 32 | (default1))
 
@@ -1948,6 +1950,134 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     (((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
     ((uint32_t)(__emul_value(enable1, default1) | host_value)))
 
+void __init calculate_hvm_max_policy(void)
+{
+    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
+    uint64_t data, *msr;
+    u32 default1_bits;
+
+    *p = raw_vmx_msr_policy;
+
+    /* XXX: vmcs_revision_id for nested virt */
+
+    /* Pinbased controls 1-settings */
+    data = PIN_BASED_EXT_INTR_MASK |
+           PIN_BASED_NMI_EXITING |
+           PIN_BASED_PREEMPT_TIMER;
+
+    msr = &p->msr[MSR_IA32_VMX_PINBASED_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_PINBASED_CTLS_DEFAULT1, *msr);
+    msr = &p->msr[MSR_IA32_VMX_TRUE_PINBASED_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_PINBASED_CTLS_DEFAULT1, *msr);
+
+    /* Procbased controls 1-settings */
+    default1_bits = VMX_PROCBASED_CTLS_DEFAULT1;
+    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 |
+           CPU_BASED_UNCOND_IO_EXITING |
+           CPU_BASED_RDTSC_EXITING |
+           CPU_BASED_MONITOR_TRAP_FLAG |
+           CPU_BASED_VIRTUAL_NMI_PENDING |
+           CPU_BASED_ACTIVATE_MSR_BITMAP |
+           CPU_BASED_PAUSE_EXITING |
+           CPU_BASED_RDPMC_EXITING |
+           CPU_BASED_TPR_SHADOW |
+           CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
+
+    msr = &p->msr[MSR_IA32_VMX_PROCBASED_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, default1_bits, *msr);
+
+    default1_bits &= ~(CPU_BASED_CR3_LOAD_EXITING |
+                       CPU_BASED_CR3_STORE_EXITING |
+                       CPU_BASED_INVLPG_EXITING);
+
+    msr = &p->msr[MSR_IA32_VMX_TRUE_PROCBASED_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, default1_bits, *msr);
+
+    /* Procbased-2 controls 1-settings */
+    data = SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
+           SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
+           SECONDARY_EXEC_ENABLE_VPID |
+           SECONDARY_EXEC_UNRESTRICTED_GUEST |
+           SECONDARY_EXEC_ENABLE_EPT;
+    msr = &p->msr[MSR_IA32_VMX_PROCBASED_CTLS2 - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, 0, *msr);
+
+    /* Vmexit controls 1-settings */
+    data = VM_EXIT_ACK_INTR_ON_EXIT |
+           VM_EXIT_IA32E_MODE |
+           VM_EXIT_SAVE_PREEMPT_TIMER |
+           VM_EXIT_SAVE_GUEST_PAT |
+           VM_EXIT_LOAD_HOST_PAT |
+           VM_EXIT_SAVE_GUEST_EFER |
+           VM_EXIT_LOAD_HOST_EFER |
+           VM_EXIT_LOAD_PERF_GLOBAL_CTRL;
+    msr = &p->msr[MSR_IA32_VMX_EXIT_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_EXIT_CTLS_DEFAULT1, *msr);
+    msr = &p->msr[MSR_IA32_VMX_TRUE_EXIT_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_EXIT_CTLS_DEFAULT1, *msr);
+
+    /* Vmentry controls 1-settings */
+    data = VM_ENTRY_LOAD_GUEST_PAT |
+           VM_ENTRY_LOAD_GUEST_EFER |
+           VM_ENTRY_LOAD_PERF_GLOBAL_CTRL |
+           VM_ENTRY_IA32E_MODE;
+    msr = &p->msr[MSR_IA32_VMX_ENTRY_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_ENTRY_CTLS_DEFAULT1, *msr);
+    msr = &p->msr[MSR_IA32_VMX_TRUE_ENTRY_CTLS - MSR_IA32_VMX_BASIC];
+    *msr = gen_vmx_msr(data, VMX_ENTRY_CTLS_DEFAULT1, *msr);
+
+    /* MSR_IA32_VMX_VMCS_ENUM */
+    /* The max index of VVMCS encoding is 0x1f. */
+    data = 0x1f << 1;
+    msr = &p->msr[MSR_IA32_VMX_VMCS_ENUM - MSR_IA32_VMX_BASIC];
+    *msr = data;
+
+    /* MSR_IA32_VMX_CR0_FIXED0 */
+    /* PG, PE bits must be 1 in VMX operation */
+    data = X86_CR0_PE | X86_CR0_PG;
+    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED0 - MSR_IA32_VMX_BASIC];
+    *msr = data;
+
+    /* MSR_IA32_VMX_CR0_FIXED1 */
+    /* allow 0-settings for all bits */
+    data = 0xffffffff;
+    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED1 - MSR_IA32_VMX_BASIC];
+    *msr = data;
+
+    /* MSR_IA32_VMX_CR4_FIXED0 */
+    /* VMXE bit must be 1 in VMX operation */
+    data = X86_CR4_VMXE;
+    msr = &p->msr[MSR_IA32_VMX_CR4_FIXED0 - MSR_IA32_VMX_BASIC];
+    *msr = data;
+
+    /* MSR_IA32_VMX_CR4_FIXED1 */
+    /* Treated dynamically */
+
+    /* MSR_IA32_VMX_MISC */
+    /* Do not support CR3-target feature now */
+    msr = &p->msr[MSR_IA32_VMX_MISC - MSR_IA32_VMX_BASIC];
+    *msr = *msr & ~VMX_MISC_CR3_TARGET;
+
+    /* MSR_IA32_VMX_EPT_VPID_CAP */
+    data = nept_get_ept_vpid_cap();
+    msr = &p->msr[MSR_IA32_VMX_EPT_VPID_CAP - MSR_IA32_VMX_BASIC];
+    *msr = data;
+
+    /* MSR_IA32_VMX_VMFUNC is N/A */
+    p->available &= ~0x20000;
+}
+
 /*
  * Capability reporting
  */
@@ -1955,171 +2085,32 @@ 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;
+    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
+    u64 data;
     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 )
+    /* TODO: disentangle feature control from nested virt */
+    if ( msr == MSR_IA32_FEATURE_CONTROL )
     {
-    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;
+        data = IA32_FEATURE_CONTROL_LOCK |
+               IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
+        *msr_content = data;
 
-    case MSR_IA32_VMX_VMFUNC:
-        if ( !cpu_has_vmx_vmfunc )
-            return 0;
-        break;
+        return r;
     }
 
-    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 & 0x7fffffff);
-        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 |
-               CPU_BASED_UNCOND_IO_EXITING |
-               CPU_BASED_RDTSC_EXITING |
-               CPU_BASED_MONITOR_TRAP_FLAG |
-               CPU_BASED_VIRTUAL_NMI_PENDING |
-               CPU_BASED_ACTIVATE_MSR_BITMAP |
-               CPU_BASED_PAUSE_EXITING |
-               CPU_BASED_RDPMC_EXITING |
-               CPU_BASED_TPR_SHADOW |
-               CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
-
-        if ( msr == MSR_IA32_VMX_TRUE_PROCBASED_CTLS )
-            default1_bits &= ~(CPU_BASED_CR3_LOAD_EXITING |
-                               CPU_BASED_CR3_STORE_EXITING |
-                               CPU_BASED_INVLPG_EXITING);
-
-        data = gen_vmx_msr(data, default1_bits, host_data);
-        break;
-    }
-    case MSR_IA32_VMX_PROCBASED_CTLS2:
-        /* 1-settings */
-        data = SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
-               SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
-               SECONDARY_EXEC_ENABLE_VPID |
-               SECONDARY_EXEC_UNRESTRICTED_GUEST |
-               SECONDARY_EXEC_ENABLE_EPT;
-        data = gen_vmx_msr(data, 0, host_data);
-        break;
-    case MSR_IA32_VMX_EXIT_CTLS:
-    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
-        /* 1-settings */
-        data = VM_EXIT_ACK_INTR_ON_EXIT |
-               VM_EXIT_IA32E_MODE |
-               VM_EXIT_SAVE_PREEMPT_TIMER |
-               VM_EXIT_SAVE_GUEST_PAT |
-               VM_EXIT_LOAD_HOST_PAT |
-               VM_EXIT_SAVE_GUEST_EFER |
-               VM_EXIT_LOAD_HOST_EFER |
-               VM_EXIT_LOAD_PERF_GLOBAL_CTRL;
-        data = gen_vmx_msr(data, VMX_EXIT_CTLS_DEFAULT1, host_data);
-        break;
-    case MSR_IA32_VMX_ENTRY_CTLS:
-    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-        /* 1-settings */
-        data = VM_ENTRY_LOAD_GUEST_PAT |
-               VM_ENTRY_LOAD_GUEST_EFER |
-               VM_ENTRY_LOAD_PERF_GLOBAL_CTRL |
-               VM_ENTRY_IA32E_MODE;
-        data = gen_vmx_msr(data, VMX_ENTRY_CTLS_DEFAULT1, host_data);
-        break;
+    if ( !vmx_msr_available(p, msr) )
+        return 0;
 
-    case MSR_IA32_FEATURE_CONTROL:
-        data = IA32_FEATURE_CONTROL_LOCK |
-               IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
-        break;
-    case MSR_IA32_VMX_VMCS_ENUM:
-        /* The max index of VVMCS encoding is 0x1f. */
-        data = 0x1f << 1;
-        break;
-    case MSR_IA32_VMX_CR0_FIXED0:
-        /* PG, PE bits must be 1 in VMX operation */
-        data = X86_CR0_PE | X86_CR0_PG;
-        break;
-    case MSR_IA32_VMX_CR0_FIXED1:
-        /* allow 0-settings for all bits */
-        data = 0xffffffff;
-        break;
-    case MSR_IA32_VMX_CR4_FIXED0:
-        /* VMXE bit must be 1 in VMX operation */
-        data = X86_CR4_VMXE;
-        break;
-    case MSR_IA32_VMX_CR4_FIXED1:
-        data = hvm_cr4_guest_valid_bits(v, 0);
-        break;
-    case MSR_IA32_VMX_MISC:
-        /* Do not support CR3-target feature now */
-        data = host_data & ~VMX_MISC_CR3_TARGET;
-        break;
-    case MSR_IA32_VMX_EPT_VPID_CAP:
-        data = nept_get_ept_vpid_cap();
-        break;
-    default:
-        r = 0;
-        break;
-    }
+    if ( msr == MSR_IA32_VMX_CR4_FIXED1 )
+        *msr_content = hvm_cr4_guest_valid_bits(v, 0);
+    else
+        *msr_content = p->msr[msr - MSR_IA32_VMX_BASIC];
 
-    *msr_content = data;
     return r;
 }
 
-- 
2.11.0


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

Re: [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Jan Beulich]

>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -244,6 +244,8 @@ static u32 adjust_vmx_controls(
>      return ctl;
>  }
>  
> +void calculate_hvm_max_policy(void);

As said for a prior patch, this once again needs to move to a header
which is also being included by the producer.

> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1941,6 +1941,8 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>      return X86EMUL_OKAY;
>  }
>  
> +struct vmx_msr_policy __read_mostly hvm_max_vmx_msr_policy;

Wouldn't vvmx_max_msr_policy be unambiguous enough a name,
but shorter to type?

> @@ -1948,6 +1950,134 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>      (((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
>      ((uint32_t)(__emul_value(enable1, default1) | host_value)))
>  
> +void __init calculate_hvm_max_policy(void)

This is not a suitable name for a VMX specific function. I should
have noticed and said this in patch 2 already, so please consider
it applicable there too.

> +{
> +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
> +    uint64_t data, *msr;
> +    u32 default1_bits;
> +
> +    *p = raw_vmx_msr_policy;
> +
> +    /* XXX: vmcs_revision_id for nested virt */

There was no such comment (presumably indicating something that
yet needs doing) in the old code - what's this about? Can't this be
implemented instead of such a comment be added?

> +    /* MSR_IA32_VMX_VMCS_ENUM */
> +    /* The max index of VVMCS encoding is 0x1f. */
> +    data = 0x1f << 1;
> +    msr = &p->msr[MSR_IA32_VMX_VMCS_ENUM - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR0_FIXED0 */
> +    /* PG, PE bits must be 1 in VMX operation */
> +    data = X86_CR0_PE | X86_CR0_PG;
> +    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED0 - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR0_FIXED1 */
> +    /* allow 0-settings for all bits */
> +    data = 0xffffffff;
> +    msr = &p->msr[MSR_IA32_VMX_CR0_FIXED1 - MSR_IA32_VMX_BASIC];
> +    *msr = data;
> +
> +    /* MSR_IA32_VMX_CR4_FIXED0 */
> +    /* VMXE bit must be 1 in VMX operation */
> +    data = X86_CR4_VMXE;
> +    msr = &p->msr[MSR_IA32_VMX_CR4_FIXED0 - MSR_IA32_VMX_BASIC];
> +    *msr = data;

I don't see a need for using "data" as an intermediate variable in any
of the three cases above.

> +    /* MSR_IA32_VMX_CR4_FIXED1 */
> +    /* Treated dynamically */
> +
> +    /* MSR_IA32_VMX_MISC */
> +    /* Do not support CR3-target feature now */
> +    msr = &p->msr[MSR_IA32_VMX_MISC - MSR_IA32_VMX_BASIC];
> +    *msr = *msr & ~VMX_MISC_CR3_TARGET;

&=

> +    /* MSR_IA32_VMX_EPT_VPID_CAP */
> +    data = nept_get_ept_vpid_cap();
> +    msr = &p->msr[MSR_IA32_VMX_EPT_VPID_CAP - MSR_IA32_VMX_BASIC];
> +    *msr = data;

No need to use "data" again.

> +    /* MSR_IA32_VMX_VMFUNC is N/A */
> +    p->available &= ~0x20000;

Please use an expression again here.

Jan


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

Re: [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Sergey Dyasli]

On Tue, 2017-07-04 at 09:04 -0600, Jan Beulich wrote:
> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> > 
> > +{
> > +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
> > +    uint64_t data, *msr;
> > +    u32 default1_bits;
> > +
> > +    *p = raw_vmx_msr_policy;
> > +
> > +    /* XXX: vmcs_revision_id for nested virt */
> 
> There was no such comment (presumably indicating something that
> yet needs doing) in the old code - what's this about? Can't this be
> implemented instead of such a comment be added?

Currently L1 sees vmcs_revision_id value from the H/W MSR. Which is
fine until live migration is concerned. The question is: what should
happen if L1 is migrated to some other H/W with different vmcs id?
One possible solution is to use "virtual vmcs id" in the policy object.

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

Re: [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Jan Beulich]

>>> On 06.07.17 at 12:23, <sergey.dyasli@citrix.com> wrote:
> On Tue, 2017-07-04 at 09:04 -0600, Jan Beulich wrote:
>> > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
>> > 
>> > +{
>> > +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
>> > +    uint64_t data, *msr;
>> > +    u32 default1_bits;
>> > +
>> > +    *p = raw_vmx_msr_policy;
>> > +
>> > +    /* XXX: vmcs_revision_id for nested virt */
>> 
>> There was no such comment (presumably indicating something that
>> yet needs doing) in the old code - what's this about? Can't this be
>> implemented instead of such a comment be added?
> 
> Currently L1 sees vmcs_revision_id value from the H/W MSR. Which is
> fine until live migration is concerned. The question is: what should
> happen if L1 is migrated to some other H/W with different vmcs id?
> One possible solution is to use "virtual vmcs id" in the policy object.

Are there any other (reasonable) ones, besides forbidding
migration (live or not). Otoh, if migration between hosts with
different IDs is allowed, won't we risk the page layout (which
is intentionally unknown to us) changing as well? Or in order
to be migrateable, such guests would have to be forced to
not use shadow VMCS, and we'd have to pin down (as part of
the guest ABI) the software layout we use.

Jan


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

Re: [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Sergey Dyasli]

On Thu, 2017-07-06 at 06:28 -0600, Jan Beulich wrote:
> > > > On 06.07.17 at 12:23, <sergey.dyasli@citrix.com> wrote:
> > 
> > On Tue, 2017-07-04 at 09:04 -0600, Jan Beulich wrote:
> > > > > > On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> > > > 
> > > > +{
> > > > +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
> > > > +    uint64_t data, *msr;
> > > > +    u32 default1_bits;
> > > > +
> > > > +    *p = raw_vmx_msr_policy;
> > > > +
> > > > +    /* XXX: vmcs_revision_id for nested virt */
> > > 
> > > There was no such comment (presumably indicating something that
> > > yet needs doing) in the old code - what's this about? Can't this be
> > > implemented instead of such a comment be added?
> > 
> > Currently L1 sees vmcs_revision_id value from the H/W MSR. Which is
> > fine until live migration is concerned. The question is: what should
> > happen if L1 is migrated to some other H/W with different vmcs id?
> > One possible solution is to use "virtual vmcs id" in the policy object.
> 
> Are there any other (reasonable) ones, besides forbidding
> migration (live or not). Otoh, if migration between hosts with
> different IDs is allowed, won't we risk the page layout (which
> is intentionally unknown to us) changing as well? Or in order
> to be migrateable, such guests would have to be forced to
> not use shadow VMCS, and we'd have to pin down (as part of
> the guest ABI) the software layout we use.

During a discussion with Andrew, we identified difficulties in migration
of an L1 hypervisor to a H/W with the different vmcs revision id when
VMCS shadowing is used.

It seems to be a reasonable requirement for migration to have H/W with
the same vmcs revision id. Therefore it is fine to provide L1 with
the real H/W id and I will remove that comment in v2.

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

Re: [PATCH v1 4/6] vvmx: add hvm_max_vmx_msr_policy

[Andrew Cooper]

On 07/07/17 14:01, Sergey Dyasli wrote:
> On Thu, 2017-07-06 at 06:28 -0600, Jan Beulich wrote:
>>>>> On 06.07.17 at 12:23, <sergey.dyasli@citrix.com> wrote:
>>> On Tue, 2017-07-04 at 09:04 -0600, Jan Beulich wrote:
>>>>>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
>>>>> +{
>>>>> +    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
>>>>> +    uint64_t data, *msr;
>>>>> +    u32 default1_bits;
>>>>> +
>>>>> +    *p = raw_vmx_msr_policy;
>>>>> +
>>>>> +    /* XXX: vmcs_revision_id for nested virt */
>>>> There was no such comment (presumably indicating something that
>>>> yet needs doing) in the old code - what's this about? Can't this be
>>>> implemented instead of such a comment be added?
>>> Currently L1 sees vmcs_revision_id value from the H/W MSR. Which is
>>> fine until live migration is concerned. The question is: what should
>>> happen if L1 is migrated to some other H/W with different vmcs id?
>>> One possible solution is to use "virtual vmcs id" in the policy object.
>> Are there any other (reasonable) ones, besides forbidding
>> migration (live or not). Otoh, if migration between hosts with
>> different IDs is allowed, won't we risk the page layout (which
>> is intentionally unknown to us) changing as well? Or in order
>> to be migrateable, such guests would have to be forced to
>> not use shadow VMCS, and we'd have to pin down (as part of
>> the guest ABI) the software layout we use.
> During a discussion with Andrew, we identified difficulties in migration
> of an L1 hypervisor to a H/W with the different vmcs revision id when
> VMCS shadowing is used.
>
> It seems to be a reasonable requirement for migration to have H/W with
> the same vmcs revision id. Therefore it is fine to provide L1 with
> the real H/W id and I will remove that comment in v2.

From the point of view of the L1 guest which is using nested-virt, it
reads VMX_BASIC at the start of day to get the revision id.  This is
just like reading the CPUID information at the start of day, and
expecting it to remain constant until the next reboot.

If Xen is fully shadowing the VMCS, and Xen's advertised revision ID
hasn't changed, then migration between different hardware is possible
(providing that the guest was first booted with all other VMX features
suitably levelled), as Xen has to shadow everything the guest vmwrote
anyway.

If Xen uses VMCS shadowing to speed up the L1 guests vmread/vmwrites,
then migration must be locked to hardware with the same revision id.  If
we don't fulfil this requirement, Xen wouldn't be able to (validly)
object to the L1 guest doing a vmptrld with the old revision ID, and
wouldn't be in a position (at all, let alone validly) to re-serialise
the VMCS in place to be suitable for the current hardware.

~Andrew

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

[PATCH v1 5/6] vvmx: add per domain vmx msr policy

[Sergey Dyasli]

Having a policy per domain allows to sensibly query what VMX features
the domain has, which unblocks some other nested virt work items.

For now, make policy for each domain equal to hvm_max_vmx_msr_policy.
In the future it should be possible to independently configure
the policy for each domain.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/domain.c              |  6 ++++++
 xen/arch/x86/hvm/vmx/vvmx.c        | 14 +++++++++++++-
 xen/include/asm-x86/domain.h       |  2 ++
 xen/include/asm-x86/hvm/vmx/vvmx.h |  3 +++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 49388f48d7..2a3518328e 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -419,6 +419,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     {
         d->arch.emulation_flags = 0;
         d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
+        d->arch.vmx_msr = ZERO_BLOCK_PTR;
     }
     else
     {
@@ -464,6 +465,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         if ( (rc = init_domain_cpuid_policy(d)) )
             goto fail;
 
+        if ( (rc = init_domain_vmx_msr_policy(d)) )
+            goto fail;
+
         d->arch.ioport_caps = 
             rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
         rc = -ENOMEM;
@@ -535,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     cleanup_domain_irq_mapping(d);
     free_xenheap_page(d->shared_info);
     xfree(d->arch.cpuid);
+    xfree(d->arch.vmx_msr);
     if ( paging_initialised )
         paging_final_teardown(d);
     free_perdomain_mappings(d);
@@ -549,6 +554,7 @@ void arch_domain_destroy(struct domain *d)
 
     xfree(d->arch.e820);
     xfree(d->arch.cpuid);
+    xfree(d->arch.vmx_msr);
 
     free_domain_pirqs(d);
     if ( !is_idle_domain(d) )
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 657371ec69..ae24dc4680 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2078,6 +2078,18 @@ void __init calculate_hvm_max_policy(void)
     p->available &= ~0x20000;
 }
 
+int init_domain_vmx_msr_policy(struct domain *d)
+{
+    d->arch.vmx_msr = xmalloc(struct vmx_msr_policy);
+
+    if ( !d->arch.vmx_msr )
+        return -ENOMEM;
+
+    *d->arch.vmx_msr = hvm_max_vmx_msr_policy;
+
+    return 0;
+}
+
 /*
  * Capability reporting
  */
@@ -2085,7 +2097,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
 {
     struct vcpu *v = current;
     struct domain *d = v->domain;
-    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
+    struct vmx_msr_policy *p = d->arch.vmx_msr;
     u64 data;
     int r = 1;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 924caac834..3cb753e46b 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -359,6 +359,8 @@ struct arch_domain
     /* CPUID Policy. */
     struct cpuid_policy *cpuid;
 
+    struct vmx_msr_policy *vmx_msr;
+
     struct PITState vpit;
 
     /* TSC management (emulation, pv, scaling, stats) */
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index ca2fb2535c..627112bea8 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -246,5 +246,8 @@ int nept_translate_l2ga(struct vcpu *v, paddr_t l2ga,
                         uint64_t *exit_qual, uint32_t *exit_reason);
 int nvmx_cpu_up_prepare(unsigned int cpu);
 void nvmx_cpu_dead(unsigned int cpu);
+
+int init_domain_vmx_msr_policy(struct domain *d);
+
 #endif /* __ASM_X86_HVM_VVMX_H__ */
 
-- 
2.11.0


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

Re: [PATCH v1 5/6] vvmx: add per domain vmx msr policy

[Jan Beulich]

>>> On 26.06.17 at 12:44, <sergey.dyasli@citrix.com> wrote:
> @@ -2085,7 +2097,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
>  {
>      struct vcpu *v = current;
>      struct domain *d = v->domain;
> -    struct vmx_msr_policy *p = &hvm_max_vmx_msr_policy;
> +    struct vmx_msr_policy *p = d->arch.vmx_msr;

I must have overlooked this in an earlier patch: This being the
read handler, you want to add const from the point on where
the variable gets being introduced. Apart from that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

[DEBUG PATCH 6/6] vmx: print H/W VMX MSRs values during startup

[Sergey Dyasli]

This is a debug patch I used when developing this series.
It's not intended for merging, I post it because it might be useful
to someone.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 405 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 405 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index da6ddf52f1..b142f29560 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -154,6 +154,408 @@ bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)
     return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
 }
 
+static char *vmx_msr_bit_status(u32 mask, u32 all_0, u32 all_1)
+{
+    if ( (all_0 & mask) && (all_1 & mask) )
+        return "1";
+    if ( !(all_0 & mask) && !(all_1 & mask) )
+        return "0";
+
+    return "0/1";
+}
+
+static char *btoa(uint32_t val)
+{
+    return val ? "yes" : "no";
+}
+
+static void print_vmx_basic_msr(struct vmx_msr_policy *p)
+{
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_BASIC",  p->basic.raw);
+    printk("  %-31s %#x\n", "VMCS revision:", p->basic.vmcs_revision_id);
+    printk("  %-31s %d\n", "VMCS/VMXON region size:",
+           p->basic.vmcs_region_size);
+    printk("  %-31s %s\n", "32-bit phys addr limit:",
+           btoa(p->basic.addresses_32bit));
+    printk("  %-31s %s\n", "Dual monitor mode:", btoa(p->basic.dual_monitor));
+    printk("  %-31s %d ", "VMCS memory type:", p->basic.memory_type);
+    switch ( p->basic.memory_type )
+    {
+        case MTRR_TYPE_UNCACHABLE:
+            printk("(Uncacheable)\n");
+            break;
+        case MTRR_TYPE_WRBACK:
+            printk("(Write Back)\n");
+            break;
+        default:
+            printk("(Unrecognized)\n");
+            break;
+    }
+    printk("  %-31s %s\n", "Report INS/OUTS VM exits:",
+           btoa(p->basic.ins_out_info));
+    printk("  %-31s %s\n", "Default1 CTLS clearable:",
+           btoa(p->basic.default1_zero));
+}
+
+static void print_vmx_msr_pinbased_bits(u32 all_0, u32 all_1)
+{
+    printk("  %-31s %s\n", "External-interrupt exiting:",
+           vmx_msr_bit_status(PIN_BASED_EXT_INTR_MASK, all_0, all_1));
+    printk("  %-31s %s\n", "NMI exiting:",
+           vmx_msr_bit_status(PIN_BASED_NMI_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Virtual NMIs:",
+           vmx_msr_bit_status(PIN_BASED_VIRTUAL_NMIS, all_0, all_1));
+    printk("  %-31s %s\n", "VMX-preemption timer:",
+           vmx_msr_bit_status(PIN_BASED_PREEMPT_TIMER, all_0, all_1));
+    printk("  %-31s %s\n", "Posted interrupts:",
+           vmx_msr_bit_status(PIN_BASED_POSTED_INTERRUPT, all_0, all_1));
+}
+
+static void print_vmx_msr_procbased_bits(u32 all_0, u32 all_1)
+{
+    printk("  %-31s %s\n", "Interrupt-window exiting:",
+           vmx_msr_bit_status(CPU_BASED_VIRTUAL_INTR_PENDING, all_0, all_1));
+    printk("  %-31s %s\n", "Use TSC offsetting:",
+           vmx_msr_bit_status(CPU_BASED_USE_TSC_OFFSETING, all_0, all_1));
+    printk("  %-31s %s\n", "HLT exiting:",
+           vmx_msr_bit_status(CPU_BASED_HLT_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "INVLPG exiting:",
+           vmx_msr_bit_status(CPU_BASED_INVLPG_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "MWAIT exiting:",
+           vmx_msr_bit_status(CPU_BASED_MWAIT_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "RDPMC exiting:",
+           vmx_msr_bit_status(CPU_BASED_RDPMC_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "RDTSC exiting:",
+           vmx_msr_bit_status(CPU_BASED_RDTSC_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "CR3-load exiting:",
+           vmx_msr_bit_status(CPU_BASED_CR3_LOAD_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "CR3-store exiting:",
+           vmx_msr_bit_status(CPU_BASED_CR3_STORE_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "CR8-load exiting:",
+           vmx_msr_bit_status(CPU_BASED_CR8_LOAD_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "CR8-store exiting:",
+           vmx_msr_bit_status(CPU_BASED_CR8_STORE_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Use TPR shadow:",
+           vmx_msr_bit_status(CPU_BASED_TPR_SHADOW, all_0, all_1));
+    printk("  %-31s %s\n", "NMI-window exiting:",
+           vmx_msr_bit_status(CPU_BASED_VIRTUAL_NMI_PENDING, all_0, all_1));
+    printk("  %-31s %s\n", "MOV-DR exiting:",
+           vmx_msr_bit_status(CPU_BASED_MOV_DR_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Unconditional I/O exiting:",
+           vmx_msr_bit_status(CPU_BASED_UNCOND_IO_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Use I/O bitmaps:",
+           vmx_msr_bit_status(CPU_BASED_ACTIVATE_IO_BITMAP, all_0, all_1));
+    printk("  %-31s %s\n", "Monitor trap flag:",
+           vmx_msr_bit_status(CPU_BASED_MONITOR_TRAP_FLAG, all_0, all_1));
+    printk("  %-31s %s\n", "Use MSR bitmaps:",
+           vmx_msr_bit_status(CPU_BASED_ACTIVATE_MSR_BITMAP, all_0, all_1));
+    printk("  %-31s %s\n", "MONITOR exiting:",
+           vmx_msr_bit_status(CPU_BASED_MONITOR_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "PAUSE exiting:",
+           vmx_msr_bit_status(CPU_BASED_PAUSE_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Activate secondary controls:",
+           vmx_msr_bit_status(CPU_BASED_ACTIVATE_SECONDARY_CONTROLS,
+                              all_0, all_1));
+}
+
+static void print_vmx_msr_exit_bits(u32 all_0, u32 all_1)
+{
+    printk("  %-31s %s\n", "Save debug controls:",
+           vmx_msr_bit_status(VM_EXIT_SAVE_DEBUG_CNTRLS, all_0, all_1));
+    printk("  %-31s %s\n", "Host address-space size:",
+           vmx_msr_bit_status(VM_EXIT_IA32E_MODE, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_PERF_GLOBAL_CTRL:",
+           vmx_msr_bit_status(VM_EXIT_LOAD_PERF_GLOBAL_CTRL, all_0, all_1));
+    printk("  %-31s %s\n", "Acknowledge interrupt on exit:",
+           vmx_msr_bit_status(VM_EXIT_ACK_INTR_ON_EXIT, all_0, all_1));
+    printk("  %-31s %s\n", "Save IA32_PAT:",
+           vmx_msr_bit_status(VM_EXIT_SAVE_GUEST_PAT, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_PAT:",
+           vmx_msr_bit_status(VM_EXIT_LOAD_HOST_PAT, all_0, all_1));
+    printk("  %-31s %s\n", "Save IA32_EFER:",
+           vmx_msr_bit_status(VM_EXIT_SAVE_GUEST_EFER, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_EFER:",
+           vmx_msr_bit_status(VM_EXIT_LOAD_HOST_EFER, all_0, all_1));
+    printk("  %-31s %s\n", "Save VMX-preempt timer value:",
+           vmx_msr_bit_status(VM_EXIT_SAVE_PREEMPT_TIMER, all_0, all_1));
+    printk("  %-31s %s\n", "Clear IA32_BNDCFGS:",
+           vmx_msr_bit_status(VM_EXIT_CLEAR_BNDCFGS, all_0, all_1));
+    printk("  %-31s %s\n", "Conceal VM exits from Intel PT:",
+           vmx_msr_bit_status(0x01000000, all_0, all_1));
+}
+
+static void print_vmx_msr_entry_bits(u32 all_0, u32 all_1)
+{
+    printk("  %-31s %s\n", "Load debug controls:",
+           vmx_msr_bit_status(0x00000004, all_0, all_1));
+    printk("  %-31s %s\n", "IA-32e mode guest:",
+           vmx_msr_bit_status(VM_ENTRY_IA32E_MODE, all_0, all_1));
+    printk("  %-31s %s\n", "Entry to SMM:",
+           vmx_msr_bit_status(VM_ENTRY_SMM, all_0, all_1));
+    printk("  %-31s %s\n", "Deactivate dual-mon treatment:",
+           vmx_msr_bit_status(VM_ENTRY_DEACT_DUAL_MONITOR, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_PERF_GLOBAL_CTRL:",
+           vmx_msr_bit_status(VM_ENTRY_LOAD_PERF_GLOBAL_CTRL, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_PAT:",
+           vmx_msr_bit_status(VM_ENTRY_LOAD_GUEST_PAT, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_EFER:",
+           vmx_msr_bit_status(VM_ENTRY_LOAD_GUEST_EFER, all_0, all_1));
+    printk("  %-31s %s\n", "Load IA32_BNDCFGS:",
+           vmx_msr_bit_status(VM_ENTRY_LOAD_BNDCFGS, all_0, all_1));
+    printk("  %-31s %s\n", "Conceal VM entries from PT:",
+           vmx_msr_bit_status(0x00020000, all_0, all_1));
+}
+
+static void print_vmx_misc_msr(struct vmx_msr_policy *p)
+{
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_MISC", p->misc.raw);
+    printk("  %-31s %d\n", "VMX-preemption timer scale:",
+           p->misc.preempt_timer_scale);
+    printk("  %-31s", "VM exit stores EFER LMA:");
+    printk(" %s\n", btoa(p->misc.vmexit_stores_lma));
+    printk("  %-31s", "HLT activity state:");
+    printk(" %s\n", btoa(p->misc.hlt_activity_state));
+    printk("  %-31s", "Shutdown activity state:");
+    printk(" %s\n", btoa(p->misc.shutdown_activity_state));
+    printk("  %-31s", "Wait-for-SIPI activity state:");
+    printk(" %s\n", btoa(p->misc.wait_for_sipi_activity_state));
+    printk("  %-31s", "Proc Trace in VMX entry:");
+    printk(" %s\n", btoa(p->misc.pt_in_vmx));
+    printk("  %-31s", "Read MSR_SMBASE in SMM:");
+    printk(" %s\n", btoa(p->misc.ia32_smbase_support));
+    printk("  %-31s", "CR3 targets:");
+    printk(" %d\n", p->misc.cr3_target);
+    printk("  %-31s", "Max MSR-load/store list:");
+    printk(" %d\n", p->misc.max_msr_load_count * 512);
+    printk("  %-31s", "VMXOFF must unblock SMIs:");
+    printk(" %s\n", btoa(p->misc.ia32_smm_monitor_ctl_bit2));
+    printk("  %-31s", "VMWRITE any MCS field:");
+    printk(" %s\n", btoa(p->misc.vmwrite_all));
+    printk("  %-31s", "VM entry SWI injection:");
+    printk(" %s\n", btoa(p->misc.inject_ilen0_event));
+    printk("  %-31s %#08x\n", "MSEG revision:", p->misc.mseg_revision_id);
+}
+
+static void print_vmx_msr_procbased2_bits(u32 all_0, u32 all_1)
+{
+    printk("  %-31s %s\n", "Virtualize APIC accesses:",
+           vmx_msr_bit_status(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "Enable EPT:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_EPT, all_0, all_1));
+    printk("  %-31s %s\n", "Descriptor-table exiting:",
+           vmx_msr_bit_status(SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "Enable RDTSCP:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_RDTSCP, all_0, all_1));
+    printk("  %-31s %s\n", "Virtualize x2APIC mode:",
+           vmx_msr_bit_status(SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "Enable VPID:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_VPID, all_0, all_1));
+    printk("  %-31s %s\n", "WBINVD exiting:",
+           vmx_msr_bit_status(SECONDARY_EXEC_WBINVD_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "Unrestricted guest:",
+           vmx_msr_bit_status(SECONDARY_EXEC_UNRESTRICTED_GUEST, all_0, all_1));
+    printk("  %-31s %s\n", "APIC-register virtualization:",
+           vmx_msr_bit_status(SECONDARY_EXEC_APIC_REGISTER_VIRT, all_0, all_1));
+    printk("  %-31s %s\n", "Virtual-interrupt delivery:",
+           vmx_msr_bit_status(SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "PAUSE-loop exiting:",
+           vmx_msr_bit_status(SECONDARY_EXEC_PAUSE_LOOP_EXITING, all_0, all_1));
+    printk("  %-31s %s\n", "RDRAND exiting:",
+           vmx_msr_bit_status(0x00000800, all_0, all_1));
+    printk("  %-31s %s\n", "Enable INVPCID:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_INVPCID, all_0, all_1));
+    printk("  %-31s %s\n", "Enable VM functions:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_VM_FUNCTIONS,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "VMCS shadowing:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_VMCS_SHADOWING,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "Enable ENCLS exiting:",
+           vmx_msr_bit_status(0x00008000, all_0, all_1));
+    printk("  %-31s %s\n", "RDSEED exiting:",
+           vmx_msr_bit_status(0x00010000, all_0, all_1));
+    printk("  %-31s %s\n", "Enable PML:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_PML, all_0, all_1));
+    printk("  %-31s %s\n", "EPT-violation #VE:",
+           vmx_msr_bit_status(SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS,
+                              all_0, all_1));
+    printk("  %-31s %s\n", "Conceal VMX non-root from PT:",
+           vmx_msr_bit_status(0x00080000, all_0, all_1));
+    printk("  %-31s %s\n", "Enable XSAVES/XRSTORS:",
+           vmx_msr_bit_status(SECONDARY_EXEC_XSAVES, all_0, all_1));
+    printk("  %-31s %s\n", "Mode-based exec ctrl for EPT:",
+           vmx_msr_bit_status(0x00400000, all_0, all_1));
+    printk("  %-31s %s\n", "Use TSC scaling:",
+           vmx_msr_bit_status(SECONDARY_EXEC_TSC_SCALING, all_0, all_1));
+}
+
+static void print_vmx_ept_vpid_cap_msr(struct vmx_msr_policy *p)
+{
+        printk("   %-31s", "EPT exec-only:");
+        printk("%s\n", btoa(p->ept_vpid_cap.exec_only_supported));
+        printk("   %-31s", "Page-walk length 4:");
+        printk("%s\n", btoa(p->ept_vpid_cap.walk_length_4_supported));
+        printk("   %-31s", "EPT uncacheable:");
+        printk("%s\n", btoa(p->ept_vpid_cap.memory_type_uc));
+        printk("   %-31s", "EPT write-back:");
+        printk("%s\n", btoa(p->ept_vpid_cap.memory_type_wb));
+        printk("   %-31s", "EPT PDE 2MB superpages:");
+        printk("%s\n", btoa(p->ept_vpid_cap.superpage_2mb));
+        printk("   %-31s", "EPT PDE 1GB superpages:");
+        printk("%s\n", btoa(p->ept_vpid_cap.superpage_1gb));
+        printk("   %-31s", "INVEPT instruction:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invept_instruction));
+        printk("   %-31s", "EPT accessed/dirty bits:");
+        printk("%s\n", btoa(p->ept_vpid_cap.ad_bit));
+        printk("   %-31s", "Advanced EPT violations:");
+        printk("%s\n", btoa(p->ept_vpid_cap.advanced_ept_violations));
+        printk("   %-31s", "INVEPT single-context:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invept_single_context));
+        printk("   %-31s", "INVEPT all-context:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invept_all_context));
+        printk("   %-31s", "INVVPID instruction:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invvpid_instruction));
+        printk("   %-31s", "INVVPID individual-addr:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invvpid_individual_addr));
+        printk("   %-31s", "INVVPID single-context:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invvpid_single_context));
+        printk("   %-31s", "INVVPID all-context:");
+        printk("%s\n", btoa(p->ept_vpid_cap.invvpid_all_context));
+        printk("   %-31s", "INVVPID sgl-ctx-ret-gbls:");
+        printk("%s\n",
+               btoa(p->ept_vpid_cap.invvpid_single_context_retaining_global));
+}
+
+static void print_vmx_msr_policy(struct vmx_msr_policy *policy)
+{
+    print_vmx_basic_msr(policy);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_PINBASED_CTLS",
+                              policy->pinbased_ctls.raw);
+    print_vmx_msr_pinbased_bits(policy->pinbased_ctls.allowed_0.raw,
+                                policy->pinbased_ctls.allowed_1.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_PROCBASED_CTLS",
+                              policy->procbased_ctls.raw);
+    print_vmx_msr_procbased_bits(policy->procbased_ctls.allowed_0.raw,
+                                 policy->procbased_ctls.allowed_1.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_EXIT_CTLS", policy->exit_ctls.raw);
+    print_vmx_msr_exit_bits(policy->exit_ctls.allowed_0.raw,
+                            policy->exit_ctls.allowed_1.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_ENTRY_CTLS",
+                              policy->entry_ctls.raw);
+    print_vmx_msr_entry_bits(policy->entry_ctls.allowed_0.raw,
+                             policy->entry_ctls.allowed_1.raw);
+
+    print_vmx_misc_msr(policy);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_CR0_FIXED0",
+                              policy->cr0_fixed_0.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_CR0_FIXED1",
+                              policy->cr0_fixed_1.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_CR4_FIXED0",
+                              policy->cr4_fixed_0.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_CR4_FIXED1",
+                              policy->cr4_fixed_1.raw);
+
+    printk("%-33s %#018lx\n", "MSR_IA32_VMX_VMCS_ENUM",
+                              policy->vmcs_enum.raw);
+    printk("  %-31s %#x\n", "Max VMCS encoding index:",
+                            policy->vmcs_enum.vmcs_encoding_max_idx);
+
+    printk("%-33s ", "MSR_IA32_VMX_PROCBASED_CTLS2");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_PROCBASED_CTLS2) )
+    {
+        printk("%#018lx\n", policy->procbased_ctls2.raw);
+        print_vmx_msr_procbased2_bits(policy->procbased_ctls2.allowed_0.raw,
+                                      policy->procbased_ctls2.allowed_1.raw);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_EPT_VPID_CAP");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_EPT_VPID_CAP) )
+    {
+        printk("%#018lx\n", policy->ept_vpid_cap.raw);
+        print_vmx_ept_vpid_cap_msr(policy);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_TRUE_PINBASED_CTLS");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_TRUE_PINBASED_CTLS) )
+    {
+        printk("%#018lx\n", policy->true_pinbased_ctls.raw);
+        print_vmx_msr_pinbased_bits(policy->true_pinbased_ctls.allowed_0.raw,
+                                    policy->true_pinbased_ctls.allowed_1.raw);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_TRUE_PROCBASED_CTLS");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_TRUE_PROCBASED_CTLS) )
+    {
+        printk("%#018lx\n", policy->true_procbased_ctls.raw);
+        print_vmx_msr_procbased_bits(policy->true_procbased_ctls.allowed_0.raw,
+                                     policy->true_procbased_ctls.allowed_1.raw);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_TRUE_EXIT_CTLS");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_TRUE_EXIT_CTLS) )
+    {
+        printk("%#018lx\n", policy->true_exit_ctls.raw);
+        print_vmx_msr_exit_bits(policy->true_exit_ctls.allowed_0.raw,
+                                policy->true_exit_ctls.allowed_1.raw);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_TRUE_ENTRY_CTLS");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_TRUE_ENTRY_CTLS) )
+    {
+        printk("%#018lx\n", policy->true_entry_ctls.raw);
+        print_vmx_msr_entry_bits(policy->true_entry_ctls.allowed_0.raw,
+                                 policy->true_entry_ctls.allowed_1.raw);
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+
+    printk("%-33s ", "MSR_IA32_VMX_VMFUNC");
+    if ( vmx_msr_available(policy, MSR_IA32_VMX_VMFUNC) )
+    {
+        printk("%#018lx\n", policy->vmfunc.raw);
+        printk("  %-31s %s\n", "EPTP switching:",
+                             btoa(policy->vmfunc.eptp_switching));
+    }
+    else
+    {
+        printk("N/A\n");
+    }
+}
+
 int calculate_raw_policy(bool bsp)
 {
     struct vmx_msr_policy policy;
@@ -219,6 +621,9 @@ int calculate_raw_policy(bool bsp)
         return -EINVAL;
     }
 
+    if ( bsp )
+        print_vmx_msr_policy(&raw_vmx_msr_policy);
+
     return 0;
 }
 
-- 
2.11.0


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

Re: [PATCH v1 0/6] VMX MSRs policy for Nested Virt: part 1

[Tian, Kevin]

> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, June 26, 2017 6:44 PM
> 
> The end goal of having VMX MSRs policy is to be able to manage
> L1 VMX features. This patch series is the first part of this work.
> There is no functional change to what L1 sees in VMX MSRs at this
> point. But each domain will have a policy object which allows to
> sensibly query what VMX features the domain has. This will unblock
> some other nested virt work items.
> 
> Currently, when nested virt is enabled, the set of L1 VMX features
> is fixed and calculated by nvmx_msr_read_intercept() as an intersection
> between the full set of Xen's supported L1 VMX features, the set of
> actual H/W features and, for MSR_IA32_VMX_EPT_VPID_CAP, the set of
> features that Xen uses.
> 
> The above makes L1 VMX feature set inconsistent between different H/W
> and there is no ability to control what features are available to L1.
> The overall set of issues has much in common with CPUID policy.
> 
> Part 1 introduces struct vmx_msr_policy and the following instances:
> 
> * Raw policy (raw_vmx_msr_policy) -- the actual contents of H/W VMX MSRs
> * HVM max policy (hvm_max_vmx_msr_policy) -- the end result of
>                                nvmx_msr_read_intercept() on current H/W
> * Per-domain policy (d->arch.vmx_msr) -- the copy of HVM max policy
>                                          (for now)

confirm here. So per-domain policy is what you plan to use to
solve inconsistency issue in the future? to make the description
complete, you may want to elaborate a bit for future usages
of new knobs.

> 
> There is no "Host policy" because Xen already has a set of variables
> (vmx_pin_based_exec_control and others) which represent the set of
> VMX features that Xen uses.  There are features that Xen doesn't use
> (i.g. CPU_BASED_PAUSE_EXITING) but they are available to L1.  This makes
> it not worthy to introduce "Host policy" at this stage.
> 
> Sergey Dyasli (6):
>   vmx: add struct vmx_msr_policy
>   vmx: add raw_vmx_msr_policy
>   vmx: refactor vmx_init_vmcs_config()
>   vvmx: add hvm_max_vmx_msr_policy
>   vvmx: add per domain vmx msr policy
>   vmx: print H/W VMX MSRs values during startup
> 
>  xen/arch/x86/domain.c              |   6 +
>  xen/arch/x86/hvm/vmx/vmcs.c        | 639
> ++++++++++++++++++++++++++++++++-----
>  xen/arch/x86/hvm/vmx/vmx.c         |   4 +
>  xen/arch/x86/hvm/vmx/vvmx.c        | 309 +++++++++---------
>  xen/include/asm-x86/domain.h       |   2 +
>  xen/include/asm-x86/hvm/vmx/vmcs.h | 346 ++++++++++++++++++++
>  xen/include/asm-x86/hvm/vmx/vvmx.h |   3 +
>  7 files changed, 1070 insertions(+), 239 deletions(-)
> 
> --
> 2.11.0


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