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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

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.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel