Re: [Xen-devel] [PATCH v7 14/16] xen: make grant resource limits per domain

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> Juergen Gross
> Sent: 19 September 2017 10:59
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>; sstabellini@xxxxxxxxxx; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>;
> julien.grall@xxxxxxx; jbeulich@xxxxxxxx; dgdegra@xxxxxxxxxxxxx
> Subject: [Xen-devel] [PATCH v7 14/16] xen: make grant resource limits per
> domain
> 
> Instead of using the same global resource limits of grant tables (max.
> number of grant frames, max. number of maptrack frames) for all domains
> make these limits per domain. Set those per-domain limits in
> grant_table_set_limits().
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

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

> ---
> V6:
> - several changes due to new patch order
> 
> V3:
> - correct error message (Paul Durrant)
> ---
>  xen/common/compat/grant_table.c   |  31 +++-------
>  xen/common/grant_table.c          | 121 ++++++++++++++++++++++++--------
> ------
>  xen/include/asm-arm/grant_table.h |   6 +-
>  3 files changed, 88 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/common/compat/grant_table.c
> b/xen/common/compat/grant_table.c
> index cce3ff0b9a..ff1d678f01 100644
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
>                  unsigned int max_frame_list_size_in_page =
>                      (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
>                      sizeof(*nat.setup->frame_list.p);
> -                if ( max_frame_list_size_in_page < max_grant_frames )
> -                {
> -                    gdprintk(XENLOG_WARNING,
> -                             "max_grant_frames is too large (%u,%u)\n",
> -                             max_grant_frames, max_frame_list_size_in_page);
> -                    rc = -EINVAL;
> -                }
> -                else
> -                {
> +
>  #define XLAT_gnttab_setup_table_HNDL_frame_list(_d_, _s_) \
> -                    set_xen_guest_handle((_d_)->frame_list, (unsigned long
> *)(nat.setup + 1))
> -                    XLAT_gnttab_setup_table(nat.setup, &cmp.setup);
> +                set_xen_guest_handle((_d_)->frame_list, (unsigned long
> *)(nat.setup + 1))
> +                XLAT_gnttab_setup_table(nat.setup, &cmp.setup);
>  #undef XLAT_gnttab_setup_table_HNDL_frame_list
> -                    rc = gnttab_setup_table(guest_handle_cast(nat.uop,
> gnttab_setup_table_t), 1);
> -                }
> +                rc = gnttab_setup_table(guest_handle_cast(nat.uop,
> +                                                          
> gnttab_setup_table_t),
> +                                        1, max_frame_list_size_in_page);
>              }
>              ASSERT(rc <= 0);
>              if ( rc == 0 )
> @@ -294,16 +287,6 @@ int compat_grant_table_op(unsigned int cmd,
>                  rc = -EFAULT;
>                  break;
>              }
> -            if ( max_frame_list_size_in_pages <
> -                 grant_to_status_frames(max_grant_frames) )
> -            {
> -                gdprintk(XENLOG_WARNING,
> -                         "grant_to_status_frames(max_grant_frames) is too 
> large
> (%u,%u)\n",
> -                         grant_to_status_frames(max_grant_frames),
> -                         max_frame_list_size_in_pages);
> -                rc = -EINVAL;
> -                break;
> -            }
> 
>  #define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
>              set_xen_guest_handle((_d_)->frame_list, (uint64_t 
> *)(nat.get_status
> + 1))
> @@ -312,7 +295,7 @@ int compat_grant_table_op(unsigned int cmd,
> 
>              rc = gnttab_get_status_frames(
>                  guest_handle_cast(nat.uop, gnttab_get_status_frames_t),
> -                count);
> +                count, max_frame_list_size_in_pages);
>              if ( rc >= 0 )
>              {
>  #define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 26f9a32656..a0d8f32869 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -54,6 +54,9 @@ struct grant_table {
>       * what version to use yet.
>       */
>      unsigned int          gt_version;
> +    /* Resource limits of the domain. */
> +    unsigned int          max_grant_frames;
> +    unsigned int          max_maptrack_frames;
>      /* Table size. Number of frames shared with guest */
>      unsigned int          nr_grant_frames;
>      /* Number of grant status frames shared with guest (for version 2) */
> @@ -290,8 +293,8 @@ num_act_frames_from_sha_frames(const unsigned
> int num)
>      return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
>  }
> 
> -#define max_nr_active_grant_frames \
> -    num_act_frames_from_sha_frames(max_grant_frames)
> +#define max_nr_active_grant_frames(gt) \
> +    num_act_frames_from_sha_frames(gt->max_grant_frames)
> 
>  static inline unsigned int
>  nr_active_grant_frames(struct grant_table *gt)
> @@ -528,7 +531,7 @@ get_maptrack_handle(
>       * out of memory, try stealing an entry from another VCPU (in case the
>       * guest isn't mapping across its VCPUs evenly).
>       */
> -    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
> +    if ( nr_maptrack_frames(lgt) < lgt->max_maptrack_frames )
>          new_mt = alloc_xenheap_page();
> 
>      if ( !new_mt )
> @@ -1667,23 +1670,26 @@ grant_table_init(struct grant_table *gt)
> 
>      /* Active grant table. */
>      gt->active = xzalloc_array(struct active_grant_entry *,
> -                               max_nr_active_grant_frames);
> +                               max_nr_active_grant_frames(gt));
>      if ( gt->active == NULL )
>          goto out;
> 
>      /* Tracking of mapped foreign frames table */
> -    gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
> -    if ( gt->maptrack == NULL )
> -        goto out;
> +    if ( gt->max_maptrack_frames )
> +    {
> +        gt->maptrack = vzalloc(gt->max_maptrack_frames * sizeof(*gt-
> >maptrack));
> +        if ( gt->maptrack == NULL )
> +            goto out;
> +    }
> 
>      /* Shared grant table. */
> -    gt->shared_raw = xzalloc_array(void *, max_grant_frames);
> +    gt->shared_raw = xzalloc_array(void *, gt->max_grant_frames);
>      if ( gt->shared_raw == NULL )
>          goto out;
> 
>      /* Status pages for grant table - for version 2 */
>      gt->status = xzalloc_array(grant_status_t *,
> -                               grant_to_status_frames(max_grant_frames));
> +                               grant_to_status_frames(gt->max_grant_frames));
>      if ( gt->status == NULL )
>          goto out;
> 
> @@ -1718,8 +1724,9 @@ gnttab_grow_table(struct domain *d, unsigned int
> req_nr_frames)
>      ASSERT(gt->active);
> 
>      if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
> -        req_nr_frames = INITIAL_NR_GRANT_FRAMES;
> -    ASSERT(req_nr_frames <= max_grant_frames);
> +        req_nr_frames = min_t(unsigned int, INITIAL_NR_GRANT_FRAMES,
> +                                            gt->max_grant_frames);
> +    ASSERT(req_nr_frames <= gt->max_grant_frames);
> 
>      gdprintk(XENLOG_INFO,
>              "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
> @@ -1777,13 +1784,15 @@ active_alloc_failed:
> 
>  static long
>  gnttab_setup_table(
> -    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int
> count)
> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int
> count,
> +    unsigned int limit_max)
>  {
>      struct vcpu *curr = current;
>      struct gnttab_setup_table op;
>      struct domain *d = NULL;
>      struct grant_table *gt;
>      unsigned int i;
> +    long ret = 0;
> 
>      if ( count != 1 )
>          return -EINVAL;
> @@ -1791,15 +1800,6 @@ gnttab_setup_table(
>      if ( unlikely(copy_from_guest(&op, uop, 1)) )
>          return -EFAULT;
> 
> -    if ( unlikely(op.nr_frames > max_grant_frames) )
> -    {
> -        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table
> frames"
> -                " per domain.\n",
> -                max_grant_frames);
> -        op.status = GNTST_general_error;
> -        goto out;
> -    }
> -
>      if ( !guest_handle_okay(op.frame_list, op.nr_frames) )
>          return -EFAULT;
> 
> @@ -1819,6 +1819,21 @@ gnttab_setup_table(
>      gt = d->grant_table;
>      grant_write_lock(gt);
> 
> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
> +    {
> +        gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table
> frames.\n",
> +                gt->max_grant_frames);
> +        op.status = GNTST_general_error;
> +        goto unlock;
> +    }
> +    if ( unlikely(limit_max < gt->max_grant_frames) )
> +    {
> +        gdprintk(XENLOG_WARNING, "max_grant_frames is too large
> (%u,%u)\n",
> +                 gt->max_grant_frames, limit_max);
> +        ret = -EINVAL;
> +        goto unlock;
> +    }
> +
>      if ( gt->gt_version == 0 )
>          gt->gt_version = 1;
> 
> @@ -1829,7 +1844,7 @@ gnttab_setup_table(
>      {
>          gdprintk(XENLOG_INFO,
>                   "Expand grant table to %u failed. Current: %u Max: %u\n",
> -                 op.nr_frames, nr_grant_frames(gt), max_grant_frames);
> +                 op.nr_frames, nr_grant_frames(gt), gt->max_grant_frames);
>          op.status = GNTST_general_error;
>          goto unlock;
>      }
> @@ -1852,10 +1867,10 @@ gnttab_setup_table(
>      if ( d )
>          rcu_unlock_domain(d);
> 
> -    if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
> +    if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
>          return -EFAULT;
> 
> -    return 0;
> +    return ret;
>  }
> 
>  static long
> @@ -1864,6 +1879,7 @@ gnttab_query_size(
>  {
>      struct gnttab_query_size op;
>      struct domain *d;
> +    struct grant_table *gt;
> 
>      if ( count != 1 )
>          return -EINVAL;
> @@ -1884,13 +1900,15 @@ gnttab_query_size(
>          goto out;
>      }
> 
> -    grant_read_lock(d->grant_table);
> +    gt = d->grant_table;
> 
> -    op.nr_frames     = nr_grant_frames(d->grant_table);
> -    op.max_nr_frames = max_grant_frames;
> +    grant_read_lock(gt);
> +
> +    op.nr_frames     = nr_grant_frames(gt);
> +    op.max_nr_frames = gt->max_grant_frames;
>      op.status        = GNTST_okay;
> 
> -    grant_read_unlock(d->grant_table);
> +    grant_read_unlock(gt);
> 
>   out:
>      if ( d )
> @@ -2965,14 +2983,14 @@
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t)
> uop)
> 
>  static long
> 
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> s_frames_t) uop,
> -                         int count)
> +                         int count, unsigned int limit_max)
>  {
>      gnttab_get_status_frames_t op;
>      struct domain *d;
>      struct grant_table *gt;
>      uint64_t       gmfn;
>      int i;
> -    int rc;
> +    int rc, ret = 0;
> 
>      if ( count != 1 )
>          return -EINVAL;
> @@ -3012,6 +3030,15 @@
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> s_frames_t) uop,
>          goto unlock;
>      }
> 
> +    if ( unlikely(limit_max < grant_to_status_frames(gt-
> >max_grant_frames)) )
> +    {
> +        gdprintk(XENLOG_WARNING,
> +                 "grant_to_status_frames(max_grant_frames) is too large
> (%u,%u)\n",
> +                 grant_to_status_frames(gt->max_grant_frames), limit_max);
> +        ret = -EINVAL;
> +        goto unlock;
> +    }
> +
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
>          gmfn = gnttab_status_gmfn(d, gt, i);
> @@ -3024,10 +3051,10 @@
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
> s_frames_t) uop,
>   out2:
>      rcu_unlock_domain(d);
>   out1:
> -    if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
> +    if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
>          return -EFAULT;
> 
> -    return 0;
> +    return ret;
>  }
> 
>  static long
> @@ -3320,7 +3347,7 @@ do_grant_table_op(
> 
>      case GNTTABOP_setup_table:
>          rc = gnttab_setup_table(
> -            guest_handle_cast(uop, gnttab_setup_table_t), count);
> +            guest_handle_cast(uop, gnttab_setup_table_t), count, ~0);
>          ASSERT(rc <= 0);
>          break;
> 
> @@ -3369,7 +3396,7 @@ do_grant_table_op(
> 
>      case GNTTABOP_get_status_frames:
>          rc = gnttab_get_status_frames(
> -            guest_handle_cast(uop, gnttab_get_status_frames_t), count);
> +            guest_handle_cast(uop, gnttab_get_status_frames_t), count, ~0);
>          break;
> 
>      case GNTTABOP_get_version:
> @@ -3442,6 +3469,8 @@ grant_table_create(
>      /* Simple stuff. */
>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>      spin_lock_init(&t->maptrack_lock);
> +    t->max_grant_frames = max_grant_frames;
> +    t->max_maptrack_frames = max_maptrack_frames;
> 
>      /* Okay, install the structure. */
>      d->grant_table = t;
> @@ -3648,6 +3677,8 @@ int grant_table_set_limits(struct domain *d,
> unsigned int grant_frames,
>      struct grant_table *gt = d->grant_table;
>      int ret = -EBUSY;
> 
> +    if ( !grant_frames )
> +        return -EINVAL;
>      if ( !gt )
>          return -ENOENT;
> 
> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d,
> unsigned int grant_frames,
> 
>      /* Set limits. */
>      if ( !gt->active )
> +    {
> +        gt->max_grant_frames = grant_frames;
> +        gt->max_maptrack_frames = maptrack_frames;
>          ret = grant_table_init(gt);
> +    }
> 
>      grant_write_unlock(gt);
> 
> @@ -3731,7 +3766,7 @@ int gnttab_map_frame(struct domain *d, unsigned
> long idx, gfn_t gfn,
>      }
>      else
>      {
> -        if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
> +        if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
>              gnttab_grow_table(d, idx + 1);
> 
>          if ( idx < nr_grant_frames(gt) )
> @@ -3759,6 +3794,12 @@ static void gnttab_usage_print(struct domain *rd)
> 
>      grant_read_lock(gt);
> 
> +    printk("grant-table for remote domain:%5d (v%d)\n"
> +           "  %d frames (%d max), %d maptrack frames (%d max)\n",
> +           rd->domain_id, gt->gt_version,
> +           nr_grant_frames(gt), gt->max_grant_frames,
> +           nr_maptrack_frames(gt), gt->max_maptrack_frames);
> +
>      for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
>      {
>          struct active_grant_entry *act;
> @@ -3786,12 +3827,7 @@ static void gnttab_usage_print(struct domain *rd)
>              status = status_entry(gt, ref);
>          }
> 
> -        if ( first )
> -        {
> -            printk("grant-table for remote domain:%5d (v%d)\n",
> -                   rd->domain_id, gt->gt_version);
> -            first = 0;
> -        }
> +        first = 0;
> 
>          /*      [0xXXX]  ddddd 0xXXXXXX 0xXXXXXXXX      ddddd 0xXXXXXX 0xXX
> */
>          printk("[0x%03x]  %5d 0x%06lx 0x%08x      %5d 0x%06"PRIx64"
> 0x%02x\n",
> @@ -3803,8 +3839,7 @@ static void gnttab_usage_print(struct domain *rd)
>      grant_read_unlock(gt);
> 
>      if ( first )
> -        printk("grant-table for remote domain:%5d ... "
> -               "no active grant table entries\n", rd->domain_id);
> +        printk("no active grant table entries\n");
>  }
> 
>  static void gnttab_usage_print_all(unsigned char key)
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-
> arm/grant_table.h
> index 0870b5b782..9a60e4e614 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
>      return 1;
>  }
> 
> -#define gnttab_init_arch(gt)                                             \
> -    ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0     \
> +#define gnttab_init_arch(gt)                                               \
> +    ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
>        ? 0 : -ENOMEM )
> 
>  #define gnttab_destroy_arch(gt)                                          \
> @@ -49,7 +49,7 @@ static inline int replace_grant_supported(void)
> 
>  #define gnttab_shared_gmfn(d, t, i)                                      \
>      ( ((i >= nr_grant_frames(t)) &&                                      \
> -       (i < max_grant_frames)) ? 0 : gfn_x(t->arch.gfn[i]))
> +       (i < (t)->max_grant_frames))? 0 : gfn_x((t)->arch.gfn[i]))
> 
>  #define gnttab_need_iommu_mapping(d)                    \
>      (is_domain_direct_mapped(d) && need_iommu(d))
> --
> 2.12.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel