[PATCH] Avoid premature update of M2P in set_typed_p2m_entry

[PATCH] 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 is
successful.

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

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b50747a..20272dc 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -818,11 +818,6 @@ 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,
@@ -832,6 +827,11 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
         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);
+    }
     return rc;
 }
 
-- 
1.8.3.1

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

[Jan Beulich]

>>> On 06.06.14 at 00:55, <mukesh.rathor@oracle.com> wrote:
> @@ -832,6 +827,11 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
>          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);
> +    }

The only possible concern here is that all other code paths in p2m.c
do the M2P updates under lock. Tim - is that a requirement (formally
the M2P isn't really considered serialized by P2M locking I think)? Yet
then again, even if it isn't, the placement here might sooner or later
result in a Coverity warning (recognizing that all other paths do the
update with a lock held)...

Jan

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

[Tim Deegan]

B41;297;0cAt 08:55 +0100 on 06 Jun (1402041318), Jan Beulich wrote:
> >>> On 06.06.14 at 00:55, <mukesh.rathor@oracle.com> wrote:
> > @@ -832,6 +827,11 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
> >          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);
> > +    }
> 
> The only possible concern here is that all other code paths in p2m.c
> do the M2P updates under lock. Tim - is that a requirement (formally
> the M2P isn't really considered serialized by P2M locking I think)? Yet
> then again, even if it isn't, the placement here might sooner or later
> result in a Coverity warning (recognizing that all other paths do the
> update with a lock held)...

No, the m2p isn't covered by the p2m lock, but it might be as well to
do so in this case anyway, to avoid worrying about race conditions
between two invocations of this code.

Tim.