Re: [Xen-devel] [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

Hi George,

Thanks for the patch, I've did some work on it and I've attached a 
version that suits our needs.

I have a few comments/questions to you and Tamas because in v3 there was 
a discussion about keeping the "if ( !mfn_valid(*mfn) || *t != 
p2m_ram_rw )" check inside the new function or let the caller do this 
check. If we let the caller do it then we need to used a bool to signal 
if the result was found in hostp2m (this is to keep the current 
functionality).

So the main questions are if we need that check? and where should it be?

On 10.04.2019 19:02, George Dunlap wrote:
> On 4/9/19 1:03 PM, Alexandru Stefan ISAILA wrote:
>> This patch moves common code from p2m_set_altp2m_mem_access() and
>> p2m_change_altp2m_gfn() into one function
>>
>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> 
> This patch contains a lot of behavioral changes which aren't mentioned
> or explained.  For instance...
> 
>> ---
>> Changes since V2:
>>      - Change var name from found_in_hostp2m to copied_from_hostp2m
>>      - Move the type check from altp2m_get_gfn_type_access() to the
>>      callers.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>   xen/arch/x86/mm/p2m.c        | 41 ++++++++++++++----------------------
>>   xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>   3 files changed, 49 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 56c06a4fc6..bf67ddb15a 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
>> p2m_domain *hp2m,
>>       unsigned int page_order;
>>       unsigned long gfn_l = gfn_x(gfn);
>>       int rc;
>> +    bool copied_from_hostp2m;
>>   
>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, &page_order, 
>> &copied_from_hostp2m);
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>>       if ( !mfn_valid(mfn) )
>> +        return -ESRCH;
>> +
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>>       {
>> +        unsigned long mask = ~((1UL << page_order) - 1);
>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>   
>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 
>> 0);
>> +        /* Note: currently it is not safe to remap to a shared entry */
>> +        if ( t != p2m_ram_rw )
>> +            return -ESRCH;
>>   
>> -        rc = -ESRCH;
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>> +        if ( rc )
>>               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;
>> -        }
>>       }
>>   
>>       /*
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index b9bbb8f485..d38d7c29ca 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
>> int idx,
>>       mfn_t mfn;
>>       unsigned int page_order;
>>       int rc = -EINVAL;
>> +    bool copied_from_hostp2m;
>>   
>>       if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == 
>> mfn_x(INVALID_MFN) )
>>           return rc;
>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
>> int idx,
>>       p2m_lock(hp2m);
>>       p2m_lock(ap2m);
>>   
>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, 
>> &copied_from_hostp2m);
> 
> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
> all.  Now, the hostp2m will have __get_gfn_type_access() called with
> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?

This has been requested by Tamas in v2.

> 
>>   
>>       if ( gfn_eq(new_gfn, INVALID_GFN) )
>>       {
>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
>> int idx,
>>           goto out;
>>       }
>>   
>> -    /* Check host p2m if no valid entry in alternate */
>> -    if ( !mfn_valid(mfn) )
>> -    {
>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                    P2M_ALLOC, &page_order, 0);
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>> +         goto out;
>>   
>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> -            goto out;
>> -
>> -        /* If this is a superpage, copy that first */
>> -        if ( page_order != PAGE_ORDER_4K )
>> -        {
>> -            gfn_t gfn;
>> -            unsigned long mask;
>> +    /* If this is a superpage, copy that first */
>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )
>> +    {
>> +        gfn_t gfn;
>> +        unsigned long mask;
>>   
>> -            mask = ~((1UL << page_order) - 1);
>> -            gfn = _gfn(gfn_x(old_gfn) & mask);
>> -            mfn = _mfn(mfn_x(mfn) & mask);
>> +        mask = ~((1UL << page_order) - 1);
>> +        gfn = _gfn(gfn_x(old_gfn) & mask);
>> +        mfn = _mfn(mfn_x(mfn) & mask);
>>   
>> -            if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> -                goto out;
>> -        }
>> +        if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
>> +            goto out;
>>       }
>>   
>> -    mfn = ap2m->get_entry(ap2m, new_gfn, &t, &a, 0, NULL, NULL);
>> -
>> -    if ( !mfn_valid(mfn) )
>> -        mfn = hp2m->get_entry(hp2m, new_gfn, &t, &a, 0, NULL, NULL);
>> +    mfn = altp2m_get_gfn_type_access(ap2m, new_gfn, &t, &a, &page_order, 
>> &copied_from_hostp2m);
> 
> Similiarly here: Before this patch, hp2m->get_entry() is called
> directly; after this patch, we go through __get_gfn_type_access(), which
> adds some extra code, and will also unshare / allocate.  Is that
> intentional, and if so, why?
> 
>>   
>>       /* Note: currently it is not safe to remap to a shared entry */
>> -    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>           goto out;
>>   
>>       if ( !ap2m->set_entry(ap2m, old_gfn, mfn, PAGE_ORDER_4K, t, a,
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 2801a8ccca..6de1546d76 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -448,6 +448,25 @@ static inline mfn_t __nonnull(3) get_gfn_type(
>>       return get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, q, NULL);
>>   }
>>   
>> +static inline mfn_t altp2m_get_gfn_type_access(
>> +    struct p2m_domain *ap2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a,
>> +    unsigned int *page_order, bool *copied_from_hostp2m)
>> +{
>> +    mfn_t mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>> +
>> +    *copied_from_hostp2m = false;
>> +
>> +    /* Check host p2m if no valid entry in alternate */
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        mfn = __get_gfn_type_access(p2m_get_hostp2m(ap2m->domain), 
>> gfn_x(gfn), t, a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, page_order, 
>> false);
>> +        *copied_from_hostp2m = mfn_valid(mfn);
>> +    }
>> +
>> +    return mfn;
>> +}
> 
> Given that the main goal here seems to be to clean up the interface, I'm
> not clear why you don't have this function do both the "host
> see-through" and the prepopulation.  Would something like the attached
> work (not even compile tested)?

I was thinking of going with  altp2m_get_entry_direct over see-through

> 
> (To be clear, this is just meant to be a sketch so you can see what I'm
> talking about; if you were to use it you'd need to fix it up
> appropriately, including considering whether "seethrough" is an
> appropriate name for the function.)
> 

Alex

Attachment: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch
Description: 0001-altp2m-Add-a-function-to-automatically-handle-copyin.patch

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