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

[Oleksandr <olekstysh@xxxxxxxxx>]

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?

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 )
+        /* 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) )
+        {
+            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).

[snip]

--
Regards,

Oleksandr Tyshchenko