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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 26/11/2021 13:51, Oleksandr wrote:
> On 25.11.21 21:04, Julien Grall wrote:
> > >   {
> > > +    mfn_t mfn = lpae_get_mfn(pte);
> > > +
> > >       ASSERT(p2m_is_valid(pte));
> > >         /*
> > > @@ -731,11 +733,22 @@ static void p2m_put_l3_page(const lpae_t pte)
> > >        */
> > >       if ( p2m_is_foreign(pte.p2m.type) )
> > >       {
> > > -        mfn_t mfn = lpae_get_mfn(pte);
> > > -
> > >           ASSERT(mfn_valid(mfn));
> > >           put_page(mfn_to_page(mfn));
> > >       }
> > > +
> > > +#ifdef CONFIG_GRANT_TABLE
> > > +    /*
> > > +     * Check whether we deal with grant table page. As the grant 
> > > table page
> > > +     * is xen_heap page and its entry has known p2m type, detect it 
> > > and mark
> > > +     * the stored GFN as invalid. Although this check is not precise 
> > > and we
> > > +     * might end up updating this for other xen_heap pages, this 
> > > action is
> > > +     * harmless to these pages since only grant table pages have 
> > > this field
> > > +     * in use. So, at worst, unnecessary action might be performed.
> > > +     */
> > > +    if ( (pte.p2m.type == p2m_ram_rw) && is_xen_heap_mfn(mfn) )
> > I would use p2m_is_ram() to cover read-only mapping. I think it would 
> > also be better to use an ``else if`` so it is clear that this doesn't 
> > cover foreign mapping (it is possible to map xenheap page from another 
> > domain).
> ok, will use p2m_is_ram() and ``else if`` construct, however I don't 
> entirely understand why we also want/need to include read-only pages (as 
> type is set to p2m_ram_rw in xenmem_add_to_physmap_one() for case 
> XENMAPSPACE_grant_table)?
Most of this code is already ready to be used by other xenheap pages 
(see other part of the discussion). So I would like to use p2m_is_ram()
as it reduces the risk of us forgetting that read-only are not handled.
 [...]

> > > + page_get_frame_gfn(pg_); \
> > > +})
> > >     #define gnttab_need_iommu_mapping(d)                    \
> > >       (is_domain_direct_mapped(d) && is_iommu_enabled(d))
> > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > > index 7b5e7b7..a00c5f5 100644
> > > --- a/xen/include/asm-arm/mm.h
> > > +++ b/xen/include/asm-arm/mm.h
> > > @@ -98,9 +98,17 @@ struct page_info
> > >   #define PGT_writable_page PG_mask(1, 1)  /* has writable 
> > > mappings?         */
> > >   #define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 
> > > 63.                 */
> > >   - /* Count of uses of this frame as its current type. */
> > > -#define PGT_count_width   PG_shift(2)
> > > -#define PGT_count_mask    ((1UL<<PGT_count_width)-1)
> > > + /* 2-bit count of uses of this frame as its current type. */
> > > +#define PGT_count_mask    PG_mask(3, 3)
> > > +
> > > +/*
> > > + * Stored in bits [28:0] or [60:0] GFN if page is used for grant 
> > > table frame.
> > I think this wording is conflicting with ...
> >
> > > + * This only valid for the xenheap pages.
> > ... this becase xen heap pages are used in other situations. But I 
> > would prefer if the comment doesn't mention grant-table frame. This 
> > would allow use to repurpose the field for other xenheap if needed.
> > Typo: This *is* only valid
>
> ok, so how about to simply mention it's purpose as xenheap GFN here and 
> down this header?
> For example,
> Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
>
> BTW, shall I rename the access helpers page_set(get)_frame_gfn() as 
> well? For me the frame is associated with grant-table.
> Something to: page_set(get)_xenheap_gfn() or even page_set(get)_gfn().
Naming them to page_{set, get)_xenheap_gfn() sounds like a good idea.
It would be clearer what is the purpose of the two helpers.

> > > +#define arch_alloc_xenheap_page(p) page_set_frame_gfn(p, INVALID_GFN)
> > > +#define arch_free_xenheap_page(p) \
> > > +    BUG_ON(!gfn_eq(page_get_frame_gfn(p), INVALID_GFN))
> > I believe this BUG_ON() could be triggered if gnttab_map_frame() 
> > succeeds but then we fail to insert the entry in the P2M. This means 
> > we would need to revert changes done in gnttab_map_frame() in case of 
> > failure.
> > However, I am still a bit unease with the BUG_ON(). A domain will not 
> > necessarily remove the grant-table mapping from its P2M before 
> > shutting down. So you are relying on Xen to go through the P2M before 
> > we free the page.
> > This is the case today, but I am not sure we would want to rely on it 
> > because it will be hard to remember this requirement if we decide to 
> > optimize p2m_relinquish().
> > One possibility would be to add a comment in p2m_relinquish(). That's 
> > assuming there are no other places which could lead to false 
> > positively hit the BUG().
> These make me think that it would be better (safer and simpler) to just 
> remove this BUG_ON() for now. Do you agree?
I would drop the BUG_ON() here.

Cheers,

--
Julien Grall