Re: [PATCH v2 1/8] xen: move XENMAPSPACE_grant_table code into grant_table.c

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 06 September 2017 09:26
> To: xen-devel@lists.xen.org
> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> jbeulich@suse.com
> Subject: [Xen-devel] [PATCH v2 1/8] xen: move XENMAPSPACE_grant_table
> code into grant_table.c
> 
> The x86 and arm versions of XENMAPSPACE_grant_table handling are nearly
> identical. Move the code into a function in grant_table.c and add an
> architecture dependant hook to handle the differences.
> 
> This at once fixes a bug in the arm code which didn't unlock the grant
> table in error case.

You also move to greater use of type-safe mfn in x86 xenmem_add_to_physmap_one(), which is a good thing :-). Might be worth a mention in the commit comment though.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - rebased to staging
> ---
>  xen/arch/arm/mm.c                 | 36 ++++------------------------------
>  xen/arch/x86/mm.c                 | 41 ++++++++++-----------------------------
>  xen/common/grant_table.c          | 38
> ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/grant_table.h |  7 +++++++
>  xen/include/asm-x86/grant_table.h |  5 +++++
>  xen/include/xen/grant_table.h     |  3 +++
>  6 files changed, 67 insertions(+), 63 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b39677eac9..3db0e3bdea 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1229,39 +1229,11 @@ int xenmem_add_to_physmap_one(
>      switch ( space )
>      {
>      case XENMAPSPACE_grant_table:
> -        grant_write_lock(d->grant_table);
> -
> -        if ( d->grant_table->gt_version == 0 )
> -            d->grant_table->gt_version = 1;
> -
> -        if ( d->grant_table->gt_version == 2 &&
> -                (idx & XENMAPIDX_grant_table_status) )
> -        {
> -            idx &= ~XENMAPIDX_grant_table_status;
> -            if ( idx < nr_status_frames(d->grant_table) )
> -                mfn = virt_to_mfn(d->grant_table->status[idx]);
> -        }
> -        else
> -        {
> -            if ( (idx >= nr_grant_frames(d->grant_table)) &&
> -                 (idx < max_grant_frames) )
> -                gnttab_grow_table(d, idx + 1);
> -
> -            if ( idx < nr_grant_frames(d->grant_table) )
> -                mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
> -        }
> -
> -        if ( !mfn_eq(mfn, INVALID_MFN) )
> -        {
> -            d->arch.grant_table_gfn[idx] = gfn;
> -
> -            t = p2m_ram_rw;
> -        }
> -
> -        grant_write_unlock(d->grant_table);
> +        rc = gnttab_map_frame(d, idx, gfn, &mfn);
> +        if ( rc )
> +            return rc;
> 
> -        if ( mfn_eq(mfn, INVALID_MFN) )
> -            return -EINVAL;
> +        t = p2m_ram_rw;
> 
>          break;
>      case XENMAPSPACE_shared_info:
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e5a029c9be..3d899e4a8e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4631,40 +4631,19 @@ int xenmem_add_to_physmap_one(
>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> -    unsigned long prev_mfn, mfn = 0, old_gpfn;
> +    unsigned long prev_mfn, old_gpfn;
>      int rc = 0;
> +    mfn_t mfn = INVALID_MFN;
>      p2m_type_t p2mt;
> 
>      switch ( space )
>      {
>          case XENMAPSPACE_shared_info:
>              if ( idx == 0 )
> -                mfn = virt_to_mfn(d->shared_info);
> +                mfn = _mfn(virt_to_mfn(d->shared_info));
>              break;
>          case XENMAPSPACE_grant_table:
> -            grant_write_lock(d->grant_table);
> -
> -            if ( d->grant_table->gt_version == 0 )
> -                d->grant_table->gt_version = 1;
> -
> -            if ( d->grant_table->gt_version == 2 &&
> -                 (idx & XENMAPIDX_grant_table_status) )
> -            {
> -                idx &= ~XENMAPIDX_grant_table_status;
> -                if ( idx < nr_status_frames(d->grant_table) )
> -                    mfn = virt_to_mfn(d->grant_table->status[idx]);
> -            }
> -            else
> -            {
> -                if ( (idx >= nr_grant_frames(d->grant_table)) &&
> -                     (idx < max_grant_frames) )
> -                    gnttab_grow_table(d, idx + 1);
> -
> -                if ( idx < nr_grant_frames(d->grant_table) )
> -                    mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
> -            }
> -
> -            grant_write_unlock(d->grant_table);
> +            gnttab_map_frame(d, idx, gpfn, &mfn);
>              break;
>          case XENMAPSPACE_gmfn_range:
>          case XENMAPSPACE_gmfn:
> @@ -4681,8 +4660,8 @@ int xenmem_add_to_physmap_one(
>              }
>              if ( !get_page_from_mfn(_mfn(idx), d) )
>                  break;
> -            mfn = idx;
> -            page = mfn_to_page(_mfn(mfn));
> +            mfn = _mfn(idx);
> +            page = mfn_to_page(mfn);
>              break;
>          }
>          case XENMAPSPACE_gmfn_foreign:
> @@ -4691,7 +4670,7 @@ int xenmem_add_to_physmap_one(
>              break;
>      }
> 
> -    if ( !paging_mode_translate(d) || (mfn == 0) )
> +    if ( !paging_mode_translate(d) || mfn_eq(mfn, INVALID_MFN) )
>      {
>          rc = -EINVAL;
>          goto put_both;
> @@ -4715,16 +4694,16 @@ int xenmem_add_to_physmap_one(
>          goto put_both;
> 
>      /* Unmap from old location, if any. */
> -    old_gpfn = get_gpfn_from_mfn(mfn);
> +    old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
>      ASSERT( old_gpfn != SHARED_M2P_ENTRY );
>      if ( space == XENMAPSPACE_gmfn || space ==
> XENMAPSPACE_gmfn_range )
>          ASSERT( old_gpfn == gfn );
>      if ( old_gpfn != INVALID_M2P_ENTRY )
> -        rc = guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn),
> PAGE_ORDER_4K);
> +        rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn,
> PAGE_ORDER_4K);
> 
>      /* Map at new location. */
>      if ( !rc )
> -        rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
> +        rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K);
> 
>   put_both:
>      /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fb3859ce8e..4c2e9e40a5 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3607,6 +3607,44 @@ int mem_sharing_gref_to_gfn(struct grant_table
> *gt, grant_ref_t ref,
>  }
>  #endif
> 
> +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> +                     mfn_t *mfn)
> +{
> +    int rc = 0;
> +    struct grant_table *gt = d->grant_table;
> +
> +    grant_write_lock(gt);
> +
> +    if ( gt->gt_version == 0 )
> +        gt->gt_version = 1;
> +
> +    if ( gt->gt_version == 2 &&
> +         (idx & XENMAPIDX_grant_table_status) )
> +    {
> +        idx &= ~XENMAPIDX_grant_table_status;
> +        if ( idx < nr_status_frames(gt) )
> +            *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +        else
> +            rc = -EINVAL;
> +    }
> +    else
> +    {
> +        if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
> +            gnttab_grow_table(d, idx + 1);
> +
> +        if ( idx < nr_grant_frames(gt) )
> +            *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> +        else
> +            rc = -EINVAL;
> +    }
> +
> +    gnttab_set_frame_gfn(d, idx, gfn);
> +
> +    grant_write_unlock(gt);
> +
> +    return rc;
> +}
> +
>  static void gnttab_usage_print(struct domain *rd)
>  {
>      int first = 1;
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-
> arm/grant_table.h
> index bc4d61a940..0a248a765a 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -2,6 +2,7 @@
>  #define __ASM_GRANT_TABLE_H__
> 
>  #include <xen/grant_table.h>
> +#include <xen/sched.h>
> 
>  #define INITIAL_NR_GRANT_FRAMES 4
> 
> @@ -21,6 +22,12 @@ static inline int replace_grant_supported(void)
>      return 1;
>  }
> 
> +static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long
> idx,
> +                                        gfn_t gfn)
> +{
> +    d->arch.grant_table_gfn[idx] = gfn;
> +}
> +
>  #define gnttab_create_shared_page(d, t, i)                               \
>      do {                                                                 \
>          share_xen_page_with_guest(                                       \
> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-
> x86/grant_table.h
> index 33b2f88b96..c865999a33 100644
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -75,6 +75,11 @@ static inline void gnttab_clear_flag(unsigned int nr,
> uint16_t *st)
>      asm volatile ("lock btrw %w1,%0" : "=m" (*st) : "Ir" (nr), "m" (*st));
>  }
> 
> +static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long
> idx,
> +                                        gfn_t gfn)
> +{
> +}
> +
>  /* Foreign mappings of HHVM-guest pages do not modify the type count. */
>  #define gnttab_host_mapping_get_page_type(ro, ld, rd)   \
>      (!(ro) && (((ld) == (rd)) || !paging_mode_external(rd)))
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index af269a108d..43ec6c4d80 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -136,4 +136,7 @@ static inline unsigned int grant_to_status_frames(int
> grant_frames)
>  int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
>                              gfn_t *gfn, uint16_t *status);
> 
> +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> +                     mfn_t *mfn);
> +
>  #endif /* __XEN_GRANT_TABLE_H__ */
> --
> 2.12.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel