[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
>     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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.