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.coop...@citrix.com>
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks, just one nit below.
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> CC: Jun Nakajima <jun.nakaj...@intel.com>
> CC: Kevin Tian <kevin.t...@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.
