[16/17] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN
diff mbox series

Message ID 20200322161418.31606-17-julien@xen.org
State New
Headers show
Series
  • Bunch of typesafe conversion
Related show

Commit Message

Julien Grall March 22, 2020, 4:14 p.m. UTC
From: Julien Grall <julien.grall@arm.com>

The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can be
switched to use the typesafe.

At the same time, replace gpfn with pfn in the helpers as they all deal
with PFN and also turn the macros to static inline.

Note that the return of the getter and the 2nd parameter of the setter
have not been converted to use typesafe PFN because it was requiring
more changes than expected.

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

---
    This was originally sent as part of "xen/arm: Properly disable M2P
    on Arm" [1].

    Changes since the original version:
        - mfn_to_gmfn() is still present for now so update it
        - Remove stray +
        - Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_
        - Remove tags
        - Fix build in mem_sharing

    [1] <20190603160350.29806-1-julien.grall@arm.com>
---
 xen/arch/x86/cpu/mcheck/mcaction.c |  2 +-
 xen/arch/x86/mm.c                  | 14 +++----
 xen/arch/x86/mm/mem_sharing.c      | 20 ++++-----
 xen/arch/x86/mm/p2m-pod.c          |  4 +-
 xen/arch/x86/mm/p2m-pt.c           | 35 ++++++++--------
 xen/arch/x86/mm/p2m.c              | 66 +++++++++++++++---------------
 xen/arch/x86/mm/paging.c           |  4 +-
 xen/arch/x86/pv/dom0_build.c       |  6 +--
 xen/arch/x86/x86_64/traps.c        |  8 ++--
 xen/common/page_alloc.c            |  2 +-
 xen/include/asm-arm/mm.h           |  2 +-
 xen/include/asm-x86/grant_table.h  |  2 +-
 xen/include/asm-x86/mm.h           | 12 ++++--
 xen/include/asm-x86/p2m.h          |  2 +-
 14 files changed, 93 insertions(+), 86 deletions(-)

Comments

Hongyan Xia March 23, 2020, 12:11 p.m. UTC | #1
On Sun, 2020-03-22 at 16:14 +0000, julien@xen.org wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can
> be
> switched to use the typesafe.
> 
> At the same time, replace gpfn with pfn in the helpers as they all
> deal
> with PFN and also turn the macros to static inline.
> 
> Note that the return of the getter and the 2nd parameter of the
> setter
> have not been converted to use typesafe PFN because it was requiring
> more changes than expected.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This was originally sent as part of "xen/arm: Properly disable
> M2P
>     on Arm" [1].
> 
>     Changes since the original version:
>         - mfn_to_gmfn() is still present for now so update it
>         - Remove stray +
>         - Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_
>         - Remove tags
>         - Fix build in mem_sharing
> 
>     [1] <20190603160350.29806-1-julien.grall@arm.com>
> ---
>  xen/arch/x86/cpu/mcheck/mcaction.c |  2 +-
>  xen/arch/x86/mm.c                  | 14 +++----
>  xen/arch/x86/mm/mem_sharing.c      | 20 ++++-----
>  xen/arch/x86/mm/p2m-pod.c          |  4 +-
>  xen/arch/x86/mm/p2m-pt.c           | 35 ++++++++--------
>  xen/arch/x86/mm/p2m.c              | 66 +++++++++++++++-------------
> --
>  xen/arch/x86/mm/paging.c           |  4 +-
>  xen/arch/x86/pv/dom0_build.c       |  6 +--
>  xen/arch/x86/x86_64/traps.c        |  8 ++--
>  xen/common/page_alloc.c            |  2 +-
>  xen/include/asm-arm/mm.h           |  2 +-
>  xen/include/asm-x86/grant_table.h  |  2 +-
>  xen/include/asm-x86/mm.h           | 12 ++++--
>  xen/include/asm-x86/p2m.h          |  2 +-
>  14 files changed, 93 insertions(+), 86 deletions(-)
> 
> 

[...]

> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index abf4cc23e4..11614f9107 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu
> *v, vaddr_t va,
>  #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>  
>  /* Xen always owns P2M on ARM */
> -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn);
> } while (0)
> +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {}
>  #define mfn_to_gmfn(_d, mfn)  (mfn) 

I do not have a setup to compile and test code for Arm, but wouldn't
the compiler complain about unused arguments here? The marco version
explicitly silenced compiler complaints.
 
> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-
> x86/grant_table.h
> index 5871238f6d..b6a09c4c6c 100644
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -41,7 +41,7 @@ static inline int
> replace_grant_host_mapping(uint64_t addr, mfn_t frame,
>  #define gnttab_get_frame_gfn(gt, st, idx)
> ({                             \
>      mfn_t mfn_ = (st) ? gnttab_status_mfn(gt,
> idx)                       \
>                        : gnttab_shared_mfn(gt,
> idx);                      \
> -    unsigned long gpfn_ =
> get_gpfn_from_mfn(mfn_x(mfn_));                \
> +    unsigned long gpfn_ =
> get_pfn_from_mfn(mfn_);                        \
>      VALID_M2P(gpfn_) ? _gfn(gpfn_) :
> INVALID_GFN;                        \
>  })
>  
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 53f2ed7c7d..2a4f42e78f 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>   */
>  extern bool machine_to_phys_mapping_valid;
>  
> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
> long pfn)
> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>  {
> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
> +    const unsigned long mfn = mfn_x(mfn_);
> +    const struct domain *d = page_get_owner(mfn_to_page(mfn_));
>      unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY :
> pfn;
>  
>      if ( !machine_to_phys_mapping_valid )
> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned
> long mfn, unsigned long pfn)
>  
>  extern struct rangeset *mmio_ro_ranges;
>  
> -#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
> +{
> +    return machine_to_phys_mapping[mfn_x(mfn)];
> +}

Any specific reason this (and some other macros) are turned into static
inline? I don't have a problem with them being inline functions but
just wondering if there is a reason to do so.
 
