RE: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition

From: Paul Durrant <xadimgnik@gmail.com>
To: "'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'Xen-devel'" <xen-devel@lists.xenproject.org>
Cc: "'George Dunlap'" <George.Dunlap@eu.citrix.com>,
	"'Ian Jackson'" <iwj@xenproject.org>,
	"'Jan Beulich'" <JBeulich@suse.com>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien@xen.org>,
	"'Michał Leszczyński'" <michal.leszczynski@cert.pl>,
	"'Hubert Jasudowicz'" <hubert.jasudowicz@cert.pl>,
	"'Tamas K Lengyel'" <tamas@tklengyel.com>
Subject: RE: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
Date: Thu, 24 Sep 2020 10:51:39 +0100	[thread overview]
Message-ID: <001601d69258$4d6a7c90$e83f75b0$@xen.org> (raw)
In-Reply-To: <20200922182444.12350-3-andrew.cooper3@citrix.com>

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 22 September 2020 19:25
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <iwj@xenproject.org>; Jan Beulich <JBeulich@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien Grall <julien@xen.org>; Paul Durrant
> <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz
> <hubert.jasudowicz@cert.pl>; Tamas K Lengyel <tamas@tklengyel.com>
> Subject: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
> 
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
> 
> The new gnttab_acquire_resource() function subsumes the previous two
> gnttab_get_{shared,status}_frame() helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> 
> v2:
>  * Fix CentOS 6 build by initialising vaddrs to NULL
>  * Add ASSERT_UNREACHABLE() and gt->status typecheck
> ---
>  xen/common/grant_table.c      | 102 +++++++++++++++++++++++++++++++-----------
>  xen/common/memory.c           |  42 +----------------
>  xen/include/xen/grant_table.h |  19 +++-----
>  3 files changed, 84 insertions(+), 79 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index a5d3ed8bda..912f07be47 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
> 
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    mfn_t tmp;
> +    void **vaddrs = NULL;
> +    int rc;
> +
> +    /* Input sanity. */

Nit: inconsistency with full stops on single line comments.

> +    if ( !nr_frames )
> +        return -EINVAL;
> +
> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;
> +
> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +        {
> +    default:

Personally I dislike this kind of thing. IMO it would be neater to initialise rc to -EINVAL at the top, then this could just be a 'break' (so no braces) and you could have a simple 'default: break;' case instead.

> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> +        break;
> +    }
> +

I think you could drop the write lock here...

> +    /* Any errors from growing the table? */
> +    if ( rc )
> +        goto out;
> +

...and acquire it read here, since we know the table cannot shrink. You'd need to re-check the gt_version for safety though. 

  Paul

> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        /* Check that void ** is a suitable representation for gt->status. */
> +        BUILD_BUG_ON(!__builtin_types_compatible_p(
> +                         typeof(gt->status), grant_status_t **));
> +        vaddrs = (void **)gt->status;
> +        break;
> +    }
> +
> +    if ( !vaddrs )
> +    {
> +        ASSERT_UNREACHABLE();
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
> +    for ( i = 0; i < nr_frames; ++i )
> +        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
> +
> + out:
> +    grant_write_unlock(gt);
> +
> +    return rc;
> +}
> +
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
>  {
>      int rc = 0;
> @@ -4047,33 +4122,6 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t
> *mfn)
>      return rc;
>  }
> 
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn)
> -{
> -    struct grant_table *gt = d->grant_table;
> -    int rc;
> -
> -    grant_write_lock(gt);
> -    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
> -    grant_write_unlock(gt);
> -
> -    return rc;
> -}
> -
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn)
> -{
> -    struct grant_table *gt = d->grant_table;
> -    int rc;
> -
> -    grant_write_lock(gt);
> -    rc = (gt->gt_version == 2) ?
> -        gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
> -    grant_write_unlock(gt);
> -
> -    return rc;
> -}
> -
>  static void gnttab_usage_print(struct domain *rd)
>  {
>      int first = 1;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 1bab0e80c2..177fc378d9 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,44 +1007,6 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
> 
> -static int acquire_grant_table(struct domain *d, unsigned int id,
> -                               unsigned long frame,
> -                               unsigned int nr_frames,
> -                               xen_pfn_t mfn_list[])
> -{
> -    unsigned int i = nr_frames;
> -
> -    /* Iterate backwards in case table needs to grow */
> -    while ( i-- != 0 )
> -    {
> -        mfn_t mfn = INVALID_MFN;
> -        int rc;
> -
> -        switch ( id )
> -        {
> -        case XENMEM_resource_grant_table_id_shared:
> -            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> -            break;
> -
> -        case XENMEM_resource_grant_table_id_status:
> -            rc = gnttab_get_status_frame(d, frame + i, &mfn);
> -            break;
> -
> -        default:
> -            rc = -EINVAL;
> -            break;
> -        }
> -
> -        if ( rc )
> -            return rc;
> -
> -        ASSERT(!mfn_eq(mfn, INVALID_MFN));
> -        mfn_list[i] = mfn_x(mfn);
> -    }
> -
> -    return 0;
> -}
> -
>  static int acquire_resource(
>      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>  {
> @@ -1099,8 +1061,8 @@ static int acquire_resource(
>      switch ( xmar.type )
>      {
>      case XENMEM_resource_grant_table:
> -        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
> -                                 mfn_list);
> +        rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
> +                                     mfn_list);
>          break;
> 
>      default:
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 98603604b8..5a2c75b880 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -56,10 +56,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> 
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>                       mfn_t *mfn);
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn);
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                            mfn_t *mfn);
> +
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[]);
> 
>  #else
> 
> @@ -93,14 +93,9 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
>      return -EINVAL;
>  }
> 
> -static inline int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> -                                          mfn_t *mfn)
> -{
> -    return -EINVAL;
> -}
> -
> -static inline int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> -                                          mfn_t *mfn)
> +static inline int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>  {
>      return -EINVAL;
>  }
> --
> 2.11.0