[1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant and shared cases better

Commit Message

Jan Beulich Feb. 23, 2022, 3:14 p.m. UTC

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.

Shared entries, when unshare is requested, would bypass the retrieval of
"page" and thus always take the error path rather than actually trying
to unshare by taking the slow path.

Signed-off-by: Jan Beulich <[e-mail]>
---
Using | instead of || helps the compiler fold the two p2m_is_*().

Comments

Tamas K Lengyel Feb. 23, 2022, 6:11 p.m. UTC | #1

> @@ -607,6 +607,7 @@ struct page_info *p2m_get_page_from_gfn(
>
>          /* Error path: not a suitable GFN at all */
>          if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> +             (!p2m_is_shared(*t) || !(q & P2M_UNSHARE)) &&
>               !mem_sharing_is_fork(p2m->domain) )
>              return NULL;
>      }

I don't follow what this is fixing. A shared entry would return true
to p2m_is_ram() - p2m_ram_shared is listed under P2M_RAM_TYPES - so
the rest of the if statement would never be checked. So if we get past
that check we know we definitely don't have a shared entry, ie
p2m_is_shared must be false ie the check for P2M_UNSHARE is dead code.
Am I missing something?

Tamas

Tamas

Jan Beulich Feb. 24, 2022, 7:54 a.m. UTC | #2

On 23.02.2022 19:11, Tamas K Lengyel wrote:
>> @@ -607,6 +607,7 @@ struct page_info *p2m_get_page_from_gfn(
>>
>>          /* Error path: not a suitable GFN at all */
>>          if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
>> +             (!p2m_is_shared(*t) || !(q & P2M_UNSHARE)) &&
>>               !mem_sharing_is_fork(p2m->domain) )
>>              return NULL;
>>      }
> 
> I don't follow what this is fixing. A shared entry would return true
> to p2m_is_ram() - p2m_ram_shared is listed under P2M_RAM_TYPES - so
> the rest of the if statement would never be checked. So if we get past
> that check we know we definitely don't have a shared entry, ie
> p2m_is_shared must be false ie the check for P2M_UNSHARE is dead code.
> Am I missing something?

No, I am. I mistakenly took p2m_is_any_ram() to include the shared case,
but p2m_is_ram() to not do so. Thanks for pointing out, and I'm actually
happy to be able to droop this hunk.

Jan

Patch

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -584,11 +584,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)) )
             {
                 struct domain *fdom = page_get_owner_and_reference(page);
 
-                ASSERT(fdom != p2m->domain);
+                ASSERT(!p2m_is_foreign(*t) || fdom != p2m->domain);
                 if ( fdom == NULL )
                     page = NULL;
             }
@@ -607,6 +607,7 @@  struct page_info *p2m_get_page_from_gfn(
 
         /* Error path: not a suitable GFN at all */
         if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
+             (!p2m_is_shared(*t) || !(q & P2M_UNSHARE)) &&
              !mem_sharing_is_fork(p2m->domain) )
             return NULL;
     }