diff mbox series

[Xen-devel,06/14] xen: Convert hotplug page function to use typesafe MFN

Message ID 20190507151458.29350-7-julien.grall@arm.com
State Superseded
Headers show
Series xen/arm: Properly disable M2P on Arm. | expand

Commit Message

Julien Grall May 7, 2019, 3:14 p.m. UTC
Convert online_page, offline_page and query_page_offline to use
typesafe MFN.

Note, for clarity, the words have been re-ordered in the error message
updated by this patch.

No functional changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
    Changes:
        - Update error message
        - Add Jan's acked-by
---
 xen/arch/x86/cpu/mcheck/mcaction.c | 18 ++++++++++--------
 xen/common/page_alloc.c            | 24 ++++++++++++------------
 xen/common/sysctl.c                | 14 +++++++-------
 xen/include/xen/mm.h               |  6 +++---
 4 files changed, 32 insertions(+), 30 deletions(-)

Comments

Stefano Stabellini May 9, 2019, 6:01 p.m. UTC | #1
On Tue, 7 May 2019, Julien Grall wrote:
> Convert online_page, offline_page and query_page_offline to use
> typesafe MFN.

I would like to have a statement in the commit message mentioning the
changes below to mci_action_add_pageoffline and mc_memerr_dhandler.

From an ARM point of view, it is fine.



> Note, for clarity, the words have been re-ordered in the error message
> updated by this patch.
> 
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
>     Changes:
>         - Update error message
>         - Add Jan's acked-by
> ---
>  xen/arch/x86/cpu/mcheck/mcaction.c | 18 ++++++++++--------
>  xen/common/page_alloc.c            | 24 ++++++++++++------------
>  xen/common/sysctl.c                | 14 +++++++-------
>  xen/include/xen/mm.h               |  6 +++---
>  4 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
> index e42267414e..69332fb84d 100644
> --- a/xen/arch/x86/cpu/mcheck/mcaction.c
> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c
> @@ -6,7 +6,7 @@
>  
>  static struct mcinfo_recovery *
>  mci_action_add_pageoffline(int bank, struct mc_info *mi,
> -                           uint64_t mfn, uint32_t status)
> +                           mfn_t mfn, uint32_t status)
>  {
>      struct mcinfo_recovery *rec;
>  
> @@ -22,7 +22,7 @@ mci_action_add_pageoffline(int bank, struct mc_info *mi,
>  
>      rec->mc_bank = bank;
>      rec->action_types = MC_ACTION_PAGE_OFFLINE;
> -    rec->action_info.page_retire.mfn = mfn;
> +    rec->action_info.page_retire.mfn = mfn_x(mfn);
>      rec->action_info.page_retire.status = status;
>      return rec;
>  }
> @@ -42,7 +42,8 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
>      struct mcinfo_bank *bank = binfo->mib;
>      struct mcinfo_global *global = binfo->mig;
>      struct domain *d;
> -    unsigned long mfn, gfn;
> +    mfn_t mfn;
> +    unsigned long gfn;
>      uint32_t status;
>      int vmce_vcpuid;
>      unsigned int mc_vcpuid;
> @@ -54,11 +55,12 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
>          return;
>      }
>  
> -    mfn = bank->mc_addr >> PAGE_SHIFT;
> +    mfn = maddr_to_mfn(bank->mc_addr);
>      if ( offline_page(mfn, 1, &status) )
>      {
>          dprintk(XENLOG_WARNING,
> -                "Failed to offline page %lx for MCE error\n", mfn);
> +                "Failed to offline page %"PRI_mfn" for MCE error\n",
> +                mfn_x(mfn));
>          return;
>      }
>  
> @@ -89,10 +91,10 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
>                  ASSERT(d);
>                  gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
>  
> -                if ( unmmap_broken_page(d, _mfn(mfn), gfn) )
> +                if ( unmmap_broken_page(d, mfn, gfn) )
>                  {
> -                    printk("Unmap broken memory %lx for DOM%d failed\n",
> -                           mfn, d->domain_id);
> +                    printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
> +                           mfn_x(mfn), d->domain_id);
>                      goto vmce_failed;
>                  }
>  
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index be44158033..f445f7daec 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg)
>  
>  }
>  
> -int offline_page(unsigned long mfn, int broken, uint32_t *status)
> +int offline_page(mfn_t mfn, int broken, uint32_t *status)
>  {
>      unsigned long old_info = 0;
>      struct domain *owner;
>      struct page_info *pg;
>  
> -    if ( !mfn_valid(_mfn(mfn)) )
> +    if ( !mfn_valid(mfn) )
>      {
>          dprintk(XENLOG_WARNING,
> -                "try to offline page out of range %lx\n", mfn);
> +                "try to offline out of range page %"PRI_mfn"\n", mfn_x(mfn));
>          return -EINVAL;
>      }
>  
>      *status = 0;
> -    pg = mfn_to_page(_mfn(mfn));
> +    pg = mfn_to_page(mfn);
>  
> -    if ( is_xen_fixed_mfn(mfn) )
> +    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
>      {
>          *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
>            (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
> @@ -1595,7 +1595,7 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status)
>       * N.B. xen's txt in x86_64 is marked reserved and handled already.
>       * Also kexec range is reserved.
>       */
> -    if ( !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
> +    if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
>      {
>          *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM;
>          return -EINVAL;
> @@ -1677,19 +1677,19 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status)
>   *   The caller should make sure end_pfn <= max_page,
>   *   if not, expand_pages() should be called prior to online_page().
>   */
> -unsigned int online_page(unsigned long mfn, uint32_t *status)
> +unsigned int online_page(mfn_t mfn, uint32_t *status)
>  {
>      unsigned long x, nx, y;
>      struct page_info *pg;
>      int ret;
>  
> -    if ( !mfn_valid(_mfn(mfn)) )
> +    if ( !mfn_valid(mfn) )
>      {
>          dprintk(XENLOG_WARNING, "call expand_pages() first\n");
>          return -EINVAL;
>      }
>  
> -    pg = mfn_to_page(_mfn(mfn));
> +    pg = mfn_to_page(mfn);
>  
>      spin_lock(&heap_lock);
>  
> @@ -1730,11 +1730,11 @@ unsigned int online_page(unsigned long mfn, uint32_t *status)
>      return ret;
>  }
>  
> -int query_page_offline(unsigned long mfn, uint32_t *status)
> +int query_page_offline(mfn_t mfn, uint32_t *status)
>  {
>      struct page_info *pg;
>  
> -    if ( !mfn_valid(_mfn(mfn)) || !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
> +    if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
>      {
>          dprintk(XENLOG_WARNING, "call expand_pages() first\n");
>          return -EINVAL;
> @@ -1743,7 +1743,7 @@ int query_page_offline(unsigned long mfn, uint32_t *status)
>      *status = 0;
>      spin_lock(&heap_lock);
>  
> -    pg = mfn_to_page(_mfn(mfn));
> +    pg = mfn_to_page(mfn);
>  
>      if ( page_state_is(pg, offlining) )
>          *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING;
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index c0aa6bde4e..ab161793e5 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -186,7 +186,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>      case XEN_SYSCTL_page_offline_op:
>      {
>          uint32_t *status, *ptr;
> -        unsigned long pfn;
> +        mfn_t mfn;
>  
>          ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
>          if ( ret )
> @@ -205,21 +205,21 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>          memset(status, PG_OFFLINE_INVALID, sizeof(uint32_t) *
>                        (op->u.page_offline.end - op->u.page_offline.start + 1));
>  
> -        for ( pfn = op->u.page_offline.start;
> -              pfn <= op->u.page_offline.end;
> -              pfn ++ )
> +        for ( mfn = _mfn(op->u.page_offline.start);
> +              mfn_x(mfn) <= op->u.page_offline.end;
> +              mfn = mfn_add(mfn, 1) )
>          {
>              switch ( op->u.page_offline.cmd )
>              {
>                  /* Shall revert her if failed, or leave caller do it? */
>                  case sysctl_page_offline:
> -                    ret = offline_page(pfn, 0, ptr++);
> +                    ret = offline_page(mfn, 0, ptr++);
>                      break;
>                  case sysctl_page_online:
> -                    ret = online_page(pfn, ptr++);
> +                    ret = online_page(mfn, ptr++);
>                      break;
>                  case sysctl_query_page_offline:
> -                    ret = query_page_offline(pfn, ptr++);
> +                    ret = query_page_offline(mfn, ptr++);
>                      break;
>                  default:
>                      ret = -EINVAL;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index e971147234..3ba7168cc9 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -206,9 +206,9 @@ unsigned long avail_domheap_pages(void);
>  unsigned long avail_node_heap_pages(unsigned int);
>  #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
>  #define free_domheap_page(p)  (free_domheap_pages(p,0))
> -unsigned int online_page(unsigned long mfn, uint32_t *status);
> -int offline_page(unsigned long mfn, int broken, uint32_t *status);
> -int query_page_offline(unsigned long mfn, uint32_t *status);
> +unsigned int online_page(mfn_t mfn, uint32_t *status);
> +int offline_page(mfn_t mfn, int broken, uint32_t *status);
> +int query_page_offline(mfn_t mfn, uint32_t *status);
>  unsigned long total_free_pages(void);
>  
>  void heap_init_late(void);
> -- 
> 2.11.0
>
Julien Grall May 9, 2019, 6:10 p.m. UTC | #2
Hi Stefano,

On 5/9/19 7:01 PM, Stefano Stabellini wrote:
> On Tue, 7 May 2019, Julien Grall wrote:
>> Convert online_page, offline_page and query_page_offline to use
>> typesafe MFN.
> 
> I would like to have a statement in the commit message mentioning the
> changes below to mci_action_add_pageoffline and mc_memerr_dhandler.

I would prefer the generic wording:

"At the same time, the typesafe is propagated as far as possible without
major modifications."

> 
>  From an ARM point of view, it is fine.

Thank you!

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
index e42267414e..69332fb84d 100644
--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -6,7 +6,7 @@ 
 
 static struct mcinfo_recovery *
 mci_action_add_pageoffline(int bank, struct mc_info *mi,
-                           uint64_t mfn, uint32_t status)
+                           mfn_t mfn, uint32_t status)
 {
     struct mcinfo_recovery *rec;
 
@@ -22,7 +22,7 @@  mci_action_add_pageoffline(int bank, struct mc_info *mi,
 
     rec->mc_bank = bank;
     rec->action_types = MC_ACTION_PAGE_OFFLINE;
-    rec->action_info.page_retire.mfn = mfn;
+    rec->action_info.page_retire.mfn = mfn_x(mfn);
     rec->action_info.page_retire.status = status;
     return rec;
 }
@@ -42,7 +42,8 @@  mc_memerr_dhandler(struct mca_binfo *binfo,
     struct mcinfo_bank *bank = binfo->mib;
     struct mcinfo_global *global = binfo->mig;
     struct domain *d;
-    unsigned long mfn, gfn;
+    mfn_t mfn;
+    unsigned long gfn;
     uint32_t status;
     int vmce_vcpuid;
     unsigned int mc_vcpuid;
@@ -54,11 +55,12 @@  mc_memerr_dhandler(struct mca_binfo *binfo,
         return;
     }
 
-    mfn = bank->mc_addr >> PAGE_SHIFT;
+    mfn = maddr_to_mfn(bank->mc_addr);
     if ( offline_page(mfn, 1, &status) )
     {
         dprintk(XENLOG_WARNING,
-                "Failed to offline page %lx for MCE error\n", mfn);
+                "Failed to offline page %"PRI_mfn" for MCE error\n",
+                mfn_x(mfn));
         return;
     }
 
@@ -89,10 +91,10 @@  mc_memerr_dhandler(struct mca_binfo *binfo,
                 ASSERT(d);
                 gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
 
-                if ( unmmap_broken_page(d, _mfn(mfn), gfn) )
+                if ( unmmap_broken_page(d, mfn, gfn) )
                 {
-                    printk("Unmap broken memory %lx for DOM%d failed\n",
-                           mfn, d->domain_id);
+                    printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
+                           mfn_x(mfn), d->domain_id);
                     goto vmce_failed;
                 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..f445f7daec 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1568,23 +1568,23 @@  static int reserve_heap_page(struct page_info *pg)
 
 }
 
-int offline_page(unsigned long mfn, int broken, uint32_t *status)
+int offline_page(mfn_t mfn, int broken, uint32_t *status)
 {
     unsigned long old_info = 0;
     struct domain *owner;
     struct page_info *pg;
 
-    if ( !mfn_valid(_mfn(mfn)) )
+    if ( !mfn_valid(mfn) )
     {
         dprintk(XENLOG_WARNING,
-                "try to offline page out of range %lx\n", mfn);
+                "try to offline out of range page %"PRI_mfn"\n", mfn_x(mfn));
         return -EINVAL;
     }
 
     *status = 0;
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
-    if ( is_xen_fixed_mfn(mfn) )
+    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
     {
         *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
           (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
@@ -1595,7 +1595,7 @@  int offline_page(unsigned long mfn, int broken, uint32_t *status)
      * N.B. xen's txt in x86_64 is marked reserved and handled already.
      * Also kexec range is reserved.
      */
-    if ( !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
+    if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
     {
         *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM;
         return -EINVAL;
@@ -1677,19 +1677,19 @@  int offline_page(unsigned long mfn, int broken, uint32_t *status)
  *   The caller should make sure end_pfn <= max_page,
  *   if not, expand_pages() should be called prior to online_page().
  */
-unsigned int online_page(unsigned long mfn, uint32_t *status)
+unsigned int online_page(mfn_t mfn, uint32_t *status)
 {
     unsigned long x, nx, y;
     struct page_info *pg;
     int ret;
 
-    if ( !mfn_valid(_mfn(mfn)) )
+    if ( !mfn_valid(mfn) )
     {
         dprintk(XENLOG_WARNING, "call expand_pages() first\n");
         return -EINVAL;
     }
 
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
     spin_lock(&heap_lock);
 
@@ -1730,11 +1730,11 @@  unsigned int online_page(unsigned long mfn, uint32_t *status)
     return ret;
 }
 
-int query_page_offline(unsigned long mfn, uint32_t *status)
+int query_page_offline(mfn_t mfn, uint32_t *status)
 {
     struct page_info *pg;
 
-    if ( !mfn_valid(_mfn(mfn)) || !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
+    if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
     {
         dprintk(XENLOG_WARNING, "call expand_pages() first\n");
         return -EINVAL;
@@ -1743,7 +1743,7 @@  int query_page_offline(unsigned long mfn, uint32_t *status)
     *status = 0;
     spin_lock(&heap_lock);
 
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
     if ( page_state_is(pg, offlining) )
         *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING;
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c0aa6bde4e..ab161793e5 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -186,7 +186,7 @@  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
     case XEN_SYSCTL_page_offline_op:
     {
         uint32_t *status, *ptr;
-        unsigned long pfn;
+        mfn_t mfn;
 
         ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
         if ( ret )
@@ -205,21 +205,21 @@  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         memset(status, PG_OFFLINE_INVALID, sizeof(uint32_t) *
                       (op->u.page_offline.end - op->u.page_offline.start + 1));
 
-        for ( pfn = op->u.page_offline.start;
-              pfn <= op->u.page_offline.end;
-              pfn ++ )
+        for ( mfn = _mfn(op->u.page_offline.start);
+              mfn_x(mfn) <= op->u.page_offline.end;
+              mfn = mfn_add(mfn, 1) )
         {
             switch ( op->u.page_offline.cmd )
             {
                 /* Shall revert her if failed, or leave caller do it? */
                 case sysctl_page_offline:
-                    ret = offline_page(pfn, 0, ptr++);
+                    ret = offline_page(mfn, 0, ptr++);
                     break;
                 case sysctl_page_online:
-                    ret = online_page(pfn, ptr++);
+                    ret = online_page(mfn, ptr++);
                     break;
                 case sysctl_query_page_offline:
-                    ret = query_page_offline(pfn, ptr++);
+                    ret = query_page_offline(mfn, ptr++);
                     break;
                 default:
                     ret = -EINVAL;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index e971147234..3ba7168cc9 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -206,9 +206,9 @@  unsigned long avail_domheap_pages(void);
 unsigned long avail_node_heap_pages(unsigned int);
 #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
 #define free_domheap_page(p)  (free_domheap_pages(p,0))
-unsigned int online_page(unsigned long mfn, uint32_t *status);
-int offline_page(unsigned long mfn, int broken, uint32_t *status);
-int query_page_offline(unsigned long mfn, uint32_t *status);
+unsigned int online_page(mfn_t mfn, uint32_t *status);
+int offline_page(mfn_t mfn, int broken, uint32_t *status);
+int query_page_offline(mfn_t mfn, uint32_t *status);
 unsigned long total_free_pages(void);
 
 void heap_init_late(void);