On 09/23/2018 06:04 PM, Razvan Cojocaru wrote: > Move p2m_{get/set}_suppress_ve() to p2m.c, replace incorrect > ASSERT() in p2m-pt.c (since a guest can run in shadow mode even on > a system with virt exceptions, which would trigger the ASSERT()), > and move the VMX-isms (cpu_has_vmx_virt_exceptions checks) to > p2m_ept_{get/set}_entry(). > > Signed-off-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
Thanks for the clean up. Two realtively minor comments... > @@ -931,6 +942,16 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m, > mfn_t mfn = INVALID_MFN; > struct ept_data *ept = &p2m->ept; > > + if ( sve ) > + { > + if ( !cpu_has_vmx_virt_exceptions ) > + return INVALID_MFN; > + > + /* #VE should be enabled for this vcpu. */ > + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) > + return INVALID_MFN; > + } Is there a good reason to return error her rather than just putting '1' in the sve location, like the p2m_pt.c version of this function does? > +int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, > + unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m; > + mfn_t mfn; > + p2m_access_t a; > + p2m_type_t t; > + > + /* #VE should be enabled for this vcpu. */ > + if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) > + return -ENXIO; What's the purpose of checking for this here, if we don't check for this in p2m_set_suppress_ve()? Everything else looks good, thanks! -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel