[1/2] x86/p2m: fix PoD accounting in guest_physmap_add_entry()

Commit Message

Jan Beulich Jan. 23, 2020, 11:51 a.m. UTC

The initial observation was that the mfn_valid() check comes too late:
Neither mfn_add() nor mfn_to_page() (let alone de-referencing the
result of the latter) are valid for MFNs failing this check. Move it up
and - noticing that there's no caller doing so - also add an assertion
that this should never produce "false" here.

In turn this would have meant that the "else" to that if() could now go
away, which didn't seem right at all. And indeed, considering callers
like memory_exchange() or various grant table functions, the PoD
accounting should have been outside of that if() from the very
beginning.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Feb. 21, 2020, 2:03 p.m. UTC | #1

On 23/01/2020 11:51, Jan Beulich wrote:
> The initial observation was that the mfn_valid() check comes too late:
> Neither mfn_add() nor mfn_to_page() (let alone de-referencing the
> result of the latter) are valid for MFNs failing this check. Move it up
> and - noticing that there's no caller doing so - also add an assertion
> that this should never produce "false" here.
>
> In turn this would have meant that the "else" to that if() could now go
> away, which didn't seem right at all. And indeed, considering callers
> like memory_exchange() or various grant table functions, the PoD
> accounting should have been outside of that if() from the very
> beginning.
>
> Signed-off-by: Jan Beulich <[e-mail]>

Acked-by: Andrew Cooper <[e-mail]>

Patch

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -883,6 +883,12 @@  guest_physmap_add_entry(struct domain *d
     if ( p2m_is_foreign(t) )
         return -EINVAL;
 
+    if ( !mfn_valid(mfn) )
+    {
+        ASSERT_UNREACHABLE();
+        return -EINVAL;
+    }
+
     p2m_lock(p2m);
 
     P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn_x(gfn), mfn_x(mfn));
@@ -984,12 +990,13 @@  guest_physmap_add_entry(struct domain *d
     }
 
     /* Now, actually do the two-way mapping */
-    if ( mfn_valid(mfn) )
+    rc = p2m_set_entry(p2m, gfn, mfn, page_order, t, p2m->default_access);
+    if ( rc == 0 )
     {
-        rc = p2m_set_entry(p2m, gfn, mfn, page_order, t,
-                           p2m->default_access);
-        if ( rc )
-            goto out; /* Failed to update p2m, bail without updating m2p. */
+        pod_lock(p2m);
+        p2m->pod.entry_count -= pod_count;
+        BUG_ON(p2m->pod.entry_count < 0);
+        pod_unlock(p2m);
 
         if ( !p2m_is_grant(t) )
         {
@@ -998,20 +1005,6 @@  guest_physmap_add_entry(struct domain *d
                                   gfn_x(gfn_add(gfn, i)));
         }
     }
-    else
-    {
-        gdprintk(XENLOG_WARNING, "Adding bad mfn to p2m map (%#lx -> %#lx)\n",
-                 gfn_x(gfn), mfn_x(mfn));
-        rc = p2m_set_entry(p2m, gfn, INVALID_MFN, page_order,
-                           p2m_invalid, p2m->default_access);
-        if ( rc == 0 )
-        {
-            pod_lock(p2m);
-            p2m->pod.entry_count -= pod_count;
-            BUG_ON(p2m->pod.entry_count < 0);
-            pod_unlock(p2m);
-        }
-    }
 
 out:
     p2m_unlock(p2m);