>  #define mfn_to_gmfn(_d, mfn)                            \
>      ( (paging_mode_translate(_d))                       \
> -      ? get_gpfn_from_mfn(mfn)                          \
> +      ? get_pfn_from_mfn(_mfn(mfn))                     \
>        : (mfn) )
>  
>  #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) |
> ((unsigned)(pfn) >> 20))
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index a2c6049834..39dae242b0 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -505,7 +505,7 @@ static inline struct page_info
> *get_page_from_gfn(
>  static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn)
>  {
>      if ( paging_mode_translate(d) )
> -        return _gfn(get_gpfn_from_mfn(mfn_x(mfn)));
> +        return _gfn(get_pfn_from_mfn(mfn));
>      else
>          return _gfn(mfn_x(mfn));
>  }

Apart from the two comments above, looks good to me.

Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
Julien Grall March 23, 2020, 12:26 p.m. UTC | #2
Hi,

On 23/03/2020 12:11, Hongyan Xia wrote:
> On Sun, 2020-03-22 at 16:14 +0000, julien@xen.org wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can
>> be
>> switched to use the typesafe.
>>
>> At the same time, replace gpfn with pfn in the helpers as they all
>> deal
>> with PFN and also turn the macros to static inline.
>>
>> Note that the return of the getter and the 2nd parameter of the
>> setter
>> have not been converted to use typesafe PFN because it was requiring
>> more changes than expected.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      This was originally sent as part of "xen/arm: Properly disable
>> M2P
>>      on Arm" [1].
>>
>>      Changes since the original version:
>>          - mfn_to_gmfn() is still present for now so update it
>>          - Remove stray +
>>          - Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_
>>          - Remove tags
>>          - Fix build in mem_sharing
>>
>>      [1] <20190603160350.29806-1-julien.grall@arm.com>
>> ---
>>   xen/arch/x86/cpu/mcheck/mcaction.c |  2 +-
>>   xen/arch/x86/mm.c                  | 14 +++----
>>   xen/arch/x86/mm/mem_sharing.c      | 20 ++++-----
>>   xen/arch/x86/mm/p2m-pod.c          |  4 +-
>>   xen/arch/x86/mm/p2m-pt.c           | 35 ++++++++--------
>>   xen/arch/x86/mm/p2m.c              | 66 +++++++++++++++-------------
>> --
>>   xen/arch/x86/mm/paging.c           |  4 +-
>>   xen/arch/x86/pv/dom0_build.c       |  6 +--
>>   xen/arch/x86/x86_64/traps.c        |  8 ++--
>>   xen/common/page_alloc.c            |  2 +-
>>   xen/include/asm-arm/mm.h           |  2 +-
>>   xen/include/asm-x86/grant_table.h  |  2 +-
>>   xen/include/asm-x86/mm.h           | 12 ++++--
>>   xen/include/asm-x86/p2m.h          |  2 +-
>>   14 files changed, 93 insertions(+), 86 deletions(-)
>>
>>
> 
> [...]
> 
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index abf4cc23e4..11614f9107 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu
>> *v, vaddr_t va,
>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>   
>>   /* Xen always owns P2M on ARM */
>> -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn);
>> } while (0)
>> +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {}
>>   #define mfn_to_gmfn(_d, mfn)  (mfn)
> 
> I do not have a setup to compile and test code for Arm, but wouldn't
> the compiler complain about unused arguments here? The marco version
> explicitly silenced compiler complaints.

The macro version does not use (void)(arg) for silencing unused 
parameter. It is for evaluating (mfn) but ignore the result. A compiler 
would warn without (void) because we build Xen with -Wall which include 
-Wunused-value.

Xen is not used with -Wunused-parameter, so there is no concern about 
unused parameters. If we ever decided to turn on -Wunused-parameter (or 
-Wextra), then we will have quite a bit of code to modify (such as 
callbacks not using all the parameters) to make it compile.

>   
>> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-
>> x86/grant_table.h
>> index 5871238f6d..b6a09c4c6c 100644
>> --- a/xen/include/asm-x86/grant_table.h
>> +++ b/xen/include/asm-x86/grant_table.h
>> @@ -41,7 +41,7 @@ static inline int
>> replace_grant_host_mapping(uint64_t addr, mfn_t frame,
>>   #define gnttab_get_frame_gfn(gt, st, idx)
>> ({                             \
>>       mfn_t mfn_ = (st) ? gnttab_status_mfn(gt,
>> idx)                       \
>>                         : gnttab_shared_mfn(gt,
>> idx);                      \
>> -    unsigned long gpfn_ =
>> get_gpfn_from_mfn(mfn_x(mfn_));                \
>> +    unsigned long gpfn_ =
>> get_pfn_from_mfn(mfn_);                        \
>>       VALID_M2P(gpfn_) ? _gfn(gpfn_) :
>> INVALID_GFN;                        \
>>   })
>>   
>> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
>> index 53f2ed7c7d..2a4f42e78f 100644
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>>    */
>>   extern bool machine_to_phys_mapping_valid;
>>   
>> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
>> long pfn)
>> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>>   {
>> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>> +    const unsigned long mfn = mfn_x(mfn_);
>> +    const struct domain *d = page_get_owner(mfn_to_page(mfn_));
>>       unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY :
>> pfn;
>>   
>>       if ( !machine_to_phys_mapping_valid )
>> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned
>> long mfn, unsigned long pfn)
>>   
>>   extern struct rangeset *mmio_ro_ranges;
>>   
>> -#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
>> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
>> +{
>> +    return machine_to_phys_mapping[mfn_x(mfn)];
>> +}
> 
> Any specific reason this (and some other macros) are turned into static
> inline? I don't have a problem with them being inline functions but
> just wondering if there is a reason to do so.

static inline provides better safety check than macro. So we tend to 
switch to static inline whenever the headers inter-dependency madness is 
not interplaying.

>   
>>   #define mfn_to_gmfn(_d, mfn)                            \
>>       ( (paging_mode_translate(_d))                       \
>> -      ? get_gpfn_from_mfn(mfn)                          \
>> +      ? get_pfn_from_mfn(_mfn(mfn))                     \
>>         : (mfn) )
>>   
>>   #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) |
>> ((unsigned)(pfn) >> 20))
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index a2c6049834..39dae242b0 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -505,7 +505,7 @@ static inline struct page_info
>> *get_page_from_gfn(
>>   static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn)
>>   {
>>       if ( paging_mode_translate(d) )
>> -        return _gfn(get_gpfn_from_mfn(mfn_x(mfn)));
>> +        return _gfn(get_pfn_from_mfn(mfn));
>>       else
>>           return _gfn(mfn_x(mfn));
>>   }
> 
> Apart from the two comments above, looks good to me.
> 
> Reviewed-by: Hongyan Xia <hongyxia@amazon.com>

Thank you!

Cheers,
Jan Beulich March 27, 2020, 1:15 p.m. UTC | #3
On 22.03.2020 17:14, julien@xen.org wrote:
> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m)
>                  /* check for 1GB super page */
>                  if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE )
>                  {
> -                    mfn = l3e_get_pfn(l3e[i3]);
> -                    ASSERT(mfn_valid(_mfn(mfn)));
> +                    mfn = l3e_get_mfn(l3e[i3]);
> +                    ASSERT(mfn_valid(mfn));
>                      /* we have to cover 512x512 4K pages */
>                      for ( i2 = 0; 
>                            i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES);
>                            i2++)
>                      {
> -                        m2pfn = get_gpfn_from_mfn(mfn+i2);
> +                        m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2));
>                          if ( m2pfn != (gfn + i2) )
>                          {
>                              pmbad++;
> -                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
> -                                       gfn + i2, mfn + i2, m2pfn);
> +                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n",
> +                                       gfn + i2, mfn_x(mfn_add(mfn, i2)),

As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and
hence imo preferable, especially in printk() and alike invocations.

I would also prefer if you left %#lx alone, with the 2nd best
option being to also use PRI_gfn alongside PRI_mfn. Primarily
I'd like to avoid having a mixture.

Same (for both) at least one more time further down.

> @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>                  P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
>                            gfn_x(ogfn) , mfn_x(omfn));
>                  if ( mfn_eq(omfn, mfn_add(mfn, i)) )
> -                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)),
> +                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i),
>                                      0);

Pull this up then onto the now shorter prior line?

> @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d,
>      spin_lock(&d->page_alloc_lock);
>      page_list_for_each ( page, &d->page_list )
>      {
> -        mfn = mfn_x(page_to_mfn(page));
> +        mfn = page_to_mfn(page);
>  
> -        P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn);
> +        P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn));
>  
>          od = page_get_owner(page);
>  
>          if ( od != d )
>          {
> -            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d);
> +            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d);
>              continue;
>          }
>  
> -        gfn = get_gpfn_from_mfn(mfn);
> +        gfn = get_pfn_from_mfn(mfn);
>          if ( gfn == INVALID_M2P_ENTRY )
>          {
>              orphans_count++;
> -            P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n",
> -                           mfn);
> +            P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n",
> +                       mfn_x(mfn));
>              continue;
>          }
>  
>          if ( SHARED_M2P(gfn) )
>          {
> -            P2M_PRINTK("shared mfn (%lx) on domain page list!\n",
> -                    mfn);
> +            P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n",
> +                       mfn_x(mfn));
>              continue;
>          }
>  
>          p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL);
> -        if ( mfn_x(p2mfn) != mfn )
> +        if ( !mfn_eq(p2mfn, mfn) )
>          {
>              mpbad++;
> -            P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx"
> +            P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn""
>                         " (-> gfn %#lx)\n",
> -                       mfn, gfn, mfn_x(p2mfn),
> +                       mfn_x(mfn), gfn, mfn_x(p2mfn),
>                         (mfn_valid(p2mfn)
> -                        ? get_gpfn_from_mfn(mfn_x(p2mfn))
> +                        ? get_pfn_from_mfn(p2mfn)
>                          : -1u));

I realize this is an entirely unrelated change, but the -1u here
is standing out too much to not mention it: Could I talk you into
making this gfn_x(INVALID_GFN) at this occasion?

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>   */
>  extern bool machine_to_phys_mapping_valid;
>  
> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>  {
> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
> +    const unsigned long mfn = mfn_x(mfn_);

I think it would be better overall if the parameter was named
"mfn" and there was no local variable altogether. This would
bring things in line with ...

> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>  
>  extern struct rangeset *mmio_ro_ranges;
>  
> -#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
> +{
> +    return machine_to_phys_mapping[mfn_x(mfn)];
> +}

... this.

Jan
Julien Grall March 28, 2020, 11:14 a.m. UTC | #4
Hi,

On 27/03/2020 13:15, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xen.org wrote:
>> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m)
>>                   /* check for 1GB super page */
>>                   if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE )
>>                   {
>> -                    mfn = l3e_get_pfn(l3e[i3]);
>> -                    ASSERT(mfn_valid(_mfn(mfn)));
>> +                    mfn = l3e_get_mfn(l3e[i3]);
>> +                    ASSERT(mfn_valid(mfn));
>>                       /* we have to cover 512x512 4K pages */
>>                       for ( i2 = 0;
>>                             i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES);
>>                             i2++)
>>                       {
>> -                        m2pfn = get_gpfn_from_mfn(mfn+i2);
>> +                        m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2));
>>                           if ( m2pfn != (gfn + i2) )
>>                           {
>>                               pmbad++;
>> -                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
>> -                                       gfn + i2, mfn + i2, m2pfn);
>> +                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n",
>> +                                       gfn + i2, mfn_x(mfn_add(mfn, i2)),
> 
> As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and
> hence imo preferable, especially in printk() and alike invocations.

The goal of using typesafe is to make the code safer not try to 
open-code everything because it might be shorter to write.

> 
> I would also prefer if you left %#lx alone, with the 2nd best
> option being to also use PRI_gfn alongside PRI_mfn. Primarily
> I'd like to avoid having a mixture.
The two options would be wrong:
	* gfn is an unsigned long and not gfn_t, so using PRI_gfn would be 
incorrect
	* mfn is now an mfn_t so using %lx would be incorrect

So the format string used in the patch is correct based on the types 
used. This...

> 
> Same (for both) at least one more time further down.

... would likely be applicable for all the other uses.

>> @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>>                   P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
>>                             gfn_x(ogfn) , mfn_x(omfn));
>>                   if ( mfn_eq(omfn, mfn_add(mfn, i)) )
>> -                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)),
>> +                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i),
>>                                       0);
> 
> Pull this up then onto the now shorter prior line?

Ok.

