diff mbox

[v3,5/6] x86/msr: update domain policy on CPUID policy changes

Message ID 20171013123512.26102-6-sergey.dyasli@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sergey Dyasli Oct. 13, 2017, 12:35 p.m. UTC 
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.

Introduce hvm_cr4_domain_valid_bits() which accepts struct domain *
instead of struct vcpu *.

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

Comments

Andrew Cooper Oct. 13, 2017, 3:25 p.m. UTC | #1 
On 13/10/17 13:35, Sergey Dyasli wrote:
> 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.

This is probably acceptable for now.  recalculate_cpuid_policy() is only
a transitory artefact between the current behaviour of the Xen, and the
future behaviour of auditing the toolstack-provided cpuid and msr policy
completely before changing the domains datastructures.

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 205b4cb685..7e6b15f8d7 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_domain_valid_bits(const struct domain *d, bool restore)
>  {
> -    const struct domain *d = v->domain;
>      const struct cpuid_policy *p;
>      bool mce, vmxe;
>  
> @@ -963,6 +962,11 @@ unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
>              (p->feat.pku      ? X86_CR4_PKE               : 0));
>  }
>  
> +unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)

I'd split this change out into a separate patch and change the existing
guest valid bits to taking a domain *.

It needed to take vcpu in the past because of the old cpuid
infrastructure, but it doesn't need to any more because of the
domain-wide struct cpuid policy.

~Andrew

> +{
> +    return hvm_cr4_domain_valid_bits(v->domain, restore);
> +}
> +
>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  {
>      int vcpuid;
>
Sergey Dyasli Oct. 16, 2017, 7:46 a.m. UTC | #2 
On Fri, 2017-10-13 at 16:25 +0100, Andrew Cooper wrote:
> On 13/10/17 13:35, Sergey Dyasli wrote:

> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c

> > index 205b4cb685..7e6b15f8d7 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_domain_valid_bits(const struct domain *d, bool restore)

> >  {

> > -    const struct domain *d = v->domain;

> >      const struct cpuid_policy *p;

> >      bool mce, vmxe;

> >  

> > @@ -963,6 +962,11 @@ unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)

> >              (p->feat.pku      ? X86_CR4_PKE               : 0));

> >  }

> >  

> > +unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)

> 

> I'd split this change out into a separate patch and change the existing

> guest valid bits to taking a domain *.

> 

> It needed to take vcpu in the past because of the old cpuid

> infrastructure, but it doesn't need to any more because of the

> domain-wide struct cpuid policy.


That was one of possibilities so I really needed a mainteiner's opinion
on this. Thanks for providing one!

-- 
Thanks,
Sergey
diff mbox

Patch

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/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..7e6b15f8d7 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_domain_valid_bits(const struct domain *d, bool restore)
 {
-    const struct domain *d = v->domain;
     const struct cpuid_policy *p;
     bool mce, vmxe;
 
@@ -963,6 +962,11 @@  unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
             (p->feat.pku      ? X86_CR4_PKE               : 0));
 }
 
+unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
+{
+    return hvm_cr4_domain_valid_bits(v->domain, restore);
+}
+
 static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
     int vcpuid;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 388f19e50d..a22e3dfaf2 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,
@@ -220,7 +221,7 @@  static void __init calculate_hvm_max_vmx_policy(struct msr_domain_policy *dp)
     dp->vmx_cr4_fixed1.available = true;
     /*
      * Allowed CR4 bits will be updated during domain creation by
-     * hvm_cr4_guest_valid_bits()
+     * hvm_cr4_domain_valid_bits()
      */
     dp->vmx_cr4_fixed1.u.raw = host_msr_domain_policy.vmx_cr4_fixed1.u.raw;
 
@@ -312,6 +313,72 @@  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 )
+    {
+        dp->vmx_basic.available = false;
+        dp->vmx_pinbased_ctls.available = false;
+        dp->vmx_procbased_ctls.available = false;
+        dp->vmx_exit_ctls.available = false;
+        dp->vmx_entry_ctls.available = false;
+        dp->vmx_misc.available = false;
+        dp->vmx_cr0_fixed0.available = false;
+        dp->vmx_cr0_fixed1.available = false;
+        dp->vmx_cr4_fixed0.available = false;
+        dp->vmx_cr4_fixed1.available = false;
+        dp->vmx_vmcs_enum.available = false;
+        dp->vmx_procbased_ctls2.available = false;
+        dp->vmx_ept_vpid_cap.available = false;
+        dp->vmx_true_pinbased_ctls.available = false;
+        dp->vmx_true_procbased_ctls.available = false;
+        dp->vmx_true_exit_ctls.available = false;
+        dp->vmx_true_entry_ctls.available = false;
+    }
+    else
+    {
+        dp->vmx_basic.available = true;
+        dp->vmx_pinbased_ctls.available = true;
+        dp->vmx_procbased_ctls.available = true;
+        dp->vmx_exit_ctls.available = true;
+        dp->vmx_entry_ctls.available = true;
+        dp->vmx_misc.available = true;
+        dp->vmx_cr0_fixed0.available = true;
+        dp->vmx_cr0_fixed1.available = true;
+        dp->vmx_cr4_fixed0.available = true;
+        dp->vmx_cr4_fixed1.available = true;
+        /* Get allowed CR4 bits from CPUID policy */
+        dp->vmx_cr4_fixed1.u.raw = hvm_cr4_domain_valid_bits(d, false);
+        dp->vmx_vmcs_enum.available = true;
+
+        if ( dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls )
+        {
+            dp->vmx_procbased_ctls2.available = true;
+
+            if ( dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
+                 dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid )
+                dp->vmx_ept_vpid_cap.available = true;
+        }
+
+        if ( dp->vmx_basic.u.default1_zero )
+        {
+            dp->vmx_true_pinbased_ctls.available = true;
+            dp->vmx_true_procbased_ctls.available = true;
+            dp->vmx_true_exit_ctls.available = true;
+            dp->vmx_true_entry_ctls.available = true;
+        }
+    }
+
+    dp->vmx_vmfunc.available = false;
+}
+
+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;
@@ -332,6 +399,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/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index b687e03dce..6ff38a6400 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -613,6 +613,7 @@  static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
 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_domain_valid_bits(const struct domain *d, bool restore);
 
 /*
  * This must be defined as a macro instead of an inline function,
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index fc99612cca..df8f60e538 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -649,6 +649,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 */