[Xen-devel,v4,12/16] xen/mm: Switch common/memory.c to use typesafe MFN

Message ID 20180221140259.29360-13-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN
Related show

Commit Message

Julien Grall Feb. 21, 2018, 2:02 p.m.
A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
the guest memory.

Not functional change intended

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v4:
        - Patch added
---
 xen/common/memory.c | 72 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 28 deletions(-)

Comments

Wei Liu Feb. 23, 2018, 5:26 p.m. | #1
On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote:
> A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
> the guest memory.
> 
> Not functional change intended

Is there a reason to not make all guest accessors tyep-safe instead?

Wei.
Julien Grall Feb. 23, 2018, 5:46 p.m. | #2
On 23/02/18 17:26, Wei Liu wrote:
> On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote:
>> A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
>> the guest memory.
>>
>> Not functional change intended
> 
> Is there a reason to not make all guest accessors tyep-safe instead?

Could you clarify what you mean? MFN and xen_pfn_t have different type 
on Arm. So I am not sure how you would be able to make them typesafe.

Cheers,
Wei Liu Feb. 23, 2018, 6:05 p.m. | #3
On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote:
> 
> 
> On 23/02/18 17:26, Wei Liu wrote:
> > On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote:
> > > A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
> > > the guest memory.
> > > 
> > > Not functional change intended
> > 
> > Is there a reason to not make all guest accessors tyep-safe instead?
> 
> Could you clarify what you mean? MFN and xen_pfn_t have different type on
> Arm. So I am not sure how you would be able to make them typesafe.

What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
a bit strange you only created a wrapper for this file. I wonder why.

Note I'm just asking question. That's not necessarily a good idea to
turn them all in the end.

Wei.
Julien Grall Feb. 23, 2018, 6:06 p.m. | #4
Hi,

On 23/02/18 18:05, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote:
>>
>>
>> On 23/02/18 17:26, Wei Liu wrote:
>>> On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote:
>>>> A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
>>>> the guest memory.
>>>>
>>>> Not functional change intended
>>>
>>> Is there a reason to not make all guest accessors tyep-safe instead?
>>
>> Could you clarify what you mean? MFN and xen_pfn_t have different type on
>> Arm. So I am not sure how you would be able to make them typesafe.
> 
> What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
> a bit strange you only created a wrapper for this file. I wonder why.

Oh, I can have a look at that.

> 
> Note I'm just asking question. That's not necessarily a good idea to
> turn them all in the end.

Cheers,
Wei Liu Feb. 23, 2018, 6:10 p.m. | #5
On Fri, Feb 23, 2018 at 06:06:55PM +0000, Julien Grall wrote:
> Hi,
> 
> On 23/02/18 18:05, Wei Liu wrote:
> > On Fri, Feb 23, 2018 at 05:46:39PM +0000, Julien Grall wrote:
> > > 
> > > 
> > > On 23/02/18 17:26, Wei Liu wrote:
> > > > On Wed, Feb 21, 2018 at 02:02:55PM +0000, Julien Grall wrote:
> > > > > A new helper copy_mfn_to_guest is introduced to easily to copy a MFN to
> > > > > the guest memory.
> > > > > 
> > > > > Not functional change intended
> > > > 
> > > > Is there a reason to not make all guest accessors tyep-safe instead?
> > > 
> > > Could you clarify what you mean? MFN and xen_pfn_t have different type on
> > > Arm. So I am not sure how you would be able to make them typesafe.
> > 
> > What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
> > a bit strange you only created a wrapper for this file. I wonder why.
> 
> Oh, I can have a look at that.
> 

Before you do any real work please wait for other people to comment.

I haven't entirely convinced myself it is a good idea. ;-)

Wei.
Jan Beulich March 2, 2018, 3:34 p.m. | #6
>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d)
>      return min(order, MAX_ORDER + 0U);
>  }
>  
> +/* Helper to copy a typesafe MFN to guest */
> +#define copy_mfn_to_guest(hnd, off, mfn)            \
> +    ({                                              \
> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
> +    })

Hmm, not really nice, but what do you do. 

>  static void increase_reservation(struct memop_args *a)
>  {
>      struct page_info *page;
>      unsigned long i;
> -    xen_pfn_t mfn;
> +    mfn_t mfn;

Please move this declaration ...

> @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a)
>               !guest_handle_is_null(a->extent_list) )
>          {
>              mfn = page_to_mfn(page);

... here, making the assignment its initializer. Or even avoid the
local variable altogether, as the macro has already got one. Same
elsewhere (whichever of the two variants fits), albeit maybe in the
other cases the scope can't be shrunk much.

Jan
Julien Grall March 5, 2018, 2:18 p.m. | #7
Hi Jan,

On 02/03/18 15:34, Jan Beulich wrote:
>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d)
>>       return min(order, MAX_ORDER + 0U);
>>   }
>>   
>> +/* Helper to copy a typesafe MFN to guest */
>> +#define copy_mfn_to_guest(hnd, off, mfn)            \
>> +    ({                                              \
>> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
>> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
>> +    })
> 
> Hmm, not really nice, but what do you do.

I am open to better suggestion. I wanted to avoid the conversion all 
over the code.

Also, do you have an opinion on Wei's suggestion:

"What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
a bit strange you only created a wrapper for this file. I wonder why.

Note I'm just asking question. That's not necessarily a good idea to
turn them all in the end."

> 
>>   static void increase_reservation(struct memop_args *a)
>>   {
>>       struct page_info *page;
>>       unsigned long i;
>> -    xen_pfn_t mfn;
>> +    mfn_t mfn;
> 
> Please move this declaration ...
> 
>> @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a)
>>                !guest_handle_is_null(a->extent_list) )
>>           {
>>               mfn = page_to_mfn(page);
> 
> ... here, making the assignment its initializer. Or even avoid the
> local variable altogether, as the macro has already got one. Same
> elsewhere (whichever of the two variants fits), albeit maybe in the
> other cases the scope can't be shrunk much.

I will have a look.

Cheers,
Jan Beulich March 5, 2018, 2:41 p.m. | #8
>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote:
> On 02/03/18 15:34, Jan Beulich wrote:
>>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
>>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d)
>>>       return min(order, MAX_ORDER + 0U);
>>>   }
>>>   
>>> +/* Helper to copy a typesafe MFN to guest */
>>> +#define copy_mfn_to_guest(hnd, off, mfn)            \
>>> +    ({                                              \
>>> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
>>> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
>>> +    })
>> 
>> Hmm, not really nice, but what do you do.
> 
> I am open to better suggestion. I wanted to avoid the conversion all 
> over the code.

I have no better suggestion, I'm sorry, hence the "but what do
you do."

> Also, do you have an opinion on Wei's suggestion:
> 
> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
> a bit strange you only created a wrapper for this file. I wonder why.
> 
> Note I'm just asking question. That's not necessarily a good idea to
> turn them all in the end."

Well, I didn't really understand what he's after (in the context of
this series) - copy_{to,from}_guest() don't take or return MFNs or
GFNs.

Jan
Wei Liu March 9, 2018, 5:33 p.m. | #9
On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote:
> > On 02/03/18 15:34, Jan Beulich wrote:
> >>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
> >>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d)
> >>>       return min(order, MAX_ORDER + 0U);
> >>>   }
> >>>   
> >>> +/* Helper to copy a typesafe MFN to guest */
> >>> +#define copy_mfn_to_guest(hnd, off, mfn)            \
> >>> +    ({                                              \
> >>> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
> >>> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
> >>> +    })
> >> 
> >> Hmm, not really nice, but what do you do.
> > 
> > I am open to better suggestion. I wanted to avoid the conversion all 
> > over the code.
> 
> I have no better suggestion, I'm sorry, hence the "but what do
> you do."
> 
> > Also, do you have an opinion on Wei's suggestion:
> > 
> > "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
> > a bit strange you only created a wrapper for this file. I wonder why.
> > 
> > Note I'm just asking question. That's not necessarily a good idea to
> > turn them all in the end."
> 
> Well, I didn't really understand what he's after (in the context of
> this series) - copy_{to,from}_guest() don't take or return MFNs or
> GFNs.
> 

Fundamentally Julien's patch is to wrap around an existing API for this
one file only. Why is this file special? Why not just make that class of
APIs do what he wants?

But that is going to be intrusive and a bit counter-intuitive.

(In the spirit of unblocking things, I won't insist making such change.)

Wei.
Julien Grall March 11, 2018, 7:44 p.m. | #10
Hi,

On 03/09/2018 05:33 PM, Wei Liu wrote:
> On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote:
>>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote:
>>> On 02/03/18 15:34, Jan Beulich wrote:
>>>>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
>>>>> @@ -95,11 +101,18 @@ static unsigned int max_order(const struct domain *d)
>>>>>        return min(order, MAX_ORDER + 0U);
>>>>>    }
>>>>>    
>>>>> +/* Helper to copy a typesafe MFN to guest */
>>>>> +#define copy_mfn_to_guest(hnd, off, mfn)            \
>>>>> +    ({                                              \
>>>>> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
>>>>> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
>>>>> +    })
>>>>
>>>> Hmm, not really nice, but what do you do.
>>>
>>> I am open to better suggestion. I wanted to avoid the conversion all
>>> over the code.
>>
>> I have no better suggestion, I'm sorry, hence the "but what do
>> you do."
>>
>>> Also, do you have an opinion on Wei's suggestion:
>>>
>>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
>>> a bit strange you only created a wrapper for this file. I wonder why.
>>>
>>> Note I'm just asking question. That's not necessarily a good idea to
>>> turn them all in the end."
>>
>> Well, I didn't really understand what he's after (in the context of
>> this series) - copy_{to,from}_guest() don't take or return MFNs or
>> GFNs.
>>
> 
> Fundamentally Julien's patch is to wrap around an existing API for this
> one file only. Why is this file special? Why not just make that class of
> APIs do what he wants?
> 
> But that is going to be intrusive and a bit counter-intuitive.

I have quickly looked at it. The major problem I can see is it is not 
possible to generically define for any typesafe. Indeed, TYPE_SAFE(...) 
cannot define new macro and, AFAICT, it is not feasible to define static 
inline for copy_* helpers.

So we would need to introduce macros for each typesafe by hand. I can 
move copy_mfn_to_guest in xen/mm.h if people think it could be useful.

Cheers,
Jan Beulich March 12, 2018, 6:39 a.m. | #11
>>> Julien Grall <julien.grall@arm.com> 03/11/18 8:44 PM >>>
>On 03/09/2018 05:33 PM, Wei Liu wrote:
>> On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote:
>>>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote:
>>>> Also, do you have an opinion on Wei's suggestion:
>>>>
>>>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
>>>> a bit strange you only created a wrapper for this file. I wonder why.
>>>>
>>>> Note I'm just asking question. That's not necessarily a good idea to
>>>> turn them all in the end."
>>>
>>> Well, I didn't really understand what he's after (in the context of
>>> this series) - copy_{to,from}_guest() don't take or return MFNs or
>>> GFNs.
>>>
>> 
>> Fundamentally Julien's patch is to wrap around an existing API for this
>> one file only. Why is this file special? Why not just make that class of
>> APIs do what he wants?
>> 
>> But that is going to be intrusive and a bit counter-intuitive.
>
>I have quickly looked at it. The major problem I can see is it is not 
>possible to generically define for any typesafe. Indeed, TYPE_SAFE(...) 
>cannot define new macro and, AFAICT, it is not feasible to define static 
>inline for copy_* helpers.
>
>So we would need to introduce macros for each typesafe by hand. I can 
>move copy_mfn_to_guest in xen/mm.h if people think it could be useful.

First of all - how often do we copy in/out individual MFNs? Not in many places,
I think. Hence I'm afraid I continue to not see the value of such a construct,
especially not as a wider-than-file-scope one.

Jan
Julien Grall March 14, 2018, 4:08 p.m. | #12
Hi Jan,

On 03/12/2018 06:39 AM, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 03/11/18 8:44 PM >>>
>> On 03/09/2018 05:33 PM, Wei Liu wrote:
>>> On Mon, Mar 05, 2018 at 07:41:54AM -0700, Jan Beulich wrote:
>>>>>>> On 05.03.18 at 15:18, <julien.grall@arm.com> wrote:
>>>>> Also, do you have an opinion on Wei's suggestion:
>>>>>
>>>>> "What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
>>>>> a bit strange you only created a wrapper for this file. I wonder why.
>>>>>
>>>>> Note I'm just asking question. That's not necessarily a good idea to
>>>>> turn them all in the end."
>>>>
>>>> Well, I didn't really understand what he's after (in the context of
>>>> this series) - copy_{to,from}_guest() don't take or return MFNs or
>>>> GFNs.
>>>>
>>>
>>> Fundamentally Julien's patch is to wrap around an existing API for this
>>> one file only. Why is this file special? Why not just make that class of
>>> APIs do what he wants?
>>>
>>> But that is going to be intrusive and a bit counter-intuitive.
>>
>> I have quickly looked at it. The major problem I can see is it is not
>> possible to generically define for any typesafe. Indeed, TYPE_SAFE(...)
>> cannot define new macro and, AFAICT, it is not feasible to define static
>> inline for copy_* helpers.
>>
>> So we would need to introduce macros for each typesafe by hand. I can
>> move copy_mfn_to_guest in xen/mm.h if people think it could be useful.
> 
> First of all - how often do we copy in/out individual MFNs? Not in many places,
> I think. Hence I'm afraid I continue to not see the value of such a construct,
> especially not as a wider-than-file-scope one.

I think you are right. Looking at the interface, we tend copy copy more 
often a GFN. We might want to provide an helper for typesafe GFN in the 
future.

Cheers.

Patch

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 59d23a2a98..93d856df02 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -33,6 +33,12 @@ 
 #include <asm/guest.h>
 #endif
 
+/* Override macros from asm/page.h to make them work with mfn_t */
+#undef page_to_mfn
+#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
+#undef mfn_to_page
+#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
+
 struct memop_args {
     /* INPUT */
     struct domain *domain;     /* Domain to be affected. */
@@ -95,11 +101,18 @@  static unsigned int max_order(const struct domain *d)
     return min(order, MAX_ORDER + 0U);
 }
 
+/* Helper to copy a typesafe MFN to guest */
+#define copy_mfn_to_guest(hnd, off, mfn)            \
+    ({                                              \
+        xen_pfn_t mfn_ = mfn_x(mfn);                \
+        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
+    })
+
 static void increase_reservation(struct memop_args *a)
 {
     struct page_info *page;
     unsigned long i;
-    xen_pfn_t mfn;
+    mfn_t mfn;
     struct domain *d = a->domain;
 
     if ( !guest_handle_is_null(a->extent_list) &&
@@ -133,7 +146,7 @@  static void increase_reservation(struct memop_args *a)
              !guest_handle_is_null(a->extent_list) )
         {
             mfn = page_to_mfn(page);
-            if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
+            if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) )
                 goto out;
         }
     }
@@ -146,7 +159,8 @@  static void populate_physmap(struct memop_args *a)
 {
     struct page_info *page;
     unsigned int i, j;
-    xen_pfn_t gpfn, mfn;
+    xen_pfn_t gpfn;
+    mfn_t mfn;
     struct domain *d = a->domain, *curr_d = current->domain;
     bool need_tlbflush = false;
     uint32_t tlbflush_timestamp = 0;
@@ -205,14 +219,15 @@  static void populate_physmap(struct memop_args *a)
         {
             if ( is_domain_direct_mapped(d) )
             {
-                mfn = gpfn;
+                mfn = _mfn(gpfn);
 
-                for ( j = 0; j < (1U << a->extent_order); j++, mfn++ )
+                for ( j = 0; j < (1U << a->extent_order); j++,
+                      mfn = mfn_add(mfn, 1) )
                 {
-                    if ( !mfn_valid(_mfn(mfn)) )
+                    if ( !mfn_valid(mfn) )
                     {
-                        gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n",
-                                 mfn);
+                        gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_mfn"\n",
+                                 mfn_x(mfn));
                         goto out;
                     }
 
@@ -220,14 +235,14 @@  static void populate_physmap(struct memop_args *a)
                     if ( !get_page(page, d) )
                     {
                         gdprintk(XENLOG_INFO,
-                                 "mfn %#"PRI_xen_pfn" doesn't belong to d%d\n",
-                                  mfn, d->domain_id);
+                                 "mfn %#"PRI_mfn" doesn't belong to d%d\n",
+                                  mfn_x(mfn), d->domain_id);
                         goto out;
                     }
                     put_page(page);
                 }
 
-                mfn = gpfn;
+                mfn = _mfn(gpfn);
             }
             else
             {
@@ -253,15 +268,15 @@  static void populate_physmap(struct memop_args *a)
                 mfn = page_to_mfn(page);
             }
 
-            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
+            guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
 
             if ( !paging_mode_translate(d) )
             {
                 for ( j = 0; j < (1U << a->extent_order); j++ )
-                    set_gpfn_from_mfn(mfn + j, gpfn + j);
+                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, j)), gpfn + j);
 
                 /* Inform the domain of the new page's machine address. */ 
-                if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
+                if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) )
                     goto out;
             }
         }
@@ -304,7 +319,7 @@  int guest_remove_page(struct domain *d, unsigned long gmfn)
         if ( p2mt == p2m_ram_paging_out )
         {
             ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn_x(mfn));
+            page = mfn_to_page(mfn);
             if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
                 put_page(page);
         }
@@ -349,7 +364,7 @@  int guest_remove_page(struct domain *d, unsigned long gmfn)
     }
 #endif /* CONFIG_X86 */
 
-    page = mfn_to_page(mfn_x(mfn));
+    page = mfn_to_page(mfn);
     if ( unlikely(!get_page(page, d)) )
     {
         put_gfn(d, gmfn);
@@ -490,7 +505,8 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
     PAGE_LIST_HEAD(in_chunk_list);
     PAGE_LIST_HEAD(out_chunk_list);
     unsigned long in_chunk_order, out_chunk_order;
-    xen_pfn_t     gpfn, gmfn, mfn;
+    xen_pfn_t     gpfn, gmfn;
+    mfn_t         mfn;
     unsigned long i, j, k;
     unsigned int  memflags = 0;
     long          rc = 0;
@@ -612,7 +628,7 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
                 p2m_type_t p2mt;
 
                 /* Shared pages cannot be exchanged */
-                mfn = mfn_x(get_gfn_unshare(d, gmfn + k, &p2mt));
+                mfn = get_gfn_unshare(d, gmfn + k, &p2mt);
                 if ( p2m_is_shared(p2mt) )
                 {
                     put_gfn(d, gmfn + k);
@@ -620,9 +636,9 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
                     goto fail; 
                 }
 #else /* !CONFIG_X86 */
-                mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn + k)));
+                mfn = gfn_to_mfn(d, _gfn(gmfn + k));
 #endif
-                if ( unlikely(!mfn_valid(_mfn(mfn))) )
+                if ( unlikely(!mfn_valid(mfn)) )
                 {
                     put_gfn(d, gmfn + k);
                     rc = -EINVAL;
@@ -669,10 +685,10 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             if ( !test_and_clear_bit(_PGC_allocated, &page->count_info) )
                 BUG();
             mfn = page_to_mfn(page);
-            gfn = mfn_to_gmfn(d, mfn);
+            gfn = mfn_to_gmfn(d, mfn_x(mfn));
             /* Pages were unshared above */
             BUG_ON(SHARED_M2P(gfn));
-            if ( guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0) )
+            if ( guest_physmap_remove_page(d, _gfn(gfn), mfn, 0) )
                 domain_crash(d);
             put_page(page);
         }
@@ -717,16 +733,16 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             }
 
             mfn = page_to_mfn(page);
-            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn),
+            guest_physmap_add_page(d, _gfn(gpfn), mfn,
                                    exch.out.extent_order);
 
             if ( !paging_mode_translate(d) )
             {
                 for ( k = 0; k < (1UL << exch.out.extent_order); k++ )
-                    set_gpfn_from_mfn(mfn + k, gpfn + k);
-                if ( __copy_to_guest_offset(exch.out.extent_start,
-                                            (i << out_chunk_order) + j,
-                                            &mfn, 1) )
+                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, k)), gpfn + k);
+                if ( copy_mfn_to_guest(exch.out.extent_start,
+                                       (i << out_chunk_order) + j,
+                                       mfn) )
                     rc = -EFAULT;
             }
         }
@@ -1221,7 +1237,7 @@  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( page )
         {
             rc = guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
-                                           _mfn(page_to_mfn(page)), 0);
+                                           page_to_mfn(page), 0);
             put_page(page);
         }
         else