diff mbox

[Xen-devel,v3,1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

Message ID 1412773221-15150-1-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini Oct. 8, 2014, 1 p.m. UTC
Remove max_nr_grant_frames, both variable and preprocessor symbol.
Introduce two global variables max_nr_dom0_grant_frames and
max_nr_domU_grant_frames, separately configurable via the Xen command line
option "gnttab_max_nr_frames".

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 docs/misc/xen-command-line.markdown |    4 +-
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/mm.c                   |    2 +-
 xen/arch/x86/mm.c                   |    2 +-
 xen/common/compat/grant_table.c     |    8 ++--
 xen/common/grant_table.c            |   72 +++++++++++++++++++++--------------
 xen/include/asm-arm/grant_table.h   |    2 +-
 xen/include/xen/grant_table.h       |    6 +--
 8 files changed, 58 insertions(+), 40 deletions(-)

Comments

David Vrabel Oct. 8, 2014, 3:15 p.m. UTC | #1
On 08/10/14 14:00, Stefano Stabellini wrote:
> Remove max_nr_grant_frames, both variable and preprocessor symbol.
> Introduce two global variables max_nr_dom0_grant_frames and
> max_nr_domU_grant_frames, separately configurable via the Xen command line
> option "gnttab_max_nr_frames".

Why?  I can't think of a sensible use case for this and you don't list
one.  dom0 doesn't really use grants so its current number grant frames
is usually 1 so it doesn't seem useful to restrict it differently.

David
Stefano Stabellini Oct. 8, 2014, 4:01 p.m. UTC | #2
On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 14:00, Stefano Stabellini wrote:
> > Remove max_nr_grant_frames, both variable and preprocessor symbol.
> > Introduce two global variables max_nr_dom0_grant_frames and
> > max_nr_domU_grant_frames, separately configurable via the Xen command line
> > option "gnttab_max_nr_frames".
> 
> Why?  I can't think of a sensible use case for this and you don't list
> one.  dom0 doesn't really use grants so its current number grant frames
> is usually 1 so it doesn't seem useful to restrict it differently.

See the recent discussion with Jan:

http://marc.info/?l=xen-devel&m=141260825209210
David Vrabel Oct. 8, 2014, 4:42 p.m. UTC | #3
On 08/10/14 17:01, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, David Vrabel wrote:
>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>> Introduce two global variables max_nr_dom0_grant_frames and
>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>> option "gnttab_max_nr_frames".
>>
>> Why?  I can't think of a sensible use case for this and you don't list
>> one.  dom0 doesn't really use grants so its current number grant frames
>> is usually 1 so it doesn't seem useful to restrict it differently.
> 
> See the recent discussion with Jan:
> 
> http://marc.info/?l=xen-devel&m=141260825209210

I still don't see a use case.  Either:

a) dom0 is the driver domain and doesn't need a large number of grants.

or

b) dom0 has a frontend driver, then the driver domain will have to scan
dom0's grant table thus dom0's grant table must be sized the same as domUs.

David
Stefano Stabellini Oct. 8, 2014, 4:59 p.m. UTC | #4
On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 17:01, Stefano Stabellini wrote:
> > On Wed, 8 Oct 2014, David Vrabel wrote:
> >> On 08/10/14 14:00, Stefano Stabellini wrote:
> >>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
> >>> Introduce two global variables max_nr_dom0_grant_frames and
> >>> max_nr_domU_grant_frames, separately configurable via the Xen command line
> >>> option "gnttab_max_nr_frames".
> >>
> >> Why?  I can't think of a sensible use case for this and you don't list
> >> one.  dom0 doesn't really use grants so its current number grant frames
> >> is usually 1 so it doesn't seem useful to restrict it differently.
> > 
> > See the recent discussion with Jan:
> > 
> > http://marc.info/?l=xen-devel&m=141260825209210
> 
> I still don't see a use case.  Either:
> 
> a) dom0 is the driver domain and doesn't need a large number of grants.
> 
> or
> 
> b) dom0 has a frontend driver, then the driver domain will have to scan
> dom0's grant table thus dom0's grant table must be sized the same as domUs.

