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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 01.12.21 18:32, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


> 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.
ok, got it, thank you for the explanation.


>  [...]
>
> > > > + 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.
agree, will rename


> > > > +#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.
ok, will drop. So arch_free_xenheap_page will become dummy on both Arm 
and x86, the question is should we retain it with all call sites in 
free_xenheap_pages() variants?

> Cheers,
--
Regards,

Oleksandr Tyshchenko