On 2/19/19 6:14 AM, Tian, Kevin wrote: >> From: Roger Pau Monne [mailto:roger....@citrix.com] >> Sent: Tuesday, February 19, 2019 1:27 AM >> >> So that the specific handling can be removed from >> atomic_write_ept_entry and be shared with npt and shadow code. >> >> This commit also removes the check that prevent non-ept PVH dom0 from >> mapping foreign pages. > > ah, so please ignore my comment to [1/4]. p2m_entry_modify > is used more than type change now. :-) > >> >> Signed-off-by: Roger Pau Monné <roger....@citrix.com> >> --- >> Cc: George Dunlap <george.dun...@eu.citrix.com> >> Cc: Jan Beulich <jbeul...@suse.com> >> Cc: Andrew Cooper <andrew.coop...@citrix.com> >> Cc: Wei Liu <wei.l...@citrix.com> >> Cc: Jun Nakajima <jun.nakaj...@intel.com> >> Cc: Kevin Tian <kevin.t...@intel.com> >> Cc: Tim Deegan <t...@xen.org> >> --- >> Changes since v3: >> - Replace the mfn_valid BUG_ONs with an assert & return. >> >> Changes since v2: >> - Return an error code from p2m_entry_modify and propagate it to the >> callers. >> >> Changes since v1: >> - Simply code since mfn_to_page cannot return NULL. >> - Check if the mfn is valid before getting/dropping the page reference. >> - Use BUG_ON instead of ASSERTs, since getting the reference counting >> wrong is more dangerous than a DoS. >> --- >> xen/arch/x86/mm/hap/hap.c | 12 +++++-- >> xen/arch/x86/mm/p2m-ept.c | 56 +++------------------------------ >> xen/arch/x86/mm/p2m-pt.c | 7 ----- >> xen/arch/x86/mm/shadow/common.c | 3 +- >> xen/include/asm-x86/p2m.h | 34 +++++++++++++++++--- >> 5 files changed, 47 insertions(+), 65 deletions(-) >> >> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c >> index 5b507376bc..2daf8424f6 100644 >> --- a/xen/arch/x86/mm/hap/hap.c >> +++ b/xen/arch/x86/mm/hap/hap.c >> @@ -714,6 +714,7 @@ hap_write_p2m_entry(struct domain *d, unsigned >> long gfn, l1_pgentry_t *p, >> { >> uint32_t old_flags; >> bool_t flush_nestedp2m = 0; >> + int rc; >> >> /* We know always use the host p2m here, regardless if the vcpu >> * is in host or guest mode. The vcpu can be in guest mode by >> @@ -734,8 +735,15 @@ hap_write_p2m_entry(struct domain *d, unsigned >> long gfn, l1_pgentry_t *p, >> && perms_strictly_increased(old_flags, l1e_get_flags(new)) ); >> } >> >> - p2m_entry_modify(p2m_get_hostp2m(d), >> p2m_flags_to_type(l1e_get_flags(new)), >> - p2m_flags_to_type(old_flags), level); >> + rc = p2m_entry_modify(p2m_get_hostp2m(d), >> + p2m_flags_to_type(l1e_get_flags(new)), >> + p2m_flags_to_type(old_flags), l1e_get_mfn(new), >> + l1e_get_mfn(*p), level); > > why not passing old/new pte to reduce #parameters and thus stable > against future changes? > >> + if ( rc ) >> + { >> + paging_unlock(d); >> + return rc; >> + } >> >> safe_write_pte(p, new); >> if ( old_flags & _PAGE_PRESENT ) >> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c >> index 0ece6608cb..83bd602fc4 100644 >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -45,65 +45,19 @@ static inline bool_t is_epte_valid(ept_entry_t *e) >> return ((e->epte & ~(1ul << 63)) != 0 && e->sa_p2mt != p2m_invalid); >> } >> >> -/* returns : 0 for success, -errno otherwise */ >> static int atomic_write_ept_entry(struct p2m_domain *p2m, >> ept_entry_t *entryptr, ept_entry_t new, >> int level) >> { >> - int rc; >> - unsigned long oldmfn = mfn_x(INVALID_MFN); >> - bool_t check_foreign = (new.mfn != entryptr->mfn || >> - new.sa_p2mt != entryptr->sa_p2mt); >> - >> - if ( level ) >> - { >> - ASSERT(!is_epte_superpage(&new) >> || !p2m_is_foreign(new.sa_p2mt)); >> - write_atomic(&entryptr->epte, new.epte); >> - return 0; >> - } >> - >> - if ( unlikely(p2m_is_foreign(new.sa_p2mt)) ) >> - { >> - rc = -EINVAL; >> - if ( !is_epte_present(&new) ) >> - goto out; >> - >> - if ( check_foreign ) >> - { >> - struct domain *fdom; >> - >> - if ( !mfn_valid(_mfn(new.mfn)) ) >> - goto out; >> - >> - rc = -ESRCH; >> - fdom = page_get_owner(mfn_to_page(_mfn(new.mfn))); >> - if ( fdom == NULL ) >> - goto out; >> + int rc = p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt, >> + _mfn(new.mfn), _mfn(entryptr->mfn), level); >> >> - /* get refcount on the page */ >> - rc = -EBUSY; >> - if ( !get_page(mfn_to_page(_mfn(new.mfn)), fdom) ) >> - goto out; >> - } >> - } >> - >> - if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && check_foreign ) >> - oldmfn = entryptr->mfn; >> - >> - p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt, level); >> + if ( rc ) >> + return rc; >> >> write_atomic(&entryptr->epte, new.epte); >> >> - if ( unlikely(oldmfn != mfn_x(INVALID_MFN)) ) >> - put_page(mfn_to_page(_mfn(oldmfn))); >> - >> - rc = 0; >> - >> - out: >> - if ( rc ) >> - gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n", >> - entryptr->epte, new.epte, rc); >> - return rc; >> + return 0; >> } >> >> static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t >> *entry, >> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c >> index 3a8dc04efc..57afa37c71 100644 >> --- a/xen/arch/x86/mm/p2m-pt.c >> +++ b/xen/arch/x86/mm/p2m-pt.c >> @@ -564,13 +564,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, >> gfn_t gfn_, mfn_t mfn, >> __trace_var(TRC_MEM_SET_P2M_ENTRY, 0, sizeof(t), &t); >> } >> >> - if ( unlikely(p2m_is_foreign(p2mt)) ) >> - { >> - /* hvm fixme: foreign types are only supported on ept at present */ >> - gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n"); >> - return -EINVAL; >> - } >> - >> /* Carry out any eventually pending earlier changes first. */ >> rc = do_recalc(p2m, gfn); >> if ( rc < 0 ) >> diff --git a/xen/arch/x86/mm/shadow/common.c >> b/xen/arch/x86/mm/shadow/common.c >> index fe48c4a02b..ad670de515 100644 >> --- a/xen/arch/x86/mm/shadow/common.c >> +++ b/xen/arch/x86/mm/shadow/common.c >> @@ -3189,7 +3189,8 @@ shadow_write_p2m_entry(struct domain *d, >> unsigned long gfn, >> sh_unshadow_for_p2m_change(d, gfn, p, new, level); >> >> p2m_entry_modify(p2m_get_hostp2m(d), >> p2m_flags_to_type(l1e_get_flags(new)), >> - p2m_flags_to_type(l1e_get_flags(*p)), level); >> + p2m_flags_to_type(l1e_get_flags(*p)), l1e_get_mfn(new), >> + l1e_get_mfn(*p), level); >> >> /* Update the entry with new content */ >> safe_write_pte(p, new); >> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h >> index f4ec2becbd..2801a8ccca 100644 >> --- a/xen/include/asm-x86/p2m.h >> +++ b/xen/include/asm-x86/p2m.h >> @@ -932,11 +932,14 @@ int p2m_set_ioreq_server(struct domain *d, >> unsigned int flags, >> struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d, >> unsigned int *flags); >> >> -static inline void p2m_entry_modify(struct p2m_domain *p2m, >> p2m_type_t nt, >> - p2m_type_t ot, unsigned int level) >> +static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t >> nt, >> + p2m_type_t ot, mfn_t nfn, mfn_t ofn, >> + unsigned int level) >> { >> - if ( level != 1 || nt == ot ) >> - return; >> + BUG_ON(level > 1 && (nt == p2m_ioreq_server || nt == >> p2m_map_foreign)); >> + >> + if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) ) >> + return 0; > > could also return here in case of nt==ot==p2m_ioreq_server, > otherwise p2m->ioreq.entry_count might be unnecessarily > inc/dec if mfn changes while type stays as p2m_ioreq_server...
That's an interesting idea. But it would have you do an extra check for all operations, rather than doing an extra increment / decrement in a very unusual corner case. Probably faster just to let the uncommon case be a tiny bit slower. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
