Re: [PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, Jun 23, 2022 at 7:54 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:

Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
the get_page() unless the grant was a local one. These need to take the
same path as foreign entries. Just the assertion there is not valid for
local grants, and hence it triggering needs to be avoided.

I think I'd say:

---

The 'fast' path of p2m_get_page_from_gfn handles three cases: normal ram, foreign p2m entries, and grant map entries.  For normal ram and grant table entries, get_page() is called, but for foreign entries, page_get_owner_and_reference() is called, since the current domain is expected not to be the owner.

Unfortunately, grant maps are *also* generally expected to be owned by foreign domains; so this function will fail for any p2m entry containing a grant map that doesn't happen to be local.

Have grant maps take the same path as foreign entries.  Since grants may actually be either foreign or local, adjust the assertion to allow for this.

---

 

One more comment...

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Using | instead of || helps the compiler fold the two p2m_is_*().
---
v2: The shared case was fine; limit to grant adjustment.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -357,11 +357,11 @@ struct page_info *p2m_get_page_from_gfn(
              && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
         {
             page = mfn_to_page(mfn);
-            if ( unlikely(p2m_is_foreign(*t)) )
+            if ( unlikely(p2m_is_foreign(*t) | p2m_is_grant(*t)) )

I'm not a fan of this.  If you replace it with || you can have my R-b immediately; otherwise we'll have to wait until we can discuss our general policy on this sort of thing at the x86 maintainer's call.

 -George