I don't have an opinion on this, so I'll leave it up to you and Jan to
come to an agreement.
David Vrabel Oct. 9, 2014, 9:35 a.m. UTC | #5
On 08/10/14 17:59, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, David Vrabel wrote:
>> On 08/10/14 17:01, Stefano Stabellini wrote:
>>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>>> option "gnttab_max_nr_frames".
>>>>
>>>> Why?  I can't think of a sensible use case for this and you don't list
>>>> one.  dom0 doesn't really use grants so its current number grant frames
>>>> is usually 1 so it doesn't seem useful to restrict it differently.
>>>
>>> See the recent discussion with Jan:
>>>
>>> http://marc.info/?l=xen-devel&m=141260825209210
>>
>> I still don't see a use case.  Either:
>>
>> a) dom0 is the driver domain and doesn't need a large number of grants.
>>
>> or
>>
>> b) dom0 has a frontend driver, then the driver domain will have to scan
>> dom0's grant table thus dom0's grant table must be sized the same as domUs.
> 
> I don't have an opinion on this, so I'll leave it up to you and Jan to
> come to an agreement.

I don't feel strongly on this so I'll defer to Jan.

David
Jan Beulich Oct. 10, 2014, 7:35 a.m. UTC | #6
>>> On 08.10.14 at 18:42, <david.vrabel@citrix.com> wrote:
> On 08/10/14 17:01, Stefano Stabellini wrote:
>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>> option "gnttab_max_nr_frames".
>>>
>>> Why?  I can't think of a sensible use case for this and you don't list
>>> one.  dom0 doesn't really use grants so its current number grant frames
>>> is usually 1 so it doesn't seem useful to restrict it differently.
>> 
>> See the recent discussion with Jan:
>> 
>> http://marc.info/?l=xen-devel&m=141260825209210 
> 
> I still don't see a use case.  Either:
> 
> a) dom0 is the driver domain and doesn't need a large number of grants.
> 
> or
> 
> b) dom0 has a frontend driver, then the driver domain will have to scan
> dom0's grant table thus dom0's grant table must be sized the same as domUs.

Looks like you missed that max_nr_maptrack_frames(), which I
think you agree is needed in Dom0, also depends on
max_nr_grant_frames. Question then of course is whether the
distinction should be made at that: Allow controlling grant frames
and maptrack frames separately, rather than Dom0 and DomU.

Jan
David Vrabel Oct. 10, 2014, 9:19 a.m. UTC | #7
On 10/10/14 08:35, Jan Beulich wrote:
>>>> On 08.10.14 at 18:42, <david.vrabel@citrix.com> wrote:
>> On 08/10/14 17:01, Stefano Stabellini wrote:
>>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>>> option "gnttab_max_nr_frames".
>>>>
>>>> Why?  I can't think of a sensible use case for this and you don't list
>>>> one.  dom0 doesn't really use grants so its current number grant frames
>>>> is usually 1 so it doesn't seem useful to restrict it differently.
>>>
>>> See the recent discussion with Jan:
>>>
>>> http://marc.info/?l=xen-devel&m=141260825209210 
>>
>> I still don't see a use case.  Either:
>>
>> a) dom0 is the driver domain and doesn't need a large number of grants.
>>
>> or
>>
>> b) dom0 has a frontend driver, then the driver domain will have to scan
>> dom0's grant table thus dom0's grant table must be sized the same as domUs.
> 
> Looks like you missed that max_nr_maptrack_frames(), which I
> think you agree is needed in Dom0, also depends on
> max_nr_grant_frames.

Agreed.

> Question then of course is whether the
> distinction should be made at that: Allow controlling grant frames
> and maptrack frames separately, rather than Dom0 and DomU.

This would seem sensible.

David
diff mbox

Patch

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 389701a..fa10f6c 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -609,7 +609,9 @@  does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
 Specify the serial parameters for the GDB stub.
 
 ### gnttab\_max\_nr\_frames
-> `= <integer>`
+> `= [<domU number>][,<dom0 number>]`
+
+> Default: `32,32`
 
 Specify the maximum number of frames per grant table operation.
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2b53931..8d60472 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -409,7 +409,7 @@  struct domain *alloc_domain_struct(void)
         return NULL;
 
     clear_page(d);
-    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_grant_frames);
+    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_domU_grant_frames);
     return d;
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c5b48ef..b65c725 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1040,7 +1040,7 @@  int xenmem_add_to_physmap_one(
         else
         {
             if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                    (idx < max_nr_grant_frames) )
+                    (idx < max_nr_grant_frames(d)) )
                 gnttab_grow_table(d, idx + 1);
 
             if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b3f06f..f72b23f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4539,7 +4539,7 @@  int xenmem_add_to_physmap_one(
             else
             {
                 if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                     (idx < max_nr_grant_frames) )
+                     (idx < max_nr_grant_frames(d)) )
                     gnttab_grow_table(d, idx + 1);
 
                 if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..6c00b09 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -146,11 +146,11 @@  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_nr_grant_frames )
