Xen project Mailing List

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@xxxxxxxxxx> >> 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@xxxxxxxxxx> >>>> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >>>>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx> >>>>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx> >>>>>>>>>>>> >>>>>>>>>>>> --- >>>>>>>>>>>> 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@xxxxxxxxxxxxxxxxxxxx > https://lists.xenproject.org/mailman/listinfo/xen-devel > ________________________ > This email was scanned by Bitdefender > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Re: [Xen-devel] [PATCH v5] x86/altp2m: Aggregate get entry and populate into common funcs