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.