[PATCH v4 0/7] VMX MSRs policy for Nested Virt: part 1

[PATCH v4 0/7] 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 virtualization 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 adds VMX MSRs into struct msr_domain_policy and initializes them
during domain creation based on CPUID policy. In the future it should be
possible to independently configure values of VMX MSRs for each domain.

v3 --> v4:
- VMX MSRs are now separated into 5 logical blocks
- Per MSR availability flags are dropped
- Separate patch for hvm_cr4_guest_valid_bits() is added

v2 --> v3:
- Rebase on top of Generic MSR Policy
- Each VMX MSR now has its own availability flag
- VMX MSRs are now completely defined during domain creation
  (all CPUID policy changes are taken into account)

Sergey Dyasli (7):
  x86/msr: add Raw and Host domain policies
  x86/msr: add VMX MSRs into struct msr_domain_policy
  x86/msr: read VMX MSRs values into Raw policy
  x86/msr: add VMX MSRs into HVM_max domain policy
  x86/cpuid: update signature of hvm_cr4_guest_valid_bits()
  x86/msr: update domain policy on CPUID policy changes
  x86/msr: handle VMX MSRs with guest_rd/wrmsr()

 xen/arch/x86/domctl.c              |   1 +
 xen/arch/x86/hvm/domain.c          |   3 +-
 xen/arch/x86/hvm/hvm.c             |   7 +-
 xen/arch/x86/hvm/svm/svmdebug.c    |   4 +-
 xen/arch/x86/hvm/vmx/vmx.c         |   6 -
 xen/arch/x86/hvm/vmx/vvmx.c        | 178 ------------------
 xen/arch/x86/msr.c                 | 343 ++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/hvm/hvm.h      |   2 +-
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 -
 xen/include/asm-x86/msr.h          | 376 +++++++++++++++++++++++++++++++++++++
 10 files changed, 727 insertions(+), 195 deletions(-)

-- 
2.11.0


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

[PATCH v4 1/7] x86/msr: add Raw and Host domain policies

[Sergey Dyasli]

Raw policy contains the actual values from H/W MSRs. PLATFORM_INFO msr
needs to be read again because probe_intel_cpuid_faulting() records
the presence of X86_FEATURE_CPUID_FAULTING but not the presence of msr
itself (if cpuid faulting is not available).

Host policy might have certain features disabled if Xen decides not
to use them. For now, make Host policy equal to Raw policy.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/msr.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index baba44f43d..9737ed706e 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,12 +24,34 @@
 #include <xen/sched.h>
 #include <asm/msr.h>
 
-struct msr_domain_policy __read_mostly hvm_max_msr_domain_policy,
+struct msr_domain_policy __read_mostly     raw_msr_domain_policy,
+                         __read_mostly    host_msr_domain_policy,
+                         __read_mostly hvm_max_msr_domain_policy,
                          __read_mostly  pv_max_msr_domain_policy;
 
 struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
                        __read_mostly  pv_max_msr_vcpu_policy;
 
+static void __init calculate_raw_policy(void)
+{
+    struct msr_domain_policy *dp = &raw_msr_domain_policy;
+    uint64_t val;
+
+    if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) == 0 )
+    {
+        dp->plaform_info.available = true;
+        if ( val & MSR_PLATFORM_INFO_CPUID_FAULTING )
+            dp->plaform_info.cpuid_faulting = true;
+    }
+}
+
+static void __init calculate_host_policy(void)
+{
+    struct msr_domain_policy *dp = &host_msr_domain_policy;
+
+    *dp = raw_msr_domain_policy;
+}
+
 static void __init calculate_hvm_max_policy(void)
 {
     struct msr_domain_policy *dp = &hvm_max_msr_domain_policy;
@@ -67,6 +89,8 @@ static void __init calculate_pv_max_policy(void)
 
 void __init init_guest_msr_policy(void)
 {
+    calculate_raw_policy();
+    calculate_host_policy();
     calculate_hvm_max_policy();
     calculate_pv_max_policy();
 }
-- 
2.11.0


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

[PATCH v4 2/7] x86/msr: add VMX MSRs into struct msr_domain_policy

[Sergey Dyasli]

New definitions provide a convenient way of accessing contents of
VMX MSRs. They are separated into 5 logical blocks:

    1. vmx: [VMX_BASIC, VMX_VMCS_ENUM]
    2. VMX_PROCBASED_CTLS2
    3. VMX_EPT_VPID_CAP
    4. vmx_true_ctls: [VMX_TRUE_PINBASED_CTLS, VMX_TRUE_ENTRY_CTLS]
    5. VMX_VMFUNC

Every bit value is accessible by its name and bit names match existing
Xen's definitions as close as possible. There is a "raw" 64-bit field
for each MSR as well as "raw" arrays for vmx and vmx_true_ctls blocks.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/msr.c        |  63 ++++++++
 xen/include/asm-x86/msr.h | 373 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 436 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9737ed706e..9dc3de8ce1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -216,6 +216,69 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     return X86EMUL_EXCEPTION;
 }
 
+static void __init __maybe_unused build_assertions(void)
+{
+    struct msr_domain_policy dp;
+
+    BUILD_BUG_ON(sizeof(dp.vmx.basic) !=
+                 sizeof(dp.vmx.basic.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.pinbased_ctls) !=
+                 sizeof(dp.vmx.pinbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.procbased_ctls) !=
+                 sizeof(dp.vmx.procbased_ctls.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.exit_ctls) !=
+                 sizeof(dp.vmx.exit_ctls.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.entry_ctls) !=
+                 sizeof(dp.vmx.entry_ctls.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.misc) !=
+                 sizeof(dp.vmx.misc.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.cr0_fixed0) !=
+                 sizeof(dp.vmx.cr0_fixed0.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.cr0_fixed1) !=
+                 sizeof(dp.vmx.cr0_fixed1.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.cr4_fixed0) !=
+                 sizeof(dp.vmx.cr4_fixed0.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.cr4_fixed1) !=
+                 sizeof(dp.vmx.cr4_fixed1.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.vmcs_enum) !=
+                 sizeof(dp.vmx.vmcs_enum.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx.raw) !=
+                     sizeof(dp.vmx.basic) +
+                     sizeof(dp.vmx.pinbased_ctls) +
+                     sizeof(dp.vmx.procbased_ctls) +
+                     sizeof(dp.vmx.exit_ctls) +
+                     sizeof(dp.vmx.entry_ctls) +
+                     sizeof(dp.vmx.misc) +
+                     sizeof(dp.vmx.cr0_fixed0) +
+                     sizeof(dp.vmx.cr0_fixed1) +
+                     sizeof(dp.vmx.cr4_fixed0) +
+                     sizeof(dp.vmx.cr4_fixed1) +
+                     sizeof(dp.vmx.vmcs_enum));
+
+    BUILD_BUG_ON(sizeof(dp.vmx_procbased_ctls2) !=
+                 sizeof(dp.vmx_procbased_ctls2.raw));
+
+    BUILD_BUG_ON(sizeof(dp.vmx_ept_vpid_cap) !=
+                 sizeof(dp.vmx_ept_vpid_cap.raw));
+
+    BUILD_BUG_ON(sizeof(dp.vmx_true_ctls.pinbased) !=
+                 sizeof(dp.vmx_true_ctls.pinbased.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx_true_ctls.procbased) !=
+                 sizeof(dp.vmx_true_ctls.procbased.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx_true_ctls.exit) !=
+                 sizeof(dp.vmx_true_ctls.exit.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx_true_ctls.entry) !=
+                 sizeof(dp.vmx_true_ctls.entry.raw));
+    BUILD_BUG_ON(sizeof(dp.vmx_true_ctls.raw) !=
+                     sizeof(dp.vmx_true_ctls.pinbased) +
+                     sizeof(dp.vmx_true_ctls.procbased) +
+                     sizeof(dp.vmx_true_ctls.exit) +
+                     sizeof(dp.vmx_true_ctls.entry));
+
+    BUILD_BUG_ON(sizeof(dp.vmx_vmfunc) !=
+                 sizeof(dp.vmx_vmfunc.raw));
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 751fa25a36..150000551f 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -202,6 +202,171 @@ void write_efer(u64 val);
 
 DECLARE_PER_CPU(u32, ler_msr);
 
+union vmx_pin_based_exec_control_bits {
+    uint32_t raw;
+    struct {
+        bool ext_intr_exiting:1;
+        uint32_t             :2;  /* 1:2 reserved */
+        bool      nmi_exiting:1;
+        uint32_t             :1;  /* 4 reserved */
+        bool     virtual_nmis:1;
+        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: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 cr0_bits {
+    bool     pe:1;
+    bool     mp:1;
+    bool     em:1;
+    bool     ts:1;
+    bool     et:1;
+    bool     ne:1;
+    uint32_t   :10; /* 6:15 reserved */
+    bool     wp:1;
+    uint32_t   :1;  /* 17 reserved */
+    bool     am:1;
+    uint32_t   :10; /* 19:28 reserved */
+    bool     nw:1;
+    bool     cd:1;
+    bool     pg:1;
+};
+
+struct cr4_bits {
+    bool        vme:1;
+    bool        pvi:1;
+    bool        tsd:1;
+    bool         de:1;
+    bool        pse:1;
+    bool        pae:1;
+    bool        mce:1;
+    bool        pge:1;
+    bool        pce:1;
+    bool     osfxsr:1;
+    bool osxmmexcpt:1;
+    bool       umip:1;
+    uint32_t       :1;  /* 12 reserved */
+    bool       vmxe:1;
+    bool       smxe:1;
+    uint32_t       :1;  /* 15 reserved */
+    bool   fsgsbase:1;
+    bool      pcide:1;
+    bool    osxsave:1;
+    uint32_t       :1;  /* 19 reserved */
+    bool       smep:1;
+    bool       smap:1;
+    bool        pke:1;
+    uint32_t       :9;  /* 23:31 reserved */
+};
+
 /* MSR policy object for shared per-domain MSRs */
 struct msr_domain_policy
 {
@@ -210,6 +375,214 @@ struct msr_domain_policy
         bool available; /* This MSR is non-architectural */
         bool cpuid_faulting;
     } plaform_info;
+
+    union {
+        uint64_t raw[MSR_IA32_VMX_VMCS_ENUM - MSR_IA32_VMX_BASIC + 1];
+
+        struct {
+            /* 0x00000480  MSR_IA32_VMX_BASIC */
+            union {
+                uint64_t raw;
+                struct {
+                    uint32_t vmcs_revision_id:31;
+                    bool                  mbz:1;  /* 31 always zero */
+                    uint32_t vmcs_region_size:13;
+                    uint32_t                 :3;  /* 45:47 reserved */
+                    bool      addresses_32bit:1;
+                    bool         dual_monitor:1;
+                    uint32_t      memory_type:4;
+                    bool         ins_out_info:1;
+                    bool        default1_zero:1;
+                    uint32_t                 :8;  /* 56:63 reserved */
+                };
+            } basic;
+
+            /* 0x00000481  MSR_IA32_VMX_PINBASED_CTLS */
+            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;
+
+            /* 0x00000482  MSR_IA32_VMX_PROCBASED_CTLS */
+            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;
+
+            /* 0x00000483  MSR_IA32_VMX_EXIT_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmexit_control_bits allowed_0;
+                    union vmx_vmexit_control_bits allowed_1;
+                };
+            } exit_ctls;
+
+            /* 0x00000484  MSR_IA32_VMX_ENTRY_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmentry_control_bits allowed_0;
+                    union vmx_vmentry_control_bits allowed_1;
+                };
+            } entry_ctls;
+
+            /* 0x00000485  MSR_IA32_VMX_MISC */
+            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;
+
+            /* 0x00000486  MSR_IA32_VMX_CR0_FIXED0 */
+            union {
+                uint64_t raw;
+                struct cr0_bits allowed_0;
+            } cr0_fixed0;
+
+            /* 0x00000487  MSR_IA32_VMX_CR0_FIXED1 */
+            union {
+                uint64_t raw;
+                struct cr0_bits allowed_1;
+            } cr0_fixed1;
+
+            /* 0x00000488  MSR_IA32_VMX_CR4_FIXED0 */
+            union {
+                uint64_t raw;
+                struct cr4_bits allowed_0;
+            } cr4_fixed0;
+
+            /* 0x00000489  MSR_IA32_VMX_CR4_FIXED1 */
+            union {
+                uint64_t raw;
+                struct cr4_bits allowed_1;
+            } cr4_fixed1;
+
+            /* 0x0000048A  MSR_IA32_VMX_VMCS_ENUM */
+            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;
+        };
+    } vmx;
+
+    /* 0x0000048B  MSR_IA32_VMX_PROCBASED_CTLS2 */
+    union {
+        uint64_t raw;
+        struct {
+            union vmx_secondary_exec_control_bits allowed_0;
+            union vmx_secondary_exec_control_bits allowed_1;
+        };
+    } vmx_procbased_ctls2;
+
+    /* 0x0000048C  MSR_IA32_VMX_EPT_VPID_CAP */
+    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 */
+        };
+    } vmx_ept_vpid_cap;
+
+    union {
+        uint64_t raw[MSR_IA32_VMX_TRUE_ENTRY_CTLS -
+                     MSR_IA32_VMX_TRUE_PINBASED_CTLS + 1];
+
+        struct {
+            /* 0x0000048D  MSR_IA32_VMX_TRUE_PINBASED_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_pin_based_exec_control_bits allowed_0;
+                    union vmx_pin_based_exec_control_bits allowed_1;
+                };
+            } pinbased;
+
+            /* 0x0000048E  MSR_IA32_VMX_TRUE_PROCBASED_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_cpu_based_exec_control_bits allowed_0;
+                    union vmx_cpu_based_exec_control_bits allowed_1;
+                };
+            } procbased;
+
+            /* 0x0000048F  MSR_IA32_VMX_TRUE_EXIT_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmexit_control_bits allowed_0;
+                    union vmx_vmexit_control_bits allowed_1;
+                };
+            } exit;
+
+            /* 0x00000490  MSR_IA32_VMX_TRUE_ENTRY_CTLS */
+            union {
+                uint64_t raw;
+                struct {
+                    union vmx_vmentry_control_bits allowed_0;
+                    union vmx_vmentry_control_bits allowed_1;
+                };
+            } entry;
+        };
+    } vmx_true_ctls;
+
+    /* 0x00000491  MSR_IA32_VMX_VMFUNC */
+    union {
+        uint64_t raw;
+        struct {
+            bool eptp_switching:1;
+            uint64_t           :63; /* 1:63 reserved */
+        };
+    } vmx_vmfunc;
 };
 
 /* MSR policy object for per-vCPU MSRs */
-- 
2.11.0


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

[PATCH v4 3/7] x86/msr: read VMX MSRs values into Raw policy

[Sergey Dyasli]

Add calculate_raw_vmx_policy() which fills Raw policy with H/W values
of VMX MSRs. Host policy will contain a copy of these values.

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

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9dc3de8ce1..7ac0fceb49 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -32,6 +32,37 @@ struct msr_domain_policy __read_mostly     raw_msr_domain_policy,
 struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
                        __read_mostly  pv_max_msr_vcpu_policy;
 
+static void __init calculate_raw_vmx_policy(struct msr_domain_policy *dp)
+{
+    unsigned int i;
+
+    if ( !cpu_has_vmx )
+        return;
+
+    for ( i = MSR_IA32_VMX_BASIC; i <= MSR_IA32_VMX_VMCS_ENUM; i++ )
+        rdmsrl(i, dp->vmx.raw[i - MSR_IA32_VMX_BASIC]);
+
+    if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
+    {
+        rdmsrl(MSR_IA32_VMX_PROCBASED_CTLS2, dp->vmx_procbased_ctls2.raw);
+
+        if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
+             dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
+            rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, dp->vmx_ept_vpid_cap.raw);
+    }
+
+    if ( dp->vmx.basic.default1_zero )
+    {
+        for ( i = MSR_IA32_VMX_TRUE_PINBASED_CTLS;
+              i <= MSR_IA32_VMX_TRUE_ENTRY_CTLS; i++ )
+            rdmsrl(i,
+                   dp->vmx_true_ctls.raw[i - MSR_IA32_VMX_TRUE_PINBASED_CTLS]);
+    }
+
+    if ( dp->vmx_procbased_ctls2.allowed_1.enable_vm_functions )
+        rdmsrl(MSR_IA32_VMX_VMFUNC, dp->vmx_vmfunc.raw);
+}
+
 static void __init calculate_raw_policy(void)
 {
     struct msr_domain_policy *dp = &raw_msr_domain_policy;
@@ -43,6 +74,8 @@ static void __init calculate_raw_policy(void)
         if ( val & MSR_PLATFORM_INFO_CPUID_FAULTING )
             dp->plaform_info.cpuid_faulting = true;
     }
+
+    calculate_raw_vmx_policy(dp);
 }
 
 static void __init calculate_host_policy(void)
-- 
2.11.0


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

[PATCH v4 4/7] x86/msr: add VMX MSRs into HVM_max domain 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 calculate_hvm_max_vmx_policy() which will save the end result of
nvmx_msr_read_intercept() on current H/W into HVM_max domain policy.
There will be no functional change to what L1 sees in VMX MSRs. But the
actual use of HVM_max domain policy will happen later, when VMX MSRs
are handled by guest_rd/wrmsr().

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

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 7ac0fceb49..ff270befbb 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -85,6 +85,133 @@ static void __init calculate_host_policy(void)
     *dp = raw_msr_domain_policy;
 }
 
+#define vmx_host_allowed_cpy(dp, msr, field)                            \
+    do {                                                                \
+        dp->msr.allowed_1.field =                                       \
+            host_msr_domain_policy.msr.allowed_1.field;                 \
+        dp->msr.allowed_0.field =                                       \
+            host_msr_domain_policy.msr.allowed_0.field;                 \
+    } while (0)
+
+#define vmx_host_allowed_cpyb(dp, block, msr, field)                    \
+    do {                                                                \
+        dp->block.msr.allowed_1.field =                                 \
+            host_msr_domain_policy.block.msr.allowed_1.field;           \
+        dp->block.msr.allowed_0.field =                                 \
+            host_msr_domain_policy.block.msr.allowed_0.field;           \
+    } while (0)
+
+static void __init calculate_hvm_max_vmx_policy(struct msr_domain_policy *dp)
+{
+    if ( !cpu_has_vmx )
+        return;
+
+    dp->vmx.basic.raw = host_msr_domain_policy.vmx.basic.raw;
+
+    dp->vmx.pinbased_ctls.raw = ((uint64_t) VMX_PINBASED_CTLS_DEFAULT1 << 32) |
+                                            VMX_PINBASED_CTLS_DEFAULT1;
+    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, ext_intr_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, nmi_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, preempt_timer);
+
+    dp->vmx.procbased_ctls.raw =
+        ((uint64_t) VMX_PROCBASED_CTLS_DEFAULT1 << 32) |
+                    VMX_PROCBASED_CTLS_DEFAULT1;
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, virtual_intr_pending);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, use_tsc_offseting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, hlt_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, invlpg_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, mwait_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, rdpmc_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, rdtsc_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, cr8_load_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, cr8_store_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, tpr_shadow);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, virtual_nmi_pending);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, mov_dr_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, uncond_io_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_io_bitmap);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, monitor_trap_flag);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_msr_bitmap);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, monitor_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, pause_exiting);
+    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_secondary_controls);
+
+    dp->vmx.exit_ctls.raw = ((uint64_t) VMX_EXIT_CTLS_DEFAULT1 << 32) |
+                                        VMX_EXIT_CTLS_DEFAULT1;
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, ia32e_mode);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, load_perf_global_ctrl);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, ack_intr_on_exit);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, save_guest_pat);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, load_host_pat);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, save_guest_efer);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, load_host_efer);
+    vmx_host_allowed_cpyb(dp, vmx, exit_ctls, save_preempt_timer);
+
+    dp->vmx.entry_ctls.raw = ((uint64_t) VMX_ENTRY_CTLS_DEFAULT1 << 32) |
+                                         VMX_ENTRY_CTLS_DEFAULT1;
+    vmx_host_allowed_cpyb(dp, vmx, entry_ctls, ia32e_mode);
+    vmx_host_allowed_cpyb(dp, vmx, entry_ctls, load_perf_global_ctrl);
+    vmx_host_allowed_cpyb(dp, vmx, entry_ctls, load_guest_pat);
+    vmx_host_allowed_cpyb(dp, vmx, entry_ctls, load_guest_efer);
+
+    dp->vmx.misc.raw = host_msr_domain_policy.vmx.misc.raw;
+    /* Do not support CR3-target feature now */
+    dp->vmx.misc.cr3_target = false;
+
+    /* PG, PE bits must be 1 in VMX operation */
+    dp->vmx.cr0_fixed0.allowed_0.pe = true;
+    dp->vmx.cr0_fixed0.allowed_0.pg = true;
+
+    /* allow 0-settings for all bits */
+    dp->vmx.cr0_fixed1.raw = 0xffffffff;
+
+    /* VMXE bit must be 1 in VMX operation */
+    dp->vmx.cr4_fixed0.allowed_0.vmxe = true;
+
+    /*
+     * Allowed CR4 bits will be updated during domain creation by
+     * hvm_cr4_guest_valid_bits()
+     */
+    dp->vmx.cr4_fixed1.raw = host_msr_domain_policy.vmx.cr4_fixed1.raw;
+
+    /* The max index of VVMCS encoding is 0x1f. */
+    dp->vmx.vmcs_enum.vmcs_encoding_max_idx = 0x1f;
+
+    if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
+    {
+        vmx_host_allowed_cpy(dp, vmx_procbased_ctls2, virtualize_apic_accesses);
+        vmx_host_allowed_cpy(dp, vmx_procbased_ctls2, enable_ept);
+        vmx_host_allowed_cpy(dp, vmx_procbased_ctls2, descriptor_table_exiting);
+        vmx_host_allowed_cpy(dp, vmx_procbased_ctls2, enable_vpid);
+        vmx_host_allowed_cpy(dp, vmx_procbased_ctls2, unrestricted_guest);
+
+        if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
+             dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
+        {
+            dp->vmx_ept_vpid_cap.raw = nept_get_ept_vpid_cap();
+        }
+    }
+
+    if ( dp->vmx.basic.default1_zero )
+    {
+        dp->vmx_true_ctls.pinbased.raw = dp->vmx.pinbased_ctls.raw;
+
+        dp->vmx_true_ctls.procbased.raw = dp->vmx.procbased_ctls.raw;
+        vmx_host_allowed_cpyb(dp, vmx_true_ctls, procbased, cr3_load_exiting);
+        vmx_host_allowed_cpyb(dp, vmx_true_ctls, procbased, cr3_store_exiting);
+
+        dp->vmx_true_ctls.exit.raw = dp->vmx.exit_ctls.raw;
+
+        dp->vmx_true_ctls.entry.raw = dp->vmx.entry_ctls.raw;
+    }
+
+    /* MSR_IA32_VMX_VMFUNC is N/A */
+}
+
+#undef vmx_host_allowed_cpy
+#undef vmx_host_allowed_cpyb
+
 static void __init calculate_hvm_max_policy(void)
 {
     struct msr_domain_policy *dp = &hvm_max_msr_domain_policy;
@@ -102,6 +229,8 @@ static void __init calculate_hvm_max_policy(void)
 
     /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
     vp->misc_features_enables.available = dp->plaform_info.available;
+
+    calculate_hvm_max_vmx_policy(dp);
 }
 
 static void __init calculate_pv_max_policy(void)
-- 
2.11.0


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

[PATCH v4 5/7] x86/cpuid: update signature of hvm_cr4_guest_valid_bits()

[Sergey Dyasli]

With the new cpuid infrastructure there is a domain-wide struct cpuid
policy and there is no need to pass a separate struct vcpu * into
hvm_cr4_guest_valid_bits() anymore. Make the function accept struct
domain * instead and update callers.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/domain.c       | 3 ++-
 xen/arch/x86/hvm/hvm.c          | 7 +++----
 xen/arch/x86/hvm/svm/svmdebug.c | 4 ++--
 xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
 xen/include/asm-x86/hvm/hvm.h   | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 60474649de..ce15ce0470 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -111,6 +111,7 @@ static int check_segment(struct segment_register *reg, enum x86_segment seg)
 /* Called by VCPUOP_initialise for HVM guests. */
 int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
 {
+    const struct domain *d = v->domain;
     struct cpu_user_regs *uregs = &v->arch.user_regs;
     struct segment_register cs, ds, ss, es, tr;
     const char *errstr;
@@ -272,7 +273,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
     if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
         v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
 
-    if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
+    if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
     {
         gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
                 v->arch.hvm_vcpu.guest_cr[4]);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..1784c32c7e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -928,9 +928,8 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
         X86_CR0_CD | X86_CR0_PG)))
 
 /* These bits in CR4 can be set by the guest. */
-unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
 {
-    const struct domain *d = v->domain;
     const struct cpuid_policy *p;
     bool mce, vmxe;
 
@@ -997,7 +996,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }
 
-    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(v, 1) )
+    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d, true) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
                d->domain_id, ctxt.cr4);
@@ -2308,7 +2307,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
     struct vcpu *v = current;
     unsigned long old_cr;
 
-    if ( value & ~hvm_cr4_guest_valid_bits(v, 0) )
+    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
     {
         HVM_DBG_LOG(DBG_LEVEL_1,
                     "Guest attempts to set reserved bit in CR4: %lx",
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 89ef2db932..e25e3e0423 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -119,9 +119,9 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
            (cr3 >> v->domain->arch.cpuid->extd.maxphysaddr))) )
         PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", cr3);
 
-    if ( cr4 & ~hvm_cr4_guest_valid_bits(v, false) )
+    if ( cr4 & ~hvm_cr4_guest_valid_bits(v->domain, false) )
         PRINTF("CR4: invalid bits are set (%#"PRIx64", valid: %#"PRIx64")\n",
-               cr4, hvm_cr4_guest_valid_bits(v, false));
+               cr4, hvm_cr4_guest_valid_bits(v->domain, false));
 
     if ( vmcb_get_dr6(vmcb) >> 32 )
         PRINTF("DR6: bits [63:32] are not zero (%#"PRIx64")\n",
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index dde02c076b..4d9ffc490c 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2136,7 +2136,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         data = X86_CR4_VMXE;
         break;
     case MSR_IA32_VMX_CR4_FIXED1:
-        data = hvm_cr4_guest_valid_bits(v, 0);
+        data = hvm_cr4_guest_valid_bits(d, false);
         break;
     case MSR_IA32_VMX_MISC:
         /* Do not support CR3-target feature now */
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index b687e03dce..47a5f7916d 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -612,7 +612,7 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
 /* Check CR4/EFER values */
 const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
                            signed int cr0_pg);
-unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore);
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore);
 
 /*
  * This must be defined as a macro instead of an inline function,
-- 
2.11.0


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

[PATCH v4 6/7] x86/msr: update domain policy on CPUID policy changes

[Sergey Dyasli]

Availability of some MSRs depends on certain CPUID bits. Add function
recalculate_domain_msr_policy() which updates availability of per-domain
MSRs based on current domain's CPUID policy. This function is called
when CPUID policy is changed from a toolstack.

Add recalculate_domain_vmx_msr_policy() which changes availability of
VMX MSRs based on domain's nested virt settings. Unavailable MSRs are
zeroed which allows checking availability bits in them directly without
preliminary checks (e.g. cpuid->basic.vmx, activate_secondary_controls,
enable_ept).

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/domctl.c     |  1 +
 xen/arch/x86/msr.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/msr.h |  3 +++
 3 files changed, 59 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 80b4df9ec9..334c67d261 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -124,6 +124,7 @@ static int update_domain_cpuid_info(struct domain *d,
     }
 
     recalculate_cpuid_policy(d);
+    recalculate_domain_msr_policy(d);
 
     switch ( ctl->input[0] )
     {
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index ff270befbb..9ea7447de3 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -23,6 +23,7 @@
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <asm/msr.h>
+#include <asm/hvm/nestedhvm.h>
 
 struct msr_domain_policy __read_mostly     raw_msr_domain_policy,
                          __read_mostly    host_msr_domain_policy,
@@ -257,6 +258,59 @@ void __init init_guest_msr_policy(void)
     calculate_pv_max_policy();
 }
 
+static void recalculate_domain_vmx_msr_policy(struct domain *d)
+{
+    struct msr_domain_policy *dp = d->arch.msr;
+
+    if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
+    {
+        memset(dp->vmx.raw, 0, sizeof(dp->vmx.raw));
+        dp->vmx_procbased_ctls2.raw = 0;
+        dp->vmx_ept_vpid_cap.raw = 0;
+        memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));
+        dp->vmx_vmfunc.raw = 0;
+    }
+    else
+    {
+        memcpy(dp->vmx.raw, hvm_max_msr_domain_policy.vmx.raw,
+               sizeof(dp->vmx.raw));
+        /* Get allowed CR4 bits from CPUID policy */
+        dp->vmx.cr4_fixed1.raw = hvm_cr4_guest_valid_bits(d, false);
+
+        if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
+        {
+            dp->vmx_procbased_ctls2.raw =
+                hvm_max_msr_domain_policy.vmx_procbased_ctls2.raw;
+
+            if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
+                 dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
+                dp->vmx_ept_vpid_cap.raw =
+                    hvm_max_msr_domain_policy.vmx_ept_vpid_cap.raw;
+            else
+                dp->vmx_ept_vpid_cap.raw = 0;
+        }
+        else
+        {
+            dp->vmx_procbased_ctls2.raw = 0;
+            dp->vmx_ept_vpid_cap.raw = 0;
+        }
+
+        if ( dp->vmx.basic.default1_zero )
+            memcpy(dp->vmx_true_ctls.raw,
+                   hvm_max_msr_domain_policy.vmx_true_ctls.raw,
+                   sizeof(dp->vmx_true_ctls.raw));
+        else
+            memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));
+
+        dp->vmx_vmfunc.raw = 0;
+    }
+}
+
+void recalculate_domain_msr_policy(struct domain *d)
+{
+    recalculate_domain_vmx_msr_policy(d);
+}
+
 int init_domain_msr_policy(struct domain *d)
 {
     struct msr_domain_policy *dp;
@@ -277,6 +331,7 @@ int init_domain_msr_policy(struct domain *d)
     }
 
     d->arch.msr = dp;
+    recalculate_domain_msr_policy(d);
 
     return 0;
 }
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 150000551f..f19e113612 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -608,6 +608,9 @@ int init_vcpu_msr_policy(struct vcpu *v);
 int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val);
 int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
 
+/* Update availability of per-domain MSRs based on CPUID policy */
+void recalculate_domain_msr_policy(struct domain *d);
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_MSR_H */
-- 
2.11.0


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

[PATCH v4 7/7] x86/msr: handle VMX MSRs with guest_rd/wrmsr()

[Sergey Dyasli]

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

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

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c         |   6 --
 xen/arch/x86/hvm/vmx/vvmx.c        | 178 -------------------------------------
 xen/arch/x86/msr.c                 |  37 ++++++++
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 -
 4 files changed, 37 insertions(+), 186 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c2148701ee..1a1cb98069 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2906,10 +2906,6 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         if ( nestedhvm_enabled(curr->domain) )
             *msr_content |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
         break;
-    case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
-        if ( !nvmx_msr_read_intercept(msr, msr_content) )
-            goto gp_fault;
-        break;
     case MSR_IA32_MISC_ENABLE:
         rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
         /* Debug Trace Store is not supported. */
@@ -3133,8 +3129,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         break;
     }
     case MSR_IA32_FEATURE_CONTROL:
-    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
-        /* None of these MSRs are writeable. */
         goto gp_fault;
 
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 4d9ffc490c..b0474ad310 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1976,184 +1976,6 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     return X86EMUL_OKAY;
 }
 
-#define __emul_value(enable1, default1) \
-    ((enable1 | default1) << 32 | (default1))
-
-#define gen_vmx_msr(enable1, default1, host_value) \
-    (((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
-    ((uint32_t)(__emul_value(enable1, default1) | host_value)))
-
-/*
- * Capability reporting
- */
-int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
-{
-    struct vcpu *v = current;
-    struct domain *d = v->domain;
-    u64 data = 0, host_data = 0;
-    int r = 1;
-
-    /* VMX capablity MSRs are available only when guest supports VMX. */
-    if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
-        return 0;
-
-    /*
-     * These MSRs are only available when flags in other MSRs are set.
-     * These prerequisites are listed in the Intel 64 and IA-32
-     * Architectures Software Developer’s Manual, Vol 3, Appendix A.
-     */
-    switch ( msr )
-    {
-    case MSR_IA32_VMX_PROCBASED_CTLS2:
-        if ( !cpu_has_vmx_secondary_exec_control )
-            return 0;
-        break;
-
-    case MSR_IA32_VMX_EPT_VPID_CAP:
-        if ( !(cpu_has_vmx_ept || cpu_has_vmx_vpid) )
-            return 0;
-        break;
-
-    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
-    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
-    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-        if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
-            return 0;
-        break;
-
-    case MSR_IA32_VMX_VMFUNC:
-        if ( !cpu_has_vmx_vmfunc )
-            return 0;
-        break;
-    }
-
-    rdmsrl(msr, host_data);
-
-    /*
-     * Remove unsupport features from n1 guest capability MSR
-     */
-    switch (msr) {
-    case MSR_IA32_VMX_BASIC:
-    {
-        const struct vmcs_struct *vmcs =
-            map_domain_page(_mfn(PFN_DOWN(v->arch.hvm_vmx.vmcs_pa)));
-
-        data = (host_data & (~0ul << 32)) |
-               (vmcs->vmcs_revision_id & 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;
-
-    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(d, false);
-        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;
-    }
-
-    *msr_content = data;
-    return r;
-}
-
 /* This function uses L2_gpa to walk the P2M page table in L1. If the
  * walk is successful, the translated value is returned in
  * L1_gpa. The result value tells what to do next.
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9ea7447de3..a1c48b6769 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -360,8 +360,10 @@ int init_vcpu_msr_policy(struct vcpu *v)
 
 int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 {
+    const struct domain *d = v->domain;
     const struct msr_domain_policy *dp = v->domain->arch.msr;
     const struct msr_vcpu_policy *vp = v->arch.msr;
+    const struct cpuid_policy *p = d->arch.cpuid;
 
     switch ( msr )
     {
@@ -379,6 +381,37 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
 
+    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMCS_ENUM:
+        if ( !nestedhvm_enabled(d) || !p->basic.vmx )
+            goto gp_fault;
+        *val = dp->vmx.raw[msr - MSR_IA32_VMX_BASIC];
+        break;
+
+    case MSR_IA32_VMX_PROCBASED_CTLS2:
+        if ( !dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
+            goto gp_fault;
+        *val = dp->vmx_procbased_ctls2.raw;
+        break;
+
+    case MSR_IA32_VMX_EPT_VPID_CAP:
+        if ( !(dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
+               dp->vmx_procbased_ctls2.allowed_1.enable_vpid) )
+            goto gp_fault;
+        *val = dp->vmx_ept_vpid_cap.raw;
+        break;
+
+    case MSR_IA32_VMX_TRUE_PINBASED_CTLS ... MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+        if ( !dp->vmx.basic.default1_zero )
+            goto gp_fault;
+        *val = dp->vmx_true_ctls.raw[msr - MSR_IA32_VMX_TRUE_PINBASED_CTLS];
+        break;
+
+    case MSR_IA32_VMX_VMFUNC:
+        if ( !dp->vmx_procbased_ctls2.allowed_1.enable_vm_functions )
+            goto gp_fault;
+        *val = dp->vmx_vmfunc.raw;
+        break;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
@@ -423,6 +456,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+        /* None of these MSRs are writeable. */
+        goto gp_fault;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 3285b03bbb..5950672e93 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -222,8 +222,6 @@ int nvmx_handle_vmresume(struct cpu_user_regs *regs);
 int nvmx_handle_vmlaunch(struct cpu_user_regs *regs);
 int nvmx_handle_invept(struct cpu_user_regs *regs);
 int nvmx_handle_invvpid(struct cpu_user_regs *regs);
-int nvmx_msr_read_intercept(unsigned int msr,
-                                u64 *msr_content);
 
 void nvmx_update_exec_control(struct vcpu *v, u32 value);
 void nvmx_update_secondary_exec_control(struct vcpu *v,
-- 
2.11.0


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

Re: [PATCH v4 1/7] x86/msr: add Raw and Host domain policies

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> Raw policy contains the actual values from H/W MSRs. PLATFORM_INFO msr
> needs to be read again because probe_intel_cpuid_faulting() records
> the presence of X86_FEATURE_CPUID_FAULTING but not the presence of msr
> itself (if cpuid faulting is not available).

It would be nice if this information could be propagated to avoid
the second read - I really dislike the traces such failed reads leave
in logs; at the first glance this always looks as if something was
wrong.

Apart from this the patch looks fine.

Jan


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

Re: [PATCH v4 2/7] x86/msr: add VMX MSRs into struct msr_domain_policy

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> New definitions provide a convenient way of accessing contents of
> VMX MSRs. They are separated into 5 logical blocks:
> 
>     1. vmx: [VMX_BASIC, VMX_VMCS_ENUM]
>     2. VMX_PROCBASED_CTLS2
>     3. VMX_EPT_VPID_CAP
>     4. vmx_true_ctls: [VMX_TRUE_PINBASED_CTLS, VMX_TRUE_ENTRY_CTLS]
>     5. VMX_VMFUNC

By merely listing them it is not really possible to judge why e.g.
PROCBASED_CTLS2 aren't being grouped into (1). Please clarify
the reasons for the split.

> +struct cr0_bits {
> +    bool     pe:1;
> +    bool     mp:1;
> +    bool     em:1;
> +    bool     ts:1;
> +    bool     et:1;
> +    bool     ne:1;
> +    uint32_t   :10; /* 6:15 reserved */
> +    bool     wp:1;
> +    uint32_t   :1;  /* 17 reserved */
> +    bool     am:1;
> +    uint32_t   :10; /* 19:28 reserved */
> +    bool     nw:1;
> +    bool     cd:1;
> +    bool     pg:1;
> +};
> +
> +struct cr4_bits {
> +    bool        vme:1;
> +    bool        pvi:1;
> +    bool        tsd:1;
> +    bool         de:1;
> +    bool        pse:1;
> +    bool        pae:1;
> +    bool        mce:1;
> +    bool        pge:1;
> +    bool        pce:1;
> +    bool     osfxsr:1;
> +    bool osxmmexcpt:1;
> +    bool       umip:1;
> +    uint32_t       :1;  /* 12 reserved */
> +    bool       vmxe:1;
> +    bool       smxe:1;
> +    uint32_t       :1;  /* 15 reserved */
> +    bool   fsgsbase:1;
> +    bool      pcide:1;
> +    bool    osxsave:1;
> +    uint32_t       :1;  /* 19 reserved */
> +    bool       smep:1;
> +    bool       smap:1;
> +    bool        pke:1;
> +    uint32_t       :9;  /* 23:31 reserved */
> +};

I don't think these belong in this header - a new x86-structs.h or
some such would seem more suitable. Also both are 64-bit registers,
and I'm missing the "raw" field anyway (allowing them to be used
elsewhere later on).

Jan


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

Re: [PATCH v4 3/7] x86/msr: read VMX MSRs values into Raw policy

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -32,6 +32,37 @@ struct msr_domain_policy __read_mostly     raw_msr_domain_policy,
>  struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
>                         __read_mostly  pv_max_msr_vcpu_policy;
>  
> +static void __init calculate_raw_vmx_policy(struct msr_domain_policy *dp)
> +{
> +    unsigned int i;
> +
> +    if ( !cpu_has_vmx )
> +        return;
> +
> +    for ( i = MSR_IA32_VMX_BASIC; i <= MSR_IA32_VMX_VMCS_ENUM; i++ )
> +        rdmsrl(i, dp->vmx.raw[i - MSR_IA32_VMX_BASIC]);
> +
> +    if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
> +    {
> +        rdmsrl(MSR_IA32_VMX_PROCBASED_CTLS2, dp->vmx_procbased_ctls2.raw);
> +
> +        if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
> +             dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
> +            rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, dp->vmx_ept_vpid_cap.raw);

For consistency please either move this out of the if() or ...

> +    }
> +
> +    if ( dp->vmx.basic.default1_zero )
> +    {
> +        for ( i = MSR_IA32_VMX_TRUE_PINBASED_CTLS;
> +              i <= MSR_IA32_VMX_TRUE_ENTRY_CTLS; i++ )
> +            rdmsrl(i,
> +                   dp->vmx_true_ctls.raw[i - MSR_IA32_VMX_TRUE_PINBASED_CTLS]);
> +    }
> +
> +    if ( dp->vmx_procbased_ctls2.allowed_1.enable_vm_functions )
> +        rdmsrl(MSR_IA32_VMX_VMFUNC, dp->vmx_vmfunc.raw);

... move this one into that same if() above.

Jan


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

Re: [PATCH v4 4/7] x86/msr: add VMX MSRs into HVM_max domain policy

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> +static void __init calculate_hvm_max_vmx_policy(struct msr_domain_policy *dp)
> +{
> +    if ( !cpu_has_vmx )
> +        return;
> +
> +    dp->vmx.basic.raw = host_msr_domain_policy.vmx.basic.raw;
> +
> +    dp->vmx.pinbased_ctls.raw = ((uint64_t) VMX_PINBASED_CTLS_DEFAULT1 << 32) |

Stray blank after cast.

> +                                            VMX_PINBASED_CTLS_DEFAULT1;
> +    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, ext_intr_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, nmi_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, pinbased_ctls, preempt_timer);
> +
> +    dp->vmx.procbased_ctls.raw =
> +        ((uint64_t) VMX_PROCBASED_CTLS_DEFAULT1 << 32) |

Again.

> +                    VMX_PROCBASED_CTLS_DEFAULT1;
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, virtual_intr_pending);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, use_tsc_offseting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, hlt_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, invlpg_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, mwait_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, rdpmc_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, rdtsc_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, cr8_load_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, cr8_store_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, tpr_shadow);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, virtual_nmi_pending);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, mov_dr_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, uncond_io_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_io_bitmap);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, monitor_trap_flag);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_msr_bitmap);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, monitor_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, pause_exiting);
> +    vmx_host_allowed_cpyb(dp, vmx, procbased_ctls, activate_secondary_controls);

This looks pretty ugly and hard to maintain to me. Wouldn't it be
possible to have static const instances of the structures describing
which fields should come from where, allowing simple & and | to
be done on the raw fields instead. I understand we won't get
away without listing the individual bits, but the overhead here is
far higher than if there was a simple initializer line for each field.

Jan


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

Re: [PATCH v4 5/7] x86/cpuid: update signature of hvm_cr4_guest_valid_bits()

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> With the new cpuid infrastructure there is a domain-wide struct cpuid
> policy and there is no need to pass a separate struct vcpu * into
> hvm_cr4_guest_valid_bits() anymore. Make the function accept struct
> domain * instead and update callers.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH v4 6/7] x86/msr: update domain policy on CPUID policy changes

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -124,6 +124,7 @@ static int update_domain_cpuid_info(struct domain *d,
>      }
>  
>      recalculate_cpuid_policy(d);
> +    recalculate_domain_msr_policy(d);

Considering the name of the other function, why not without the
"_domain"?

> +static void recalculate_domain_vmx_msr_policy(struct domain *d)
> +{
> +    struct msr_domain_policy *dp = d->arch.msr;
> +
> +    if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
> +    {
> +        memset(dp->vmx.raw, 0, sizeof(dp->vmx.raw));
> +        dp->vmx_procbased_ctls2.raw = 0;
> +        dp->vmx_ept_vpid_cap.raw = 0;
> +        memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));
> +        dp->vmx_vmfunc.raw = 0;
> +    }
> +    else
> +    {
> +        memcpy(dp->vmx.raw, hvm_max_msr_domain_policy.vmx.raw,
> +               sizeof(dp->vmx.raw));
> +        /* Get allowed CR4 bits from CPUID policy */
> +        dp->vmx.cr4_fixed1.raw = hvm_cr4_guest_valid_bits(d, false);
> +
> +        if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
> +        {
> +            dp->vmx_procbased_ctls2.raw =
> +                hvm_max_msr_domain_policy.vmx_procbased_ctls2.raw;
> +
> +            if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
> +                 dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
> +                dp->vmx_ept_vpid_cap.raw =
> +                    hvm_max_msr_domain_policy.vmx_ept_vpid_cap.raw;
> +            else
> +                dp->vmx_ept_vpid_cap.raw = 0;
> +        }
> +        else
> +        {
> +            dp->vmx_procbased_ctls2.raw = 0;
> +            dp->vmx_ept_vpid_cap.raw = 0;
> +        }
> +
> +        if ( dp->vmx.basic.default1_zero )
> +            memcpy(dp->vmx_true_ctls.raw,
> +                   hvm_max_msr_domain_policy.vmx_true_ctls.raw,
> +                   sizeof(dp->vmx_true_ctls.raw));
> +        else
> +            memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));

Except for the "else" paths this looks pretty similar to code in one of
the earlier patches. Any chance the two copies could be morphed
into a single way of doing this, so the same logic (which will only get
more complicated over time) won't be present in two places?

Also it looks like there's an implied assumption that this won't be
called anymore after the guest is first started. While just like for
CPUID this is reasonable, is this actually being enforced anywhere?

Jan


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

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

[Jan Beulich]

>>> On 18.10.17 at 10:27, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -360,8 +360,10 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> +    const struct domain *d = v->domain;
>      const struct msr_domain_policy *dp = v->domain->arch.msr;
>      const struct msr_vcpu_policy *vp = v->arch.msr;
> +    const struct cpuid_policy *p = d->arch.cpuid;

"p" is clearly not a name helpful to the reader. Why not "cpuid"?

> @@ -379,6 +381,37 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMCS_ENUM:
> +        if ( !nestedhvm_enabled(d) || !p->basic.vmx )
> +            goto gp_fault;
> +        *val = dp->vmx.raw[msr - MSR_IA32_VMX_BASIC];
> +        break;
> +
> +    case MSR_IA32_VMX_PROCBASED_CTLS2:
> +        if ( !dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )

Here and ...

> +            goto gp_fault;
> +        *val = dp->vmx_procbased_ctls2.raw;
> +        break;
> +
> +    case MSR_IA32_VMX_EPT_VPID_CAP:
> +        if ( !(dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
> +               dp->vmx_procbased_ctls2.allowed_1.enable_vpid) )

... here (and more below) again similar logic to what was there in an
earlier patch? Wouldn't it be better to generically express the
existence of an MSR in the policy (and then perhaps also its
writeability, albeit that may need to be a bitmap instead of a single
flag for the entire register)?

Jan


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