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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

Sorry for the late reply.

On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> +/*
> + * All accesses to the GFN portion of type_info field should always be
> + * protected by the P2M lock. In case when it is not feasible to satisfy
> + * that requirement (risk of deadlock, lock inversion, etc) it is important
> + * to make sure that all non-protected updates to this field are atomic.
Here you say the non-protected updates should be atomic but...

[...]

> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7b1f2f4..c94bdaf 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1400,8 +1400,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);
... this is not going to be atomic. So I would suggest to add a comment 
explaining why this is fine.
>   
>       page_set_owner(page, d);
>       smp_wmb(); /* install valid domain ptr before updating refcnt. */
> @@ -1505,7 +1507,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);
I would expand the comment above to explain why you need a different 
path for xenheap mapped as RAM. AFAICT, this is because we need to call 
page_set_xenheap_gfn().
> +    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) )
Sorry to only notice it now. This check will also change the behavior 
for XENMAPSPACE_shared_info. Now, we are only allowed to map the shared 
info once.
I believe this is fine because AFAICT x86 already prevents it. But this 
is probably something that ought to be explained in the already long 
commit message.
My comments are mainly seeking for clarifications. The code itself looks 
correct to me. I can handle the comments on commit to save you a round 
trip once we agree on them.
Cheers,

--
Julien Grall