> 
>> @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d,
>>       spin_lock(&d->page_alloc_lock);
>>       page_list_for_each ( page, &d->page_list )
>>       {
>> -        mfn = mfn_x(page_to_mfn(page));
>> +        mfn = page_to_mfn(page);
>>   
>> -        P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn);
>> +        P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn));
>>   
>>           od = page_get_owner(page);
>>   
>>           if ( od != d )
>>           {
>> -            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d);
>> +            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d);
>>               continue;
>>           }
>>   
>> -        gfn = get_gpfn_from_mfn(mfn);
>> +        gfn = get_pfn_from_mfn(mfn);
>>           if ( gfn == INVALID_M2P_ENTRY )
>>           {
>>               orphans_count++;
>> -            P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n",
>> -                           mfn);
>> +            P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n",
>> +                       mfn_x(mfn));
>>               continue;
>>           }
>>   
>>           if ( SHARED_M2P(gfn) )
>>           {
>> -            P2M_PRINTK("shared mfn (%lx) on domain page list!\n",
>> -                    mfn);
>> +            P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n",
>> +                       mfn_x(mfn));
>>               continue;
>>           }
>>   
>>           p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL);
>> -        if ( mfn_x(p2mfn) != mfn )
>> +        if ( !mfn_eq(p2mfn, mfn) )
>>           {
>>               mpbad++;
>> -            P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx"
>> +            P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn""
>>                          " (-> gfn %#lx)\n",
>> -                       mfn, gfn, mfn_x(p2mfn),
>> +                       mfn_x(mfn), gfn, mfn_x(p2mfn),
>>                          (mfn_valid(p2mfn)
>> -                        ? get_gpfn_from_mfn(mfn_x(p2mfn))
>> +                        ? get_pfn_from_mfn(p2mfn)
>>                           : -1u));
> 
> I realize this is an entirely unrelated change, but the -1u here
> is standing out too much to not mention it: Could I talk you into
> making this gfn_x(INVALID_GFN) at this occasion?

Hmmm, I am not sure why I missed this one. I will use gfn_x(INVALID_GFN).

> 
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>>    */
>>   extern bool machine_to_phys_mapping_valid;
>>   
>> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>>   {
>> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>> +    const unsigned long mfn = mfn_x(mfn_);
> 
> I think it would be better overall if the parameter was named
> "mfn" and there was no local variable altogether. This would
> bring things in line with ...

You asked for this approach on the previous version [1]:

"Btw, the cheaper (in terms of code churn) change here would seem to
be to name the function parameter mfn_, and the local variable mfn.
That'll also reduce the number of uses of the unfortunate trailing-
underscore-name."

So can you pick a side and stick with it?

> 
>> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>   
>>   extern struct rangeset *mmio_ro_ranges;
>>   
>> -#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
>> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
>> +{
>> +    return machine_to_phys_mapping[mfn_x(mfn)];
>> +}
> 
> ... this.
> 
> Jan
> 

Cheers,

[1] <5CF7A1090200007800235782@prv1-mh.provo.novell.com>
Jan Beulich March 30, 2020, 8:10 a.m. UTC | #5
On 28.03.2020 12:14, Julien Grall wrote:
> On 27/03/2020 13:15, Jan Beulich wrote:
>> On 22.03.2020 17:14, julien@xen.org wrote:
>>> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m)
>>>                   /* check for 1GB super page */
>>>                   if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE )
>>>                   {
>>> -                    mfn = l3e_get_pfn(l3e[i3]);
>>> -                    ASSERT(mfn_valid(_mfn(mfn)));
>>> +                    mfn = l3e_get_mfn(l3e[i3]);
>>> +                    ASSERT(mfn_valid(mfn));
>>>                       /* we have to cover 512x512 4K pages */
>>>                       for ( i2 = 0;
>>>                             i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES);
>>>                             i2++)
>>>                       {
>>> -                        m2pfn = get_gpfn_from_mfn(mfn+i2);
>>> +                        m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2));
>>>                           if ( m2pfn != (gfn + i2) )
>>>                           {
>>>                               pmbad++;
>>> -                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
>>> -                                       gfn + i2, mfn + i2, m2pfn);
>>> +                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n",
>>> +                                       gfn + i2, mfn_x(mfn_add(mfn, i2)),
>>
>> As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and
>> hence imo preferable, especially in printk() and alike invocations.
> 
> The goal of using typesafe is to make the code safer not try to
> open-code everything because it might be shorter to write.

I'm not talking about "everything". As soon as you use mfn_x()
_anywhere_, type-safety is gone. Since in printk() and alike you
unavoidably have to use it (at least for now), there's no win
from using e.g. mfn_add() as you do here, imo. And hence the
readability aspect gets even higher significance.

>> I would also prefer if you left %#lx alone, with the 2nd best
>> option being to also use PRI_gfn alongside PRI_mfn. Primarily
>> I'd like to avoid having a mixture.
> The two options would be wrong:
>     * gfn is an unsigned long and not gfn_t, so using PRI_gfn would be incorrect
>     * mfn is now an mfn_t so using %lx would be incorrect
> 
> So the format string used in the patch is correct based on the types used.

Hmm, xen/mm.h suggests a partial connection between e.g. mfn_t
and PRI_mfn, yes, but I think this is unhelpful as long as
mfn_x() needs to be explicitly used when specifying the printk()
arguments. Instead I view PRI_mfn and alike as a more general
format usable also for MFNs stored in unsigned long rather than
mfn_t.

I agree though that views here may differ. Hence wider agreement
on what the intentions are (also mid/long term), and hence how
well formed code ought to look like, would seem necessary here.

> This...
> 
>>
>> Same (for both) at least one more time further down.
> 
> ... would likely be applicable for all the other uses.

Agreed.

