1. Patchew
  2. Xen
  3. [PATCH 0/8] x86: Untangle Nested virt and CPUID interactions
  4. View patch

[PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()

Andrew Cooper posted 8 patches 5 years, 4 months ago
Maintainers: Anthony PERARD <anthony.perard@citrix.com>, Wei Liu <wl@xen.org>, Ian Jackson <iwj@xenproject.org>
[PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()
Posted by Andrew Cooper 5 years, 4 months ago 
Previously, migration was reordered so the CPUID data was available before
register state.  nestedhvm_enabled() has recently been made accurate for the
entire lifetime of the domain.

Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
previously to tolerate a guests' CR4 being set/restored before
HVM_PARAM_NESTEDHVM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/domain.c       | 2 +-
 xen/arch/x86/hvm/hvm.c          | 8 ++++----
 xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
 xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
 xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
 xen/include/asm-x86/hvm/hvm.h   | 2 +-
 6 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 8e3375265c..0ce132b308 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
     if ( v->arch.hvm.guest_efer & EFER_LME )
         v->arch.hvm.guest_efer |= EFER_LMA;
 
-    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
+    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
     {
         gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
                 v->arch.hvm.guest_cr[4]);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 101a739952..54e32e4fe8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -972,14 +972,14 @@ 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 domain *d, bool restore)
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
 {
     const struct cpuid_policy *p = d->arch.cpuid;
     bool mce, vmxe;
 
     /* Logic broken out simply to aid readability below. */
     mce  = p->basic.mce || p->basic.mca;
-    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
+    vmxe = p->basic.vmx && nestedhvm_enabled(d);
 
     return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
             (p->basic.tsc     ? X86_CR4_TSD               : 0) |
@@ -1033,7 +1033,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(d, true) )
+    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
                d->domain_id, ctxt.cr4);
@@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
     struct vcpu *v = current;
     unsigned long old_cr;
 
-    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
+    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
     {
         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 ba26b6a80b..f450391df4 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
     unsigned long cr0 = vmcb_get_cr0(vmcb);
     unsigned long cr3 = vmcb_get_cr3(vmcb);
     unsigned long cr4 = vmcb_get_cr4(vmcb);
+    unsigned long valid;
     uint64_t efer = vmcb_get_efer(vmcb);
 
 #define PRINTF(fmt, args...) do { \
@@ -130,9 +131,10 @@ 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->domain, false) )
+    valid = hvm_cr4_guest_valid_bits(v->domain);
+    if ( cr4 & ~valid )
         PRINTF("CR4: invalid bits are set (%#"PRIx64", valid: %#"PRIx64")\n",
-               cr4, hvm_cr4_guest_valid_bits(v->domain, false));
+               cr4, valid);
 
     if ( vmcb_get_dr6(vmcb) >> 32 )
         PRINTF("DR6: bits [63:32] are not zero (%#"PRIx64")\n",
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 95d109f962..86b8916a5d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1529,7 +1529,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr,
              */
             v->arch.hvm.vmx.cr4_host_mask =
                 (HVM_CR4_HOST_MASK | X86_CR4_PKE |
-                 ~hvm_cr4_guest_valid_bits(v->domain, false));
+                 ~hvm_cr4_guest_valid_bits(v->domain));
 
             v->arch.hvm.vmx.cr4_host_mask |= v->arch.hvm.vmx.vmx_realmode ?
                                              X86_CR4_VME : 0;
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 1e51689ef3..3a37e9ebea 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2323,7 +2323,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(d, false);
+        data = hvm_cr4_guest_valid_bits(d);
         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 be0d8b0a4d..334bd573b9 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -334,7 +334,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
 /* 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 domain *d, bool restore);
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d);
 
 int hvm_copy_context_and_params(struct domain *src, struct domain *dst);
 
-- 
2.11.0
Re: [PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()
Posted by Roger Pau Monné 5 years, 4 months ago 
On Wed, Sep 30, 2020 at 02:42:47PM +0100, Andrew Cooper wrote:
> Previously, migration was reordered so the CPUID data was available before
> register state.  nestedhvm_enabled() has recently been made accurate for the
> entire lifetime of the domain.
> 
> Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
> previously to tolerate a guests' CR4 being set/restored before
> HVM_PARAM_NESTEDHVM.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, just one nit below.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/domain.c       | 2 +-
>  xen/arch/x86/hvm/hvm.c          | 8 ++++----
>  xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
>  xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
>  xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
>  xen/include/asm-x86/hvm/hvm.h   | 2 +-
>  6 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 8e3375265c..0ce132b308 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
>      if ( v->arch.hvm.guest_efer & EFER_LME )
>          v->arch.hvm.guest_efer |= EFER_LMA;
>  
> -    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
> +    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
>      {
>          gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
>                  v->arch.hvm.guest_cr[4]);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 101a739952..54e32e4fe8 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -972,14 +972,14 @@ 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 domain *d, bool restore)
> +unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
>  {
>      const struct cpuid_policy *p = d->arch.cpuid;
>      bool mce, vmxe;
>  
>      /* Logic broken out simply to aid readability below. */
>      mce  = p->basic.mce || p->basic.mca;
> -    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
> +    vmxe = p->basic.vmx && nestedhvm_enabled(d);
>  
>      return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
>              (p->basic.tsc     ? X86_CR4_TSD               : 0) |
> @@ -1033,7 +1033,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(d, true) )
> +    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>      {
>          printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
>                 d->domain_id, ctxt.cr4);
> @@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
>      struct vcpu *v = current;
>      unsigned long old_cr;
>  
> -    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
> +    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
>      {
>          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 ba26b6a80b..f450391df4 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>      unsigned long cr0 = vmcb_get_cr0(vmcb);
>      unsigned long cr3 = vmcb_get_cr3(vmcb);
>      unsigned long cr4 = vmcb_get_cr4(vmcb);
> +    unsigned long valid;

Could you init valid here at definition time? Also cr4_valid might be
a better name since the sacope of the variable is quite wide.

Thanks, Roger.
Re: [PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()
Posted by Andrew Cooper 5 years, 4 months ago 
On 01/10/2020 12:00, Roger Pau Monné wrote:
> On Wed, Sep 30, 2020 at 02:42:47PM +0100, Andrew Cooper wrote:
>> Previously, migration was reordered so the CPUID data was available before
>> register state.  nestedhvm_enabled() has recently been made accurate for the
>> entire lifetime of the domain.
>>
>> Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
>> previously to tolerate a guests' CR4 being set/restored before
>> HVM_PARAM_NESTEDHVM.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,

>
> Thanks, just one nit below.
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> ---
>>  xen/arch/x86/hvm/domain.c       | 2 +-
>>  xen/arch/x86/hvm/hvm.c          | 8 ++++----
>>  xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
>>  xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
>>  xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
>>  xen/include/asm-x86/hvm/hvm.h   | 2 +-
>>  6 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
>> index 8e3375265c..0ce132b308 100644
>> --- a/xen/arch/x86/hvm/domain.c
>> +++ b/xen/arch/x86/hvm/domain.c
>> @@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
>>      if ( v->arch.hvm.guest_efer & EFER_LME )
>>          v->arch.hvm.guest_efer |= EFER_LMA;
>>  
>> -    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
>> +    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
>>      {
>>          gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
>>                  v->arch.hvm.guest_cr[4]);
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 101a739952..54e32e4fe8 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -972,14 +972,14 @@ 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 domain *d, bool restore)
>> +unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
>>  {
>>      const struct cpuid_policy *p = d->arch.cpuid;
>>      bool mce, vmxe;
>>  
>>      /* Logic broken out simply to aid readability below. */
>>      mce  = p->basic.mce || p->basic.mca;
>> -    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
>> +    vmxe = p->basic.vmx && nestedhvm_enabled(d);
>>  
>>      return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
>>              (p->basic.tsc     ? X86_CR4_TSD               : 0) |
>> @@ -1033,7 +1033,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(d, true) )
>> +    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>>      {
>>          printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
>>                 d->domain_id, ctxt.cr4);
>> @@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
>>      struct vcpu *v = current;
>>      unsigned long old_cr;
>>  
>> -    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
>> +    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
>>      {
>>          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 ba26b6a80b..f450391df4 100644
>> --- a/xen/arch/x86/hvm/svm/svmdebug.c
>> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
>> @@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>>      unsigned long cr0 = vmcb_get_cr0(vmcb);
>>      unsigned long cr3 = vmcb_get_cr3(vmcb);
>>      unsigned long cr4 = vmcb_get_cr4(vmcb);
>> +    unsigned long valid;
> Could you init valid here at definition time? Also cr4_valid might be
> a better name since the sacope of the variable is quite wide.

I have some further cleanup in mind, which is why I did it like this.

~Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.