[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 5/4] x86/hvm: Drop hvm_{get,set}_guest_bndcfgs() and use {get,set}_regs() instead
On 19/01/2022 13:50, Jan Beulich wrote: > On 17.01.2022 20:25, Andrew Cooper wrote: >> hvm_{get,set}_guest_bndcfgs() are thin wrappers around accessing MSR_BNDCFGS. >> >> MPX was implemented on Skylake uarch CPUs and dropped in subsequent CPUs, and >> is disabled by default in Xen VMs. >> >> It would be nice to move all the logic into vmx_msr_{read,write}_intercept(), >> but the common HVM migration code uses guest_{rd,wr}msr(). Therefore, use >> {get,set}_regs() to reduce the quantity of "common" HVM code. >> >> In lieu of having hvm_set_guest_bndcfgs() split out, use some #ifdef >> CONFIG_HVM in guest_wrmsr(). In vmx_{get,set}_regs(), split the switch >> statements into two depending on whether the require remote VMCS acquisition >> or not. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> > > One remark: > >> @@ -323,10 +324,9 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t >> *val) >> break; >> >> case MSR_IA32_BNDCFGS: >> - if ( !cp->feat.mpx || !is_hvm_domain(d) || >> - !hvm_get_guest_bndcfgs(v, val) ) >> + if ( !cp->feat.mpx ) /* Implies Intel HVM only */ > Wouldn't it make sense to accompany this comment by ... > >> goto gp_fault; >> - break; > ASSERT(is_hvm_domain(d)); > > (and then the same on the "set" path)? So this is the reason for the default logic in the {get,set}_reg() path. The absence of MSR_BNDCFGS in the PV and SVM paths will cause the VM to be crashed cleanly. If you're on a VMX on a non-MPX capable system, the VMREAD/VMWRITE will hit a BUG (which in due course I want to downgrade to a domain crash). It's a bit more friendly than an ASSERT() (doesn't take the system down), is present in release builds too, and more precise as it excludes SVM too. ~Andrew P.S. I'm still trying to decide on an acceptable name to hide { ASSERT_UNREACHABLE(); gprink(); domain_crash() } behind, so we can downgrade more BUG()/etc to more runtime-friendly options.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |