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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 01.12.21 18:22, Julien Grall wrote:
> On 29/11/2021 15:58, Oleksandr wrote:
> > Hi Julien
> Hi,

Hi Julien


> > [snip]
> >
> >
> > > > > ! Please note, there is still unresolved locking question here for 
> > > > > which
> > > > > I failed to find a suitable solution. So, it is still an RFC !
> > > > >
> > > > > According to the internal conversation:
> > > > > Now the GFN field in the struct page_info is accessed from
> > > > > gnttab_set_frame_gfn() in the grant table code and from 
> > > > > page_set_frame_gfn()
> > > > > in the P2M code (the former uses the latter).
> > > > >
> > > > > We need to prevent the concurrent access to this field. But, we 
> > > > > cannot grab
> > > > > the grant lock from the P2M code because we will introduce a lock 
> > > > > inversion.
> > > > > The page_set_frame_gfn() will be called from the P2M code with the 
> > > > > p2m lock held
> > > > > and then acquire the grant table lock. The gnttab_map_frame() will 
> > > > > do the inverse.
> > > > This is a tricky one. I think, we will:
> > > >
> > > >   1) Need to use the P2M lock to protect the access to the GFN in 
> > > > the struct page_info *.
> > > >   2) Defer the call to page_set_frame_gfn() from gnttab_map_frame() 
> > > > to xenmem_add_to_physmap_one()
> > > >   3) In xenmem_add_to_physmap_one() hold the P2M lock while 
> > > > checking the page was not already mapped (e.g. page_get_frame_gfn() 
> > > > == INVALID_GFN) and do the mapping. Call page_set_frame_gfn() on 
> > > > success.
> > > > This would still allow the guest to shot itself in the foot (e.g. 
> > > > potentially removing the wrong mapping) if it tries concurrent 
> > > > hypercall but I believe we would not introduce issue like XSA-380.
> > > > At the end this would look quite similar to how x86 deal with the 
> > > > M2P update.
> > > Thank you for the suggestion, I need to analyze the code to better 
> > > understand your idea and technical possibility to implement it, I 
> > > will come up with questions if any.
> > I experimented a bit... Could you please clarify, is the code snippet 
> > below is close to what you meant?
> It is close to what I had in my mind. 
Great, thank you for the clarifying.


> A few comments below.
>
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index b594db4..dba9258 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1488,8 +1488,27 @@ int xenmem_add_to_physmap_one(
> >           return -ENOSYS;
> >       }
> >
> > -    /* Map at new location. */
> > -    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
> > +    if ( space != XENMAPSPACE_grant_table )
> I would consider to use this approach for any xenheap pages.

Well, I think (from the first look) we need to:

1. Drop #ifdef CONFIG_GRANT_TABLE down this function and in 
p2m_put_l3_page()
2. Replace "space != XENMAPSPACE_grant_table" with 
"!is_xen_heap_mfn(mfn)" above.
Is my understanding correct?


> > +        /* Map at new location. */
> > +        rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
> > +    else
> > +    {
> > +#ifdef CONFIG_GRANT_TABLE
> > +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +        p2m_write_lock(p2m);
> > +        if ( gfn_eq(page_get_frame_gfn(page), INVALID_GFN) )
> I think we want to return an error if page_get_frame_gfn() return a 
> valid GFN.
ok, I assume -EBUSY would be appropriate


> > +        {
> > +            rc = p2m_set_entry(p2m, gfn, 1, mfn, t, 
> > p2m->default_access);
> > +            if ( !rc )
> > +                page_set_frame_gfn(page, gfn);
> > +        }
> > +        p2m_write_unlock(p2m);
> > +#else
> > +        ASSERT_UNREACHABLE();
> > +        rc = -EINVAL;
> > +#endif
> > +    }
> >
> >       /*
> >        * For XENMAPSPACE_gmfn_foreign if we failed to add the 
> > mapping, we need
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index 59604b1..64e9e77 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -4167,10 +4167,8 @@ int gnttab_map_frame(struct domain *d, 
> > unsigned long idx, gfn_t gfn, mfn_t *mfn)
> >            * Make sure gnttab_unpopulate_status_frames() won't 
> > (successfully)
> >            * free the page until our caller has completed its operation.
> >            */
> > -        if ( get_page(mfn_to_page(*mfn), d) )
> > -            gnttab_set_frame_gfn(gt, status, idx, gfn);
> > -        else
> > -            rc = -EBUSY;
> > +        if ( !get_page(mfn_to_page(*mfn), d) )
> > +           rc = -EBUSY;
> >       }
> >
> >       grant_write_unlock(gt);
> > (END)
> >
> > If yes *and* I correctly understand the code, then looks like 
> > gnttab_set_frame_gfn becomes useless on Arm and can be dropped 
> > globally (x86's variant is already dummy).
> It will not be a dummy version soon see [1].
Indeed, I just remembered about this patch, ok.


> Cheers,
>
> [1] 
> https://lore.kernel.org/xen-devel/8b73ff7c-4dd6-ff2e-14b9-088fdce0beb9@xxxxxxxx/
--
Regards,

Oleksandr Tyshchenko