>>> --- a/xen/include/asm-x86/mm.h
>>> +++ b/xen/include/asm-x86/mm.h
>>> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
>>>    */
>>>   extern bool machine_to_phys_mapping_valid;
>>>   -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
>>>   {
>>> -    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>> +    const unsigned long mfn = mfn_x(mfn_);
>>
>> I think it would be better overall if the parameter was named
>> "mfn" and there was no local variable altogether. This would
>> bring things in line with ...
> 
> You asked for this approach on the previous version [1]:
> 
> "Btw, the cheaper (in terms of code churn) change here would seem to
> be to name the function parameter mfn_, and the local variable mfn.
> That'll also reduce the number of uses of the unfortunate trailing-
> underscore-name."
> 
> So can you pick a side and stick with it?

Well, things like this happen when you see the final result, sorry.
And indeed I recalled commenting on this before, but upon searching
I didn't manage to find the earlier reply, to better justify what I
also suspected might have been a change of mind.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
index 69332fb84d..5e78fb7703 100644
--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -89,7 +89,7 @@  mc_memerr_dhandler(struct mca_binfo *binfo,
             {
                 d = get_domain_by_id(bank->mc_domid);
                 ASSERT(d);
-                gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
+                gfn = get_pfn_from_mfn(maddr_to_mfn(bank->mc_addr));
 
                 if ( unmmap_broken_page(d, mfn, gfn) )
                 {
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2516548e49..2feb7a5993 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -476,7 +476,7 @@  void share_xen_page_with_guest(struct page_info *page, struct domain *d,
     if ( page_get_owner(page) == d )
         return;
 
-    set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), INVALID_M2P_ENTRY);
+    set_pfn_from_mfn(page_to_mfn(page), INVALID_M2P_ENTRY);
 
     spin_lock(&d->page_alloc_lock);
 
@@ -1040,7 +1040,7 @@  get_page_from_l1e(
 
             gdprintk(XENLOG_WARNING, "Error updating mappings for mfn %" PRI_mfn
                      " (pfn %" PRI_pfn ", from L1 entry %" PRIpte ") for d%d\n",
-                     mfn, get_gpfn_from_mfn(mfn),
+                     mfn, get_pfn_from_mfn(_mfn(mfn)),
                      l1e_get_intpte(l1e), l1e_owner->domain_id);
             return err;
         }
@@ -1051,7 +1051,7 @@  get_page_from_l1e(
  could_not_pin:
     gdprintk(XENLOG_WARNING, "Error getting mfn %" PRI_mfn " (pfn %" PRI_pfn
              ") from L1 entry %" PRIpte " for l1e_owner d%d, pg_owner d%d\n",
-             mfn, get_gpfn_from_mfn(mfn),
+             mfn, get_pfn_from_mfn(_mfn(mfn)),
              l1e_get_intpte(l1e), l1e_owner->domain_id, pg_owner->domain_id);
     if ( real_pg_owner != NULL )
         put_page(page);
@@ -2636,7 +2636,7 @@  static int validate_page(struct page_info *page, unsigned long type,
                  " (pfn %" PRI_pfn ") for type %" PRtype_info
                  ": caf=%08lx taf=%" PRtype_info "\n",
                  mfn_x(page_to_mfn(page)),
-                 get_gpfn_from_mfn(mfn_x(page_to_mfn(page))),
+                 get_pfn_from_mfn(page_to_mfn(page)),
                  type, page->count_info, page->u.inuse.type_info);
         if ( page != current->arch.old_guest_table )
             page->u.inuse.type_info = 0;
@@ -2946,7 +2946,7 @@  static int _get_page_type(struct page_info *page, unsigned long type,
                      "Bad type (saw %" PRtype_info " != exp %" PRtype_info ") "
                      "for mfn %" PRI_mfn " (pfn %" PRI_pfn ")\n",
                      x, type, mfn_x(page_to_mfn(page)),
-                     get_gpfn_from_mfn(mfn_x(page_to_mfn(page))));
+                     get_pfn_from_mfn(page_to_mfn(page)));
             return -EINVAL;
         }
         else if ( unlikely(!(x & PGT_validated)) )
@@ -4106,7 +4106,7 @@  long do_mmu_update(
                 break;
             }
 
-            set_gpfn_from_mfn(mfn_x(mfn), gpfn);
+            set_pfn_from_mfn(mfn, gpfn);
             paging_mark_pfn_dirty(pg_owner, _pfn(gpfn));
 
             put_page(page);
@@ -4590,7 +4590,7 @@  int xenmem_add_to_physmap_one(
         goto put_both;
 
     /* Unmap from old location, if any. */
-    old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
+    old_gpfn = get_pfn_from_mfn(mfn);
     ASSERT(!SHARED_M2P(old_gpfn));
     if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn )
     {
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 3835bc928f..018beec10f 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -426,15 +426,15 @@  static void mem_sharing_gfn_destroy(struct page_info *page, struct domain *d,
     xfree(gfn_info);
 }
 
-static struct page_info *mem_sharing_lookup(unsigned long mfn)
+static struct page_info *mem_sharing_lookup(mfn_t mfn)
 {
     struct page_info *page;
     unsigned long t;
 
-    if ( !mfn_valid(_mfn(mfn)) )
+    if ( !mfn_valid(mfn) )
         return NULL;
 
-    page = mfn_to_page(_mfn(mfn));
+    page = mfn_to_page(mfn);
     if ( page_get_owner(page) != dom_cow )
         return NULL;
 
@@ -446,7 +446,7 @@  static struct page_info *mem_sharing_lookup(unsigned long mfn)
     t = read_atomic(&page->u.inuse.type_info);
     ASSERT((t & PGT_type_mask) == PGT_shared_page);
     ASSERT((t & PGT_count_mask) >= 2);
-    ASSERT(SHARED_M2P(get_gpfn_from_mfn(mfn)));
+    ASSERT(SHARED_M2P(get_pfn_from_mfn(mfn)));
 
     return page;
 }
@@ -505,10 +505,10 @@  static int audit(void)
         }
 
         /* Check the m2p entry */
-        if ( !SHARED_M2P(get_gpfn_from_mfn(mfn_x(mfn))) )
+        if ( !SHARED_M2P(get_pfn_from_mfn(mfn)) )
         {
-            gdprintk(XENLOG_ERR, "mfn %lx shared, but wrong m2p entry (%lx)!\n",
-                     mfn_x(mfn), get_gpfn_from_mfn(mfn_x(mfn)));
+            gdprintk(XENLOG_ERR, "mfn %"PRI_mfn" shared, but wrong m2p entry (%lx)!\n",
+                     mfn_x(mfn), get_pfn_from_mfn(mfn));
             errors++;
         }
 
@@ -736,7 +736,7 @@  static struct page_info *__grab_shared_page(mfn_t mfn)
     if ( !mem_sharing_page_lock(pg) )
         return NULL;
 
-    if ( mem_sharing_lookup(mfn_x(mfn)) == NULL )
+    if ( mem_sharing_lookup(mfn) == NULL )
     {
         mem_sharing_page_unlock(pg);
         return NULL;
@@ -918,7 +918,7 @@  static int nominate_page(struct domain *d, gfn_t gfn,
     atomic_inc(&nr_shared_mfns);
 
     /* Update m2p entry to SHARED_M2P_ENTRY */
-    set_gpfn_from_mfn(mfn_x(mfn), SHARED_M2P_ENTRY);
+    set_pfn_from_mfn(mfn, SHARED_M2P_ENTRY);
 
     *phandle = page->sharing->handle;
     audit_add_list(page);
@@ -1306,7 +1306,7 @@  int __mem_sharing_unshare_page(struct domain *d,
     }
 
     /* Update m2p entry */
-    set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), gfn);
+    set_pfn_from_mfn(page_to_mfn(page), gfn);
 
     /*
      * Now that the gfn<->mfn map is properly established,
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 2a7b8c117b..a9ac44a65c 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -644,7 +644,7 @@  p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
             }
             p2m_tlb_flush_sync(p2m);
             for ( j = 0; j < n; ++j )
-                set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
+                set_pfn_from_mfn(mfn, INVALID_M2P_ENTRY);
             p2m_pod_cache_add(p2m, page, cur_order);
 
             steal_for_cache =  ( p2m->pod.entry_count > p2m->pod.count );
@@ -1194,7 +1194,7 @@  p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn,
 
     for( i = 0; i < (1UL << order); i++ )
     {
-        set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn_aligned) + i);
+        set_pfn_from_mfn(mfn_add(mfn, i), gfn_x(gfn_aligned) + i);
         paging_mark_pfn_dirty(d, _pfn(gfn_x(gfn_aligned) + i));
     }
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index e9da34d668..1601e9e5e9 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -944,7 +944,8 @@  static int p2m_pt_change_entry_type_range(struct p2m_domain *p2m,
 long p2m_pt_audit_p2m(struct p2m_domain *p2m)
 {
     unsigned long entry_count = 0, pmbad = 0;
-    unsigned long mfn, gfn, m2pfn;
+    unsigned long gfn, m2pfn;
+    mfn_t mfn;
 
     ASSERT(p2m_locked_by_me(p2m));
     ASSERT(pod_locked_by_me(p2m));
@@ -983,19 +984,20 @@  long p2m_pt_audit_p2m(struct p2m_domain *p2m)
                 /* check for 1GB super page */
                 if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE )
                 {
-                    mfn = l3e_get_pfn(l3e[i3]);
-                    ASSERT(mfn_valid(_mfn(mfn)));
+                    mfn = l3e_get_mfn(l3e[i3]);
+                    ASSERT(mfn_valid(mfn));
                     /* we have to cover 512x512 4K pages */
                     for ( i2 = 0; 
                           i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES);
                           i2++)
                     {
-                        m2pfn = get_gpfn_from_mfn(mfn+i2);
+                        m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2));
                         if ( m2pfn != (gfn + i2) )
                         {
                             pmbad++;
-                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
-                                       gfn + i2, mfn + i2, m2pfn);
+                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n",
+                                       gfn + i2, mfn_x(mfn_add(mfn, i2)),
+                                       m2pfn);
                             BUG();
                         }
                         gfn += 1 << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
@@ -1019,17 +1021,18 @@  long p2m_pt_audit_p2m(struct p2m_domain *p2m)
                     /* check for super page */
                     if ( l2e_get_flags(l2e[i2]) & _PAGE_PSE )
                     {
-                        mfn = l2e_get_pfn(l2e[i2]);
-                        ASSERT(mfn_valid(_mfn(mfn)));
+                        mfn = l2e_get_mfn(l2e[i2]);
+                        ASSERT(mfn_valid(mfn));
                         for ( i1 = 0; i1 < L1_PAGETABLE_ENTRIES; i1++)
                         {
-                            m2pfn = get_gpfn_from_mfn(mfn+i1);
+                            m2pfn = get_pfn_from_mfn(mfn_add(mfn, i1));
                             /* Allow shared M2Ps */
                             if ( (m2pfn != (gfn + i1)) && !SHARED_M2P(m2pfn) )
                             {
                                 pmbad++;
-                                P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
-                                           gfn + i1, mfn + i1, m2pfn);
+                                P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" -> gfn %#lx\n",
+                                           gfn + i1, mfn_x(mfn_add(mfn, i1)),
+                                           m2pfn);
                                 BUG();
                             }
                         }
@@ -1050,17 +1053,17 @@  long p2m_pt_audit_p2m(struct p2m_domain *p2m)
                                 entry_count++;
                             continue;
                         }
-                        mfn = l1e_get_pfn(l1e[i1]);
-                        ASSERT(mfn_valid(_mfn(mfn)));
-                        m2pfn = get_gpfn_from_mfn(mfn);
+                        mfn = l1e_get_mfn(l1e[i1]);
+                        ASSERT(mfn_valid(mfn));
+                        m2pfn = get_pfn_from_mfn(mfn);
                         if ( m2pfn != gfn &&
                              type != p2m_mmio_direct &&
                              !p2m_is_grant(type) &&
                              !p2m_is_shared(type) )
                         {
                             pmbad++;
-                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n",
-                                       gfn, mfn, m2pfn);
+                            P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" -> gfn %#lx\n",
+                                       gfn, mfn_x(mfn), m2pfn);
                             BUG();
                         }
                     }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b6b01a71c8..587c062481 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -769,7 +769,7 @@  void p2m_final_teardown(struct domain *d)
 
 
 static int
-p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn,
+p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, mfn_t mfn,
                 unsigned int page_order)
 {
     unsigned long i;
@@ -783,17 +783,17 @@  p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn,
         return 0;
 
     ASSERT(gfn_locked_by_me(p2m, gfn));
-    P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn);
+    P2M_DEBUG("removing gfn=%#lx mfn=%"PRI_mfn"\n", gfn_l, mfn_x(mfn));
 
-    if ( mfn_valid(_mfn(mfn)) )
+    if ( mfn_valid(mfn) )
     {
         for ( i = 0; i < (1UL << page_order); i++ )
         {
             mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0,
                                         NULL, NULL);
             if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) )
-                set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY);
-            ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) );
+                set_pfn_from_mfn(mfn_add(mfn, i), INVALID_M2P_ENTRY);
+            ASSERT( !p2m_is_valid(t) || mfn_eq(mfn_add(mfn, i), mfn_return) );
         }
     }
     return p2m_set_entry(p2m, gfn, INVALID_MFN, page_order, p2m_invalid,
@@ -807,7 +807,7 @@  guest_physmap_remove_page(struct domain *d, gfn_t gfn,
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc;
     gfn_lock(p2m, gfn, page_order);
-    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order);
+    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn, page_order);
     gfn_unlock(p2m, gfn, page_order);
     return rc;
 }
@@ -842,7 +842,7 @@  guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
             else
                 return -EINVAL;
 
-            set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i);
+            set_pfn_from_mfn(mfn_add(mfn, i), gfn_x(gfn) + i);
         }
 
         return 0;
@@ -930,7 +930,7 @@  guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
         else if ( p2m_is_ram(ot) && !p2m_is_paged(ot) )
         {
             ASSERT(mfn_valid(omfn));
-            set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
+            set_pfn_from_mfn(omfn, INVALID_M2P_ENTRY);
         }
         else if ( ot == p2m_populate_on_demand )
         {
@@ -974,7 +974,7 @@  guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
                 P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
                           gfn_x(ogfn) , mfn_x(omfn));
                 if ( mfn_eq(omfn, mfn_add(mfn, i)) )
-                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)),
+                    p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i),
                                     0);
             }
         }
@@ -992,8 +992,8 @@  guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
         if ( !p2m_is_grant(t) )
         {
             for ( i = 0; i < (1UL << page_order); i++ )
-                set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)),
-                                  gfn_x(gfn_add(gfn, i)));
+                set_pfn_from_mfn(mfn_add(mfn, i),
+                                 gfn_x(gfn_add(gfn, i)));
         }
     }
 
@@ -1279,7 +1279,7 @@  static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
         for ( i = 0; i < (1UL << order); ++i )
         {
             ASSERT(mfn_valid(mfn_add(omfn, i)));
-            set_gpfn_from_mfn(mfn_x(omfn) + i, INVALID_M2P_ENTRY);
+            set_pfn_from_mfn(mfn_add(omfn, i), INVALID_M2P_ENTRY);
         }
     }
 
@@ -1475,7 +1475,7 @@  int set_shared_p2m_entry(struct domain *d, unsigned long gfn_l, mfn_t mfn)
     pg_type = read_atomic(&(mfn_to_page(omfn)->u.inuse.type_info));
     if ( (pg_type & PGT_count_mask) == 0
          || (pg_type & PGT_type_mask) != PGT_shared_page )
-        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
+        set_pfn_from_mfn(omfn, INVALID_M2P_ENTRY);
 
     P2M_DEBUG("set shared %lx %lx\n", gfn_l, mfn_x(mfn));
     rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_shared,
@@ -1829,7 +1829,7 @@  int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t buffer)
     ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
                         paging_mode_log_dirty(d) ? p2m_ram_logdirty
                                                  : p2m_ram_rw, a);
-    set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
+    set_pfn_from_mfn(mfn, gfn_l);
 
     if ( !page_extant )
         atomic_dec(&d->paged_pages);
@@ -1880,7 +1880,7 @@  void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
                                    p2m_ram_rw, a);
 
             if ( !rc )
-                set_gpfn_from_mfn(mfn_x(mfn), gfn_x(gfn));
+                set_pfn_from_mfn(mfn, gfn_x(gfn));
         }
         gfn_unlock(p2m, gfn, 0);
     }
@@ -2706,7 +2706,7 @@  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     {
         mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) )
-            p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
+            p2m_remove_page(ap2m, gfn_x(old_gfn), mfn, PAGE_ORDER_4K);
         rc = 0;
         goto out;
     }
@@ -2820,8 +2820,8 @@  void audit_p2m(struct domain *d,
 {
     struct page_info *page;
     struct domain *od;
-    unsigned long mfn, gfn;
-    mfn_t p2mfn;
+    unsigned long gfn;
+    mfn_t p2mfn, mfn;
     unsigned long orphans_count = 0, mpbad = 0, pmbad = 0;
     p2m_access_t p2ma;
     p2m_type_t type;
@@ -2843,53 +2843,53 @@  void audit_p2m(struct domain *d,
     spin_lock(&d->page_alloc_lock);
     page_list_for_each ( page, &d->page_list )
     {
-        mfn = mfn_x(page_to_mfn(page));
+        mfn = page_to_mfn(page);
 
-        P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn);
+        P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn));
 
         od = page_get_owner(page);
 
         if ( od != d )
         {
-            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d);
+            P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d);
             continue;
         }
 
-        gfn = get_gpfn_from_mfn(mfn);
+        gfn = get_pfn_from_mfn(mfn);
         if ( gfn == INVALID_M2P_ENTRY )
         {
             orphans_count++;
-            P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n",
-                           mfn);
+            P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n",
+                       mfn_x(mfn));
             continue;
         }
 
         if ( SHARED_M2P(gfn) )
         {
-            P2M_PRINTK("shared mfn (%lx) on domain page list!\n",
-                    mfn);
+            P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n",
+                       mfn_x(mfn));
             continue;
         }
 
         p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL);
-        if ( mfn_x(p2mfn) != mfn )
+        if ( !mfn_eq(p2mfn, mfn) )
         {
             mpbad++;
-            P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx"
+            P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn""
                        " (-> gfn %#lx)\n",
-                       mfn, gfn, mfn_x(p2mfn),
+                       mfn_x(mfn), gfn, mfn_x(p2mfn),
                        (mfn_valid(p2mfn)
-                        ? get_gpfn_from_mfn(mfn_x(p2mfn))
+                        ? get_pfn_from_mfn(p2mfn)
                         : -1u));
             /* This m2p entry is stale: the domain has another frame in
              * this physical slot.  No great disaster, but for neatness,
              * blow away the m2p entry. */
-            set_gpfn_from_mfn(mfn, INVALID_M2P_ENTRY);
+            set_pfn_from_mfn(mfn, INVALID_M2P_ENTRY);
         }
         __put_gfn(p2m, gfn);
 
