Re: [PATCH V4] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr <olekstysh@gmail.com>]

On 14.12.21 15:37, Jan Beulich wrote:

Hi Jan, Julien.

> On 03.12.2021 21:33, Oleksandr Tyshchenko wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1382,8 +1382,10 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>       spin_lock(&d->page_alloc_lock);
>>   
>>       /* The incremented type count pins as writable or read-only. */
>> -    page->u.inuse.type_info =
>> -        (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
>> +    page->u.inuse.type_info &= ~(PGT_type_mask | PGT_count_mask);
>> +    page->u.inuse.type_info |= (flags == SHARE_ro ? PGT_none
>> +                                                  : PGT_writable_page) |
>> +                                MASK_INSR(1, PGT_count_mask);
> It's certainly up to the Arm maintainers to judge, but I would have
> deemed it better (less risky going forward) if PGT_count_mask
> continued to use the bottom bits. (I guess I may have said so before.)

If I am not mistaken the request was to make sure (re-check) that moving 
the count portion up
was compatible with all present uses. The code above is only a single 
place on Arm
which needs updating.


>
>> @@ -1487,7 +1489,23 @@ int xenmem_add_to_physmap_one(
>>       }
>>   
>>       /* Map at new location. */
>> -    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
>> +    if ( !p2m_is_ram(t) || !is_xen_heap_mfn(mfn) )
>> +        rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
>> +    else
>> +    {
>> +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +        p2m_write_lock(p2m);
>> +        if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)), INVALID_GFN) )
>> +        {
>> +            rc = p2m_set_entry(p2m, gfn, 1, mfn, t, p2m->default_access);
>> +            if ( !rc )
>> +                page_set_xenheap_gfn(mfn_to_page(mfn), gfn);
>> +        }
>> +        else
>> +            rc = -EBUSY;
> May I suggest to avoid failing here when page_get_xenheap_gfn(mfn_to_page(mfn))
> matches the passed in GFN?


Good question...
There was an explicit request to fail here if page_get_xenheap_gfn() 
returns a valid GFN.
 From the other side, if old GFN matches new GFN we do not remove the 
mapping in gnttab_set_frame_gfn(),
so probably we could avoid failing here in that particular case. 
@Julien, what do you think?


>
>> @@ -2169,6 +2170,9 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>       if ( unlikely(pg == NULL) )
>>           return NULL;
>>   
>> +    for ( i = 0; i < (1u << order); i++ )
>> +        arch_alloc_xenheap_page(&pg[i]);
>> +
>>       memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
> I think this and ...
>
>> @@ -2177,14 +2181,22 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>>   
>>   void free_xenheap_pages(void *v, unsigned int order)
>>   {
>> +    struct page_info *pg;
>> +    unsigned int i;
>> +
>>       ASSERT(!in_irq());
>>   
>>       if ( v == NULL )
>>           return;
>>   
>> +    pg = virt_to_page(v);
>> +
>>       memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
> ... this really want to (logically) move into the new arch hooks.
> That'll effectively mean to simply drop the Arm stubs afaict (and I
> notice there's some dead code there on x86, which I guess I'll make
> a patch to clean up). But first of all this suggests that you want
> to call the hooks with base page and order, putting the loops there.

I see your point and agree ... However I see the on-list patches that 
remove common memguard_* invocations and x86 bits.
So I assume, this request is not actual anymore, or I still need to pass 
an order to new arch hooks? Please clarify.


>
>> @@ -166,6 +173,32 @@ extern unsigned long xenheap_base_pdx;
>>   
>>   #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>>   
>> +static inline gfn_t page_get_xenheap_gfn(struct page_info *p)
> const please wherever possible.

ok, will do.


Thank you.


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko