[v2,09/10] xen/arm: Set foreign page type to p2m_map_foreign

Message ID 1386560047-17500-10-git-send-email-julien.grall@linaro.org
State Superseded
Headers show

Commit Message

Julien Grall Dec. 9, 2013, 3:34 a.m.
Xen needs to know that the current page belongs to another domain. Also take
the refcount to this page.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
        define p2m type per mapspace to let the compiler warns about unitialized
        values.
---
 xen/arch/arm/mm.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Ian Campbell Dec. 9, 2013, 4:40 p.m. | #1
On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Xen needs to know that the current page belongs to another domain. Also take
> the refcount to this page.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>         define p2m type per mapspace to let the compiler warns about unitialized
>         values.
> ---
>  xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ba51f6e..b08ffa0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>  {
>      unsigned long mfn = 0;
>      int rc;
> +    p2m_type_t t;
>  
>      switch ( space )
>      {
> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>          
>          d->arch.grant_table_gpfn[idx] = gpfn;
>  
> +        t = p2m_ram_rw;
> +
>          spin_unlock(&d->grant_table->lock);
>          break;
>      case XENMAPSPACE_shared_info:
> -        if ( idx == 0 )
> -            mfn = virt_to_mfn(d->shared_info);
> -        else
> +        if ( idx != 0 )
>              return -EINVAL;
> +
> +        mfn = virt_to_mfn(d->shared_info);
> +        t = p2m_ram_rw;
> +
>          break;
>      case XENMAPSPACE_gmfn_foreign:
>      {
> -        paddr_t maddr;
>          struct domain *od;
> +        struct page_info *page;
>          od = rcu_lock_domain_by_any_id(foreign_domid);
>          if ( od == NULL )
>              return -ESRCH;
> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>              return rc;
>          }
>  
> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> -        if ( maddr == INVALID_PADDR )
> +        /* Take refcount to the foreign domain page.

"refcount on...". Or "Take a reference to..."

> +         * Refcount will be release in XENMEM_remove_from_physmap */

and a few other places...

Like with the unmap it might be best to push this reference manipulation
down into the eventual function which makes the mapping. That would keep
this stuff much more localised.

> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);

This made me wonder what happens if the guest drops its mapping while
the foreign one is around. At least the page won't be freed until the
foreign mapping goes away, but it won't be what the tools think it is.

I think this is probably OK, or at least I'm not sure right now what we
could do about it.

> +        if ( !page )
>          {
>              dump_p2m_lookup(od, pfn_to_paddr(idx));
>              rcu_unlock_domain(od);
>              return -EINVAL;
>          }
>  
> -        mfn = maddr >> PAGE_SHIFT;
> +        mfn = page_to_mfn(page);
> +        t = p2m_map_foreign;
>  
>          rcu_unlock_domain(od);
>          break;
> @@ -1067,7 +1075,7 @@ static int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
> +    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
>  
>      return rc;
>  }
Julien Grall Dec. 10, 2013, 1:46 a.m. | #2
On 12/09/2013 04:40 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>          define p2m type per mapspace to let the compiler warns about unitialized
>>          values.
>> ---
>>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index ba51f6e..b08ffa0 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t;
>>
>>       switch ( space )
>>       {
>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>
>>           d->arch.grant_table_gpfn[idx] = gpfn;
>>
>> +        t = p2m_ram_rw;
>> +
>>           spin_unlock(&d->grant_table->lock);
>>           break;
>>       case XENMAPSPACE_shared_info:
>> -        if ( idx == 0 )
>> -            mfn = virt_to_mfn(d->shared_info);
>> -        else
>> +        if ( idx != 0 )
>>               return -EINVAL;
>> +
>> +        mfn = virt_to_mfn(d->shared_info);
>> +        t = p2m_ram_rw;
>> +
>>           break;
>>       case XENMAPSPACE_gmfn_foreign:
>>       {
>> -        paddr_t maddr;
>>           struct domain *od;
>> +        struct page_info *page;
>>           od = rcu_lock_domain_by_any_id(foreign_domid);
>>           if ( od == NULL )
>>               return -ESRCH;
>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>               return rc;
>>           }
>>
>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>> -        if ( maddr == INVALID_PADDR )
>> +        /* Take refcount to the foreign domain page.
>
> "refcount on...". Or "Take a reference to..."
>
>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>
> and a few other places...
>
> Like with the unmap it might be best to push this reference manipulation
> down into the eventual function which makes the mapping. That would keep
> this stuff much more localised.
>
>> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
>
> This made me wonder what happens if the guest drops its mapping while
> the foreign one is around. At least the page won't be freed until the
> foreign mapping goes away, but it won't be what the tools think it is.

In this case, the domain will get wrong data and can crash. It won't 
corrupted the domain who mapped the foreign mapping.
Julien Grall Dec. 10, 2013, 1:51 a.m. | #3
On 12/09/2013 04:40 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Xen needs to know that the current page belongs to another domain. Also take
>> the refcount to this page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>          define p2m type per mapspace to let the compiler warns about unitialized
>>          values.
>> ---
>>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index ba51f6e..b08ffa0 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> +    p2m_type_t t;
>>
>>       switch ( space )
>>       {
>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>
>>           d->arch.grant_table_gpfn[idx] = gpfn;
>>
>> +        t = p2m_ram_rw;
>> +
>>           spin_unlock(&d->grant_table->lock);
>>           break;
>>       case XENMAPSPACE_shared_info:
>> -        if ( idx == 0 )
>> -            mfn = virt_to_mfn(d->shared_info);
>> -        else
>> +        if ( idx != 0 )
>>               return -EINVAL;
>> +
>> +        mfn = virt_to_mfn(d->shared_info);
>> +        t = p2m_ram_rw;
>> +
>>           break;
>>       case XENMAPSPACE_gmfn_foreign:
>>       {
>> -        paddr_t maddr;
>>           struct domain *od;
>> +        struct page_info *page;
>>           od = rcu_lock_domain_by_any_id(foreign_domid);
>>           if ( od == NULL )
>>               return -ESRCH;
>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>               return rc;
>>           }
>>
>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>> -        if ( maddr == INVALID_PADDR )
>> +        /* Take refcount to the foreign domain page.
>
> "refcount on...". Or "Take a reference to..."
>
>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>
> and a few other places...
>
> Like with the unmap it might be best to push this reference manipulation
> down into the eventual function which makes the mapping. That would keep
> this stuff much more localised.

Forget to answer to this part:

We need to take the reference with foreign domain in parameter, 
otherwise we don't really know if the foreign domain has unlikely 
removed the mapping between the hypercall was called and now.
Ian Campbell Dec. 10, 2013, 9:37 a.m. | #4
On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
> 
> On 12/09/2013 04:40 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Xen needs to know that the current page belongs to another domain. Also take
> >> the refcount to this page.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>      Changes in v2:
> >>          - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
> >>          define p2m type per mapspace to let the compiler warns about unitialized
> >>          values.
> >> ---
> >>   xen/arch/arm/mm.c |   24 ++++++++++++++++--------
> >>   1 file changed, 16 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >> index ba51f6e..b08ffa0 100644
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
> >>   {
> >>       unsigned long mfn = 0;
> >>       int rc;
> >> +    p2m_type_t t;
> >>
> >>       switch ( space )
> >>       {
> >> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
> >>
> >>           d->arch.grant_table_gpfn[idx] = gpfn;
> >>
> >> +        t = p2m_ram_rw;
> >> +
> >>           spin_unlock(&d->grant_table->lock);
> >>           break;
> >>       case XENMAPSPACE_shared_info:
> >> -        if ( idx == 0 )
> >> -            mfn = virt_to_mfn(d->shared_info);
> >> -        else
> >> +        if ( idx != 0 )
> >>               return -EINVAL;
> >> +
> >> +        mfn = virt_to_mfn(d->shared_info);
> >> +        t = p2m_ram_rw;
> >> +
> >>           break;
> >>       case XENMAPSPACE_gmfn_foreign:
> >>       {
> >> -        paddr_t maddr;
> >>           struct domain *od;
> >> +        struct page_info *page;
> >>           od = rcu_lock_domain_by_any_id(foreign_domid);
> >>           if ( od == NULL )
> >>               return -ESRCH;
> >> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
> >>               return rc;
> >>           }
> >>
> >> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >> -        if ( maddr == INVALID_PADDR )
> >> +        /* Take refcount to the foreign domain page.
> >
> > "refcount on...". Or "Take a reference to..."
> >
> >> +         * Refcount will be release in XENMEM_remove_from_physmap */
> >
> > and a few other places...
> >
> > Like with the unmap it might be best to push this reference manipulation
> > down into the eventual function which makes the mapping. That would keep
> > this stuff much more localised.
> 
> Forget to answer to this part:
> 
> We need to take the reference with foreign domain in parameter, 
> otherwise we don't really know if the foreign domain has unlikely 
> removed the mapping between the hypercall was called and now.

Are we not holding the p2m lock at this point?

We probably do need to do the final check and unmap with the lock held.
Either by moving the unmap within the same lock as the lookup or perhaps
with some sort of cmpxchg mechanism where we pass in the pte value we
think we are tearing down. Just checking the type may not be sufficient
if the guest tears down one foreign mapping and replaces it with
another.

Ian.
Julien Grall Dec. 10, 2013, 2:44 p.m. | #5
On 12/10/2013 09:37 AM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
>>
>> On 12/09/2013 04:40 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> Xen needs to know that the current page belongs to another domain. Also take
>>>> the refcount to this page.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> ---
>>>>       Changes in v2:
>>>>           - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>>>           define p2m type per mapspace to let the compiler warns about unitialized
>>>>           values.
>>>> ---
>>>>    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>>>    1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index ba51f6e..b08ffa0 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>>>    {
>>>>        unsigned long mfn = 0;
>>>>        int rc;
>>>> +    p2m_type_t t;
>>>>
>>>>        switch ( space )
>>>>        {
>>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>>>
>>>>            d->arch.grant_table_gpfn[idx] = gpfn;
>>>>
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            spin_unlock(&d->grant_table->lock);
>>>>            break;
>>>>        case XENMAPSPACE_shared_info:
>>>> -        if ( idx == 0 )
>>>> -            mfn = virt_to_mfn(d->shared_info);
>>>> -        else
>>>> +        if ( idx != 0 )
>>>>                return -EINVAL;
>>>> +
>>>> +        mfn = virt_to_mfn(d->shared_info);
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            break;
>>>>        case XENMAPSPACE_gmfn_foreign:
>>>>        {
>>>> -        paddr_t maddr;
>>>>            struct domain *od;
>>>> +        struct page_info *page;
>>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
>>>>            if ( od == NULL )
>>>>                return -ESRCH;
>>>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>>>>                return rc;
>>>>            }
>>>>
>>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
>>>> -        if ( maddr == INVALID_PADDR )
>>>> +        /* Take refcount to the foreign domain page.
>>>
>>> "refcount on...". Or "Take a reference to..."
>>>
>>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>>>
>>> and a few other places...
>>>
>>> Like with the unmap it might be best to push this reference manipulation
>>> down into the eventual function which makes the mapping. That would keep
>>> this stuff much more localised.
>>
>> Forget to answer to this part:
>>
>> We need to take the reference with foreign domain in parameter,
>> otherwise we don't really know if the foreign domain has unlikely
>> removed the mapping between the hypercall was called and now.
>
> Are we not holding the p2m lock at this point?

p2m lock of which domain? For now, neither of foreign and current domain 
lock are taken in this function.

> We probably do need to do the final check and unmap with the lock held.
> Either by moving the unmap within the same lock as the lookup or perhaps
> with some sort of cmpxchg mechanism where we pass in the pte value we
> think we are tearing down. Just checking the type may not be sufficient
> if the guest tears down one foreign mapping and replaces it with
> another.

I'm not sure to follow you, are you talking about remove the foreign 
mapping? If so, I have reworking all the removing code to include the 
put_page (see V3).
Ian Campbell Dec. 10, 2013, 3:13 p.m. | #6
On Tue, 2013-12-10 at 14:44 +0000, Julien Grall wrote:
> 
> On 12/10/2013 09:37 AM, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
> >>
> >> On 12/09/2013 04:40 PM, Ian Campbell wrote:
> >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >>>> Xen needs to know that the current page belongs to another domain. Also take
> >>>> the refcount to this page.
> >>>>
> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>>>
> >>>> ---
> >>>>       Changes in v2:
> >>>>           - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
> >>>>           define p2m type per mapspace to let the compiler warns about unitialized
> >>>>           values.
> >>>> ---
> >>>>    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
> >>>>    1 file changed, 16 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >>>> index ba51f6e..b08ffa0 100644
> >>>> --- a/xen/arch/arm/mm.c
> >>>> +++ b/xen/arch/arm/mm.c
> >>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
> >>>>    {
> >>>>        unsigned long mfn = 0;
> >>>>        int rc;
> >>>> +    p2m_type_t t;
> >>>>
> >>>>        switch ( space )
> >>>>        {
> >>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
> >>>>
> >>>>            d->arch.grant_table_gpfn[idx] = gpfn;
> >>>>
> >>>> +        t = p2m_ram_rw;
> >>>> +
> >>>>            spin_unlock(&d->grant_table->lock);
> >>>>            break;
> >>>>        case XENMAPSPACE_shared_info:
> >>>> -        if ( idx == 0 )
> >>>> -            mfn = virt_to_mfn(d->shared_info);
> >>>> -        else
> >>>> +        if ( idx != 0 )
> >>>>                return -EINVAL;
> >>>> +
> >>>> +        mfn = virt_to_mfn(d->shared_info);
> >>>> +        t = p2m_ram_rw;
> >>>> +
> >>>>            break;
> >>>>        case XENMAPSPACE_gmfn_foreign:
> >>>>        {
> >>>> -        paddr_t maddr;
> >>>>            struct domain *od;
> >>>> +        struct page_info *page;
> >>>>            od = rcu_lock_domain_by_any_id(foreign_domid);
> >>>>            if ( od == NULL )
> >>>>                return -ESRCH;
> >>>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
> >>>>                return rc;
> >>>>            }
> >>>>
> >>>> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> >>>> -        if ( maddr == INVALID_PADDR )
> >>>> +        /* Take refcount to the foreign domain page.
> >>>
> >>> "refcount on...". Or "Take a reference to..."
> >>>
> >>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
> >>>
> >>> and a few other places...
> >>>
> >>> Like with the unmap it might be best to push this reference manipulation
> >>> down into the eventual function which makes the mapping. That would keep
> >>> this stuff much more localised.
> >>
> >> Forget to answer to this part:
> >>
> >> We need to take the reference with foreign domain in parameter,
> >> otherwise we don't really know if the foreign domain has unlikely
> >> removed the mapping between the hypercall was called and now.
> >
> > Are we not holding the p2m lock at this point?
> 
> p2m lock of which domain? For now, neither of foreign and current domain 
> lock are taken in this function.

Possibly both? We need to make sure that the foreign entry we are
referencing doesn't get pulled out from under us, and also protect
against simultaneous modification of the one we are adding two.

> > We probably do need to do the final check and unmap with the lock held.
> > Either by moving the unmap within the same lock as the lookup or perhaps
> > with some sort of cmpxchg mechanism where we pass in the pte value we
> > think we are tearing down. Just checking the type may not be sufficient
> > if the guest tears down one foreign mapping and replaces it with
> > another.
> 
> I'm not sure to follow you, are you talking about remove the foreign 
> mapping? If so, I have reworking all the removing code to include the 
> put_page (see V3).

This will probably help, by moving it within the locked region. On
teardown we only need to care about the p2m of the entry we are
removing, not the foreign one.

Ian.

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ba51f6e..b08ffa0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -993,6 +993,7 @@  static int xenmem_add_to_physmap_one(
 {
     unsigned long mfn = 0;
     int rc;
+    p2m_type_t t;
 
     switch ( space )
     {
@@ -1025,18 +1026,22 @@  static int xenmem_add_to_physmap_one(
         
         d->arch.grant_table_gpfn[idx] = gpfn;
 
+        t = p2m_ram_rw;
+
         spin_unlock(&d->grant_table->lock);
         break;
     case XENMAPSPACE_shared_info:
-        if ( idx == 0 )
-            mfn = virt_to_mfn(d->shared_info);
-        else
+        if ( idx != 0 )
             return -EINVAL;
+
+        mfn = virt_to_mfn(d->shared_info);
+        t = p2m_ram_rw;
+
         break;
     case XENMAPSPACE_gmfn_foreign:
     {
-        paddr_t maddr;
         struct domain *od;
+        struct page_info *page;
         od = rcu_lock_domain_by_any_id(foreign_domid);
         if ( od == NULL )
             return -ESRCH;
@@ -1048,15 +1053,18 @@  static int xenmem_add_to_physmap_one(
             return rc;
         }
 
-        maddr = p2m_lookup(od, pfn_to_paddr(idx));
-        if ( maddr == INVALID_PADDR )
+        /* Take refcount to the foreign domain page.
+         * Refcount will be release in XENMEM_remove_from_physmap */
+        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
+        if ( !page )
         {
             dump_p2m_lookup(od, pfn_to_paddr(idx));
             rcu_unlock_domain(od);
             return -EINVAL;
         }
 
-        mfn = maddr >> PAGE_SHIFT;
+        mfn = page_to_mfn(page);
+        t = p2m_map_foreign;
 
         rcu_unlock_domain(od);
         break;
@@ -1067,7 +1075,7 @@  static int xenmem_add_to_physmap_one(
     }
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
+    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
 
     return rc;
 }