-        P2M_PRINTK("OK: mfn=%#lx, gfn=%#lx, p2mfn=%#lx\n",
-                       mfn, gfn, mfn_x(p2mfn));
+        P2M_PRINTK("OK: mfn=%"PRI_mfn", gfn=%#lx, p2mfn=%"PRI_mfn"\n",
+                   mfn_x(mfn), gfn, mfn_x(p2mfn));
     }
     spin_unlock(&d->page_alloc_lock);
 
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 469bb76429..2f6df74135 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -344,7 +344,7 @@  void paging_mark_dirty(struct domain *d, mfn_t gmfn)
         return;
 
     /* We /really/ mean PFN here, even for non-translated guests. */
-    pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
+    pfn = _pfn(get_pfn_from_mfn(gmfn));
 
     paging_mark_pfn_dirty(d, pfn);
 }
@@ -362,7 +362,7 @@  int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
     ASSERT(paging_mode_log_dirty(d));
 
     /* We /really/ mean PFN here, even for non-translated guests. */
-    pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
+    pfn = _pfn(get_pfn_from_mfn(gmfn));
     /* Invalid pages can't be dirty. */
     if ( unlikely(!VALID_M2P(pfn_x(pfn))) )
         return 0;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 8abd5d255c..9f558b2932 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -39,7 +39,7 @@  void __init dom0_update_physmap(struct domain *d, unsigned long pfn,
     else
         ((unsigned int *)vphysmap_s)[pfn] = mfn;
 
-    set_gpfn_from_mfn(mfn, pfn);
+    set_pfn_from_mfn(_mfn(mfn), pfn);
 }
 
 static __init void mark_pv_pt_pages_rdonly(struct domain *d,
@@ -789,8 +789,8 @@  int __init dom0_construct_pv(struct domain *d,
     page_list_for_each ( page, &d->page_list )
     {
         mfn = mfn_x(page_to_mfn(page));
-        BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
-        if ( get_gpfn_from_mfn(mfn) >= count )
+        BUG_ON(SHARED_M2P(get_pfn_from_mfn(_mfn(mfn))));
+        if ( get_pfn_from_mfn(_mfn(mfn)) >= count )
         {
             BUG_ON(is_pv_32bit_domain(d));
             if ( !page->u.inuse.type_info &&
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 811c2cb37b..bf5c2060e7 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -200,7 +200,7 @@  void show_page_walk(unsigned long addr)
     unmap_domain_page(l4t);
     mfn = l4e_get_mfn(l4e);
     pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ?
-          get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY;
+          get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY;
     printk(" L4[0x%03lx] = %"PRIpte" %016lx\n",
            l4_table_offset(addr), l4e_get_intpte(l4e), pfn);
     if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) || !mfn_valid(mfn) )
@@ -211,7 +211,7 @@  void show_page_walk(unsigned long addr)
     unmap_domain_page(l3t);
     mfn = l3e_get_mfn(l3e);
     pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ?
-          get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY;
+          get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY;
     printk(" L3[0x%03lx] = %"PRIpte" %016lx%s\n",
            l3_table_offset(addr), l3e_get_intpte(l3e), pfn,
            (l3e_get_flags(l3e) & _PAGE_PSE) ? " (PSE)" : "");
@@ -225,7 +225,7 @@  void show_page_walk(unsigned long addr)
     unmap_domain_page(l2t);
     mfn = l2e_get_mfn(l2e);
     pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ?
-          get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY;
+          get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY;
     printk(" L2[0x%03lx] = %"PRIpte" %016lx%s\n",
            l2_table_offset(addr), l2e_get_intpte(l2e), pfn,
            (l2e_get_flags(l2e) & _PAGE_PSE) ? " (PSE)" : "");
@@ -239,7 +239,7 @@  void show_page_walk(unsigned long addr)
     unmap_domain_page(l1t);
     mfn = l1e_get_mfn(l1e);
     pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ?
-          get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY;
+          get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY;
     printk(" L1[0x%03lx] = %"PRIpte" %016lx\n",
            l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
 }
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 41e4fa899d..239aac18dd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1430,7 +1430,7 @@  static void free_heap_pages(
 
         /* This page is not a guest frame any more. */
         page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
-        set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
+        set_pfn_from_mfn(mfn_add(mfn, i), INVALID_M2P_ENTRY);
 
         if ( need_scrub )
         {
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index abf4cc23e4..11614f9107 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -319,7 +319,7 @@  struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
 #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
 
 /* Xen always owns P2M on ARM */
-#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
+static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {}
 #define mfn_to_gmfn(_d, mfn)  (mfn)
 
 
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 5871238f6d..b6a09c4c6c 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -41,7 +41,7 @@  static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
     mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
                       : gnttab_shared_mfn(gt, idx);                      \
-    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_));                \
+    unsigned long gpfn_ = get_pfn_from_mfn(mfn_);                        \
     VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
 })
 
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 53f2ed7c7d..2a4f42e78f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -500,9 +500,10 @@  extern paddr_t mem_hotplug;
  */
 extern bool machine_to_phys_mapping_valid;
 
-static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
+static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
 {
-    const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
+    const unsigned long mfn = mfn_x(mfn_);
+    const struct domain *d = page_get_owner(mfn_to_page(mfn_));
     unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
 
     if ( !machine_to_phys_mapping_valid )
@@ -515,11 +516,14 @@  static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
 
 extern struct rangeset *mmio_ro_ranges;
 
-#define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
+static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
+{
+    return machine_to_phys_mapping[mfn_x(mfn)];
+}
 
 #define mfn_to_gmfn(_d, mfn)                            \
     ( (paging_mode_translate(_d))                       \
-      ? get_gpfn_from_mfn(mfn)                          \
+      ? get_pfn_from_mfn(_mfn(mfn))                     \
       : (mfn) )
 
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20))
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index a2c6049834..39dae242b0 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -505,7 +505,7 @@  static inline struct page_info *get_page_from_gfn(
 static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn)
 {
     if ( paging_mode_translate(d) )
-        return _gfn(get_gpfn_from_mfn(mfn_x(mfn)));
+        return _gfn(get_pfn_from_mfn(mfn));
     else
         return _gfn(mfn_x(mfn));
 }