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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.14 at 23:45, <mukesh.rathor@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel