[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.