|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5] x86/altp2m: Aggregate get entry and populate into common funcs
On Tue, Apr 23, 2019 at 5:45 AM George Dunlap <george.dunlap@xxxxxxxxxx> 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@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.
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |