On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote: > This patch moves common code from p2m_set_altp2m_mem_access() and > p2m_change_altp2m_gfn() into one function > > Signed-off-by: Alexandru Isaila This patch contains a lot of behavioral changes which aren't mentioned or explained. For instance... > --- > Changes since V2: > - Change var name from found_in_hostp2m to copied_from_hostp2m > - Move the type check from altp2m_get_gfn_type_access() to the > callers. > --- > xen/arch/x86/mm/mem_access.c | 32 ++++++++++++---------------- > xen/arch/x86/mm/p2m.c | 41 ++++++++++++++---------------------- > xen/include/asm-x86/p2m.h | 19 +++++++++++++++++ > 3 files changed, 49 insertions(+), 43 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > index 56c06a4fc6..bf67ddb15a 100644 > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > unsigned int page_order; > unsigned long gfn_l = gfn_x(gfn); > int rc; > + bool copied_from_hostp2m; > > - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > + mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, &copied_from_hostp2m); > > - /* Check host p2m if no valid entry in alternate */ > if ( !mfn_valid(mfn) ) > + return -ESRCH; > + > + /* If this is a superpage, copy that first */ > + if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m ) > { > + unsigned long mask = ~((1UL << page_order) - 1); > + gfn_t gfn2 = _gfn(gfn_l & mask); > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > > - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > + /* Note: currently it is not safe to remap to a shared entry */ > + if ( t != p2m_ram_rw ) > + return -ESRCH; > > - rc = -ESRCH; > - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > + rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1); > + if ( rc ) > return rc; > - > - /* If this is a superpage, copy that first */ > - if ( page_order != PAGE_ORDER_4K ) > - { > - unsigned long mask = ~((1UL << page_order) - 1); > - gfn_t gfn2 = _gfn(gfn_l & mask); > - mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > - > - rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1); > - if ( rc ) > - return rc; > - } > } > > /* > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index b9bbb8f485..d38d7c29ca 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, > mfn_t mfn; > unsigned int page_order; > int rc = -EINVAL; > + bool copied_from_hostp2m; > > if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) > return rc; > @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, > p2m_lock(hp2m); > p2m_lock(ap2m); > > - mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL); > + mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m); Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at all. Now, the hostp2m will have __get_gfn_type_access() called with P2M_ALLOC | P2M_UNSHARE. Is that change intentional, and if so, why? > > if ( gfn_eq(new_gfn, INVALID_GFN) ) > { > @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, > goto out; > } > > - /* Check host p2m if no valid entry in alternate */ > - if ( !mfn_valid(mfn) ) > - { > - mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a, > - P2M_ALLOC, &page_order, 0); > + if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) ) > + goto out; > > - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > - goto out; > - > - /* If this is a superpage, copy that first */ > - if ( page_order != PAGE_ORDER_4K ) > - { > - gfn_t gfn; > - unsigned long mask; > + /* If this is a superpage, copy that first */ > + if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m ) > + { > + gfn_t gfn; > + unsigned long mask; > > - mask = ~((1UL << page_order) - 1); > - gfn = _gfn(gfn_x(old_gfn) & mask); > - mfn = _mfn(mfn_x(mfn) & mask); > + mask = ~((1UL << page_order) - 1); > + gfn = _gfn(gfn_x(old_gfn) & mask); > + mfn = _mfn(mfn_x(mfn) & mask); > > - if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) ) > - goto out; > - } > + if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) ) > + goto out; > } > > - mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL); > - > - if ( !mfn_valid(mfn) ) > - mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL); > + mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, &copied_from_hostp2m); Similiarly here: Before this patch, hp2m->get_entry() is called directly; after this patch, we go through __get_gfn_type_access(), which adds some extra code, and will also unshare / allocate. Is that intentional, and if so, why? > > /* Note: currently it is not safe to remap to a shared entry */ > - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) ) > + if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) ) > goto out; > > if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a, > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index 2801a8ccca..6de1546d76 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type( > return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL); > } > > +static inline mfn_t altp2m_get_gfn_type_access( > + struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a, > + unsigned int *page_order, bool *copied_from_hostp2m) > +{ > + mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > + > + *copied_from_hostp2m = false; > + > + /* Check host p2m if no valid entry in alternate */ > + if ( !mfn_valid(mfn) ) > + { > + mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), gfn_x(gfn), t, a, > + P2M_ALLOC | P2M_UNSHARE, page_order, false); > + *copied_from_hostp2m = mfn_valid(mfn); > + } > + > + return mfn; > +} Given that the main goal here seems to be to clean up the interface, I'm not clear why you don't have this function do both the "host see-through" and the prepopulation. Would something like the attached work (not even compile tested)? (To be clear, this is just meant to be a sketch so you can see what I'm talking about; if you were to use it you'd need to fix it up appropriately, including considering whether "seethrough" is an appropriate name for the function.) -George