Re: [Xen-devel] [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The x86 version of the function xenmem_add_to_physmap_one contains
> variable name gpfn and gfn which make the code very confusing.
> I have left unchanged for now.
> 
> Also, rename gpfn to gfn in the ARM version as the latter is the correct
> acronym for a guest physical frame.
> 
> Finally, remove the trailing whitespace around the changes.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
>     Changes in v3:
>         - Add Jan's acked-by for non-ARM bits
> ---
>  xen/arch/arm/mm.c    | 10 +++++-----
>  xen/arch/x86/mm.c    | 15 +++++++--------
>  xen/common/memory.c  |  6 +++---
>  xen/include/xen/mm.h |  2 +-
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5ab9b75..6882d54 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gfn)

I think there is a possible loss of information here: xen_pfn_t is
always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
its current definition. Or am I missing something?

In fact, given that ARM supports LPAE, shouldn't gfn by defined as
xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?


>  {
>      unsigned long mfn = 0;
>      int rc;
> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
>              else
>                  return -EINVAL;
>          }
> -        
> -        d->arch.grant_table_gpfn[idx] = gpfn;
> +
> +        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);

Similarly grant_table_gpfn is an array of xen_pfn_t (uint64_t), while
gfn_x unboxes to the storage size of gfn (unsigned long).


>          t = p2m_ram_rw;
>  
> @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one(
>          if ( extra.res0 )
>              return -EOPNOTSUPP;
>  
> -        rc = map_dev_mmio_region(d, gpfn, 1, idx);
> +        rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx);
>          return rc;
>  
>      default:
> @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
> +    rc = guest_physmap_add_entry(d, gfn, _mfn(mfn), 0, t);
>  
>      /* If we fail to add the mapping, we need to drop the reference we
>       * took earlier on foreign pages */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7fbc94e..dbcf6cb 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4775,7 +4775,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gpfn)
>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> @@ -4834,7 +4834,7 @@ int xenmem_add_to_physmap_one(
>              break;
>          }
>          case XENMAPSPACE_gmfn_foreign:
> -            return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid);
> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
>          default:
>              break;
>      }
> @@ -4849,19 +4849,18 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Remove previously mapped page if it was present. */
> -    prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt));
> +    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
>      if ( mfn_valid(prev_mfn) )
>      {
>          if ( is_xen_heap_mfn(prev_mfn) )
>              /* Xen heap frames are simply unhooked from this phys slot. */
> -            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
> -                                      PAGE_ORDER_4K);
> +            guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), 
> PAGE_ORDER_4K);
>          else
>              /* Normal domain memory is freed, to avoid leaking memory. */
> -            guest_remove_page(d, gpfn);
> +            guest_remove_page(d, gfn_x(gpfn));
>      }
>      /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
> -    put_gfn(d, gpfn);
> +    put_gfn(d, gfn_x(gpfn));
>  
>      /* Unmap from old location, if any. */
>      old_gpfn = get_gpfn_from_mfn(mfn);
> @@ -4872,7 +4871,7 @@ int xenmem_add_to_physmap_one(
>          guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), 
> PAGE_ORDER_4K);
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
> +    rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
>      if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index a8a75e0..812334b 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -649,7 +649,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>  
>      if ( xatp->space != XENMAPSPACE_gmfn_range )
>          return xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                         xatp->idx, xatp->gpfn);
> +                                         xatp->idx, _gfn(xatp->gpfn));
>  
>      if ( xatp->size < start )
>          return -EILSEQ;
> @@ -666,7 +666,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>      while ( xatp->size > done )
>      {
>          rc = xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                       xatp->idx, xatp->gpfn);
> +                                       xatp->idx, _gfn(xatp->gpfn));
>          if ( rc < 0 )
>              break;
>  
> @@ -735,7 +735,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
>  
>          rc = xenmem_add_to_physmap_one(d, xatpb->space,
>                                         xatpb->u,
> -                                       idx, gpfn);
> +                                       idx, _gfn(gpfn));
>  
>          if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
>          {
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index b62f473..afbb1a1 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -548,7 +548,7 @@ void scrub_one_page(struct page_info *);
>  
>  int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
>                                union xen_add_to_physmap_batch_extra extra,
> -                              unsigned long idx, xen_pfn_t gpfn);
> +                              unsigned long idx, gfn_t gfn);
>  
>  /* Returns 1 on success, 0 on error, negative if the ring
>   * for event propagation is full in the presence of paging */
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel