|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/4] x86/mm: handle foreign mappings in p2m_entry_modify
> From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx] > 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.pau@xxxxxxxxxx> > --- > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx> > Cc: Jan Beulich <jbeulich@xxxxxxxx> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx> > Cc: Tim Deegan <tim@xxxxxxx> > --- > 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... > > switch ( nt ) > { > @@ -948,6 +951,18 @@ static inline void p2m_entry_modify(struct > p2m_domain *p2m, p2m_type_t nt, > p2m->ioreq.entry_count++; > break; > > + case p2m_map_foreign: > + if ( !mfn_valid(nfn) ) > + { > + ASSERT_UNREACHABLE(); > + return -EINVAL; > + } > + > + if ( !page_get_owner_and_reference(mfn_to_page(nfn)) ) > + return -EBUSY; > + > + break; > + > default: > break; > } > @@ -959,9 +974,20 @@ static inline void p2m_entry_modify(struct > p2m_domain *p2m, p2m_type_t nt, > p2m->ioreq.entry_count--; > break; > > + case p2m_map_foreign: > + if ( !mfn_valid(ofn) ) > + { > + ASSERT_UNREACHABLE(); > + return -EINVAL; > + } > + put_page(mfn_to_page(ofn)); > + break; > + > default: > break; > } > + > + return 0; > } > > #endif /* _XEN_ASM_X86_P2M_H */ > -- > 2.17.2 (Apple Git-113) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |