[PATCH V1] Avoid premature update of M2P in set_typed_p2m_entry

[PATCH V1] Avoid premature update of M2P in set_typed_p2m_entry

[Mukesh Rathor]

Update M2P for ram type after p2m_set_entry call has been made, and it
succeeds.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/mm/p2m.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b50747a..bd2e03f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -818,20 +818,20 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
         domain_crash(d);
         return -ENOENT;
     }
-    else if ( p2m_is_ram(ot) )
-    {
-        ASSERT(mfn_valid(omfn));
-        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
-    }
 
     P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn));
     rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt,
                        p2m->default_access);
-    gfn_unlock(p2m, gfn, 0);
-    if ( rc )
+    if ( unlikely(rc) )
         gdprintk(XENLOG_ERR,
                  "p2m_set_entry failed! mfn=%08lx rc:%d\n",
                  mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)), rc);
+    else if ( p2m_is_ram(ot) )
+    {
+        ASSERT(mfn_valid(omfn));
+        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
+    }
+    gfn_unlock(p2m, gfn, 0);
     return rc;
 }
 
-- 
1.8.3.1

Re: [PATCH V1] Avoid premature update of M2P in set_typed_p2m_entry

[Jan Beulich]

>>> On 06.06.14 at 23:45, <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -818,20 +818,20 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
>          domain_crash(d);
>          return -ENOENT;
>      }
> -    else if ( p2m_is_ram(ot) )
> -    {
> -        ASSERT(mfn_valid(omfn));
> -        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
> -    }
>  
>      P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn));
>      rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt,
>                         p2m->default_access);
> -    gfn_unlock(p2m, gfn, 0);
> -    if ( rc )
> +    if ( unlikely(rc) )
>          gdprintk(XENLOG_ERR,
>                   "p2m_set_entry failed! mfn=%08lx rc:%d\n",
>                   mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)), rc);
> +    else if ( p2m_is_ram(ot) )
> +    {
> +        ASSERT(mfn_valid(omfn));
> +        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
> +    }
> +    gfn_unlock(p2m, gfn, 0);
>      return rc;
>  }
>  

Looking at this a second time makes me wonder
- whether there aren't more places with the same problem (e.g.
  guest_physmap_add_entry())
- where the other set_gpfn_from_mfn() (setting the new mapping)
  sits (or do we not need one here because it is a hidden requirement
  that only set_foreign_p2m_entry() and set_mmio_p2m_entry() may
  call this function)
- what happens if omfn == mfn (moot if no 2nd set_gpfn_from_mfn()
  is needed).

Jan