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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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?

 -George



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