| Message ID | 20190416084531.5084-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] x86/altp2m: Aggregate get entry and populate into common funcs | expand |
On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA <aisaila@bitdefender.com> wrote: > > The code for getting the entry and then populating was repeated in > p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > > The code is now in one place with a bool param that lets the caller choose > if it populates after get_entry(). > > If remapping is being done then both the old and new gfn's should be > unshared in the hostp2m for keeping things consistent. The page type > of old_gfn was already checked whether it's p2m_ram_rw and bail if it > wasn't so functionality-wise this just simplifies things as a user > doesn't have to request unsharing manually before remapping. > Now, if the new_gfn is invalid it shouldn't query the hostp2m as > that is effectively a request to remove the entry from the altp2m. > But provided that scenario is used only when removing entries that > were previously remapped/copied to the altp2m, those entries already > went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > affect so the core function get_altp2m_entry() is calling > __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > > altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > because on a new altp2m view the function will fail with invalid mfn if > p2m->set_entry() was not called before. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > Reviewed-by: George Dunlap <george.dunlap@citrix.com> > > --- > Changes since V4: > - Add altp2m to patch name > - Change func name from get_altp2m_entry() to > altp2m_get_entry(). > --- > xen/arch/x86/mm/mem_access.c | 30 ++----------- > xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > xen/include/asm-x86/p2m.h | 17 ++++++++ > 3 files changed, 66 insertions(+), 65 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > index a144bb0ce4..ddfe0169c0 100644 > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > mfn_t mfn; > p2m_type_t t; > p2m_access_t old_a; > - unsigned int page_order; > - unsigned long gfn_l = gfn_x(gfn); > int rc; > > - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > - > - /* Check host p2m if no valid entry in alternate */ > - if ( !mfn_valid(mfn) ) > - { > - > - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > - > - rc = -ESRCH; > - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > - 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; > - } > - } > + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > + if ( rc ) > + return rc; > > /* > * Inherit the old suppress #VE bit value if it is already set, or set it > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 9e81a30cc4..7bedfd593b 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c Wouldn't it make more sense to start adding new altp2m functions to mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could also be relocated there at some point in the future. > @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > mm_write_unlock(&p2m->lock); > } > > +int altp2m_get_entry(struct p2m_domain *ap2m, > + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > + p2m_access_t *a, bool prepopulate) > +{ > + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > + > + /* Check host p2m if no valid entry in alternate */ > + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > + { > + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > + unsigned int page_order; > + int rc; > + > + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); So despite the name being altp2m_get_entry you now return an entry from the hostp2m, even if prepopulate is false. If the caller knows it doesn't want that entry to be copied into the altp2m, why not have it call __get_gfn_type_access itself for the hostp2m? IMHO this is just confusing and doesn't help readability of the altp2m code. > + > + rc = -ESRCH; > + if ( !mfn_valid(*mfn) || *t != p2m_ram_rw ) > + return rc; > + > + /* If this is a superpage, copy that first */ > + if ( prepopulate && page_order != PAGE_ORDER_4K ) > + { > + unsigned long mask = ~((1UL << page_order) - 1); > + gfn_t gfn_aligned = _gfn(gfn_x(gfn) & mask); > + mfn_t mfn_aligned = _mfn(mfn_x(*mfn) & mask); > + > + rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, *t, *a, 1); > + if ( rc ) > + return rc; > + } > + } > + > + return 0; > +} > + > + > mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l, > p2m_type_t *t, p2m_access_t *a, p2m_query_t q, > unsigned int *page_order, bool_t locked) > @@ -2618,7 +2655,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, > p2m_access_t a; > p2m_type_t t; > mfn_t mfn; > - unsigned int page_order; > int rc = -EINVAL; > > if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) > @@ -2630,47 +2666,21 @@ 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); > - > if ( gfn_eq(new_gfn, INVALID_GFN) ) > { > + mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL); > if ( mfn_valid(mfn) ) > p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K); > rc = 0; > 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 ) > - goto out; > - > - /* If this is a superpage, copy that first */ > - if ( page_order != PAGE_ORDER_4K ) > - { > - gfn_t gfn; > - unsigned long 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; > - } > - } > - > - 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); > + rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a); > + if ( rc ) > + goto out; > > - /* Note: currently it is not safe to remap to a shared entry */ > - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) ) > + rc = altp2m_get_entry_direct(ap2m, new_gfn, &mfn, &t, &a); > + if ( rc ) > goto out; > > if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a, > @@ -3002,12 +3012,10 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > if ( ap2m ) > p2m_lock(ap2m); > > - mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL); > - if ( !mfn_valid(mfn) ) > - { > - rc = -ESRCH; > + rc = altp2m_get_entry_direct(p2m, gfn, &mfn, &t, &a); > + > + if ( rc ) > goto out; > - } > > rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve); > > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index 2801a8ccca..8dc4353645 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) > return mfn_x(mfn); > } > > +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, > + p2m_type_t *t, p2m_access_t *a, bool prepopulate); > + > +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m, > + gfn_t gfn, mfn_t *mfn, > + p2m_type_t *t, p2m_access_t *a) > +{ > + return altp2m_get_entry(ap2m, gfn, mfn, t, a, false); > +} > + > +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m, > + gfn_t gfn, mfn_t *mfn, > + p2m_type_t *t, p2m_access_t *a) > +{ > + return altp2m_get_entry(ap2m, gfn, mfn, t, a, true); > +} Are these wrappers really required? I don't think they add anything to readability, it's just yet another layer that does almost nothing. Tamas
On 16/04/2019 14:44, Tamas K Lengyel wrote: > >> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h >> index 2801a8ccca..8dc4353645 100644 >> --- a/xen/include/asm-x86/p2m.h >> +++ b/xen/include/asm-x86/p2m.h >> @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) >> return mfn_x(mfn); >> } >> >> +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, >> + p2m_type_t *t, p2m_access_t *a, bool prepopulate); >> + >> +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m, >> + gfn_t gfn, mfn_t *mfn, >> + p2m_type_t *t, p2m_access_t *a) >> +{ >> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, false); >> +} >> + >> +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m, >> + gfn_t gfn, mfn_t *mfn, >> + p2m_type_t *t, p2m_access_t *a) >> +{ >> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, true); >> +} > Are these wrappers really required? I don't think they add anything to > readability, it's just yet another layer that does almost nothing. From a readability point of view, boolean parameters are about as opaque as they come. The same goes for all other scalar parameters which don't have a mnemonic. For someone who is not an expert of the intricacies of the subsystem, having the options spelt out in wrappers like this is extremely helpful to reduce the cognitive load of trying to follow the code. If it were up to me, all of our code would be wrapped like this, but I know that others have differing opinions. ~Andrew
On 4/16/19 2:51 PM, Andrew Cooper wrote: > On 16/04/2019 14:44, Tamas K Lengyel wrote: >> >>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h >>> index 2801a8ccca..8dc4353645 100644 >>> --- a/xen/include/asm-x86/p2m.h >>> +++ b/xen/include/asm-x86/p2m.h >>> @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) >>> return mfn_x(mfn); >>> } >>> >>> +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, >>> + p2m_type_t *t, p2m_access_t *a, bool prepopulate); >>> + >>> +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m, >>> + gfn_t gfn, mfn_t *mfn, >>> + p2m_type_t *t, p2m_access_t *a) >>> +{ >>> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, false); >>> +} >>> + >>> +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m, >>> + gfn_t gfn, mfn_t *mfn, >>> + p2m_type_t *t, p2m_access_t *a) >>> +{ >>> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, true); >>> +} >> Are these wrappers really required? I don't think they add anything to >> readability, it's just yet another layer that does almost nothing. > > From a readability point of view, boolean parameters are about as opaque > as they come. The same goes for all other scalar parameters which don't > have a mnemonic. > > For someone who is not an expert of the intricacies of the subsystem, > having the options spelt out in wrappers like this is extremely helpful > to reduce the cognitive load of trying to follow the code. This is exactly why I did it this way. The other pattern I think is tolerable is having a #define to use as an argument; for example: #define AP2MGET_prepopulate true #define AP2MGET_peek false Then you get: mfn = altp2m_get_entry(..., AP2MGET_prepopulate); But then you run into namespacing issues with the prefixes. I wouldn't object to doing it the above way, but I think the wrappers is probably simpler and clearer for this case. I do object to passing in naked booleans. -George
On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > <aisaila@bitdefender.com> wrote: >> >> The code for getting the entry and then populating was repeated in >> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >> >> The code is now in one place with a bool param that lets the caller choose >> if it populates after get_entry(). >> >> If remapping is being done then both the old and new gfn's should be >> unshared in the hostp2m for keeping things consistent. The page type >> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >> wasn't so functionality-wise this just simplifies things as a user >> doesn't have to request unsharing manually before remapping. >> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >> that is effectively a request to remove the entry from the altp2m. >> But provided that scenario is used only when removing entries that >> were previously remapped/copied to the altp2m, those entries already >> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >> affect so the core function get_altp2m_entry() is calling >> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >> >> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >> because on a new altp2m view the function will fail with invalid mfn if >> p2m->set_entry() was not called before. >> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >> >> --- >> Changes since V4: >> - Add altp2m to patch name >> - Change func name from get_altp2m_entry() to >> altp2m_get_entry(). >> --- >> xen/arch/x86/mm/mem_access.c | 30 ++----------- >> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >> xen/include/asm-x86/p2m.h | 17 ++++++++ >> 3 files changed, 66 insertions(+), 65 deletions(-) >> >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >> index a144bb0ce4..ddfe0169c0 100644 >> --- a/xen/arch/x86/mm/mem_access.c >> +++ b/xen/arch/x86/mm/mem_access.c >> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >> mfn_t mfn; >> p2m_type_t t; >> p2m_access_t old_a; >> - unsigned int page_order; >> - unsigned long gfn_l = gfn_x(gfn); >> int rc; >> >> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >> - >> - /* Check host p2m if no valid entry in alternate */ >> - if ( !mfn_valid(mfn) ) >> - { >> - >> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >> - >> - rc = -ESRCH; >> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >> - 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; >> - } >> - } >> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >> + if ( rc ) >> + return rc; >> >> /* >> * Inherit the old suppress #VE bit value if it is already set, or set it >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index 9e81a30cc4..7bedfd593b 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c > > Wouldn't it make more sense to start adding new altp2m functions to > mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > also be relocated there at some point in the future. > >> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >> mm_write_unlock(&p2m->lock); >> } >> >> +int altp2m_get_entry(struct p2m_domain *ap2m, >> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >> + p2m_access_t *a, bool prepopulate) >> +{ >> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >> + >> + /* Check host p2m if no valid entry in alternate */ >> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >> + { >> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >> + unsigned int page_order; >> + int rc; >> + >> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > > So despite the name being altp2m_get_entry you now return an entry > from the hostp2m, even if prepopulate is false. If the caller knows it > doesn't want that entry to be copied into the altp2m, why not have it > call __get_gfn_type_access itself for the hostp2m? IMHO this is just > confusing and doesn't help readability of the altp2m code. You return the ap2m entry if it's present, or the hp2m entry if it's not. It's not a lot of duplication, but it makes the logic cleaner I think; why not deduplicate it? -George
On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > > On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > > <aisaila@bitdefender.com> wrote: > >> > >> The code for getting the entry and then populating was repeated in > >> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > >> > >> The code is now in one place with a bool param that lets the caller choose > >> if it populates after get_entry(). > >> > >> If remapping is being done then both the old and new gfn's should be > >> unshared in the hostp2m for keeping things consistent. The page type > >> of old_gfn was already checked whether it's p2m_ram_rw and bail if it > >> wasn't so functionality-wise this just simplifies things as a user > >> doesn't have to request unsharing manually before remapping. > >> Now, if the new_gfn is invalid it shouldn't query the hostp2m as > >> that is effectively a request to remove the entry from the altp2m. > >> But provided that scenario is used only when removing entries that > >> were previously remapped/copied to the altp2m, those entries already > >> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > >> affect so the core function get_altp2m_entry() is calling > >> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > >> > >> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > >> because on a new altp2m view the function will fail with invalid mfn if > >> p2m->set_entry() was not called before. > >> > >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > >> Reviewed-by: George Dunlap <george.dunlap@citrix.com> > >> > >> --- > >> Changes since V4: > >> - Add altp2m to patch name > >> - Change func name from get_altp2m_entry() to > >> altp2m_get_entry(). > >> --- > >> xen/arch/x86/mm/mem_access.c | 30 ++----------- > >> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > >> xen/include/asm-x86/p2m.h | 17 ++++++++ > >> 3 files changed, 66 insertions(+), 65 deletions(-) > >> > >> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > >> index a144bb0ce4..ddfe0169c0 100644 > >> --- a/xen/arch/x86/mm/mem_access.c > >> +++ b/xen/arch/x86/mm/mem_access.c > >> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > >> mfn_t mfn; > >> p2m_type_t t; > >> p2m_access_t old_a; > >> - unsigned int page_order; > >> - unsigned long gfn_l = gfn_x(gfn); > >> int rc; > >> > >> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >> - > >> - /* Check host p2m if no valid entry in alternate */ > >> - if ( !mfn_valid(mfn) ) > >> - { > >> - > >> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > >> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >> - > >> - rc = -ESRCH; > >> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >> - 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; > >> - } > >> - } > >> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > >> + if ( rc ) > >> + return rc; > >> > >> /* > >> * Inherit the old suppress #VE bit value if it is already set, or set it > >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >> index 9e81a30cc4..7bedfd593b 100644 > >> --- a/xen/arch/x86/mm/p2m.c > >> +++ b/xen/arch/x86/mm/p2m.c > > > > Wouldn't it make more sense to start adding new altp2m functions to > > mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > > also be relocated there at some point in the future. > > > >> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > >> mm_write_unlock(&p2m->lock); > >> } > >> > >> +int altp2m_get_entry(struct p2m_domain *ap2m, > >> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > >> + p2m_access_t *a, bool prepopulate) > >> +{ > >> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > >> + > >> + /* Check host p2m if no valid entry in alternate */ > >> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > >> + { > >> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > >> + unsigned int page_order; > >> + int rc; > >> + > >> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > >> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > > > > So despite the name being altp2m_get_entry you now return an entry > > from the hostp2m, even if prepopulate is false. If the caller knows it > > doesn't want that entry to be copied into the altp2m, why not have it > > call __get_gfn_type_access itself for the hostp2m? IMHO this is just > > confusing and doesn't help readability of the altp2m code. > > You return the ap2m entry if it's present, or the hp2m entry if it's > not. It's not a lot of duplication, but it makes the logic cleaner I > think; why not deduplicate it? I have no problem with making the code more streamlined. The problem I have is that the function's name doesn't suggest it would get you anything but the entry from the specified altp2m. So you could be reading the code assuming you are dealing with an entry from that specified table when in fact you are not. That is not an expected behavior based on just the name of the function. This is going to make reading the altp2m code that much harder in the future. Tamas
On Tue, Apr 16, 2019 at 7:58 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 4/16/19 2:51 PM, Andrew Cooper wrote: > > On 16/04/2019 14:44, Tamas K Lengyel wrote: > >> > >>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > >>> index 2801a8ccca..8dc4353645 100644 > >>> --- a/xen/include/asm-x86/p2m.h > >>> +++ b/xen/include/asm-x86/p2m.h > >>> @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) > >>> return mfn_x(mfn); > >>> } > >>> > >>> +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, > >>> + p2m_type_t *t, p2m_access_t *a, bool prepopulate); > >>> + > >>> +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m, > >>> + gfn_t gfn, mfn_t *mfn, > >>> + p2m_type_t *t, p2m_access_t *a) > >>> +{ > >>> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, false); > >>> +} > >>> + > >>> +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m, > >>> + gfn_t gfn, mfn_t *mfn, > >>> + p2m_type_t *t, p2m_access_t *a) > >>> +{ > >>> + return altp2m_get_entry(ap2m, gfn, mfn, t, a, true); > >>> +} > >> Are these wrappers really required? I don't think they add anything to > >> readability, it's just yet another layer that does almost nothing. > > > > From a readability point of view, boolean parameters are about as opaque > > as they come. The same goes for all other scalar parameters which don't > > have a mnemonic. > > > > For someone who is not an expert of the intricacies of the subsystem, > > having the options spelt out in wrappers like this is extremely helpful > > to reduce the cognitive load of trying to follow the code. > > This is exactly why I did it this way. > > The other pattern I think is tolerable is having a #define to use as an > argument; for example: > > #define AP2MGET_prepopulate true > #define AP2MGET_peek false > > Then you get: > > mfn = altp2m_get_entry(..., AP2MGET_prepopulate); > > But then you run into namespacing issues with the prefixes. > > I wouldn't object to doing it the above way, but I think the wrappers is > probably simpler and clearer for this case. I do object to passing in > naked booleans. Fair enough. I personally prefer the #define route though, it just avoids creating lasagna code. With wrappers by the time I dig through them trying to figure out where they actually land I tend to forget why I was looking for it in the first place. Perhaps that's just me.. Tamas
On 4/16/19 3:19 PM, Tamas K Lengyel wrote: > On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >> >> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>> <aisaila@bitdefender.com> wrote: >>>> >>>> The code for getting the entry and then populating was repeated in >>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>> >>>> The code is now in one place with a bool param that lets the caller choose >>>> if it populates after get_entry(). >>>> >>>> If remapping is being done then both the old and new gfn's should be >>>> unshared in the hostp2m for keeping things consistent. The page type >>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>> wasn't so functionality-wise this just simplifies things as a user >>>> doesn't have to request unsharing manually before remapping. >>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>> that is effectively a request to remove the entry from the altp2m. >>>> But provided that scenario is used only when removing entries that >>>> were previously remapped/copied to the altp2m, those entries already >>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>> affect so the core function get_altp2m_entry() is calling >>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>> >>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>> because on a new altp2m view the function will fail with invalid mfn if >>>> p2m->set_entry() was not called before. >>>> >>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>> >>>> --- >>>> Changes since V4: >>>> - Add altp2m to patch name >>>> - Change func name from get_altp2m_entry() to >>>> altp2m_get_entry(). >>>> --- >>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>> >>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>> index a144bb0ce4..ddfe0169c0 100644 >>>> --- a/xen/arch/x86/mm/mem_access.c >>>> +++ b/xen/arch/x86/mm/mem_access.c >>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>> mfn_t mfn; >>>> p2m_type_t t; >>>> p2m_access_t old_a; >>>> - unsigned int page_order; >>>> - unsigned long gfn_l = gfn_x(gfn); >>>> int rc; >>>> >>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>> - >>>> - /* Check host p2m if no valid entry in alternate */ >>>> - if ( !mfn_valid(mfn) ) >>>> - { >>>> - >>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>> - >>>> - rc = -ESRCH; >>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>> - 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; >>>> - } >>>> - } >>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>> + if ( rc ) >>>> + return rc; >>>> >>>> /* >>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>> index 9e81a30cc4..7bedfd593b 100644 >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>> >>> Wouldn't it make more sense to start adding new altp2m functions to >>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>> also be relocated there at some point in the future. >>> >>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>> mm_write_unlock(&p2m->lock); >>>> } >>>> >>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>> + p2m_access_t *a, bool prepopulate) >>>> +{ >>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>> + >>>> + /* Check host p2m if no valid entry in alternate */ >>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>> + { >>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>> + unsigned int page_order; >>>> + int rc; >>>> + >>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>> >>> So despite the name being altp2m_get_entry you now return an entry >>> from the hostp2m, even if prepopulate is false. If the caller knows it >>> doesn't want that entry to be copied into the altp2m, why not have it >>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>> confusing and doesn't help readability of the altp2m code. >> >> You return the ap2m entry if it's present, or the hp2m entry if it's >> not. It's not a lot of duplication, but it makes the logic cleaner I >> think; why not deduplicate it? > > I have no problem with making the code more streamlined. The problem I > have is that the function's name doesn't suggest it would get you > anything but the entry from the specified altp2m. So you could be > reading the code assuming you are dealing with an entry from that > specified table when in fact you are not. That is not an expected > behavior based on just the name of the function. This is going to make > reading the altp2m code that much harder in the future. Right -- I wasn't a huge fan of 'direct' either; it didn't really convey to me 100% what the function did. My PoC had "seethrough", but that wasn't that great either. "Peek"? Any other suggestions? Other options: * If we have a single function with a #define, this might get a bit easier; we could have one be AP2MGET_dont_prepopulate or something. ( We could have the "core" function named _altp2m_get_entry, and have altp2m_get_entry() call with prepopulate = false, and altp2m_get_entry_prepopulate() call it with prepopulate = true. Thoughts? -George
On 16.04.2019 18:07, George Dunlap wrote: > On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>> >>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>> <aisaila@bitdefender.com> wrote: >>>>> >>>>> The code for getting the entry and then populating was repeated in >>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>> >>>>> The code is now in one place with a bool param that lets the caller choose >>>>> if it populates after get_entry(). >>>>> >>>>> If remapping is being done then both the old and new gfn's should be >>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>> wasn't so functionality-wise this just simplifies things as a user >>>>> doesn't have to request unsharing manually before remapping. >>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>> that is effectively a request to remove the entry from the altp2m. >>>>> But provided that scenario is used only when removing entries that >>>>> were previously remapped/copied to the altp2m, those entries already >>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>> affect so the core function get_altp2m_entry() is calling >>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>> >>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>> p2m->set_entry() was not called before. >>>>> >>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>> >>>>> --- >>>>> Changes since V4: >>>>> - Add altp2m to patch name >>>>> - Change func name from get_altp2m_entry() to >>>>> altp2m_get_entry(). >>>>> --- >>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>> mfn_t mfn; >>>>> p2m_type_t t; >>>>> p2m_access_t old_a; >>>>> - unsigned int page_order; >>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>> int rc; >>>>> >>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>> - >>>>> - /* Check host p2m if no valid entry in alternate */ >>>>> - if ( !mfn_valid(mfn) ) >>>>> - { >>>>> - >>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>> - >>>>> - rc = -ESRCH; >>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>> - 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; >>>>> - } >>>>> - } >>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>> + if ( rc ) >>>>> + return rc; >>>>> >>>>> /* >>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>> --- a/xen/arch/x86/mm/p2m.c >>>>> +++ b/xen/arch/x86/mm/p2m.c >>>> >>>> Wouldn't it make more sense to start adding new altp2m functions to >>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>> also be relocated there at some point in the future. >>>> >>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>> mm_write_unlock(&p2m->lock); >>>>> } >>>>> >>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>> + p2m_access_t *a, bool prepopulate) >>>>> +{ >>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>> + >>>>> + /* Check host p2m if no valid entry in alternate */ >>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>> + { >>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>> + unsigned int page_order; >>>>> + int rc; >>>>> + >>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>> >>>> So despite the name being altp2m_get_entry you now return an entry >>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>> doesn't want that entry to be copied into the altp2m, why not have it >>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>> confusing and doesn't help readability of the altp2m code. >>> >>> You return the ap2m entry if it's present, or the hp2m entry if it's >>> not. It's not a lot of duplication, but it makes the logic cleaner I >>> think; why not deduplicate it? >> >> I have no problem with making the code more streamlined. The problem I >> have is that the function's name doesn't suggest it would get you >> anything but the entry from the specified altp2m. So you could be >> reading the code assuming you are dealing with an entry from that >> specified table when in fact you are not. That is not an expected >> behavior based on just the name of the function. This is going to make >> reading the altp2m code that much harder in the future. > > Right -- I wasn't a huge fan of 'direct' either; it didn't really convey > to me 100% what the function did. My PoC had "seethrough", but that > wasn't that great either. "Peek"? Any other suggestions? > > Other options: > > * If we have a single function with a #define, this might get a bit > easier; we could have one be AP2MGET_dont_prepopulate or something. > > ( We could have the "core" function named _altp2m_get_entry, and have > altp2m_get_entry() call with prepopulate = false, and > altp2m_get_entry_prepopulate() call it with prepopulate = true. This option with no defines seems to solve more of the naming problems but it will still introduce the spaghetti code. I vote for this one and if Tamas agrees I will have it this way in the next version. Alex
On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA <aisaila@bitdefender.com> wrote: > > > > On 16.04.2019 18:07, George Dunlap wrote: > > On 4/16/19 3:19 PM, Tamas K Lengyel wrote: > >> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>> > >>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > >>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > >>>> <aisaila@bitdefender.com> wrote: > >>>>> > >>>>> The code for getting the entry and then populating was repeated in > >>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > >>>>> > >>>>> The code is now in one place with a bool param that lets the caller choose > >>>>> if it populates after get_entry(). > >>>>> > >>>>> If remapping is being done then both the old and new gfn's should be > >>>>> unshared in the hostp2m for keeping things consistent. The page type > >>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it > >>>>> wasn't so functionality-wise this just simplifies things as a user > >>>>> doesn't have to request unsharing manually before remapping. > >>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as > >>>>> that is effectively a request to remove the entry from the altp2m. > >>>>> But provided that scenario is used only when removing entries that > >>>>> were previously remapped/copied to the altp2m, those entries already > >>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > >>>>> affect so the core function get_altp2m_entry() is calling > >>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > >>>>> > >>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > >>>>> because on a new altp2m view the function will fail with invalid mfn if > >>>>> p2m->set_entry() was not called before. > >>>>> > >>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > >>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > >>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> > >>>>> > >>>>> --- > >>>>> Changes since V4: > >>>>> - Add altp2m to patch name > >>>>> - Change func name from get_altp2m_entry() to > >>>>> altp2m_get_entry(). > >>>>> --- > >>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- > >>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > >>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ > >>>>> 3 files changed, 66 insertions(+), 65 deletions(-) > >>>>> > >>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > >>>>> index a144bb0ce4..ddfe0169c0 100644 > >>>>> --- a/xen/arch/x86/mm/mem_access.c > >>>>> +++ b/xen/arch/x86/mm/mem_access.c > >>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > >>>>> mfn_t mfn; > >>>>> p2m_type_t t; > >>>>> p2m_access_t old_a; > >>>>> - unsigned int page_order; > >>>>> - unsigned long gfn_l = gfn_x(gfn); > >>>>> int rc; > >>>>> > >>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >>>>> - > >>>>> - /* Check host p2m if no valid entry in alternate */ > >>>>> - if ( !mfn_valid(mfn) ) > >>>>> - { > >>>>> - > >>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > >>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>> - > >>>>> - rc = -ESRCH; > >>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >>>>> - 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; > >>>>> - } > >>>>> - } > >>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > >>>>> + if ( rc ) > >>>>> + return rc; > >>>>> > >>>>> /* > >>>>> * Inherit the old suppress #VE bit value if it is already set, or set it > >>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >>>>> index 9e81a30cc4..7bedfd593b 100644 > >>>>> --- a/xen/arch/x86/mm/p2m.c > >>>>> +++ b/xen/arch/x86/mm/p2m.c > >>>> > >>>> Wouldn't it make more sense to start adding new altp2m functions to > >>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > >>>> also be relocated there at some point in the future. > >>>> > >>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > >>>>> mm_write_unlock(&p2m->lock); > >>>>> } > >>>>> > >>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, > >>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > >>>>> + p2m_access_t *a, bool prepopulate) > >>>>> +{ > >>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > >>>>> + > >>>>> + /* Check host p2m if no valid entry in alternate */ > >>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > >>>>> + { > >>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > >>>>> + unsigned int page_order; > >>>>> + int rc; > >>>>> + > >>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > >>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>> > >>>> So despite the name being altp2m_get_entry you now return an entry > >>>> from the hostp2m, even if prepopulate is false. If the caller knows it > >>>> doesn't want that entry to be copied into the altp2m, why not have it > >>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just > >>>> confusing and doesn't help readability of the altp2m code. > >>> > >>> You return the ap2m entry if it's present, or the hp2m entry if it's > >>> not. It's not a lot of duplication, but it makes the logic cleaner I > >>> think; why not deduplicate it? > >> > >> I have no problem with making the code more streamlined. The problem I > >> have is that the function's name doesn't suggest it would get you > >> anything but the entry from the specified altp2m. So you could be > >> reading the code assuming you are dealing with an entry from that > >> specified table when in fact you are not. That is not an expected > >> behavior based on just the name of the function. This is going to make > >> reading the altp2m code that much harder in the future. > > > > Right -- I wasn't a huge fan of 'direct' either; it didn't really convey > > to me 100% what the function did. My PoC had "seethrough", but that > > wasn't that great either. "Peek"? Any other suggestions? > > > > Other options: > > > > * If we have a single function with a #define, this might get a bit > > easier; we could have one be AP2MGET_dont_prepopulate or something. > > > > ( We could have the "core" function named _altp2m_get_entry, and have > > altp2m_get_entry() call with prepopulate = false, and > > altp2m_get_entry_prepopulate() call it with prepopulate = true. > > This option with no defines seems to solve more of the naming problems > but it will still introduce the spaghetti code. I vote for this one and > if Tamas agrees I will have it this way in the next version. > Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a better name for them, as long as altp2m_get_entry doesn't return an entry from the hostp2m if there isn't one in the altp2m, and altp2m_get_entry_prepopulate returns an entry only if prepopulation actually worked. In both of those cases the functions would only return entries from the altp2m, as their name actually suggests. Tamas
On 4/17/19 7:22 PM, Tamas K Lengyel wrote: > On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA > <aisaila@bitdefender.com> wrote: >> >> >> >> On 16.04.2019 18:07, George Dunlap wrote: >>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>> >>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>>>> <aisaila@bitdefender.com> wrote: >>>>>>> >>>>>>> The code for getting the entry and then populating was repeated in >>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>>>> >>>>>>> The code is now in one place with a bool param that lets the caller choose >>>>>>> if it populates after get_entry(). >>>>>>> >>>>>>> If remapping is being done then both the old and new gfn's should be >>>>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>>>> wasn't so functionality-wise this just simplifies things as a user >>>>>>> doesn't have to request unsharing manually before remapping. >>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>>>> that is effectively a request to remove the entry from the altp2m. >>>>>>> But provided that scenario is used only when removing entries that >>>>>>> were previously remapped/copied to the altp2m, those entries already >>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>>>> affect so the core function get_altp2m_entry() is calling >>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>>>> >>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>>>> p2m->set_entry() was not called before. >>>>>>> >>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>>>> >>>>>>> --- >>>>>>> Changes since V4: >>>>>>> - Add altp2m to patch name >>>>>>> - Change func name from get_altp2m_entry() to >>>>>>> altp2m_get_entry(). >>>>>>> --- >>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>>>> >>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>>>> mfn_t mfn; >>>>>>> p2m_type_t t; >>>>>>> p2m_access_t old_a; >>>>>>> - unsigned int page_order; >>>>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>>>> int rc; >>>>>>> >>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>>>> - >>>>>>> - /* Check host p2m if no valid entry in alternate */ >>>>>>> - if ( !mfn_valid(mfn) ) >>>>>>> - { >>>>>>> - >>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>> - >>>>>>> - rc = -ESRCH; >>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>>>> - 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; >>>>>>> - } >>>>>>> - } >>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>>>> + if ( rc ) >>>>>>> + return rc; >>>>>>> >>>>>>> /* >>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>>>> --- a/xen/arch/x86/mm/p2m.c >>>>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>>> >>>>>> Wouldn't it make more sense to start adding new altp2m functions to >>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>>>> also be relocated there at some point in the future. >>>>>> >>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>>>> mm_write_unlock(&p2m->lock); >>>>>>> } >>>>>>> >>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>>>> + p2m_access_t *a, bool prepopulate) >>>>>>> +{ >>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>>>> + >>>>>>> + /* Check host p2m if no valid entry in alternate */ >>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>>>> + { >>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>>>> + unsigned int page_order; >>>>>>> + int rc; >>>>>>> + >>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>> >>>>>> So despite the name being altp2m_get_entry you now return an entry >>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>>>> doesn't want that entry to be copied into the altp2m, why not have it >>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>>>> confusing and doesn't help readability of the altp2m code. >>>>> >>>>> You return the ap2m entry if it's present, or the hp2m entry if it's >>>>> not. It's not a lot of duplication, but it makes the logic cleaner I >>>>> think; why not deduplicate it? >>>> >>>> I have no problem with making the code more streamlined. The problem I >>>> have is that the function's name doesn't suggest it would get you >>>> anything but the entry from the specified altp2m. So you could be >>>> reading the code assuming you are dealing with an entry from that >>>> specified table when in fact you are not. That is not an expected >>>> behavior based on just the name of the function. This is going to make >>>> reading the altp2m code that much harder in the future. >>> >>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey >>> to me 100% what the function did. My PoC had "seethrough", but that >>> wasn't that great either. "Peek"? Any other suggestions? >>> >>> Other options: >>> >>> * If we have a single function with a #define, this might get a bit >>> easier; we could have one be AP2MGET_dont_prepopulate or something. >>> >>> ( We could have the "core" function named _altp2m_get_entry, and have >>> altp2m_get_entry() call with prepopulate = false, and >>> altp2m_get_entry_prepopulate() call it with prepopulate = true. >> >> This option with no defines seems to solve more of the naming problems >> but it will still introduce the spaghetti code. I vote for this one and >> if Tamas agrees I will have it this way in the next version. >> > > Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a > better name for them, as long as altp2m_get_entry doesn't return an > entry from the hostp2m if there isn't one in the altp2m, and > altp2m_get_entry_prepopulate returns an entry only if prepopulation > actually worked. In both of those cases the functions would only > return entries from the altp2m, as their name actually suggests. You seem to have missed the whole point of this patch then. Instead of saying, "I don't like these names" (but not offering alternative), or saying, "If you use these names, the functions have to do the exact opposite of what they do in this patch", it would be more constructive if you proposed names which you would prefer for the functionality actually in this patch. -George
On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 4/17/19 7:22 PM, Tamas K Lengyel wrote: > > On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA > > <aisaila@bitdefender.com> wrote: > >> > >> > >> > >> On 16.04.2019 18:07, George Dunlap wrote: > >>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: > >>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>>>> > >>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > >>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > >>>>>> <aisaila@bitdefender.com> wrote: > >>>>>>> > >>>>>>> The code for getting the entry and then populating was repeated in > >>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > >>>>>>> > >>>>>>> The code is now in one place with a bool param that lets the caller choose > >>>>>>> if it populates after get_entry(). > >>>>>>> > >>>>>>> If remapping is being done then both the old and new gfn's should be > >>>>>>> unshared in the hostp2m for keeping things consistent. The page type > >>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it > >>>>>>> wasn't so functionality-wise this just simplifies things as a user > >>>>>>> doesn't have to request unsharing manually before remapping. > >>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as > >>>>>>> that is effectively a request to remove the entry from the altp2m. > >>>>>>> But provided that scenario is used only when removing entries that > >>>>>>> were previously remapped/copied to the altp2m, those entries already > >>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > >>>>>>> affect so the core function get_altp2m_entry() is calling > >>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > >>>>>>> > >>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > >>>>>>> because on a new altp2m view the function will fail with invalid mfn if > >>>>>>> p2m->set_entry() was not called before. > >>>>>>> > >>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > >>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>> > >>>>>>> --- > >>>>>>> Changes since V4: > >>>>>>> - Add altp2m to patch name > >>>>>>> - Change func name from get_altp2m_entry() to > >>>>>>> altp2m_get_entry(). > >>>>>>> --- > >>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- > >>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > >>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ > >>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) > >>>>>>> > >>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > >>>>>>> index a144bb0ce4..ddfe0169c0 100644 > >>>>>>> --- a/xen/arch/x86/mm/mem_access.c > >>>>>>> +++ b/xen/arch/x86/mm/mem_access.c > >>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > >>>>>>> mfn_t mfn; > >>>>>>> p2m_type_t t; > >>>>>>> p2m_access_t old_a; > >>>>>>> - unsigned int page_order; > >>>>>>> - unsigned long gfn_l = gfn_x(gfn); > >>>>>>> int rc; > >>>>>>> > >>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >>>>>>> - > >>>>>>> - /* Check host p2m if no valid entry in alternate */ > >>>>>>> - if ( !mfn_valid(mfn) ) > >>>>>>> - { > >>>>>>> - > >>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > >>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>>> - > >>>>>>> - rc = -ESRCH; > >>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >>>>>>> - 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; > >>>>>>> - } > >>>>>>> - } > >>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > >>>>>>> + if ( rc ) > >>>>>>> + return rc; > >>>>>>> > >>>>>>> /* > >>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it > >>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >>>>>>> index 9e81a30cc4..7bedfd593b 100644 > >>>>>>> --- a/xen/arch/x86/mm/p2m.c > >>>>>>> +++ b/xen/arch/x86/mm/p2m.c > >>>>>> > >>>>>> Wouldn't it make more sense to start adding new altp2m functions to > >>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > >>>>>> also be relocated there at some point in the future. > >>>>>> > >>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > >>>>>>> mm_write_unlock(&p2m->lock); > >>>>>>> } > >>>>>>> > >>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, > >>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > >>>>>>> + p2m_access_t *a, bool prepopulate) > >>>>>>> +{ > >>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > >>>>>>> + > >>>>>>> + /* Check host p2m if no valid entry in alternate */ > >>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > >>>>>>> + { > >>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > >>>>>>> + unsigned int page_order; > >>>>>>> + int rc; > >>>>>>> + > >>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > >>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>> > >>>>>> So despite the name being altp2m_get_entry you now return an entry > >>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it > >>>>>> doesn't want that entry to be copied into the altp2m, why not have it > >>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just > >>>>>> confusing and doesn't help readability of the altp2m code. > >>>>> > >>>>> You return the ap2m entry if it's present, or the hp2m entry if it's > >>>>> not. It's not a lot of duplication, but it makes the logic cleaner I > >>>>> think; why not deduplicate it? > >>>> > >>>> I have no problem with making the code more streamlined. The problem I > >>>> have is that the function's name doesn't suggest it would get you > >>>> anything but the entry from the specified altp2m. So you could be > >>>> reading the code assuming you are dealing with an entry from that > >>>> specified table when in fact you are not. That is not an expected > >>>> behavior based on just the name of the function. This is going to make > >>>> reading the altp2m code that much harder in the future. > >>> > >>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey > >>> to me 100% what the function did. My PoC had "seethrough", but that > >>> wasn't that great either. "Peek"? Any other suggestions? > >>> > >>> Other options: > >>> > >>> * If we have a single function with a #define, this might get a bit > >>> easier; we could have one be AP2MGET_dont_prepopulate or something. > >>> > >>> ( We could have the "core" function named _altp2m_get_entry, and have > >>> altp2m_get_entry() call with prepopulate = false, and > >>> altp2m_get_entry_prepopulate() call it with prepopulate = true. > >> > >> This option with no defines seems to solve more of the naming problems > >> but it will still introduce the spaghetti code. I vote for this one and > >> if Tamas agrees I will have it this way in the next version. > >> > > > > Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a > > better name for them, as long as altp2m_get_entry doesn't return an > > entry from the hostp2m if there isn't one in the altp2m, and > > altp2m_get_entry_prepopulate returns an entry only if prepopulation > > actually worked. In both of those cases the functions would only > > return entries from the altp2m, as their name actually suggests. > > You seem to have missed the whole point of this patch then. Forgive me but then I don't see anywhere in the patch description that explain why these functions _have to_ perform a fall-back and return an entry from the hostp2m at all cost. > > Instead of saying, "I don't like these names" (but not offering > alternative), or saying, "If you use these names, the functions have to > do the exact opposite of what they do in this patch", it would be more > constructive if you proposed names which you would prefer for the > functionality actually in this patch. > I'm not the maintainer of this code so feel free to ignore my comments. I just see way too many functions in Xen that are "do_x()" but in in fact turn out to be "do_x_and_y_and_z()" which does not help readability or even really understanding what is happening. I guess at least adding comments describing these additional and sometimes unexpected behaviors would be an improvement. Tamas
On 4/18/19 2:59 PM, Tamas K Lengyel wrote: > On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: >> >> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: >>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA >>> <aisaila@bitdefender.com> wrote: >>>> >>>> >>>> >>>> On 16.04.2019 18:07, George Dunlap wrote: >>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>> >>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>>>>>> <aisaila@bitdefender.com> wrote: >>>>>>>>> >>>>>>>>> The code for getting the entry and then populating was repeated in >>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>>>>>> >>>>>>>>> The code is now in one place with a bool param that lets the caller choose >>>>>>>>> if it populates after get_entry(). >>>>>>>>> >>>>>>>>> If remapping is being done then both the old and new gfn's should be >>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>>>>>> wasn't so functionality-wise this just simplifies things as a user >>>>>>>>> doesn't have to request unsharing manually before remapping. >>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>>>>>> that is effectively a request to remove the entry from the altp2m. >>>>>>>>> But provided that scenario is used only when removing entries that >>>>>>>>> were previously remapped/copied to the altp2m, those entries already >>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>>>>>> affect so the core function get_altp2m_entry() is calling >>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>>>>>> >>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>>>>>> p2m->set_entry() was not called before. >>>>>>>>> >>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> Changes since V4: >>>>>>>>> - Add altp2m to patch name >>>>>>>>> - Change func name from get_altp2m_entry() to >>>>>>>>> altp2m_get_entry(). >>>>>>>>> --- >>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>>>>>> mfn_t mfn; >>>>>>>>> p2m_type_t t; >>>>>>>>> p2m_access_t old_a; >>>>>>>>> - unsigned int page_order; >>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>>>>>> int rc; >>>>>>>>> >>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>>>>>> - >>>>>>>>> - /* Check host p2m if no valid entry in alternate */ >>>>>>>>> - if ( !mfn_valid(mfn) ) >>>>>>>>> - { >>>>>>>>> - >>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>> - >>>>>>>>> - rc = -ESRCH; >>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>>>>>> - 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; >>>>>>>>> - } >>>>>>>>> - } >>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>>>>>> + if ( rc ) >>>>>>>>> + return rc; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>>>>>> --- a/xen/arch/x86/mm/p2m.c >>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>>>>> >>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to >>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>>>>>> also be relocated there at some point in the future. >>>>>>>> >>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>>>>>> mm_write_unlock(&p2m->lock); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>>>>>> + p2m_access_t *a, bool prepopulate) >>>>>>>>> +{ >>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>>>>>> + >>>>>>>>> + /* Check host p2m if no valid entry in alternate */ >>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>>>>>> + { >>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>>>>>> + unsigned int page_order; >>>>>>>>> + int rc; >>>>>>>>> + >>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>> >>>>>>>> So despite the name being altp2m_get_entry you now return an entry >>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it >>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>>>>>> confusing and doesn't help readability of the altp2m code. >>>>>>> >>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's >>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I >>>>>>> think; why not deduplicate it? >>>>>> >>>>>> I have no problem with making the code more streamlined. The problem I >>>>>> have is that the function's name doesn't suggest it would get you >>>>>> anything but the entry from the specified altp2m. So you could be >>>>>> reading the code assuming you are dealing with an entry from that >>>>>> specified table when in fact you are not. That is not an expected >>>>>> behavior based on just the name of the function. This is going to make >>>>>> reading the altp2m code that much harder in the future. >>>>> >>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey >>>>> to me 100% what the function did. My PoC had "seethrough", but that >>>>> wasn't that great either. "Peek"? Any other suggestions? >>>>> >>>>> Other options: >>>>> >>>>> * If we have a single function with a #define, this might get a bit >>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. >>>>> >>>>> ( We could have the "core" function named _altp2m_get_entry, and have >>>>> altp2m_get_entry() call with prepopulate = false, and >>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. >>>> >>>> This option with no defines seems to solve more of the naming problems >>>> but it will still introduce the spaghetti code. I vote for this one and >>>> if Tamas agrees I will have it this way in the next version. >>>> >>> >>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a >>> better name for them, as long as altp2m_get_entry doesn't return an >>> entry from the hostp2m if there isn't one in the altp2m, and >>> altp2m_get_entry_prepopulate returns an entry only if prepopulation >>> actually worked. In both of those cases the functions would only >>> return entries from the altp2m, as their name actually suggests. >> >> You seem to have missed the whole point of this patch then. > > Forgive me but then I don't see anywhere in the patch description that > explain why these functions _have to_ perform a fall-back and return > an entry from the hostp2m at all cost. The primary effect of this patch is to move duplicated code into a single common function. The code being de-duplicated: 1. Tries to read the altp2m entry; if it's there it uses it 2. If it's not there, it tries to read the host p2m entry 3. In most cases it then propagates the hostp2m entry to the altp2m entry. Obviously the new "common" function has to do it because that's what the original code does. The original code does it because that's what altp2m is -- a "patch" over the host p2m, such that you use the altp2m if entries are present, but use the hostp2m otherwise. >> Instead of saying, "I don't like these names" (but not offering >> alternative), or saying, "If you use these names, the functions have to >> do the exact opposite of what they do in this patch", it would be more >> constructive if you proposed names which you would prefer for the >> functionality actually in this patch. >> > > I'm not the maintainer of this code so feel free to ignore my > comments. I just see way too many functions in Xen that are "do_x()" > but in in fact turn out to be "do_x_and_y_and_z()" which does not help > readability or even really understanding what is happening. I guess at > least adding comments describing these additional and sometimes > unexpected behaviors would be an improvement. You are a maintainer for mem_access.c, which has a non-trivial change in this patch. It can go in with Razvan's ack, but not while you have open objections. I feel your pain with function naming; I've been digging through x86/mm.c recently and the function names are unnecessarily confusing. I also agree that "altp2m_get_entry" isn't terribly informative (although it's a bit more obvious if you know how altp2m is meant to work). I'm just trying to make sure that there's a clear way for Alexandru to move this patch forward. I don't mind trying to come up with a better name, but the patch shouldn't be blocked if we can't. I agree that the function should have a comment that describes its purpose. What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? -George
On Thu, Apr 18, 2019 at 11:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 4/18/19 2:59 PM, Tamas K Lengyel wrote: > > On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: > >> > >> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: > >>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA > >>> <aisaila@bitdefender.com> wrote: > >>>> > >>>> > >>>> > >>>> On 16.04.2019 18:07, George Dunlap wrote: > >>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: > >>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>>>>>> > >>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > >>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > >>>>>>>> <aisaila@bitdefender.com> wrote: > >>>>>>>>> > >>>>>>>>> The code for getting the entry and then populating was repeated in > >>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > >>>>>>>>> > >>>>>>>>> The code is now in one place with a bool param that lets the caller choose > >>>>>>>>> if it populates after get_entry(). > >>>>>>>>> > >>>>>>>>> If remapping is being done then both the old and new gfn's should be > >>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type > >>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it > >>>>>>>>> wasn't so functionality-wise this just simplifies things as a user > >>>>>>>>> doesn't have to request unsharing manually before remapping. > >>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as > >>>>>>>>> that is effectively a request to remove the entry from the altp2m. > >>>>>>>>> But provided that scenario is used only when removing entries that > >>>>>>>>> were previously remapped/copied to the altp2m, those entries already > >>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > >>>>>>>>> affect so the core function get_altp2m_entry() is calling > >>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > >>>>>>>>> > >>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > >>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if > >>>>>>>>> p2m->set_entry() was not called before. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > >>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>>>> > >>>>>>>>> --- > >>>>>>>>> Changes since V4: > >>>>>>>>> - Add altp2m to patch name > >>>>>>>>> - Change func name from get_altp2m_entry() to > >>>>>>>>> altp2m_get_entry(). > >>>>>>>>> --- > >>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- > >>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > >>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ > >>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > >>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 > >>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c > >>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c > >>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > >>>>>>>>> mfn_t mfn; > >>>>>>>>> p2m_type_t t; > >>>>>>>>> p2m_access_t old_a; > >>>>>>>>> - unsigned int page_order; > >>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); > >>>>>>>>> int rc; > >>>>>>>>> > >>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >>>>>>>>> - > >>>>>>>>> - /* Check host p2m if no valid entry in alternate */ > >>>>>>>>> - if ( !mfn_valid(mfn) ) > >>>>>>>>> - { > >>>>>>>>> - > >>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > >>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>>>>> - > >>>>>>>>> - rc = -ESRCH; > >>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >>>>>>>>> - 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; > >>>>>>>>> - } > >>>>>>>>> - } > >>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > >>>>>>>>> + if ( rc ) > >>>>>>>>> + return rc; > >>>>>>>>> > >>>>>>>>> /* > >>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it > >>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 > >>>>>>>>> --- a/xen/arch/x86/mm/p2m.c > >>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c > >>>>>>>> > >>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to > >>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > >>>>>>>> also be relocated there at some point in the future. > >>>>>>>> > >>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > >>>>>>>>> mm_write_unlock(&p2m->lock); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, > >>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > >>>>>>>>> + p2m_access_t *a, bool prepopulate) > >>>>>>>>> +{ > >>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > >>>>>>>>> + > >>>>>>>>> + /* Check host p2m if no valid entry in alternate */ > >>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > >>>>>>>>> + { > >>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > >>>>>>>>> + unsigned int page_order; > >>>>>>>>> + int rc; > >>>>>>>>> + > >>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > >>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>>>> > >>>>>>>> So despite the name being altp2m_get_entry you now return an entry > >>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it > >>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it > >>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just > >>>>>>>> confusing and doesn't help readability of the altp2m code. > >>>>>>> > >>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's > >>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I > >>>>>>> think; why not deduplicate it? > >>>>>> > >>>>>> I have no problem with making the code more streamlined. The problem I > >>>>>> have is that the function's name doesn't suggest it would get you > >>>>>> anything but the entry from the specified altp2m. So you could be > >>>>>> reading the code assuming you are dealing with an entry from that > >>>>>> specified table when in fact you are not. That is not an expected > >>>>>> behavior based on just the name of the function. This is going to make > >>>>>> reading the altp2m code that much harder in the future. > >>>>> > >>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey > >>>>> to me 100% what the function did. My PoC had "seethrough", but that > >>>>> wasn't that great either. "Peek"? Any other suggestions? > >>>>> > >>>>> Other options: > >>>>> > >>>>> * If we have a single function with a #define, this might get a bit > >>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. > >>>>> > >>>>> ( We could have the "core" function named _altp2m_get_entry, and have > >>>>> altp2m_get_entry() call with prepopulate = false, and > >>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. > >>>> > >>>> This option with no defines seems to solve more of the naming problems > >>>> but it will still introduce the spaghetti code. I vote for this one and > >>>> if Tamas agrees I will have it this way in the next version. > >>>> > >>> > >>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a > >>> better name for them, as long as altp2m_get_entry doesn't return an > >>> entry from the hostp2m if there isn't one in the altp2m, and > >>> altp2m_get_entry_prepopulate returns an entry only if prepopulation > >>> actually worked. In both of those cases the functions would only > >>> return entries from the altp2m, as their name actually suggests. > >> > >> You seem to have missed the whole point of this patch then. > > > > Forgive me but then I don't see anywhere in the patch description that > > explain why these functions _have to_ perform a fall-back and return > > an entry from the hostp2m at all cost. > > The primary effect of this patch is to move duplicated code into a > single common function. The code being de-duplicated: > 1. Tries to read the altp2m entry; if it's there it uses it > 2. If it's not there, it tries to read the host p2m entry > 3. In most cases it then propagates the hostp2m entry to the altp2m entry. > > Obviously the new "common" function has to do it because that's what the > original code does. The original code does it because that's what > altp2m is -- a "patch" over the host p2m, such that you use the altp2m > if entries are present, but use the hostp2m otherwise. > > >> Instead of saying, "I don't like these names" (but not offering > >> alternative), or saying, "If you use these names, the functions have to > >> do the exact opposite of what they do in this patch", it would be more > >> constructive if you proposed names which you would prefer for the > >> functionality actually in this patch. > >> > > > > I'm not the maintainer of this code so feel free to ignore my > > comments. I just see way too many functions in Xen that are "do_x()" > > but in in fact turn out to be "do_x_and_y_and_z()" which does not help > > readability or even really understanding what is happening. I guess at > > least adding comments describing these additional and sometimes > > unexpected behaviors would be an improvement. > > You are a maintainer for mem_access.c, which has a non-trivial change in > this patch. It can go in with Razvan's ack, but not while you have open > objections. Yes, I meant that where this code is being relocated to is no longer under our mem_access umbrella so I'm not going to be the maintainer of it. If the new maintainers of this code are OK with how it is, than that's that. The changes being made in this patch to mem_access I have no objection to. There at least its implied that a copy will happen from the hostp2m or an error is returned so the entry that _is_ returned should not be used. Although it would be better if *mfn is not changed until the final return with no error, but it's a minor enough issue that I would not block this patch because of it. > > I feel your pain with function naming; I've been digging through > x86/mm.c recently and the function names are unnecessarily confusing. I > also agree that "altp2m_get_entry" isn't terribly informative (although > it's a bit more obvious if you know how altp2m is meant to work). I'm > just trying to make sure that there's a clear way for Alexandru to move > this patch forward. I don't mind trying to come up with a better name, > but the patch shouldn't be blocked if we can't. > > I agree that the function should have a comment that describes its purpose. > > What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? Perhaps get_effective_entry is the best so far but even that I would have no idea what it means without reading the code or reading the comment describing the function. How about "p2m_search_altp2m_then_hostp2m" with a comment saying hostp2m is a fallback? Tamas
On 18.04.2019 21:42, Tamas K Lengyel wrote: > On Thu, Apr 18, 2019 at 11:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >> >> On 4/18/19 2:59 PM, Tamas K Lengyel wrote: >>> On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>> >>>> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: >>>>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA >>>>> <aisaila@bitdefender.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 16.04.2019 18:07, George Dunlap wrote: >>>>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >>>>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>>>> >>>>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>>>>>>>> <aisaila@bitdefender.com> wrote: >>>>>>>>>>> >>>>>>>>>>> The code for getting the entry and then populating was repeated in >>>>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>>>>>>>> >>>>>>>>>>> The code is now in one place with a bool param that lets the caller choose >>>>>>>>>>> if it populates after get_entry(). >>>>>>>>>>> >>>>>>>>>>> If remapping is being done then both the old and new gfn's should be >>>>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>>>>>>>> wasn't so functionality-wise this just simplifies things as a user >>>>>>>>>>> doesn't have to request unsharing manually before remapping. >>>>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>>>>>>>> that is effectively a request to remove the entry from the altp2m. >>>>>>>>>>> But provided that scenario is used only when removing entries that >>>>>>>>>>> were previously remapped/copied to the altp2m, those entries already >>>>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>>>>>>>> affect so the core function get_altp2m_entry() is calling >>>>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>>>>>>>> >>>>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>>>>>>>> p2m->set_entry() was not called before. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> Changes since V4: >>>>>>>>>>> - Add altp2m to patch name >>>>>>>>>>> - Change func name from get_altp2m_entry() to >>>>>>>>>>> altp2m_get_entry(). >>>>>>>>>>> --- >>>>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>>>>>>>> mfn_t mfn; >>>>>>>>>>> p2m_type_t t; >>>>>>>>>>> p2m_access_t old_a; >>>>>>>>>>> - unsigned int page_order; >>>>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>>>>>>>> int rc; >>>>>>>>>>> >>>>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>>>>>>>> - >>>>>>>>>>> - /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>> - if ( !mfn_valid(mfn) ) >>>>>>>>>>> - { >>>>>>>>>>> - >>>>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>>> - >>>>>>>>>>> - rc = -ESRCH; >>>>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>>>>>>>> - 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; >>>>>>>>>>> - } >>>>>>>>>>> - } >>>>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>>>>>>>> + if ( rc ) >>>>>>>>>>> + return rc; >>>>>>>>>>> >>>>>>>>>>> /* >>>>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>>>>>>>> --- a/xen/arch/x86/mm/p2m.c >>>>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>>>>>>> >>>>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to >>>>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>>>>>>>> also be relocated there at some point in the future. >>>>>>>>>> >>>>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>>>>>>>> mm_write_unlock(&p2m->lock); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>>>>>>>> + p2m_access_t *a, bool prepopulate) >>>>>>>>>>> +{ >>>>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>>>>>>>> + >>>>>>>>>>> + /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>>>>>>>> + { >>>>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>>>>>>>> + unsigned int page_order; >>>>>>>>>>> + int rc; >>>>>>>>>>> + >>>>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>> >>>>>>>>>> So despite the name being altp2m_get_entry you now return an entry >>>>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it >>>>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>>>>>>>> confusing and doesn't help readability of the altp2m code. >>>>>>>>> >>>>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's >>>>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I >>>>>>>>> think; why not deduplicate it? >>>>>>>> >>>>>>>> I have no problem with making the code more streamlined. The problem I >>>>>>>> have is that the function's name doesn't suggest it would get you >>>>>>>> anything but the entry from the specified altp2m. So you could be >>>>>>>> reading the code assuming you are dealing with an entry from that >>>>>>>> specified table when in fact you are not. That is not an expected >>>>>>>> behavior based on just the name of the function. This is going to make >>>>>>>> reading the altp2m code that much harder in the future. >>>>>>> >>>>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey >>>>>>> to me 100% what the function did. My PoC had "seethrough", but that >>>>>>> wasn't that great either. "Peek"? Any other suggestions? >>>>>>> >>>>>>> Other options: >>>>>>> >>>>>>> * If we have a single function with a #define, this might get a bit >>>>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. >>>>>>> >>>>>>> ( We could have the "core" function named _altp2m_get_entry, and have >>>>>>> altp2m_get_entry() call with prepopulate = false, and >>>>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. >>>>>> >>>>>> This option with no defines seems to solve more of the naming problems >>>>>> but it will still introduce the spaghetti code. I vote for this one and >>>>>> if Tamas agrees I will have it this way in the next version. >>>>>> >>>>> >>>>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a >>>>> better name for them, as long as altp2m_get_entry doesn't return an >>>>> entry from the hostp2m if there isn't one in the altp2m, and >>>>> altp2m_get_entry_prepopulate returns an entry only if prepopulation >>>>> actually worked. In both of those cases the functions would only >>>>> return entries from the altp2m, as their name actually suggests. >>>> >>>> You seem to have missed the whole point of this patch then. >>> >>> Forgive me but then I don't see anywhere in the patch description that >>> explain why these functions _have to_ perform a fall-back and return >>> an entry from the hostp2m at all cost. >> >> The primary effect of this patch is to move duplicated code into a >> single common function. The code being de-duplicated: >> 1. Tries to read the altp2m entry; if it's there it uses it >> 2. If it's not there, it tries to read the host p2m entry >> 3. In most cases it then propagates the hostp2m entry to the altp2m entry. >> >> Obviously the new "common" function has to do it because that's what the >> original code does. The original code does it because that's what >> altp2m is -- a "patch" over the host p2m, such that you use the altp2m >> if entries are present, but use the hostp2m otherwise. >> >>>> Instead of saying, "I don't like these names" (but not offering >>>> alternative), or saying, "If you use these names, the functions have to >>>> do the exact opposite of what they do in this patch", it would be more >>>> constructive if you proposed names which you would prefer for the >>>> functionality actually in this patch. >>>> >>> >>> I'm not the maintainer of this code so feel free to ignore my >>> comments. I just see way too many functions in Xen that are "do_x()" >>> but in in fact turn out to be "do_x_and_y_and_z()" which does not help >>> readability or even really understanding what is happening. I guess at >>> least adding comments describing these additional and sometimes >>> unexpected behaviors would be an improvement. >> >> You are a maintainer for mem_access.c, which has a non-trivial change in >> this patch. It can go in with Razvan's ack, but not while you have open >> objections. > > Yes, I meant that where this code is being relocated to is no longer > under our mem_access umbrella so I'm not going to be the maintainer of > it. If the new maintainers of this code are OK with how it is, than > that's that. The changes being made in this patch to mem_access I have > no objection to. There at least its implied that a copy will happen > from the hostp2m or an error is returned so the entry that _is_ > returned should not be used. Although it would be better if *mfn is > not changed until the final return with no error, but it's a minor > enough issue that I would not block this patch because of it. > >> >> I feel your pain with function naming; I've been digging through >> x86/mm.c recently and the function names are unnecessarily confusing. I >> also agree that "altp2m_get_entry" isn't terribly informative (although >> it's a bit more obvious if you know how altp2m is meant to work). I'm >> just trying to make sure that there's a clear way for Alexandru to move >> this patch forward. I don't mind trying to come up with a better name, >> but the patch shouldn't be blocked if we can't. >> >> I agree that the function should have a comment that describes its purpose. >> >> What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? > > Perhaps get_effective_entry is the best so far but even that I would > have no idea what it means without reading the code or reading the > comment describing the function. How about > "p2m_search_altp2m_then_hostp2m" with a comment saying hostp2m is a > fallback? > I guess p2m_search_altp2m_then_hostp2m is a bit long but it solves the problem. If Goerge is ok with this I will put it in. Just to clarify, altp2m_get_entry will change to p2m_search_altp2m_then_hostp2m and then the rest will remain the same (altp2m_get_entry_direct, altp2m_get_entry_prepopulate)? And then add a comment for the main function. Hope I've got that right form the long name changing conversation. Regards, Alex
Ping! Hi George, How do we proceed with the function naming? Regards, Alex On 19.04.2019 11:32, Alexandru Stefan ISAILA wrote: > > > On 18.04.2019 21:42, Tamas K Lengyel wrote: >> On Thu, Apr 18, 2019 at 11:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>> >>> On 4/18/19 2:59 PM, Tamas K Lengyel wrote: >>>> On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>> >>>>> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: >>>>>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA >>>>>> <aisaila@bitdefender.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 16.04.2019 18:07, George Dunlap wrote: >>>>>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >>>>>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>>>>> >>>>>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>>>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>>>>>>>>> <aisaila@bitdefender.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> The code for getting the entry and then populating was repeated in >>>>>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>>>>>>>>> >>>>>>>>>>>> The code is now in one place with a bool param that lets the caller choose >>>>>>>>>>>> if it populates after get_entry(). >>>>>>>>>>>> >>>>>>>>>>>> If remapping is being done then both the old and new gfn's should be >>>>>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>>>>>>>>> wasn't so functionality-wise this just simplifies things as a user >>>>>>>>>>>> doesn't have to request unsharing manually before remapping. >>>>>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>>>>>>>>> that is effectively a request to remove the entry from the altp2m. >>>>>>>>>>>> But provided that scenario is used only when removing entries that >>>>>>>>>>>> were previously remapped/copied to the altp2m, those entries already >>>>>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>>>>>>>>> affect so the core function get_altp2m_entry() is calling >>>>>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>>>>>>>>> >>>>>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>>>>>>>>> p2m->set_entry() was not called before. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>>> >>>>>>>>>>>> --- >>>>>>>>>>>> Changes since V4: >>>>>>>>>>>> - Add altp2m to patch name >>>>>>>>>>>> - Change func name from get_altp2m_entry() to >>>>>>>>>>>> altp2m_get_entry(). >>>>>>>>>>>> --- >>>>>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>>>>>>>>> mfn_t mfn; >>>>>>>>>>>> p2m_type_t t; >>>>>>>>>>>> p2m_access_t old_a; >>>>>>>>>>>> - unsigned int page_order; >>>>>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>>>>>>>>> int rc; >>>>>>>>>>>> >>>>>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>>>>>>>>> - >>>>>>>>>>>> - /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>>> - if ( !mfn_valid(mfn) ) >>>>>>>>>>>> - { >>>>>>>>>>>> - >>>>>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>>>> - >>>>>>>>>>>> - rc = -ESRCH; >>>>>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>>>>>>>>> - 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; >>>>>>>>>>>> - } >>>>>>>>>>>> - } >>>>>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>>>>>>>>> + if ( rc ) >>>>>>>>>>>> + return rc; >>>>>>>>>>>> >>>>>>>>>>>> /* >>>>>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>>>>>>>>> --- a/xen/arch/x86/mm/p2m.c >>>>>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>>>>>>>> >>>>>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to >>>>>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>>>>>>>>> also be relocated there at some point in the future. >>>>>>>>>>> >>>>>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>>>>>>>>> mm_write_unlock(&p2m->lock); >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>>>>>>>>> + p2m_access_t *a, bool prepopulate) >>>>>>>>>>>> +{ >>>>>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>>>>>>>>> + >>>>>>>>>>>> + /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>>>>>>>>> + { >>>>>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>>>>>>>>> + unsigned int page_order; >>>>>>>>>>>> + int rc; >>>>>>>>>>>> + >>>>>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>>> >>>>>>>>>>> So despite the name being altp2m_get_entry you now return an entry >>>>>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>>>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it >>>>>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>>>>>>>>> confusing and doesn't help readability of the altp2m code. >>>>>>>>>> >>>>>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's >>>>>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I >>>>>>>>>> think; why not deduplicate it? >>>>>>>>> >>>>>>>>> I have no problem with making the code more streamlined. The problem I >>>>>>>>> have is that the function's name doesn't suggest it would get you >>>>>>>>> anything but the entry from the specified altp2m. So you could be >>>>>>>>> reading the code assuming you are dealing with an entry from that >>>>>>>>> specified table when in fact you are not. That is not an expected >>>>>>>>> behavior based on just the name of the function. This is going to make >>>>>>>>> reading the altp2m code that much harder in the future. >>>>>>>> >>>>>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey >>>>>>>> to me 100% what the function did. My PoC had "seethrough", but that >>>>>>>> wasn't that great either. "Peek"? Any other suggestions? >>>>>>>> >>>>>>>> Other options: >>>>>>>> >>>>>>>> * If we have a single function with a #define, this might get a bit >>>>>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. >>>>>>>> >>>>>>>> ( We could have the "core" function named _altp2m_get_entry, and have >>>>>>>> altp2m_get_entry() call with prepopulate = false, and >>>>>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. >>>>>>> >>>>>>> This option with no defines seems to solve more of the naming problems >>>>>>> but it will still introduce the spaghetti code. I vote for this one and >>>>>>> if Tamas agrees I will have it this way in the next version. >>>>>>> >>>>>> >>>>>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a >>>>>> better name for them, as long as altp2m_get_entry doesn't return an >>>>>> entry from the hostp2m if there isn't one in the altp2m, and >>>>>> altp2m_get_entry_prepopulate returns an entry only if prepopulation >>>>>> actually worked. In both of those cases the functions would only >>>>>> return entries from the altp2m, as their name actually suggests. >>>>> >>>>> You seem to have missed the whole point of this patch then. >>>> >>>> Forgive me but then I don't see anywhere in the patch description that >>>> explain why these functions _have to_ perform a fall-back and return >>>> an entry from the hostp2m at all cost. >>> >>> The primary effect of this patch is to move duplicated code into a >>> single common function. The code being de-duplicated: >>> 1. Tries to read the altp2m entry; if it's there it uses it >>> 2. If it's not there, it tries to read the host p2m entry >>> 3. In most cases it then propagates the hostp2m entry to the altp2m entry. >>> >>> Obviously the new "common" function has to do it because that's what the >>> original code does. The original code does it because that's what >>> altp2m is -- a "patch" over the host p2m, such that you use the altp2m >>> if entries are present, but use the hostp2m otherwise. >>> >>>>> Instead of saying, "I don't like these names" (but not offering >>>>> alternative), or saying, "If you use these names, the functions have to >>>>> do the exact opposite of what they do in this patch", it would be more >>>>> constructive if you proposed names which you would prefer for the >>>>> functionality actually in this patch. >>>>> >>>> >>>> I'm not the maintainer of this code so feel free to ignore my >>>> comments. I just see way too many functions in Xen that are "do_x()" >>>> but in in fact turn out to be "do_x_and_y_and_z()" which does not help >>>> readability or even really understanding what is happening. I guess at >>>> least adding comments describing these additional and sometimes >>>> unexpected behaviors would be an improvement. >>> >>> You are a maintainer for mem_access.c, which has a non-trivial change in >>> this patch. It can go in with Razvan's ack, but not while you have open >>> objections. >> >> Yes, I meant that where this code is being relocated to is no longer >> under our mem_access umbrella so I'm not going to be the maintainer of >> it. If the new maintainers of this code are OK with how it is, than >> that's that. The changes being made in this patch to mem_access I have >> no objection to. There at least its implied that a copy will happen >> from the hostp2m or an error is returned so the entry that _is_ >> returned should not be used. Although it would be better if *mfn is >> not changed until the final return with no error, but it's a minor >> enough issue that I would not block this patch because of it. >> >>> >>> I feel your pain with function naming; I've been digging through >>> x86/mm.c recently and the function names are unnecessarily confusing. I >>> also agree that "altp2m_get_entry" isn't terribly informative (although >>> it's a bit more obvious if you know how altp2m is meant to work). I'm >>> just trying to make sure that there's a clear way for Alexandru to move >>> this patch forward. I don't mind trying to come up with a better name, >>> but the patch shouldn't be blocked if we can't. >>> >>> I agree that the function should have a comment that describes its purpose. >>> >>> What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? >> >> Perhaps get_effective_entry is the best so far but even that I would >> have no idea what it means without reading the code or reading the >> comment describing the function. How about >> "p2m_search_altp2m_then_hostp2m" with a comment saying hostp2m is a >> fallback? >> > > I guess p2m_search_altp2m_then_hostp2m is a bit long but it solves the > problem. If Goerge is ok with this I will put it in. > > Just to clarify, altp2m_get_entry will change to > p2m_search_altp2m_then_hostp2m and then the rest will remain the same > (altp2m_get_entry_direct, altp2m_get_entry_prepopulate)? And then add a > comment for the main function. > > Hope I've got that right form the long name changing conversation. > > > Regards, > Alex > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel > ________________________ > This email was scanned by Bitdefender >
On 4/19/19 9:32 AM, Alexandru Stefan ISAILA wrote: > > > On 18.04.2019 21:42, Tamas K Lengyel wrote: >> On Thu, Apr 18, 2019 at 11:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>> >>> On 4/18/19 2:59 PM, Tamas K Lengyel wrote: >>>> On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>> >>>>> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: >>>>>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA >>>>>> <aisaila@bitdefender.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 16.04.2019 18:07, George Dunlap wrote: >>>>>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: >>>>>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>>>>> >>>>>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: >>>>>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA >>>>>>>>>>> <aisaila@bitdefender.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> The code for getting the entry and then populating was repeated in >>>>>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). >>>>>>>>>>>> >>>>>>>>>>>> The code is now in one place with a bool param that lets the caller choose >>>>>>>>>>>> if it populates after get_entry(). >>>>>>>>>>>> >>>>>>>>>>>> If remapping is being done then both the old and new gfn's should be >>>>>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type >>>>>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it >>>>>>>>>>>> wasn't so functionality-wise this just simplifies things as a user >>>>>>>>>>>> doesn't have to request unsharing manually before remapping. >>>>>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as >>>>>>>>>>>> that is effectively a request to remove the entry from the altp2m. >>>>>>>>>>>> But provided that scenario is used only when removing entries that >>>>>>>>>>>> were previously remapped/copied to the altp2m, those entries already >>>>>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an >>>>>>>>>>>> affect so the core function get_altp2m_entry() is calling >>>>>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. >>>>>>>>>>>> >>>>>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() >>>>>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if >>>>>>>>>>>> p2m->set_entry() was not called before. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>>>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>>>>> >>>>>>>>>>>> --- >>>>>>>>>>>> Changes since V4: >>>>>>>>>>>> - Add altp2m to patch name >>>>>>>>>>>> - Change func name from get_altp2m_entry() to >>>>>>>>>>>> altp2m_get_entry(). >>>>>>>>>>>> --- >>>>>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- >>>>>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- >>>>>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ >>>>>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 >>>>>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c >>>>>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, >>>>>>>>>>>> mfn_t mfn; >>>>>>>>>>>> p2m_type_t t; >>>>>>>>>>>> p2m_access_t old_a; >>>>>>>>>>>> - unsigned int page_order; >>>>>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); >>>>>>>>>>>> int rc; >>>>>>>>>>>> >>>>>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >>>>>>>>>>>> - >>>>>>>>>>>> - /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>>> - if ( !mfn_valid(mfn) ) >>>>>>>>>>>> - { >>>>>>>>>>>> - >>>>>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, >>>>>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>>>> - >>>>>>>>>>>> - rc = -ESRCH; >>>>>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >>>>>>>>>>>> - 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; >>>>>>>>>>>> - } >>>>>>>>>>>> - } >>>>>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); >>>>>>>>>>>> + if ( rc ) >>>>>>>>>>>> + return rc; >>>>>>>>>>>> >>>>>>>>>>>> /* >>>>>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>>>>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 >>>>>>>>>>>> --- a/xen/arch/x86/mm/p2m.c >>>>>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>>>>>>>> >>>>>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to >>>>>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could >>>>>>>>>>> also be relocated there at some point in the future. >>>>>>>>>>> >>>>>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) >>>>>>>>>>>> mm_write_unlock(&p2m->lock); >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, >>>>>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, >>>>>>>>>>>> + p2m_access_t *a, bool prepopulate) >>>>>>>>>>>> +{ >>>>>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); >>>>>>>>>>>> + >>>>>>>>>>>> + /* Check host p2m if no valid entry in alternate */ >>>>>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) >>>>>>>>>>>> + { >>>>>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); >>>>>>>>>>>> + unsigned int page_order; >>>>>>>>>>>> + int rc; >>>>>>>>>>>> + >>>>>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, >>>>>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); >>>>>>>>>>> >>>>>>>>>>> So despite the name being altp2m_get_entry you now return an entry >>>>>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it >>>>>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it >>>>>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just >>>>>>>>>>> confusing and doesn't help readability of the altp2m code. >>>>>>>>>> >>>>>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's >>>>>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I >>>>>>>>>> think; why not deduplicate it? >>>>>>>>> >>>>>>>>> I have no problem with making the code more streamlined. The problem I >>>>>>>>> have is that the function's name doesn't suggest it would get you >>>>>>>>> anything but the entry from the specified altp2m. So you could be >>>>>>>>> reading the code assuming you are dealing with an entry from that >>>>>>>>> specified table when in fact you are not. That is not an expected >>>>>>>>> behavior based on just the name of the function. This is going to make >>>>>>>>> reading the altp2m code that much harder in the future. >>>>>>>> >>>>>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey >>>>>>>> to me 100% what the function did. My PoC had "seethrough", but that >>>>>>>> wasn't that great either. "Peek"? Any other suggestions? >>>>>>>> >>>>>>>> Other options: >>>>>>>> >>>>>>>> * If we have a single function with a #define, this might get a bit >>>>>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. >>>>>>>> >>>>>>>> ( We could have the "core" function named _altp2m_get_entry, and have >>>>>>>> altp2m_get_entry() call with prepopulate = false, and >>>>>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. >>>>>>> >>>>>>> This option with no defines seems to solve more of the naming problems >>>>>>> but it will still introduce the spaghetti code. I vote for this one and >>>>>>> if Tamas agrees I will have it this way in the next version. >>>>>>> >>>>>> >>>>>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a >>>>>> better name for them, as long as altp2m_get_entry doesn't return an >>>>>> entry from the hostp2m if there isn't one in the altp2m, and >>>>>> altp2m_get_entry_prepopulate returns an entry only if prepopulation >>>>>> actually worked. In both of those cases the functions would only >>>>>> return entries from the altp2m, as their name actually suggests. >>>>> >>>>> You seem to have missed the whole point of this patch then. >>>> >>>> Forgive me but then I don't see anywhere in the patch description that >>>> explain why these functions _have to_ perform a fall-back and return >>>> an entry from the hostp2m at all cost. >>> >>> The primary effect of this patch is to move duplicated code into a >>> single common function. The code being de-duplicated: >>> 1. Tries to read the altp2m entry; if it's there it uses it >>> 2. If it's not there, it tries to read the host p2m entry >>> 3. In most cases it then propagates the hostp2m entry to the altp2m entry. >>> >>> Obviously the new "common" function has to do it because that's what the >>> original code does. The original code does it because that's what >>> altp2m is -- a "patch" over the host p2m, such that you use the altp2m >>> if entries are present, but use the hostp2m otherwise. >>> >>>>> Instead of saying, "I don't like these names" (but not offering >>>>> alternative), or saying, "If you use these names, the functions have to >>>>> do the exact opposite of what they do in this patch", it would be more >>>>> constructive if you proposed names which you would prefer for the >>>>> functionality actually in this patch. >>>>> >>>> >>>> I'm not the maintainer of this code so feel free to ignore my >>>> comments. I just see way too many functions in Xen that are "do_x()" >>>> but in in fact turn out to be "do_x_and_y_and_z()" which does not help >>>> readability or even really understanding what is happening. I guess at >>>> least adding comments describing these additional and sometimes >>>> unexpected behaviors would be an improvement. >>> >>> You are a maintainer for mem_access.c, which has a non-trivial change in >>> this patch. It can go in with Razvan's ack, but not while you have open >>> objections. >> >> Yes, I meant that where this code is being relocated to is no longer >> under our mem_access umbrella so I'm not going to be the maintainer of >> it. If the new maintainers of this code are OK with how it is, than >> that's that. The changes being made in this patch to mem_access I have >> no objection to. There at least its implied that a copy will happen >> from the hostp2m or an error is returned so the entry that _is_ >> returned should not be used. Although it would be better if *mfn is >> not changed until the final return with no error, but it's a minor >> enough issue that I would not block this patch because of it. >> >>> >>> I feel your pain with function naming; I've been digging through >>> x86/mm.c recently and the function names are unnecessarily confusing. I >>> also agree that "altp2m_get_entry" isn't terribly informative (although >>> it's a bit more obvious if you know how altp2m is meant to work). I'm >>> just trying to make sure that there's a clear way for Alexandru to move >>> this patch forward. I don't mind trying to come up with a better name, >>> but the patch shouldn't be blocked if we can't. >>> >>> I agree that the function should have a comment that describes its purpose. >>> >>> What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? >> >> Perhaps get_effective_entry is the best so far but even that I would >> have no idea what it means without reading the code or reading the >> comment describing the function. How about >> "p2m_search_altp2m_then_hostp2m" with a comment saying hostp2m is a >> fallback? >> > > I guess p2m_search_altp2m_then_hostp2m is a bit long but it solves the > problem. If Goerge is ok with this I will put it in. > > Just to clarify, altp2m_get_entry will change to > p2m_search_altp2m_then_hostp2m and then the rest will remain the same > (altp2m_get_entry_direct, altp2m_get_entry_prepopulate)? And then add a > comment for the main function. > > Hope I've got that right form the long name changing conversation. Unfortunately, it looks like you haven't. :-) Tamas' initial objection, as I understand them, were specifically that altp2m_get_entry_direct() sounds to him like it should "directly" get the backing value from the hostp2m always, and never get the "patched" value from the altp2m. But p2m_search_altp2m_then_hostp2m() isn't better, because it doesn't mention that you stop searching the altp2m if you found what you were looking for, nor what you're searching for, nor why. I think altp2m_get_effective_entry() is the best. "Effective" in CS is commonly used to mean, "the end result after combining several inputs" (e.g., "effective address" is often base + offset); If you know what altp2m is, then it's pretty obvious what the inputs would be and how it's used. If you *don't* know what altp2m is, then "p2m_search_altp2m_then_hostp2m()" is probably going to be just as mystifying. Probably just have a single function that takes a 'prepopulate argument; and make some #defines to make it easier to follow; probably something like 'AP2MGET_prepopulate' and 'AP2MGET_query'. And make sure to have a comment describing what the function does. Thanks, -George
On Tue, Apr 23, 2019 at 5:45 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 4/19/19 9:32 AM, Alexandru Stefan ISAILA wrote: > > > > > > On 18.04.2019 21:42, Tamas K Lengyel wrote: > >> On Thu, Apr 18, 2019 at 11:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>> > >>> On 4/18/19 2:59 PM, Tamas K Lengyel wrote: > >>>> On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>>>> > >>>>> On 4/17/19 7:22 PM, Tamas K Lengyel wrote: > >>>>>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA > >>>>>> <aisaila@bitdefender.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 16.04.2019 18:07, George Dunlap wrote: > >>>>>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote: > >>>>>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@citrix.com> wrote: > >>>>>>>>>> > >>>>>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote: > >>>>>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA > >>>>>>>>>>> <aisaila@bitdefender.com> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> The code for getting the entry and then populating was repeated in > >>>>>>>>>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access(). > >>>>>>>>>>>> > >>>>>>>>>>>> The code is now in one place with a bool param that lets the caller choose > >>>>>>>>>>>> if it populates after get_entry(). > >>>>>>>>>>>> > >>>>>>>>>>>> If remapping is being done then both the old and new gfn's should be > >>>>>>>>>>>> unshared in the hostp2m for keeping things consistent. The page type > >>>>>>>>>>>> of old_gfn was already checked whether it's p2m_ram_rw and bail if it > >>>>>>>>>>>> wasn't so functionality-wise this just simplifies things as a user > >>>>>>>>>>>> doesn't have to request unsharing manually before remapping. > >>>>>>>>>>>> Now, if the new_gfn is invalid it shouldn't query the hostp2m as > >>>>>>>>>>>> that is effectively a request to remove the entry from the altp2m. > >>>>>>>>>>>> But provided that scenario is used only when removing entries that > >>>>>>>>>>>> were previously remapped/copied to the altp2m, those entries already > >>>>>>>>>>>> went through P2M_ALLOC | P2M_UNSHARE before, so it won't have an > >>>>>>>>>>>> affect so the core function get_altp2m_entry() is calling > >>>>>>>>>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE. > >>>>>>>>>>>> > >>>>>>>>>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve() > >>>>>>>>>>>> because on a new altp2m view the function will fail with invalid mfn if > >>>>>>>>>>>> p2m->set_entry() was not called before. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > >>>>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com> > >>>>>>>>>>>> > >>>>>>>>>>>> --- > >>>>>>>>>>>> Changes since V4: > >>>>>>>>>>>> - Add altp2m to patch name > >>>>>>>>>>>> - Change func name from get_altp2m_entry() to > >>>>>>>>>>>> altp2m_get_entry(). > >>>>>>>>>>>> --- > >>>>>>>>>>>> xen/arch/x86/mm/mem_access.c | 30 ++----------- > >>>>>>>>>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++---------------- > >>>>>>>>>>>> xen/include/asm-x86/p2m.h | 17 ++++++++ > >>>>>>>>>>>> 3 files changed, 66 insertions(+), 65 deletions(-) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > >>>>>>>>>>>> index a144bb0ce4..ddfe0169c0 100644 > >>>>>>>>>>>> --- a/xen/arch/x86/mm/mem_access.c > >>>>>>>>>>>> +++ b/xen/arch/x86/mm/mem_access.c > >>>>>>>>>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > >>>>>>>>>>>> mfn_t mfn; > >>>>>>>>>>>> p2m_type_t t; > >>>>>>>>>>>> p2m_access_t old_a; > >>>>>>>>>>>> - unsigned int page_order; > >>>>>>>>>>>> - unsigned long gfn_l = gfn_x(gfn); > >>>>>>>>>>>> int rc; > >>>>>>>>>>>> > >>>>>>>>>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >>>>>>>>>>>> - > >>>>>>>>>>>> - /* Check host p2m if no valid entry in alternate */ > >>>>>>>>>>>> - if ( !mfn_valid(mfn) ) > >>>>>>>>>>>> - { > >>>>>>>>>>>> - > >>>>>>>>>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, > >>>>>>>>>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>>>>>>>> - > >>>>>>>>>>>> - rc = -ESRCH; > >>>>>>>>>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >>>>>>>>>>>> - 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; > >>>>>>>>>>>> - } > >>>>>>>>>>>> - } > >>>>>>>>>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); > >>>>>>>>>>>> + if ( rc ) > >>>>>>>>>>>> + return rc; > >>>>>>>>>>>> > >>>>>>>>>>>> /* > >>>>>>>>>>>> * Inherit the old suppress #VE bit value if it is already set, or set it > >>>>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >>>>>>>>>>>> index 9e81a30cc4..7bedfd593b 100644 > >>>>>>>>>>>> --- a/xen/arch/x86/mm/p2m.c > >>>>>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c > >>>>>>>>>>> > >>>>>>>>>>> Wouldn't it make more sense to start adding new altp2m functions to > >>>>>>>>>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could > >>>>>>>>>>> also be relocated there at some point in the future. > >>>>>>>>>>> > >>>>>>>>>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) > >>>>>>>>>>>> mm_write_unlock(&p2m->lock); > >>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> +int altp2m_get_entry(struct p2m_domain *ap2m, > >>>>>>>>>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, > >>>>>>>>>>>> + p2m_access_t *a, bool prepopulate) > >>>>>>>>>>>> +{ > >>>>>>>>>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); > >>>>>>>>>>>> + > >>>>>>>>>>>> + /* Check host p2m if no valid entry in alternate */ > >>>>>>>>>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) > >>>>>>>>>>>> + { > >>>>>>>>>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); > >>>>>>>>>>>> + unsigned int page_order; > >>>>>>>>>>>> + int rc; > >>>>>>>>>>>> + > >>>>>>>>>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, > >>>>>>>>>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); > >>>>>>>>>>> > >>>>>>>>>>> So despite the name being altp2m_get_entry you now return an entry > >>>>>>>>>>> from the hostp2m, even if prepopulate is false. If the caller knows it > >>>>>>>>>>> doesn't want that entry to be copied into the altp2m, why not have it > >>>>>>>>>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just > >>>>>>>>>>> confusing and doesn't help readability of the altp2m code. > >>>>>>>>>> > >>>>>>>>>> You return the ap2m entry if it's present, or the hp2m entry if it's > >>>>>>>>>> not. It's not a lot of duplication, but it makes the logic cleaner I > >>>>>>>>>> think; why not deduplicate it? > >>>>>>>>> > >>>>>>>>> I have no problem with making the code more streamlined. The problem I > >>>>>>>>> have is that the function's name doesn't suggest it would get you > >>>>>>>>> anything but the entry from the specified altp2m. So you could be > >>>>>>>>> reading the code assuming you are dealing with an entry from that > >>>>>>>>> specified table when in fact you are not. That is not an expected > >>>>>>>>> behavior based on just the name of the function. This is going to make > >>>>>>>>> reading the altp2m code that much harder in the future. > >>>>>>>> > >>>>>>>> Right -- I wasn't a huge fan of 'direct' either; it didn't really convey > >>>>>>>> to me 100% what the function did. My PoC had "seethrough", but that > >>>>>>>> wasn't that great either. "Peek"? Any other suggestions? > >>>>>>>> > >>>>>>>> Other options: > >>>>>>>> > >>>>>>>> * If we have a single function with a #define, this might get a bit > >>>>>>>> easier; we could have one be AP2MGET_dont_prepopulate or something. > >>>>>>>> > >>>>>>>> ( We could have the "core" function named _altp2m_get_entry, and have > >>>>>>>> altp2m_get_entry() call with prepopulate = false, and > >>>>>>>> altp2m_get_entry_prepopulate() call it with prepopulate = true. > >>>>>>> > >>>>>>> This option with no defines seems to solve more of the naming problems > >>>>>>> but it will still introduce the spaghetti code. I vote for this one and > >>>>>>> if Tamas agrees I will have it this way in the next version. > >>>>>>> > >>>>>> > >>>>>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a > >>>>>> better name for them, as long as altp2m_get_entry doesn't return an > >>>>>> entry from the hostp2m if there isn't one in the altp2m, and > >>>>>> altp2m_get_entry_prepopulate returns an entry only if prepopulation > >>>>>> actually worked. In both of those cases the functions would only > >>>>>> return entries from the altp2m, as their name actually suggests. > >>>>> > >>>>> You seem to have missed the whole point of this patch then. > >>>> > >>>> Forgive me but then I don't see anywhere in the patch description that > >>>> explain why these functions _have to_ perform a fall-back and return > >>>> an entry from the hostp2m at all cost. > >>> > >>> The primary effect of this patch is to move duplicated code into a > >>> single common function. The code being de-duplicated: > >>> 1. Tries to read the altp2m entry; if it's there it uses it > >>> 2. If it's not there, it tries to read the host p2m entry > >>> 3. In most cases it then propagates the hostp2m entry to the altp2m entry. > >>> > >>> Obviously the new "common" function has to do it because that's what the > >>> original code does. The original code does it because that's what > >>> altp2m is -- a "patch" over the host p2m, such that you use the altp2m > >>> if entries are present, but use the hostp2m otherwise. > >>> > >>>>> Instead of saying, "I don't like these names" (but not offering > >>>>> alternative), or saying, "If you use these names, the functions have to > >>>>> do the exact opposite of what they do in this patch", it would be more > >>>>> constructive if you proposed names which you would prefer for the > >>>>> functionality actually in this patch. > >>>>> > >>>> > >>>> I'm not the maintainer of this code so feel free to ignore my > >>>> comments. I just see way too many functions in Xen that are "do_x()" > >>>> but in in fact turn out to be "do_x_and_y_and_z()" which does not help > >>>> readability or even really understanding what is happening. I guess at > >>>> least adding comments describing these additional and sometimes > >>>> unexpected behaviors would be an improvement. > >>> > >>> You are a maintainer for mem_access.c, which has a non-trivial change in > >>> this patch. It can go in with Razvan's ack, but not while you have open > >>> objections. > >> > >> Yes, I meant that where this code is being relocated to is no longer > >> under our mem_access umbrella so I'm not going to be the maintainer of > >> it. If the new maintainers of this code are OK with how it is, than > >> that's that. The changes being made in this patch to mem_access I have > >> no objection to. There at least its implied that a copy will happen > >> from the hostp2m or an error is returned so the entry that _is_ > >> returned should not be used. Although it would be better if *mfn is > >> not changed until the final return with no error, but it's a minor > >> enough issue that I would not block this patch because of it. > >> > >>> > >>> I feel your pain with function naming; I've been digging through > >>> x86/mm.c recently and the function names are unnecessarily confusing. I > >>> also agree that "altp2m_get_entry" isn't terribly informative (although > >>> it's a bit more obvious if you know how altp2m is meant to work). I'm > >>> just trying to make sure that there's a clear way for Alexandru to move > >>> this patch forward. I don't mind trying to come up with a better name, > >>> but the patch shouldn't be blocked if we can't. > >>> > >>> I agree that the function should have a comment that describes its purpose. > >>> > >>> What about "altp2m_resolve_entry()"? "altp2m_get_effective_entry"? > >> > >> Perhaps get_effective_entry is the best so far but even that I would > >> have no idea what it means without reading the code or reading the > >> comment describing the function. How about > >> "p2m_search_altp2m_then_hostp2m" with a comment saying hostp2m is a > >> fallback? > >> > > > > I guess p2m_search_altp2m_then_hostp2m is a bit long but it solves the > > problem. If Goerge is ok with this I will put it in. > > > > Just to clarify, altp2m_get_entry will change to > > p2m_search_altp2m_then_hostp2m and then the rest will remain the same > > (altp2m_get_entry_direct, altp2m_get_entry_prepopulate)? And then add a > > comment for the main function. > > > > Hope I've got that right form the long name changing conversation. > > Unfortunately, it looks like you haven't. :-) Tamas' initial objection, > as I understand them, were specifically that altp2m_get_entry_direct() > sounds to him like it should "directly" get the backing value from the > hostp2m always, and never get the "patched" value from the altp2m. > > But p2m_search_altp2m_then_hostp2m() isn't better, because it doesn't > mention that you stop searching the altp2m if you found what you were > looking for, nor what you're searching for, nor why. > > I think altp2m_get_effective_entry() is the best. "Effective" in CS is > commonly used to mean, "the end result after combining several inputs" > (e.g., "effective address" is often base + offset); If you know what > altp2m is, then it's pretty obvious what the inputs would be and how > it's used. If you *don't* know what altp2m is, then > "p2m_search_altp2m_then_hostp2m()" is probably going to be just as > mystifying. > > Probably just have a single function that takes a 'prepopulate argument; > and make some #defines to make it easier to follow; probably something > like 'AP2MGET_prepopulate' and 'AP2MGET_query'. > > And make sure to have a comment describing what the function does. Thanks George, that is reasonable and sounds good to me. Tamas
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index a144bb0ce4..ddfe0169c0 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, mfn_t mfn; p2m_type_t t; p2m_access_t old_a; - unsigned int page_order; - unsigned long gfn_l = gfn_x(gfn); int rc; - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); - - /* Check host p2m if no valid entry in alternate */ - if ( !mfn_valid(mfn) ) - { - - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a, - P2M_ALLOC | P2M_UNSHARE, &page_order, 0); - - rc = -ESRCH; - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) - 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; - } - } + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a); + if ( rc ) + return rc; /* * Inherit the old suppress #VE bit value if it is already set, or set it diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 9e81a30cc4..7bedfd593b 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m) mm_write_unlock(&p2m->lock); } +int altp2m_get_entry(struct p2m_domain *ap2m, + gfn_t gfn, mfn_t *mfn, p2m_type_t *t, + p2m_access_t *a, bool prepopulate) +{ + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL); + + /* Check host p2m if no valid entry in alternate */ + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) ) + { + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain); + unsigned int page_order; + int rc; + + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a, + P2M_ALLOC | P2M_UNSHARE, &page_order, 0); + + rc = -ESRCH; + if ( !mfn_valid(*mfn) || *t != p2m_ram_rw ) + return rc; + + /* If this is a superpage, copy that first */ + if ( prepopulate && page_order != PAGE_ORDER_4K ) + { + unsigned long mask = ~((1UL << page_order) - 1); + gfn_t gfn_aligned = _gfn(gfn_x(gfn) & mask); + mfn_t mfn_aligned = _mfn(mfn_x(*mfn) & mask); + + rc = ap2m->set_entry(ap2m, gfn_aligned, mfn_aligned, page_order, *t, *a, 1); + if ( rc ) + return rc; + } + } + + return 0; +} + + mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l, p2m_type_t *t, p2m_access_t *a, p2m_query_t q, unsigned int *page_order, bool_t locked) @@ -2618,7 +2655,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, p2m_access_t a; p2m_type_t t; mfn_t mfn; - unsigned int page_order; int rc = -EINVAL; if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) @@ -2630,47 +2666,21 @@ 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); - if ( gfn_eq(new_gfn, INVALID_GFN) ) { + mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL); if ( mfn_valid(mfn) ) p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K); rc = 0; 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 ) - goto out; - - /* If this is a superpage, copy that first */ - if ( page_order != PAGE_ORDER_4K ) - { - gfn_t gfn; - unsigned long 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; - } - } - - 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); + rc = altp2m_get_entry_prepopulate(ap2m, old_gfn, &mfn, &t, &a); + if ( rc ) + goto out; - /* Note: currently it is not safe to remap to a shared entry */ - if ( !mfn_valid(mfn) || (t != p2m_ram_rw) ) + rc = altp2m_get_entry_direct(ap2m, new_gfn, &mfn, &t, &a); + if ( rc ) goto out; if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a, @@ -3002,12 +3012,10 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, if ( ap2m ) p2m_lock(ap2m); - mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL); - if ( !mfn_valid(mfn) ) - { - rc = -ESRCH; + rc = altp2m_get_entry_direct(p2m, gfn, &mfn, &t, &a); + + if ( rc ) goto out; - } rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 2801a8ccca..8dc4353645 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn) return mfn_x(mfn); } +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, + p2m_type_t *t, p2m_access_t *a, bool prepopulate); + +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m, + gfn_t gfn, mfn_t *mfn, + p2m_type_t *t, p2m_access_t *a) +{ + return altp2m_get_entry(ap2m, gfn, mfn, t, a, false); +} + +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m, + gfn_t gfn, mfn_t *mfn, + p2m_type_t *t, p2m_access_t *a) +{ + return altp2m_get_entry(ap2m, gfn, mfn, t, a, true); +} + /* Deadlock-avoidance scheme when calling get_gfn on different gfn's */ struct two_gfns { struct domain *first_domain, *second_domain;