[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 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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.