+                if ( max_frame_list_size_in_page < max_nr_grant_frames(current->domain) )
                 {
                     gdprintk(XENLOG_WARNING,
                              "max_nr_grant_frames is too large (%u,%u)\n",
-                             max_nr_grant_frames, max_frame_list_size_in_page);
+                             max_nr_grant_frames(current->domain), max_frame_list_size_in_page);
                     rc = -EINVAL;
                 }
                 else
@@ -284,11 +284,11 @@  int compat_grant_table_op(unsigned int cmd,
                 break;
             }
             if ( max_frame_list_size_in_pages <
-                 grant_to_status_frames(max_nr_grant_frames) )
+                 grant_to_status_frames(max_nr_grant_frames(current->domain)) )
             {
                 gdprintk(XENLOG_WARNING,
                          "grant_to_status_frames(max_nr_grant_frames) is too large (%u,%u)\n",
-                         grant_to_status_frames(max_nr_grant_frames),
+                         grant_to_status_frames(max_nr_grant_frames(current->domain)),
                          max_frame_list_size_in_pages);
                 rc = -EINVAL;
                 break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..a56a1bb 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -24,6 +24,7 @@ 
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <xen/ctype.h>
 #include <xen/err.h>
 #include <xen/iocap.h>
 #include <xen/lib.h>
@@ -40,10 +41,25 @@ 
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
-#ifndef max_nr_grant_frames
-unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
-integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
-#endif
+unsigned int max_nr_dom0_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+unsigned int max_nr_domU_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+
+unsigned int max_nr_grant_frames(struct domain *d)
+{
+    if ( is_hardware_domain(d) )
+        return max_nr_dom0_grant_frames;
+    else
+        return max_nr_domU_grant_frames;
+}
+
+static void __init parse_gnttab_max_nr_frames(const char *s)
+{
+    if ( isdigit(*s) )
+        max_nr_domU_grant_frames = simple_strtoul(s, &s, 0);
+    if ( *s == ',' && isdigit(*++s) )
+        max_nr_dom0_grant_frames = simple_strtoul(s, &s, 0);
+}
+custom_param("gnttab_max_nr_frames", parse_gnttab_max_nr_frames);
 
 /* The maximum number of grant mappings is defined as a multiplier of the
  * maximum number of grant table entries. This defines the multiplier used.
@@ -102,9 +118,9 @@  nr_maptrack_frames(struct grant_table *t)
     return t->maptrack_limit / MAPTRACK_PER_PAGE;
 }
 
-static unsigned inline int max_nr_maptrack_frames(void)
+static unsigned inline int max_nr_maptrack_frames(struct domain *d)
 {
-    return (max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO);
+    return (max_nr_grant_frames(d) * MAX_MAPTRACK_TO_GRANTS_RATIO);
 }
 
 #define MAPTRACK_TAIL (~0u)
@@ -163,8 +179,8 @@  num_act_frames_from_sha_frames(const unsigned int num)
     return (num_sha_entries + (ACGNT_PER_PAGE - 1)) / ACGNT_PER_PAGE;
 }
 
-#define max_nr_active_grant_frames \
-    num_act_frames_from_sha_frames(max_nr_grant_frames)
+#define max_nr_active_grant_frames(d) \
+    num_act_frames_from_sha_frames(max_nr_grant_frames(d))
 
 static inline unsigned int
 nr_active_grant_frames(struct grant_table *gt)
@@ -259,19 +275,20 @@  put_maptrack_handle(
 
 static inline int
 get_maptrack_handle(
-    struct grant_table *lgt)
+    struct domain *ld)
 {
     int                   i;
     grant_handle_t        handle;
     struct grant_mapping *new_mt;
     unsigned int          new_mt_limit, nr_frames;
+    struct grant_table *lgt = ld->grant_table;
 
     spin_lock(&lgt->lock);
 
     while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
     {
         nr_frames = nr_maptrack_frames(lgt);
-        if ( nr_frames >= max_nr_maptrack_frames() )
+        if ( nr_frames >= max_nr_maptrack_frames(ld) )
             break;
 
         new_mt = alloc_xenheap_page();
@@ -567,7 +584,7 @@  __gnttab_map_grant_ref(
     }
 
     lgt = ld->grant_table;
-    if ( unlikely((handle = get_maptrack_handle(lgt)) == -1) )
+    if ( unlikely((handle = get_maptrack_handle(ld)) == -1) )
     {
         rcu_unlock_domain(rd);
         gdprintk(XENLOG_INFO, "Failed to obtain maptrack handle.\n");
@@ -1265,7 +1282,7 @@  gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
     struct grant_table *gt = d->grant_table;
     unsigned int i;
 
-    ASSERT(req_nr_frames <= max_nr_grant_frames);
+    ASSERT(req_nr_frames <= max_nr_grant_frames(d));
 
     gdprintk(XENLOG_INFO,
             "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1338,15 +1355,6 @@  gnttab_setup_table(
         return -EFAULT;
     }
 
-    if ( unlikely(op.nr_frames > max_nr_grant_frames) )
-    {
-        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
-                " per domain.\n",
-                max_nr_grant_frames);
-        op.status = GNTST_general_error;
-        goto out1;
-    }
-
     if ( !guest_handle_okay(op.frame_list, op.nr_frames) )
         return -EFAULT;
 
@@ -1358,6 +1366,15 @@  gnttab_setup_table(
         goto out2;
     }
 
+    if ( unlikely(op.nr_frames > max_nr_grant_frames(d)) )
+    {
+        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
+                " per domain.\n",
+                max_nr_grant_frames(d));
+        op.status = GNTST_general_error;
+        goto out2;
+    }
+
     if ( xsm_grant_setup(XSM_TARGET, current->domain, d) )
     {
         op.status = GNTST_permission_denied;
@@ -1377,7 +1394,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_nr_grant_frames);
+                 op.nr_frames, nr_grant_frames(gt), max_nr_grant_frames(d));
         op.status = GNTST_general_error;
         goto out3;
     }
@@ -1396,7 +1413,6 @@  gnttab_setup_table(
     spin_unlock(&gt->lock);
  out2:
     rcu_unlock_domain(d);
- out1:
     if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
         return -EFAULT;
 
@@ -1438,7 +1454,7 @@  gnttab_query_size(
     spin_lock(&d->grant_table->lock);
 
     op.nr_frames     = nr_grant_frames(d->grant_table);
-    op.max_nr_frames = max_nr_grant_frames;
+    op.max_nr_frames = max_nr_grant_frames(d);
     op.status        = GNTST_okay;
 
     spin_unlock(&d->grant_table->lock);
@@ -2647,7 +2663,7 @@  grant_table_create(
 
     /* Active grant table. */
     if ( (t->active = xzalloc_array(struct active_grant_entry *,
-                                    max_nr_active_grant_frames)) == NULL )
+                                    max_nr_active_grant_frames(d))) == NULL )
         goto no_mem_1;
     for ( i = 0;
           i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
@@ -2659,7 +2675,7 @@  grant_table_create(
 
     /* Tracking of mapped foreign frames table */
     if ( (t->maptrack = xzalloc_array(struct grant_mapping *,
-                                      max_nr_maptrack_frames())) == NULL )
+                                      max_nr_maptrack_frames(d))) == NULL )
         goto no_mem_2;
     if ( (t->maptrack[0] = alloc_xenheap_page()) == NULL )
         goto no_mem_3;
@@ -2670,7 +2686,7 @@  grant_table_create(
     t->maptrack[0][i - 1].ref = MAPTRACK_TAIL;
 
     /* Shared grant table. */
-    if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames)) == NULL )
+    if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames(d))) == NULL )
         goto no_mem_3;
     for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
     {
@@ -2681,7 +2697,7 @@  grant_table_create(
     
     /* Status pages for grant table - for version 2 */
     t->status = xzalloc_array(grant_status_t *,
-                              grant_to_status_frames(max_nr_grant_frames));
+                              grant_to_status_frames(max_nr_grant_frames(d)));
     if ( t->status == NULL )
         goto no_mem_4;
 
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 47147ce..e81c973 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -31,7 +31,7 @@  static inline int replace_grant_supported(void)
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
-     (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
+     (i < max_nr_grant_frames(d))) ? 0 : (d->arch.grant_table_gpfn[i]))
 
 #define gnttab_need_iommu_mapping(d)           (is_domain_direct_mapped(d))
 
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5941191..7269d90 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -49,10 +49,10 @@ 
 /* Default maximum size of a grant table. [POLICY] */
 #define DEFAULT_MAX_NR_GRANT_FRAMES   32
 #endif
-#ifndef max_nr_grant_frames /* to allow arch to override */
 /* The maximum size of a grant table. */
-extern unsigned int max_nr_grant_frames;
-#endif
+extern unsigned int max_nr_dom0_grant_frames;
+extern unsigned int max_nr_domU_grant_frames;
+unsigned int max_nr_grant_frames(struct domain *d);
 
 /*
  * Tracks a mapping of another domain's grant reference. Each domain has a