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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	Jan Beulich <JBeulich@suse.com>, Wei Liu <wl@xen.org>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Kevin Tian <kevin.tian@intel.com>
Subject: Re: [PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()
Date: Thu, 1 Oct 2020 13:00:03 +0200	[thread overview]
Message-ID: <20201001110003.GE19254@Air-de-Roger> (raw)
In-Reply-To: <20200930134248.4918-8-andrew.cooper3@citrix.com>

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.