[PATCH 0/5] xen: better grant v2 support

[PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

Currently Linux has no support for grant v2 as this would reduce the
maximum number of active grants by a factor of 2 compared to v1,
because the number of possible grants are limited by the allowed number
of grant frames and grant entries of v2 need twice as much bytes as
those of v1.

Unfortunately grant v2 is the only way to support either guests with
more than 16TB memory size or PV guests with memory above the 16TB
border, as grant v1 limits the frame number to be 32 bits wide.

In order to remove the disadvantage of grant v2 this patch series
enables configuring different maximum grant frame numbers for v1 and
v2. In order to let the guest know about the actual numbers an
interface for probing the v1 and v2 limits is added.

Juergen Gross (5):
  xen: remove deprecated boot parameter gnttab_max_nr_frames
  xen: move XENMAPSPACE_grant_table code into grant_table.c
  xen: clean up grant_table.h
  xen: support different gnttab_max_frames for grant v1 and v2
  xen: add new hypercall to get grant table limits

 xen/arch/arm/domain.c             |   2 +-
 xen/arch/arm/domain_build.c       |   2 +-
 xen/arch/arm/mm.c                 |  34 +----
 xen/arch/x86/mm.c                 |  41 ++----
 xen/common/compat/grant_table.c   |   9 +-
 xen/common/grant_table.c          | 267 ++++++++++++++++++++++++++++++--------
 xen/include/asm-arm/grant_table.h |   9 +-
 xen/include/asm-x86/grant_table.h |   5 +
 xen/include/public/grant_table.h  |  14 ++
 xen/include/xen/grant_table.h     |  92 +------------
 xen/include/xen/sched.h           |   1 +
 11 files changed, 269 insertions(+), 207 deletions(-)

-- 
2.12.3


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

[PATCH 1/5] xen: remove deprecated boot parameter gnttab_max_nr_frames

[Juergen Gross]

The boot parameter gnttab_max_nr_frames has been deprecated in Xen 4.5.
Remove it now.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/grant_table.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 36895aa549..fb3859ce8e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -40,13 +40,6 @@
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
-/*
- * This option is deprecated, use gnttab_max_frames and
- * gnttab_max_maptrack_frames instead.
- */
-static unsigned int __initdata max_nr_grant_frames;
-integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
-
 unsigned int __read_mostly max_grant_frames;
 integer_param("gnttab_max_frames", max_grant_frames);
 
@@ -3684,17 +3677,7 @@ static void gnttab_usage_print_all(unsigned char key)
 
 static int __init gnttab_usage_init(void)
 {
-    if ( max_nr_grant_frames )
-    {
-        printk(XENLOG_WARNING
-               "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
-        if ( !max_grant_frames )
-            max_grant_frames = max_nr_grant_frames;
-        BUILD_BUG_ON(DEFAULT_MAX_MAPTRACK_FRAMES < DEFAULT_MAX_NR_GRANT_FRAMES);
-        if ( !max_maptrack_frames )
-            max_maptrack_frames = max_nr_grant_frames *
-                (DEFAULT_MAX_MAPTRACK_FRAMES / DEFAULT_MAX_NR_GRANT_FRAMES);
-    }
+    BUILD_BUG_ON(DEFAULT_MAX_MAPTRACK_FRAMES < DEFAULT_MAX_NR_GRANT_FRAMES);
 
     if ( !max_grant_frames )
         max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
-- 
2.12.3


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

Re: [PATCH 1/5] xen: remove deprecated boot parameter gnttab_max_nr_frames

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> The boot parameter gnttab_max_nr_frames has been deprecated in Xen 4.5.
> Remove it now.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

[PATCH 2/5] xen: move XENMAPSPACE_grant_table code into grant_table.c

[Juergen Gross]

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.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/mm.c                 | 34 ++++----------------------------
 xen/arch/x86/mm.c                 | 41 ++++++++++-----------------------------
 xen/common/grant_table.c          | 38 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/grant_table.h |  6 ++++++
 xen/include/asm-x86/grant_table.h |  5 +++++
 xen/include/xen/grant_table.h     |  3 +++
 6 files changed, 66 insertions(+), 61 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a810a056d7..6dad167a8e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -24,6 +24,7 @@
 #include <xen/mm.h>
 #include <xen/preempt.h>
 #include <xen/errno.h>
+#include <xen/sched.h>
 #include <xen/grant_table.h>
 #include <xen/softirq.h>
 #include <xen/event.h>
@@ -34,7 +35,6 @@
 #include <asm/current.h>
 #include <asm/flushtlb.h>
 #include <public/memory.h>
-#include <xen/sched.h>
 #include <xen/vmap.h>
 #include <xsm/xsm.h>
 #include <xen/pfn.h>
@@ -1229,37 +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
-                return -EINVAL;
-        }
-        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]);
-            else
-                return -EINVAL;
-        }
-
-        d->arch.grant_table_gfn[idx] = gfn;
+        rc = gnttab_map_frame(d, idx, gfn, &mfn);
+        if ( rc )
+            return rc;
 
         t = p2m_ram_rw;
-
-        grant_write_unlock(d->grant_table);
         break;
     case XENMAPSPACE_shared_info:
         if ( idx != 0 )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ed77270586..e339987c01 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4601,40 +4601,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:
@@ -4651,8 +4630,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:
@@ -4661,7 +4640,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;
@@ -4685,16 +4664,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..9e18dc0493 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;
+
+    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 = _mfn(virt_to_mfn(d->grant_table->status[idx]));
+        else
+            rc = -EINVAL;
+    }
+    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 = _mfn(virt_to_mfn(d->grant_table->shared_raw[idx]));
+        else
+            rc = -EINVAL;
+    }
+
+    gnttab_set_frame_gfn(d, idx, gfn);
+
+    grant_write_unlock(d->grant_table);
+
+    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..581c4bf928 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -21,6 +21,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

Re: [PATCH 2/5] xen: move XENMAPSPACE_grant_table code into grant_table.c

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> 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.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

This will need re-basing over the XSA-235 fix which went in yesterday.
Perhaps worth having a local variable for d->grant_table in
gnttab_map_frame(), but apart from that the change looks fine.

Jan


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

Re: [PATCH 2/5] xen: move XENMAPSPACE_grant_table code into grant_table.c

[Julien Grall]

Hello,

I was expecting to be CCed on this patch.

On 21/08/17 19:05, Juergen Gross wrote:
> 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.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/arch/arm/mm.c                 | 34 ++++----------------------------
>  xen/arch/x86/mm.c                 | 41 ++++++++++-----------------------------
>  xen/common/grant_table.c          | 38 ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/grant_table.h |  6 ++++++
>  xen/include/asm-x86/grant_table.h |  5 +++++
>  xen/include/xen/grant_table.h     |  3 +++
>  6 files changed, 66 insertions(+), 61 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a810a056d7..6dad167a8e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -24,6 +24,7 @@
>  #include <xen/mm.h>
>  #include <xen/preempt.h>
>  #include <xen/errno.h>
> +#include <xen/sched.h>

Why sched.h has been moved earlier? Likely it means one of the header 
doesn't include all its dependency.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/5] xen: move XENMAPSPACE_grant_table code into grant_table.c

[Juergen Gross]

On 24/08/17 18:12, Julien Grall wrote:
> Hello,
> 
> I was expecting to be CCed on this patch.

Oops, sorry for that.

> 
> On 21/08/17 19:05, Juergen Gross wrote:
>> 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.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/arch/arm/mm.c                 | 34 ++++----------------------------
>>  xen/arch/x86/mm.c                 | 41
>> ++++++++++-----------------------------
>>  xen/common/grant_table.c          | 38
>> ++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/grant_table.h |  6 ++++++
>>  xen/include/asm-x86/grant_table.h |  5 +++++
>>  xen/include/xen/grant_table.h     |  3 +++
>>  6 files changed, 66 insertions(+), 61 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index a810a056d7..6dad167a8e 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -24,6 +24,7 @@
>>  #include <xen/mm.h>
>>  #include <xen/preempt.h>
>>  #include <xen/errno.h>
>> +#include <xen/sched.h>
> 
> Why sched.h has been moved earlier? Likely it means one of the header
> doesn't include all its dependency.

I'll have a look.


Juergen


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

[PATCH 3/5] xen: clean up grant_table.h

[Juergen Gross]

Many definitions can be moved from xen/grant_table.h to
common/grant_table.c now, as they are no longer used in other sources.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/grant_table.c      | 81 +++++++++++++++++++++++++++++++++++++++-
 xen/include/xen/grant_table.h | 86 +------------------------------------------
 xen/include/xen/sched.h       |  1 +
 3 files changed, 81 insertions(+), 87 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 9e18dc0493..01d09945da 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -40,6 +40,42 @@
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
+/* Per-domain grant information. */
+struct grant_table {
+    /*
+     * Lock protecting updates to grant table state (version, active
+     * entry list, etc.)
+     */
+    percpu_rwlock_t       lock;
+    /* Table size. Number of frames shared with guest */
+    unsigned int          nr_grant_frames;
+    /* Shared grant table (see include/public/grant_table.h). */
+    union {
+        void **shared_raw;
+        struct grant_entry_v1 **shared_v1;
+        union grant_entry_v2 **shared_v2;
+    };
+    /* Number of grant status frames shared with guest (for version 2) */
+    unsigned int          nr_status_frames;
+    /* State grant table (see include/public/grant_table.h). */
+    grant_status_t       **status;
+    /* Active grant table. */
+    struct active_grant_entry **active;
+    /* Mapping tracking table per vcpu. */
+    struct grant_mapping **maptrack;
+    unsigned int          maptrack_limit;
+    /* Lock protecting the maptrack limit */
+    spinlock_t            maptrack_lock;
+    /* The defined versions are 1 and 2.  Set to 0 if we don't know
+       what version to use yet. */
+    unsigned              gt_version;
+};
+
+#ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
+/* Default maximum size of a grant table. [POLICY] */
+#define DEFAULT_MAX_NR_GRANT_FRAMES   32
+#endif
+
 unsigned int __read_mostly max_grant_frames;
 integer_param("gnttab_max_frames", max_grant_frames);
 
@@ -118,6 +154,18 @@ struct grant_mapping {
     uint32_t pad;           /* round size to a power of 2 */
 };
 
+/* Number of grant table frames. Caller must hold d's grant table lock. */
+static inline unsigned int nr_grant_frames(struct grant_table *gt)
+{
+    return gt->nr_grant_frames;
+}
+
+/* Number of status grant table frames. Caller must hold d's gr. table lock.*/
+static inline unsigned int nr_status_frames(struct grant_table *gt)
+{
+    return gt->nr_status_frames;
+}
+
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
 #define maptrack_entry(t, e) \
     ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
@@ -197,7 +245,27 @@ static inline void act_set_gfn(struct active_grant_entry *act, gfn_t gfn)
 #endif
 }
 
-DEFINE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
+static DEFINE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
+
+static inline void grant_read_lock(struct grant_table *gt)
+{
+    percpu_read_lock(grant_rwlock, &gt->lock);
+}
+
+static inline void grant_read_unlock(struct grant_table *gt)
+{
+    percpu_read_unlock(grant_rwlock, &gt->lock);
+}
+
+static inline void grant_write_lock(struct grant_table *gt)
+{
+    percpu_write_lock(grant_rwlock, &gt->lock);
+}
+
+static inline void grant_write_unlock(struct grant_table *gt)
+{
+    percpu_write_unlock(grant_rwlock, &gt->lock);
+}
 
 static inline void gnttab_flush_tlb(const struct domain *d)
 {
@@ -250,6 +318,15 @@ static inline void active_entry_release(struct active_grant_entry *act)
     spin_unlock(&act->lock);
 }
 
+#define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
+#define GRANT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_v2_t))
+/* Number of grant table status entries. Caller must hold d's gr. table lock.*/
+static inline unsigned int grant_to_status_frames(int grant_frames)
+{
+    return (grant_frames * GRANT_PER_PAGE + GRANT_STATUS_PER_PAGE - 1) /
+        GRANT_STATUS_PER_PAGE;
+}
+
 /* Check if the page has been paged out, or needs unsharing.
    If rc == GNTST_okay, *page contains the page struct with a ref taken.
    Caller must do put_page(*page).
@@ -1580,7 +1657,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
  * Grow the grant table. The caller must hold the grant table's
  * write lock before calling this function.
  */
-int
+static int
 gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
 {
     struct grant_table *gt = d->grant_table;
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 43ec6c4d80..06faf61a6b 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -29,65 +29,8 @@
 #include <asm/page.h>
 #include <asm/grant_table.h>
 
-#ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
-/* Default maximum size of a grant table. [POLICY] */
-#define DEFAULT_MAX_NR_GRANT_FRAMES   32
-#endif
 /* The maximum size of a grant table. */
-extern unsigned int max_grant_frames;
-
-DECLARE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
-
-/* Per-domain grant information. */
-struct grant_table {
-    /*
-     * Lock protecting updates to grant table state (version, active
-     * entry list, etc.)
-     */
-    percpu_rwlock_t       lock;
-    /* Table size. Number of frames shared with guest */
-    unsigned int          nr_grant_frames;
-    /* Shared grant table (see include/public/grant_table.h). */
-    union {
-        void **shared_raw;
-        struct grant_entry_v1 **shared_v1;
-        union grant_entry_v2 **shared_v2;
-    };
-    /* Number of grant status frames shared with guest (for version 2) */
-    unsigned int          nr_status_frames;
-    /* State grant table (see include/public/grant_table.h). */
-    grant_status_t       **status;
-    /* Active grant table. */
-    struct active_grant_entry **active;
-    /* Mapping tracking table per vcpu. */
-    struct grant_mapping **maptrack;
-    unsigned int          maptrack_limit;
-    /* Lock protecting the maptrack limit */
-    spinlock_t            maptrack_lock;
-    /* The defined versions are 1 and 2.  Set to 0 if we don't know
-       what version to use yet. */
-    unsigned              gt_version;
-};
-
-static inline void grant_read_lock(struct grant_table *gt)
-{
-    percpu_read_lock(grant_rwlock, &gt->lock);
-}
-
-static inline void grant_read_unlock(struct grant_table *gt)
-{
-    percpu_read_unlock(grant_rwlock, &gt->lock);
-}
-
-static inline void grant_write_lock(struct grant_table *gt)
-{
-    percpu_write_lock(grant_rwlock, &gt->lock);
-}
-
-static inline void grant_write_unlock(struct grant_table *gt)
-{
-    percpu_write_unlock(grant_rwlock, &gt->lock);
-}
+extern unsigned int __read_mostly max_grant_frames;
 
 /* Create/destroy per-domain grant table context. */
 int grant_table_create(
@@ -106,33 +49,6 @@ void
 gnttab_release_mappings(
     struct domain *d);
 
-/* Increase the size of a domain's grant table.
- * Caller must hold d's grant table write lock.
- */
-int
-gnttab_grow_table(struct domain *d, unsigned int req_nr_frames);
-
-/* Number of grant table frames. Caller must hold d's grant table lock. */
-static inline unsigned int nr_grant_frames(struct grant_table *gt)
-{
-    return gt->nr_grant_frames;
-}
-
-/* Number of status grant table frames. Caller must hold d's gr. table lock.*/
-static inline unsigned int nr_status_frames(struct grant_table *gt)
-{
-    return gt->nr_status_frames;
-}
-
-#define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
-#define GRANT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_v2_t))
-/* Number of grant table status entries. Caller must hold d's gr. table lock.*/
-static inline unsigned int grant_to_status_frames(int grant_frames)
-{
-    return (grant_frames * GRANT_PER_PAGE + GRANT_STATUS_PER_PAGE - 1) /
-        GRANT_STATUS_PER_PAGE;
-}
-
 int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
                             gfn_t *gfn, uint16_t *status);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 5828a0123c..2dc9728993 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -307,6 +307,7 @@ struct vm_event_per_domain
 };
 
 struct evtchn_port_ops;
+struct grant_table;
 
 enum guest_type {
     guest_type_pv, guest_type_hvm
-- 
2.12.3


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

Re: [PATCH 3/5] xen: clean up grant_table.h

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> @@ -118,6 +154,18 @@ struct grant_mapping {
>      uint32_t pad;           /* round size to a power of 2 */
>  };
>  
> +/* Number of grant table frames. Caller must hold d's grant table lock. */
> +static inline unsigned int nr_grant_frames(struct grant_table *gt)
> +{
> +    return gt->nr_grant_frames;
> +}
> +
> +/* Number of status grant table frames. Caller must hold d's gr. table lock.*/
> +static inline unsigned int nr_status_frames(struct grant_table *gt)
> +{
> +    return gt->nr_status_frames;
> +}

For both the parameters want to be constified.

> @@ -250,6 +318,15 @@ static inline void active_entry_release(struct active_grant_entry *act)
>      spin_unlock(&act->lock);
>  }
>  
> +#define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
> +#define GRANT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_v2_t))
> +/* Number of grant table status entries. Caller must hold d's gr. table lock.*/
> +static inline unsigned int grant_to_status_frames(int grant_frames)

unsigned int also for the parameter.

> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -29,65 +29,8 @@
>  #include <asm/page.h>
>  #include <asm/grant_table.h>
>  
> -#ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
> -/* Default maximum size of a grant table. [POLICY] */
> -#define DEFAULT_MAX_NR_GRANT_FRAMES   32
> -#endif
>  /* The maximum size of a grant table. */
> -extern unsigned int max_grant_frames;
> -
> -DECLARE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
> -
> -/* Per-domain grant information. */
> -struct grant_table {
> -    /*
> -     * Lock protecting updates to grant table state (version, active
> -     * entry list, etc.)
> -     */
> -    percpu_rwlock_t       lock;
> -    /* Table size. Number of frames shared with guest */
> -    unsigned int          nr_grant_frames;
> -    /* Shared grant table (see include/public/grant_table.h). */
> -    union {
> -        void **shared_raw;
> -        struct grant_entry_v1 **shared_v1;
> -        union grant_entry_v2 **shared_v2;
> -    };
> -    /* Number of grant status frames shared with guest (for version 2) */
> -    unsigned int          nr_status_frames;
> -    /* State grant table (see include/public/grant_table.h). */
> -    grant_status_t       **status;
> -    /* Active grant table. */
> -    struct active_grant_entry **active;
> -    /* Mapping tracking table per vcpu. */
> -    struct grant_mapping **maptrack;
> -    unsigned int          maptrack_limit;
> -    /* Lock protecting the maptrack limit */
> -    spinlock_t            maptrack_lock;
> -    /* The defined versions are 1 and 2.  Set to 0 if we don't know
> -       what version to use yet. */
> -    unsigned              gt_version;
> -};
> -
> -static inline void grant_read_lock(struct grant_table *gt)
> -{
> -    percpu_read_lock(grant_rwlock, &gt->lock);
> -}
> -
> -static inline void grant_read_unlock(struct grant_table *gt)
> -{
> -    percpu_read_unlock(grant_rwlock, &gt->lock);
> -}
> -
> -static inline void grant_write_lock(struct grant_table *gt)
> -{
> -    percpu_write_lock(grant_rwlock, &gt->lock);
> -}
> -
> -static inline void grant_write_unlock(struct grant_table *gt)
> -{
> -    percpu_write_unlock(grant_rwlock, &gt->lock);
> -}
> +extern unsigned int __read_mostly max_grant_frames;

Why are you adding __read_mostly here? Like all section placement
annotations it belongs on definitions only, not declarations.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -307,6 +307,7 @@ struct vm_event_per_domain
>  };
>  
>  struct evtchn_port_ops;
> +struct grant_table;

Why is this needed? There's no function with a respective parameter
in the header here.

Jan


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

Re: [PATCH 3/5] xen: clean up grant_table.h

[Juergen Gross]

On 24/08/17 16:17, Jan Beulich wrote:
>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>> @@ -118,6 +154,18 @@ struct grant_mapping {
>>      uint32_t pad;           /* round size to a power of 2 */
>>  };
>>  
>> +/* Number of grant table frames. Caller must hold d's grant table lock. */
>> +static inline unsigned int nr_grant_frames(struct grant_table *gt)
>> +{
>> +    return gt->nr_grant_frames;
>> +}
>> +
>> +/* Number of status grant table frames. Caller must hold d's gr. table lock.*/
>> +static inline unsigned int nr_status_frames(struct grant_table *gt)
>> +{
>> +    return gt->nr_status_frames;
>> +}
> 
> For both the parameters want to be constified.

Okay.

> 
>> @@ -250,6 +318,15 @@ static inline void active_entry_release(struct active_grant_entry *act)
>>      spin_unlock(&act->lock);
>>  }
>>  
>> +#define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
>> +#define GRANT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_v2_t))
>> +/* Number of grant table status entries. Caller must hold d's gr. table lock.*/
>> +static inline unsigned int grant_to_status_frames(int grant_frames)
> 
> unsigned int also for the parameter.

Okay.

> 
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -29,65 +29,8 @@
>>  #include <asm/page.h>
>>  #include <asm/grant_table.h>
>>  
>> -#ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
>> -/* Default maximum size of a grant table. [POLICY] */
>> -#define DEFAULT_MAX_NR_GRANT_FRAMES   32
>> -#endif
>>  /* The maximum size of a grant table. */
>> -extern unsigned int max_grant_frames;
>> -
>> -DECLARE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
>> -
>> -/* Per-domain grant information. */
>> -struct grant_table {
>> -    /*
>> -     * Lock protecting updates to grant table state (version, active
>> -     * entry list, etc.)
>> -     */
>> -    percpu_rwlock_t       lock;
>> -    /* Table size. Number of frames shared with guest */
>> -    unsigned int          nr_grant_frames;
>> -    /* Shared grant table (see include/public/grant_table.h). */
>> -    union {
>> -        void **shared_raw;
>> -        struct grant_entry_v1 **shared_v1;
>> -        union grant_entry_v2 **shared_v2;
>> -    };
>> -    /* Number of grant status frames shared with guest (for version 2) */
>> -    unsigned int          nr_status_frames;
>> -    /* State grant table (see include/public/grant_table.h). */
>> -    grant_status_t       **status;
>> -    /* Active grant table. */
>> -    struct active_grant_entry **active;
>> -    /* Mapping tracking table per vcpu. */
>> -    struct grant_mapping **maptrack;
>> -    unsigned int          maptrack_limit;
>> -    /* Lock protecting the maptrack limit */
>> -    spinlock_t            maptrack_lock;
>> -    /* The defined versions are 1 and 2.  Set to 0 if we don't know
>> -       what version to use yet. */
>> -    unsigned              gt_version;
>> -};
>> -
>> -static inline void grant_read_lock(struct grant_table *gt)
>> -{
>> -    percpu_read_lock(grant_rwlock, &gt->lock);
>> -}
>> -
>> -static inline void grant_read_unlock(struct grant_table *gt)
>> -{
>> -    percpu_read_unlock(grant_rwlock, &gt->lock);
>> -}
>> -
>> -static inline void grant_write_lock(struct grant_table *gt)
>> -{
>> -    percpu_write_lock(grant_rwlock, &gt->lock);
>> -}
>> -
>> -static inline void grant_write_unlock(struct grant_table *gt)
>> -{
>> -    percpu_write_unlock(grant_rwlock, &gt->lock);
>> -}
>> +extern unsigned int __read_mostly max_grant_frames;
> 
> Why are you adding __read_mostly here? Like all section placement
> annotations it belongs on definitions only, not declarations.

Oops, sorry.

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -307,6 +307,7 @@ struct vm_event_per_domain
>>  };
>>  
>>  struct evtchn_port_ops;
>> +struct grant_table;
> 
> Why is this needed? There's no function with a respective parameter
> in the header here.

You are right, of course. Will remove it again.


Juergen

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

[PATCH 4/5] xen: support different gnttab_max_frames for grant v1 and v2

[Juergen Gross]

The number of grants a domain can setup is limited by the maximum
number of grant frames it is allowed to use. Today the limit is the
same regardless whether the domain uses grant v1 or v2. Using v2
will therefor be a disadvantage for the domain as only half the
number of grants compared to v1 can be used, because a grant v2 entry
is twice as large as the v1 entry. This is the reason for the lack of
grant v2 support in the Linux kernel (in fact grant v2 support has
been removed from Linux for this reason).

OTOH using only grant v1 will limit a pv domain to the low 16TB of
memory of the host, as grant v1 entries only use a 32 bit mfn. So
if we want to support more than 16TB of memory and be able to use
that memory in pv domains, we have to remove the disadvantage of
using grant v2 by being able to setup the same number of grants as
with v1.

In order to achieve this add support for limiting the number of grant
frames for v1 and v2 independently from each other. Per default let
the v2 number be twice the value of the v1 number. Modify the boot
parameter gnttab_max_frames to accept either a single value which
will set the v1 limit to that value and the v2 limit to 2*value, or
two values separated by a comma to set both limits to dedicated
values.

Add some sanity checks to make sure the maximum number of frames isn't
lower than the initial number, as this leads to rather strange crashes.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/domain.c             |   2 +-
 xen/arch/arm/domain_build.c       |   2 +-
 xen/common/compat/grant_table.c   |   9 ++--
 xen/common/grant_table.c          | 107 ++++++++++++++++++++++++++------------
 xen/include/asm-arm/grant_table.h |   3 +-
 xen/include/xen/grant_table.h     |   5 +-
 6 files changed, 84 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index eeebbdb39a..5a0bec0b4b 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -469,7 +469,7 @@ struct domain *alloc_domain_struct(void)
         return NULL;
 
     clear_page(d);
-    d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames);
+    d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames());
     return d;
 }
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1bec4fa23d..ffc5e5e87c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2095,7 +2095,7 @@ static void __init find_gnttab_region(struct domain *d,
     kinfo->gnttab_size = (_etext - _stext) & PAGE_MASK;
 
     /* Make sure the grant table will fit in the region */
-    if ( (kinfo->gnttab_size >> PAGE_SHIFT) < max_grant_frames )
+    if ( (kinfo->gnttab_size >> PAGE_SHIFT) < max_grant_frames() )
         panic("Cannot find a space for the grant table region\n");
 
 #ifdef CONFIG_ARM_32
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index f8c60a1bdf..a437340215 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -157,11 +157,12 @@ 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 )
+
+                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);
+                             max_grant_frames(), max_frame_list_size_in_page);
                     rc = -EINVAL;
                 }
                 else
@@ -295,11 +296,11 @@ int compat_grant_table_op(unsigned int cmd,
                 break;
             }
             if ( max_frame_list_size_in_pages <
-                 grant_to_status_frames(max_grant_frames) )
+                 grant_to_status_frames(max_grant_frames_v2) )
             {
                 gdprintk(XENLOG_WARNING,
                          "grant_to_status_frames(max_grant_frames) is too large (%u,%u)\n",
-                         grant_to_status_frames(max_grant_frames),
+                         grant_to_status_frames(max_grant_frames_v2),
                          max_frame_list_size_in_pages);
                 rc = -EINVAL;
                 break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 01d09945da..33cf1f9ea2 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -76,8 +76,34 @@ struct grant_table {
 #define DEFAULT_MAX_NR_GRANT_FRAMES   32
 #endif
 
-unsigned int __read_mostly max_grant_frames;
-integer_param("gnttab_max_frames", max_grant_frames);
+static unsigned int __read_mostly max_grant_frames_v1;
+static unsigned int __read_mostly max_grant_frames_v2;
+
+static int set_max_grant_frames(const char *str)
+{
+    const char *s;
+    unsigned int val;
+
+    val = simple_strtoul(str, &s, 0);
+    if ( val < INITIAL_NR_GRANT_FRAMES )
+        return -EINVAL;
+    max_grant_frames_v1 = val;
+    max_grant_frames_v2 = 2 * max_grant_frames_v1;
+    if ( !*s )
+        return 0;
+
+    if ( *s != ',' )
+        return -EINVAL;
+
+    val = simple_strtoul(s + 1, &s, 0);
+    if ( val < INITIAL_NR_GRANT_FRAMES )
+        return -EINVAL;
+    max_grant_frames_v2 = val;
+
+    return *s ? -EINVAL : 0;
+}
+
+custom_param("gnttab_max_frames", set_max_grant_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.
@@ -274,24 +300,33 @@ static inline void gnttab_flush_tlb(const struct domain *d)
 }
 
 static inline unsigned int
-num_act_frames_from_sha_frames(const unsigned int num)
+num_act_frames_from_sha_frames(const unsigned int num, unsigned int version)
 {
     /*
      * How many frames are needed for the active grant table,
      * given the size of the shared grant table?
      */
-    unsigned int sha_per_page = PAGE_SIZE / sizeof(grant_entry_v1_t);
+    unsigned int sha_per_page;
 
+    sha_per_page = (version == 2) ? SHGNT_PER_PAGE_V2 : SHGNT_PER_PAGE_V1;
     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)
-
 static inline unsigned int
 nr_active_grant_frames(struct grant_table *gt)
 {
-    return num_act_frames_from_sha_frames(nr_grant_frames(gt));
+    return num_act_frames_from_sha_frames(nr_grant_frames(gt), gt->gt_version);
+}
+
+unsigned int max_grant_frames(void)
+{
+    return max(max_grant_frames_v1, max_grant_frames_v2);
+}
+
+static inline unsigned int
+max_grant_frames_v(struct grant_table *t)
+{
+    return (t->gt_version == 2) ? max_grant_frames_v2 : max_grant_frames_v1;
 }
 
 static inline struct active_grant_entry *
@@ -1663,7 +1698,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
     struct grant_table *gt = d->grant_table;
     unsigned int i, j;
 
-    ASSERT(req_nr_frames <= max_grant_frames);
+    ASSERT(req_nr_frames <= max_grant_frames_v(gt));
 
     gdprintk(XENLOG_INFO,
             "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1671,7 +1706,8 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
 
     /* Active */
     for ( i = nr_active_grant_frames(gt);
-          i < num_act_frames_from_sha_frames(req_nr_frames); i++ )
+          i < num_act_frames_from_sha_frames(req_nr_frames, gt->gt_version);
+          i++ )
     {
         if ( (gt->active[i] = alloc_xenheap_page()) == NULL )
             goto active_alloc_failed;
@@ -1710,7 +1746,8 @@ shared_alloc_failed:
     }
 active_alloc_failed:
     for ( i = nr_active_grant_frames(gt);
-          i < num_act_frames_from_sha_frames(req_nr_frames); i++ )
+          i < num_act_frames_from_sha_frames(req_nr_frames, gt->gt_version);
+          i++ )
     {
         free_xenheap_page(gt->active[i]);
         gt->active[i] = NULL;
@@ -1735,15 +1772,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;
 
@@ -1763,6 +1791,15 @@ gnttab_setup_table(
     gt = d->grant_table;
     grant_write_lock(gt);
 
+    if ( unlikely(op.nr_frames > max_grant_frames_v(gt)) )
+    {
+        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
+                " per domain.\n",
+                max_grant_frames_v(gt));
+        op.status = GNTST_general_error;
+        goto unlock;
+    }
+
     if ( gt->gt_version == 0 )
         gt->gt_version = 1;
 
@@ -1773,7 +1810,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), max_grant_frames_v(gt));
         op.status = GNTST_general_error;
         goto unlock;
     }
@@ -1831,7 +1868,7 @@ gnttab_query_size(
     grant_read_lock(d->grant_table);
 
     op.nr_frames     = nr_grant_frames(d->grant_table);
-    op.max_nr_frames = max_grant_frames;
+    op.max_nr_frames = max_grant_frames_v(d->grant_table);
     op.status        = GNTST_okay;
 
     grant_read_unlock(d->grant_table);
@@ -3378,7 +3415,7 @@ grant_table_create(
     struct domain *d)
 {
     struct grant_table *t;
-    unsigned int i, j;
+    unsigned int i, j, max_frames;
 
     if ( (t = xzalloc(struct grant_table)) == NULL )
         goto no_mem_0;
@@ -3389,11 +3426,12 @@ grant_table_create(
     t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
 
     /* Active grant table. */
+    max_frames = max(num_act_frames_from_sha_frames(max_grant_frames_v1, 1),
+                     num_act_frames_from_sha_frames(max_grant_frames_v2, 2));
     if ( (t->active = xzalloc_array(struct active_grant_entry *,
-                                    max_nr_active_grant_frames)) == NULL )
+                                    max_frames)) == NULL )
         goto no_mem_1;
-    for ( i = 0;
-          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
+    for ( i = 0; i < max_frames; i++ )
     {
         if ( (t->active[i] = alloc_xenheap_page()) == NULL )
             goto no_mem_2;
@@ -3408,7 +3446,7 @@ grant_table_create(
         goto no_mem_2;
 
     /* Shared grant table. */
-    if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
+    if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames())) == NULL )
         goto no_mem_3;
     for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
     {
@@ -3419,7 +3457,7 @@ grant_table_create(
 
     /* Status pages for grant table - for version 2 */
     t->status = xzalloc_array(grant_status_t *,
-                              grant_to_status_frames(max_grant_frames));
+                              grant_to_status_frames(max_grant_frames_v2));
     if ( t->status == NULL )
         goto no_mem_4;
 
@@ -3439,8 +3477,7 @@ grant_table_create(
  no_mem_3:
     vfree(t->maptrack);
  no_mem_2:
-    for ( i = 0;
-          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
+    for ( i = 0; i < max_frames; i++ )
         free_xenheap_page(t->active[i]);
     xfree(t->active);
  no_mem_1:
@@ -3706,7 +3743,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
     else
     {
         if ( (idx >= nr_grant_frames(d->grant_table)) &&
-             (idx < max_grant_frames) )
+             (idx < max_grant_frames_v(d->grant_table)) )
             gnttab_grow_table(d, idx + 1);
 
         if ( idx < nr_grant_frames(d->grant_table) )
@@ -3794,10 +3831,12 @@ static int __init gnttab_usage_init(void)
 {
     BUILD_BUG_ON(DEFAULT_MAX_MAPTRACK_FRAMES < DEFAULT_MAX_NR_GRANT_FRAMES);
 
-    if ( !max_grant_frames )
-        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+    if ( !max_grant_frames_v1 )
+        max_grant_frames_v1 = DEFAULT_MAX_NR_GRANT_FRAMES;
+    if ( !max_grant_frames_v2 )
+        max_grant_frames_v2 = 2 * DEFAULT_MAX_NR_GRANT_FRAMES;
 
-    if ( !max_maptrack_frames )
+    if ( max_maptrack_frames < INITIAL_NR_GRANT_FRAMES )
         max_maptrack_frames = DEFAULT_MAX_MAPTRACK_FRAMES;
 
     register_keyhandler('g', gnttab_usage_print_all,
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 581c4bf928..2b51dc954a 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -36,7 +36,8 @@ static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long idx,
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
-     (i < max_grant_frames)) ? 0 : gfn_x(d->arch.grant_table_gfn[i]))
+     (i < max_grant_frames_v(d->grant_table)))                           \
+        ? 0 : gfn_x(d->arch.grant_table_gfn[i]))
 
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && need_iommu(d))
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 06faf61a6b..e003cb39f3 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -29,9 +29,6 @@
 #include <asm/page.h>
 #include <asm/grant_table.h>
 
-/* The maximum size of a grant table. */
-extern unsigned int __read_mostly max_grant_frames;
-
 /* Create/destroy per-domain grant table context. */
 int grant_table_create(
     struct domain *d);
@@ -55,4 +52,6 @@ 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);
 
+unsigned int max_grant_frames(void);
+
 #endif /* __XEN_GRANT_TABLE_H__ */
-- 
2.12.3


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

Re: [PATCH 4/5] xen: support different gnttab_max_frames for grant v1 and v2

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> The number of grants a domain can setup is limited by the maximum
> number of grant frames it is allowed to use. Today the limit is the
> same regardless whether the domain uses grant v1 or v2. Using v2
> will therefor be a disadvantage for the domain as only half the
> number of grants compared to v1 can be used, because a grant v2 entry
> is twice as large as the v1 entry. This is the reason for the lack of
> grant v2 support in the Linux kernel (in fact grant v2 support has
> been removed from Linux for this reason).
> 
> OTOH using only grant v1 will limit a pv domain to the low 16TB of
> memory of the host, as grant v1 entries only use a 32 bit mfn. So
> if we want to support more than 16TB of memory and be able to use
> that memory in pv domains, we have to remove the disadvantage of
> using grant v2 by being able to setup the same number of grants as
> with v1.
> 
> In order to achieve this add support for limiting the number of grant
> frames for v1 and v2 independently from each other. Per default let
> the v2 number be twice the value of the v1 number. Modify the boot
> parameter gnttab_max_frames to accept either a single value which
> will set the v1 limit to that value and the v2 limit to 2*value, or
> two values separated by a comma to set both limits to dedicated
> values.
> 
> Add some sanity checks to make sure the maximum number of frames isn't
> lower than the initial number, as this leads to rather strange crashes.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

As discussed elsewhere, this probably rather wants to become a
per-domain setting then. Looking also at what patch 5 adds, I
additionally wonder whether we shouldn't allow Dom0 to know
whether it needs to use v2 at all (or maybe it can derive that
already).

Jan


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

Re: [PATCH 4/5] xen: support different gnttab_max_frames for grant v1 and v2

[Juergen Gross]

On 24/08/17 16:21, Jan Beulich wrote:
>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>> The number of grants a domain can setup is limited by the maximum
>> number of grant frames it is allowed to use. Today the limit is the
>> same regardless whether the domain uses grant v1 or v2. Using v2
>> will therefor be a disadvantage for the domain as only half the
>> number of grants compared to v1 can be used, because a grant v2 entry
>> is twice as large as the v1 entry. This is the reason for the lack of
>> grant v2 support in the Linux kernel (in fact grant v2 support has
>> been removed from Linux for this reason).
>>
>> OTOH using only grant v1 will limit a pv domain to the low 16TB of
>> memory of the host, as grant v1 entries only use a 32 bit mfn. So
>> if we want to support more than 16TB of memory and be able to use
>> that memory in pv domains, we have to remove the disadvantage of
>> using grant v2 by being able to setup the same number of grants as
>> with v1.
>>
>> In order to achieve this add support for limiting the number of grant
>> frames for v1 and v2 independently from each other. Per default let
>> the v2 number be twice the value of the v1 number. Modify the boot
>> parameter gnttab_max_frames to accept either a single value which
>> will set the v1 limit to that value and the v2 limit to 2*value, or
>> two values separated by a comma to set both limits to dedicated
>> values.
>>
>> Add some sanity checks to make sure the maximum number of frames isn't
>> lower than the initial number, as this leads to rather strange crashes.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> As discussed elsewhere, this probably rather wants to become a
> per-domain setting then. Looking also at what patch 5 adds, I
> additionally wonder whether we shouldn't allow Dom0 to know
> whether it needs to use v2 at all (or maybe it can derive that
> already).

I'm absolutely in favor of adding a way to make this a per-domain
setting.

OTOH I think there should be a default especially on huge hosts
allowing to use v2 grants without reducing the number of allowed
grants, which doesn't need adding another parameter to the domain
config. Or do you want the tools to always set the per-domain value
possibly based on a xl.conf value? Then we could remove the
gnttab_max_frames command line parameter completely.


Juergen

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

Re: [PATCH 4/5] xen: support different gnttab_max_frames for grant v1 and v2

[Jan Beulich]

>>> On 24.08.17 at 16:54, <jgross@suse.com> wrote:
> OTOH I think there should be a default especially on huge hosts
> allowing to use v2 grants without reducing the number of allowed
> grants, which doesn't need adding another parameter to the domain
> config. Or do you want the tools to always set the per-domain value
> possibly based on a xl.conf value? Then we could remove the
> gnttab_max_frames command line parameter completely.

Yes, obsoleting that command line option would seem quite
desirable to me.

Jan


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

Re: [PATCH 4/5] xen: support different gnttab_max_frames for grant v1 and v2

[Juergen Gross]

On 24/08/17 17:01, Jan Beulich wrote:
>>>> On 24.08.17 at 16:54, <jgross@suse.com> wrote:
>> OTOH I think there should be a default especially on huge hosts
>> allowing to use v2 grants without reducing the number of allowed
>> grants, which doesn't need adding another parameter to the domain
>> config. Or do you want the tools to always set the per-domain value
>> possibly based on a xl.conf value? Then we could remove the
>> gnttab_max_frames command line parameter completely.
> 
> Yes, obsoleting that command line option would seem quite
> desirable to me.

Okay, I can modify my patches in that regard.


Juergen

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

[PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

Today a guest can get the maximum grant table frame number for the
currently selected grant table interface version (1 or 2) only. Add
a new grant table operation to get the limits of both variants in
order to give the guest an opportunity to select the version serving
its needs best.

Background for the need for this new hypercall is that e.g. the Linux
kernel won't use v2 as long as this will allow less active grants as
v1. As soon as the kernel can detect it can create as many v2 entries
as for v1, it will have no reason not to use v2. And this will allow
Xen placing a pv-domain with such a kernel above the current 16TB
RAM limit.

For setting up the grant table a kernel needs the following
information:
- current version (kexec case)
- current size (kexec case)
- max size v1
- max size v2
In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
hypercall return all the needed information.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/grant_table.c         | 26 ++++++++++++++++++++++++++
 xen/include/public/grant_table.h | 14 ++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 33cf1f9ea2..83370a0390 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3041,6 +3041,28 @@ gnttab_get_version(XEN_GUEST_HANDLE_PARAM(gnttab_get_version_t) uop)
     return 0;
 }
 
+static long
+gnttab_get_setup_info(XEN_GUEST_HANDLE_PARAM(gnttab_setup_info_t) uop)
+{
+    gnttab_setup_info_t op;
+    struct domain *d = rcu_lock_current_domain();
+    struct grant_table *gt = d->grant_table;
+
+    grant_write_lock(gt);
+    op.version = gt->gt_version;
+    op.nr_frames = nr_grant_frames(gt);
+    grant_write_unlock(gt);
+    op.max_nr_frames_v1 = max_grant_frames_v1;
+    op.max_nr_frames_v2 = max_grant_frames_v2;
+
+    rcu_unlock_domain(d);
+
+    if ( __copy_to_guest(uop, &op, 1) )
+        return -EFAULT;
+
+    return 0;
+}
+
 static s16
 swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
 {
@@ -3389,6 +3411,10 @@ do_grant_table_op(
         break;
     }
 
+    case GNTTABOP_get_setup_info:
+        rc = gnttab_get_setup_info(guest_handle_cast(uop, gnttab_setup_info_t));
+        break;
+
     default:
         rc = -ENOSYS;
         break;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index 018036e825..b30f85b2f8 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -312,6 +312,7 @@ typedef uint16_t grant_status_t;
 #define GNTTABOP_get_version          10
 #define GNTTABOP_swap_grant_ref	      11
 #define GNTTABOP_cache_flush	      12
+#define GNTTABOP_get_setup_info       13
 #endif /* __XEN_INTERFACE_VERSION__ */
 /* ` } */
 
@@ -597,6 +598,19 @@ struct gnttab_cache_flush {
 typedef struct gnttab_cache_flush gnttab_cache_flush_t;
 DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
 
+/*
+ * GNTTABOP_get_setup_info: Get information needed for grant table setup
+ * of the calling domain.
+ */
+struct gnttab_setup_info {
+    uint32_t version;
+    uint32_t nr_frames;
+    uint32_t max_nr_frames_v1;
+    uint32_t max_nr_frames_v2;
+};
+typedef struct gnttab_setup_info gnttab_setup_info_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_setup_info_t);
+
 #endif /* __XEN_INTERFACE_VERSION__ */
 
 /*
-- 
2.12.3


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> Today a guest can get the maximum grant table frame number for the
> currently selected grant table interface version (1 or 2) only. Add
> a new grant table operation to get the limits of both variants in
> order to give the guest an opportunity to select the version serving
> its needs best.
> 
> Background for the need for this new hypercall is that e.g. the Linux
> kernel won't use v2 as long as this will allow less active grants as
> v1. As soon as the kernel can detect it can create as many v2 entries
> as for v1, it will have no reason not to use v2. And this will allow
> Xen placing a pv-domain with such a kernel above the current 16TB
> RAM limit.
> 
> For setting up the grant table a kernel needs the following
> information:
> - current version (kexec case)
> - current size (kexec case)
> - max size v1
> - max size v2
> In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
> GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
> hypercall return all the needed information.

I'm not sure I follow: v2 is always going to allow less active grants
than v1, as the limit is always derived from the number of frames
allowed for a domain. I also don't see a problem with issuing multiple
calls - none of this ought to be performance critical. I would,
however, agree that rather than adding a new hypercall to just
return the max sizes adding one like you suggest would be
preferable. I'm simply not convinced yet that returning the max
sizes is actually necessary.

Jan


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

On 24/08/17 16:28, Jan Beulich wrote:
>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>> Today a guest can get the maximum grant table frame number for the
>> currently selected grant table interface version (1 or 2) only. Add
>> a new grant table operation to get the limits of both variants in
>> order to give the guest an opportunity to select the version serving
>> its needs best.
>>
>> Background for the need for this new hypercall is that e.g. the Linux
>> kernel won't use v2 as long as this will allow less active grants as
>> v1. As soon as the kernel can detect it can create as many v2 entries
>> as for v1, it will have no reason not to use v2. And this will allow
>> Xen placing a pv-domain with such a kernel above the current 16TB
>> RAM limit.
>>
>> For setting up the grant table a kernel needs the following
>> information:
>> - current version (kexec case)
>> - current size (kexec case)
>> - max size v1
>> - max size v2
>> In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
>> GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
>> hypercall return all the needed information.
> 
> I'm not sure I follow: v2 is always going to allow less active grants
> than v1, as the limit is always derived from the number of frames
> allowed for a domain.

Right. Patch 3 adds support for different number of allowed frames for
v1 and v2. So the system can be configured to allow the same max.
number of grants for v1 and v2, or even more v2 grants than v1.

> I also don't see a problem with issuing multiple
> calls - none of this ought to be performance critical. I would,
> however, agree that rather than adding a new hypercall to just
> return the max sizes adding one like you suggest would be
> preferable. I'm simply not convinced yet that returning the max
> sizes is actually necessary.

How would the guest know whether using v2 grants is no disadvantage?


Juergen

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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

On 24/08/17 16:48, Juergen Gross wrote:
> On 24/08/17 16:28, Jan Beulich wrote:
>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>> Today a guest can get the maximum grant table frame number for the
>>> currently selected grant table interface version (1 or 2) only. Add
>>> a new grant table operation to get the limits of both variants in
>>> order to give the guest an opportunity to select the version serving
>>> its needs best.
>>>
>>> Background for the need for this new hypercall is that e.g. the Linux
>>> kernel won't use v2 as long as this will allow less active grants as
>>> v1. As soon as the kernel can detect it can create as many v2 entries
>>> as for v1, it will have no reason not to use v2. And this will allow
>>> Xen placing a pv-domain with such a kernel above the current 16TB
>>> RAM limit.
>>>
>>> For setting up the grant table a kernel needs the following
>>> information:
>>> - current version (kexec case)
>>> - current size (kexec case)
>>> - max size v1
>>> - max size v2
>>> In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
>>> GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
>>> hypercall return all the needed information.
>>
>> I'm not sure I follow: v2 is always going to allow less active grants
>> than v1, as the limit is always derived from the number of frames
>> allowed for a domain.
> 
> Right. Patch 3 adds support for different number of allowed frames for

This should read "Patch 4", of course

> v1 and v2. So the system can be configured to allow the same max.
> number of grants for v1 and v2, or even more v2 grants than v1.
> 
>> I also don't see a problem with issuing multiple
>> calls - none of this ought to be performance critical. I would,
>> however, agree that rather than adding a new hypercall to just
>> return the max sizes adding one like you suggest would be
>> preferable. I'm simply not convinced yet that returning the max
>> sizes is actually necessary.
> 
> How would the guest know whether using v2 grants is no disadvantage?
> 
> 
> Juergen
> 
> _______________________________________________
> 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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Jan Beulich]

>>> On 24.08.17 at 16:48, <jgross@suse.com> wrote:
> On 24/08/17 16:28, Jan Beulich wrote:
>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>> Today a guest can get the maximum grant table frame number for the
>>> currently selected grant table interface version (1 or 2) only. Add
>>> a new grant table operation to get the limits of both variants in
>>> order to give the guest an opportunity to select the version serving
>>> its needs best.
>>>
>>> Background for the need for this new hypercall is that e.g. the Linux
>>> kernel won't use v2 as long as this will allow less active grants as
>>> v1. As soon as the kernel can detect it can create as many v2 entries
>>> as for v1, it will have no reason not to use v2. And this will allow
>>> Xen placing a pv-domain with such a kernel above the current 16TB
>>> RAM limit.
>>>
>>> For setting up the grant table a kernel needs the following
>>> information:
>>> - current version (kexec case)
>>> - current size (kexec case)
>>> - max size v1
>>> - max size v2
>>> In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
>>> GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
>>> hypercall return all the needed information.
>> 
>> I'm not sure I follow: v2 is always going to allow less active grants
>> than v1, as the limit is always derived from the number of frames
>> allowed for a domain.
> 
> Right. Patch 3 adds support for different number of allowed frames for
> v1 and v2. So the system can be configured to allow the same max.
> number of grants for v1 and v2, or even more v2 grants than v1.
> 
>> I also don't see a problem with issuing multiple
>> calls - none of this ought to be performance critical. I would,
>> however, agree that rather than adding a new hypercall to just
>> return the max sizes adding one like you suggest would be
>> preferable. I'm simply not convinced yet that returning the max
>> sizes is actually necessary.
> 
> How would the guest know whether using v2 grants is no disadvantage?

As said - it's always going to be a disadvantage. Even if controlling
the number of frames per-domain, that same number of frames
will always fit more v1 than v2 entries. I don't think the config
setting should directly control the number of active grants. Or if
we did it that way, then the answer to your question would be
"based on hypervisor version".

Jan


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

On 24/08/17 17:04, Jan Beulich wrote:
>>>> On 24.08.17 at 16:48, <jgross@suse.com> wrote:
>> On 24/08/17 16:28, Jan Beulich wrote:
>>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>>> Today a guest can get the maximum grant table frame number for the
>>>> currently selected grant table interface version (1 or 2) only. Add
>>>> a new grant table operation to get the limits of both variants in
>>>> order to give the guest an opportunity to select the version serving
>>>> its needs best.
>>>>
>>>> Background for the need for this new hypercall is that e.g. the Linux
>>>> kernel won't use v2 as long as this will allow less active grants as
>>>> v1. As soon as the kernel can detect it can create as many v2 entries
>>>> as for v1, it will have no reason not to use v2. And this will allow
>>>> Xen placing a pv-domain with such a kernel above the current 16TB
>>>> RAM limit.
>>>>
>>>> For setting up the grant table a kernel needs the following
>>>> information:
>>>> - current version (kexec case)
>>>> - current size (kexec case)
>>>> - max size v1
>>>> - max size v2
>>>> In order not to have to issue 3 hypercalls (GNTTABOP_query_size,
>>>> GNTTABOP_get_version, GNTTABOP_get_v1_and_v2_max), let the new
>>>> hypercall return all the needed information.
>>>
>>> I'm not sure I follow: v2 is always going to allow less active grants
>>> than v1, as the limit is always derived from the number of frames
>>> allowed for a domain.
>>
>> Right. Patch 3 adds support for different number of allowed frames for
>> v1 and v2. So the system can be configured to allow the same max.
>> number of grants for v1 and v2, or even more v2 grants than v1.
>>
>>> I also don't see a problem with issuing multiple
>>> calls - none of this ought to be performance critical. I would,
>>> however, agree that rather than adding a new hypercall to just
>>> return the max sizes adding one like you suggest would be
>>> preferable. I'm simply not convinced yet that returning the max
>>> sizes is actually necessary.
>>
>> How would the guest know whether using v2 grants is no disadvantage?
> 
> As said - it's always going to be a disadvantage. Even if controlling
> the number of frames per-domain, that same number of frames
> will always fit more v1 than v2 entries.

And my patches break the assumption "same number of frames".

> I don't think the config
> setting should directly control the number of active grants.

Why not? In the end that number is the one the guest is interested in.
BTW: the number of needed maptrack frames is depending on the number
of grants only, not on v1 or v2. And the default for the max. number
of maptrack frames is much higher than the one of the grant frames
(1024 vs 32).

> Or if we did it that way, then the answer to your question would be
> "based on hypervisor version".

Yeah, or based on the answer from the hypervisor regarding my new
info call (if the answer is "-ENOSYS" the guest probably would choose
v1 as today).


Juergen


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Jan Beulich]

>>> On 24.08.17 at 17:13, <jgross@suse.com> wrote:
> On 24/08/17 17:04, Jan Beulich wrote:
>>>>> On 24.08.17 at 16:48, <jgross@suse.com> wrote:
>>> How would the guest know whether using v2 grants is no disadvantage?
>> 
>> As said - it's always going to be a disadvantage. Even if controlling
>> the number of frames per-domain, that same number of frames
>> will always fit more v1 than v2 entries.
> 
> And my patches break the assumption "same number of frames".
> 
>> I don't think the config
>> setting should directly control the number of active grants.
> 
> Why not? In the end that number is the one the guest is interested in.

And the resource use is what the admin is interested in.

> BTW: the number of needed maptrack frames is depending on the number
> of grants only, not on v1 or v2. And the default for the max. number
> of maptrack frames is much higher than the one of the grant frames
> (1024 vs 32).

Of course.

Jan


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

On 24/08/17 17:20, Jan Beulich wrote:
>>>> On 24.08.17 at 17:13, <jgross@suse.com> wrote:
>> On 24/08/17 17:04, Jan Beulich wrote:
>>>>>> On 24.08.17 at 16:48, <jgross@suse.com> wrote:
>>>> How would the guest know whether using v2 grants is no disadvantage?
>>>
>>> As said - it's always going to be a disadvantage. Even if controlling
>>> the number of frames per-domain, that same number of frames
>>> will always fit more v1 than v2 entries.
>>
>> And my patches break the assumption "same number of frames".
>>
>>> I don't think the config
>>> setting should directly control the number of active grants.
>>
>> Why not? In the end that number is the one the guest is interested in.
> 
> And the resource use is what the admin is interested in.
> 
>> BTW: the number of needed maptrack frames is depending on the number
>> of grants only, not on v1 or v2. And the default for the max. number
>> of maptrack frames is much higher than the one of the grant frames
>> (1024 vs 32).
> 
> Of course.

So what about the following idea:

- Set the default max number of grant frames to 32 on "small" hosts
  (max. physical memory address below 16TB) and to 64 on "large"
  hosts
- Add a per-domain parameter for the max number of grant frames

In the Linux kernel I would then:

- Re-add grant v2 support
- Add boot parameter for selecting grant v1 or v2
- Use grant v2 if selected via boot parameter, or (pv only:) max. host
  address is above 16TB


Juergen

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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Jan Beulich]

>>> On 25.08.17 at 13:40, <jgross@suse.com> wrote:
> So what about the following idea:
> 
> - Set the default max number of grant frames to 32 on "small" hosts
>   (max. physical memory address below 16TB) and to 64 on "large"
>   hosts

This looks reasonable to me as long as it's properly documented.

> - Add a per-domain parameter for the max number of grant frames
> 
> In the Linux kernel I would then:
> 
> - Re-add grant v2 support
> - Add boot parameter for selecting grant v1 or v2
> - Use grant v2 if selected via boot parameter, or (pv only:) max. host
>   address is above 16TB

Can a PV DomU know? Plus can you easily switch between
versions after having been migrated? And wouldn't HVM or ARM
guests need to judge by max guest address?

Jan


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Juergen Gross]

On 25/08/17 13:58, Jan Beulich wrote:
>>>> On 25.08.17 at 13:40, <jgross@suse.com> wrote:
>> So what about the following idea:
>>
>> - Set the default max number of grant frames to 32 on "small" hosts
>>   (max. physical memory address below 16TB) and to 64 on "large"
>>   hosts
> 
> This looks reasonable to me as long as it's properly documented.
> 
>> - Add a per-domain parameter for the max number of grant frames
>>
>> In the Linux kernel I would then:
>>
>> - Re-add grant v2 support
>> - Add boot parameter for selecting grant v1 or v2
>> - Use grant v2 if selected via boot parameter, or (pv only:) max. host
>>   address is above 16TB
> 
> Can a PV DomU know?

XENMEM_maximum_ram_page

> Plus can you easily switch between
> versions after having been migrated?

Other than console and xenbus there should be no active grant after
migration. So yes, I'm planning to support both, v1->v2 and v2->v1.

> And wouldn't HVM or ARM
> guests need to judge by max guest address?

Yes.


Juergen


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

Re: [PATCH 5/5] xen: add new hypercall to get grant table limits

[Jan Beulich]

>>> On 25.08.17 at 14:03, <jgross@suse.com> wrote:
> On 25/08/17 13:58, Jan Beulich wrote:
>>>>> On 25.08.17 at 13:40, <jgross@suse.com> wrote:
>>> In the Linux kernel I would then:
>>>
>>> - Re-add grant v2 support
>>> - Add boot parameter for selecting grant v1 or v2
>>> - Use grant v2 if selected via boot parameter, or (pv only:) max. host
>>>   address is above 16TB
>> 
>> Can a PV DomU know?
> 
> XENMEM_maximum_ram_page

Oh, I always forget that we bogusly expose a machine property here.

Jan


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

Re: [PATCH 0/5] xen: better grant v2 support

[Jan Beulich]

>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> Currently Linux has no support for grant v2 as this would reduce the
> maximum number of active grants by a factor of 2 compared to v1,
> because the number of possible grants are limited by the allowed number
> of grant frames and grant entries of v2 need twice as much bytes as
> those of v1.
> 
> Unfortunately grant v2 is the only way to support either guests with
> more than 16TB memory size or PV guests with memory above the 16TB
> border, as grant v1 limits the frame number to be 32 bits wide.
> 
> In order to remove the disadvantage of grant v2 this patch series
> enables configuring different maximum grant frame numbers for v1 and
> v2.

But that does imply higher memory footprint of such a guest in Xen,
doesn't it? The limit, after all, is there to bound resource use of
DomU-s.  I wonder whether we shouldn't make any such increase
dependent on first putting in place proper accounting of the memory
used for individual domains.

Jan


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

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 22/08/17 14:48, Jan Beulich wrote:
>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>> Currently Linux has no support for grant v2 as this would reduce the
>> maximum number of active grants by a factor of 2 compared to v1,
>> because the number of possible grants are limited by the allowed number
>> of grant frames and grant entries of v2 need twice as much bytes as
>> those of v1.
>>
>> Unfortunately grant v2 is the only way to support either guests with
>> more than 16TB memory size or PV guests with memory above the 16TB
>> border, as grant v1 limits the frame number to be 32 bits wide.
>>
>> In order to remove the disadvantage of grant v2 this patch series
>> enables configuring different maximum grant frame numbers for v1 and
>> v2.
> 
> But that does imply higher memory footprint of such a guest in Xen,
> doesn't it?

With current defaults this would need up to 128kB more for a guest using
v2 grants.

> The limit, after all, is there to bound resource use of
> DomU-s.  I wonder whether we shouldn't make any such increase
> dependent on first putting in place proper accounting of the memory
> used for individual domains.

So you would want to have a way to count pages (or bytes?) allocated for
hypervisor internal needs on a per-domain basis, right?

Would that be additional to struct domain -> xenheap_pages or would you
want to merge the new counter into it? I guess a new field would be
required in order to avoid counting some data twice.

Do you have an idea what to do with that value? Do you want to expose it
to the user (dom0 admin), or should it be used just inside the
hypervisor and e.g. printed by a debug key handler?

Do you want an additional set of allocating functions doing the
accounting, or should the existing functions be used with an additional
domain pointer, or should the caller be responsible doing the additional
accounting?

Do you want an all-or-nothing approach or a gradual move to add the new
accounting step by step?


Juergen


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

Re: [PATCH 0/5] xen: better grant v2 support

[Jan Beulich]

>>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
> On 22/08/17 14:48, Jan Beulich wrote:
>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>> Currently Linux has no support for grant v2 as this would reduce the
>>> maximum number of active grants by a factor of 2 compared to v1,
>>> because the number of possible grants are limited by the allowed number
>>> of grant frames and grant entries of v2 need twice as much bytes as
>>> those of v1.
>>>
>>> Unfortunately grant v2 is the only way to support either guests with
>>> more than 16TB memory size or PV guests with memory above the 16TB
>>> border, as grant v1 limits the frame number to be 32 bits wide.
>>>
>>> In order to remove the disadvantage of grant v2 this patch series
>>> enables configuring different maximum grant frame numbers for v1 and
>>> v2.
>> 
>> But that does imply higher memory footprint of such a guest in Xen,
>> doesn't it?
> 
> With current defaults this would need up to 128kB more for a guest using
> v2 grants.

At least in an auto-ballooned setup this may make the difference
between a guest being able or failing to start.

>> The limit, after all, is there to bound resource use of
>> DomU-s.  I wonder whether we shouldn't make any such increase
>> dependent on first putting in place proper accounting of the memory
>> used for individual domains.
> 
> So you would want to have a way to count pages (or bytes?) allocated for
> hypervisor internal needs on a per-domain basis, right?
> 
> Would that be additional to struct domain -> xenheap_pages or would you
> want to merge the new counter into it? I guess a new field would be
> required in order to avoid counting some data twice.
> 
> Do you have an idea what to do with that value? Do you want to expose it
> to the user (dom0 admin), or should it be used just inside the
> hypervisor and e.g. printed by a debug key handler?
> 
> Do you want an additional set of allocating functions doing the
> accounting, or should the existing functions be used with an additional
> domain pointer, or should the caller be responsible doing the additional
> accounting?
> 
> Do you want an all-or-nothing approach or a gradual move to add the new
> accounting step by step?

We've been vaguely discussing this in the past on a few occasions.
My personal thinking is that the "memory=" setting in a guest config
really ought to express all the memory associated with a guest. But
of course there'll be problems with us starting to do so, and that's
beyond people observing less memory in their guests. Switching to
such a full accounting model will require some careful thought (and
discussion up front). Hence I've only said "I wonder whether", i.e.
I don't mean to make this a strict prerequisite to the proposed
changes here. I'd be in particular interested to hear opinions of a
few other people.

Jan


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

Re: [PATCH 0/5] xen: better grant v2 support

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 23 August 2017 09:36
> To: Juergen Gross <jgross@suse.com>
> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
> 
> >>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
> > On 22/08/17 14:48, Jan Beulich wrote:
> >>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> >>> Currently Linux has no support for grant v2 as this would reduce the
> >>> maximum number of active grants by a factor of 2 compared to v1,
> >>> because the number of possible grants are limited by the allowed
> number
> >>> of grant frames and grant entries of v2 need twice as much bytes as
> >>> those of v1.
> >>>
> >>> Unfortunately grant v2 is the only way to support either guests with
> >>> more than 16TB memory size or PV guests with memory above the 16TB
> >>> border, as grant v1 limits the frame number to be 32 bits wide.
> >>>
> >>> In order to remove the disadvantage of grant v2 this patch series
> >>> enables configuring different maximum grant frame numbers for v1 and
> >>> v2.
> >>
> >> But that does imply higher memory footprint of such a guest in Xen,
> >> doesn't it?
> >
> > With current defaults this would need up to 128kB more for a guest using
> > v2 grants.
> 
> At least in an auto-ballooned setup this may make the difference
> between a guest being able or failing to start.
> 
> >> The limit, after all, is there to bound resource use of
> >> DomU-s.  I wonder whether we shouldn't make any such increase
> >> dependent on first putting in place proper accounting of the memory
> >> used for individual domains.
> >
> > So you would want to have a way to count pages (or bytes?) allocated for
> > hypervisor internal needs on a per-domain basis, right?
> >
> > Would that be additional to struct domain -> xenheap_pages or would you
> > want to merge the new counter into it? I guess a new field would be
> > required in order to avoid counting some data twice.
> >
> > Do you have an idea what to do with that value? Do you want to expose it
> > to the user (dom0 admin), or should it be used just inside the
> > hypervisor and e.g. printed by a debug key handler?
> >
> > Do you want an additional set of allocating functions doing the
> > accounting, or should the existing functions be used with an additional
> > domain pointer, or should the caller be responsible doing the additional
> > accounting?
> >
> > Do you want an all-or-nothing approach or a gradual move to add the new
> > accounting step by step?
> 
> We've been vaguely discussing this in the past on a few occasions.
> My personal thinking is that the "memory=" setting in a guest config
> really ought to express all the memory associated with a guest. But
> of course there'll be problems with us starting to do so, and that's
> beyond people observing less memory in their guests. Switching to
> such a full accounting model will require some careful thought (and
> discussion up front). Hence I've only said "I wonder whether", i.e.
> I don't mean to make this a strict prerequisite to the proposed
> changes here. I'd be in particular interested to hear opinions of a
> few other people.
> 

Making a the number of grant frames a per-vm-configurable quantity would seem like a reasonable first step. I'm not convinced of the need for separate v1 and v2 limits if this were the case.

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 23/08/17 10:47, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan
>> Beulich
>> Sent: 23 August 2017 09:36
>> To: Juergen Gross <jgross@suse.com>
>> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
>> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
>>
>>>>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
>>> On 22/08/17 14:48, Jan Beulich wrote:
>>>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>>>> Currently Linux has no support for grant v2 as this would reduce the
>>>>> maximum number of active grants by a factor of 2 compared to v1,
>>>>> because the number of possible grants are limited by the allowed
>> number
>>>>> of grant frames and grant entries of v2 need twice as much bytes as
>>>>> those of v1.
>>>>>
>>>>> Unfortunately grant v2 is the only way to support either guests with
>>>>> more than 16TB memory size or PV guests with memory above the 16TB
>>>>> border, as grant v1 limits the frame number to be 32 bits wide.
>>>>>
>>>>> In order to remove the disadvantage of grant v2 this patch series
>>>>> enables configuring different maximum grant frame numbers for v1 and
>>>>> v2.
>>>>
>>>> But that does imply higher memory footprint of such a guest in Xen,
>>>> doesn't it?
>>>
>>> With current defaults this would need up to 128kB more for a guest using
>>> v2 grants.
>>
>> At least in an auto-ballooned setup this may make the difference
>> between a guest being able or failing to start.
>>
>>>> The limit, after all, is there to bound resource use of
>>>> DomU-s.  I wonder whether we shouldn't make any such increase
>>>> dependent on first putting in place proper accounting of the memory
>>>> used for individual domains.
>>>
>>> So you would want to have a way to count pages (or bytes?) allocated for
>>> hypervisor internal needs on a per-domain basis, right?
>>>
>>> Would that be additional to struct domain -> xenheap_pages or would you
>>> want to merge the new counter into it? I guess a new field would be
>>> required in order to avoid counting some data twice.
>>>
>>> Do you have an idea what to do with that value? Do you want to expose it
>>> to the user (dom0 admin), or should it be used just inside the
>>> hypervisor and e.g. printed by a debug key handler?
>>>
>>> Do you want an additional set of allocating functions doing the
>>> accounting, or should the existing functions be used with an additional
>>> domain pointer, or should the caller be responsible doing the additional
>>> accounting?
>>>
>>> Do you want an all-or-nothing approach or a gradual move to add the new
>>> accounting step by step?
>>
>> We've been vaguely discussing this in the past on a few occasions.
>> My personal thinking is that the "memory=" setting in a guest config
>> really ought to express all the memory associated with a guest. But
>> of course there'll be problems with us starting to do so, and that's
>> beyond people observing less memory in their guests. Switching to
>> such a full accounting model will require some careful thought (and
>> discussion up front). Hence I've only said "I wonder whether", i.e.
>> I don't mean to make this a strict prerequisite to the proposed
>> changes here. I'd be in particular interested to hear opinions of a
>> few other people.
>>
> 
> Making a the number of grant frames a per-vm-configurable quantity would seem like a reasonable first step. I'm not convinced of the need for separate v1 and v2 limits if this were the case.

Really? I don't think so. I believe the default should be to allow the
same number of grants regardless whether they are v1 or v2. Having to
modify the guest config to achieve this isn't good practice IMO.


Juergen

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

Re: [PATCH 0/5] xen: better grant v2 support

[Paul Durrant]

> -----Original Message-----
> From: Juergen Gross [mailto:jgross@suse.com]
> Sent: 23 August 2017 10:23
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
> 
> On 23/08/17 10:47, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Jan
> >> Beulich
> >> Sent: 23 August 2017 09:36
> >> To: Juergen Gross <jgross@suse.com>
> >> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
> >> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
> >>
> >>>>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
> >>> On 22/08/17 14:48, Jan Beulich wrote:
> >>>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
> >>>>> Currently Linux has no support for grant v2 as this would reduce the
> >>>>> maximum number of active grants by a factor of 2 compared to v1,
> >>>>> because the number of possible grants are limited by the allowed
> >> number
> >>>>> of grant frames and grant entries of v2 need twice as much bytes as
> >>>>> those of v1.
> >>>>>
> >>>>> Unfortunately grant v2 is the only way to support either guests with
> >>>>> more than 16TB memory size or PV guests with memory above the
> 16TB
> >>>>> border, as grant v1 limits the frame number to be 32 bits wide.
> >>>>>
> >>>>> In order to remove the disadvantage of grant v2 this patch series
> >>>>> enables configuring different maximum grant frame numbers for v1
> and
> >>>>> v2.
> >>>>
> >>>> But that does imply higher memory footprint of such a guest in Xen,
> >>>> doesn't it?
> >>>
> >>> With current defaults this would need up to 128kB more for a guest
> using
> >>> v2 grants.
> >>
> >> At least in an auto-ballooned setup this may make the difference
> >> between a guest being able or failing to start.
> >>
> >>>> The limit, after all, is there to bound resource use of
> >>>> DomU-s.  I wonder whether we shouldn't make any such increase
> >>>> dependent on first putting in place proper accounting of the memory
> >>>> used for individual domains.
> >>>
> >>> So you would want to have a way to count pages (or bytes?) allocated
> for
> >>> hypervisor internal needs on a per-domain basis, right?
> >>>
> >>> Would that be additional to struct domain -> xenheap_pages or would
> you
> >>> want to merge the new counter into it? I guess a new field would be
> >>> required in order to avoid counting some data twice.
> >>>
> >>> Do you have an idea what to do with that value? Do you want to expose
> it
> >>> to the user (dom0 admin), or should it be used just inside the
> >>> hypervisor and e.g. printed by a debug key handler?
> >>>
> >>> Do you want an additional set of allocating functions doing the
> >>> accounting, or should the existing functions be used with an additional
> >>> domain pointer, or should the caller be responsible doing the additional
> >>> accounting?
> >>>
> >>> Do you want an all-or-nothing approach or a gradual move to add the
> new
> >>> accounting step by step?
> >>
> >> We've been vaguely discussing this in the past on a few occasions.
> >> My personal thinking is that the "memory=" setting in a guest config
> >> really ought to express all the memory associated with a guest. But
> >> of course there'll be problems with us starting to do so, and that's
> >> beyond people observing less memory in their guests. Switching to
> >> such a full accounting model will require some careful thought (and
> >> discussion up front). Hence I've only said "I wonder whether", i.e.
> >> I don't mean to make this a strict prerequisite to the proposed
> >> changes here. I'd be in particular interested to hear opinions of a
> >> few other people.
> >>
> >
> > Making a the number of grant frames a per-vm-configurable quantity
> would seem like a reasonable first step. I'm not convinced of the need for
> separate v1 and v2 limits if this were the case.
> 
> Really? I don't think so. I believe the default should be to allow the
> same number of grants regardless whether they are v1 or v2. Having to
> modify the guest config to achieve this isn't good practice IMO.
>

In that case I guess we should have per-vm config for the number of grants that the guest is allowed (since the admin should not really have to know about then nature of v1 or v2) and we'd always need to reserve sufficient pages to cover v2 even if v1 is being used (unless we also have a way to disallow use of v2 on a per-guest basis).

  Paul
 
> 
> Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 23/08/17 11:28, Paul Durrant wrote:
>> -----Original Message-----
>> From: Juergen Gross [mailto:jgross@suse.com]
>> Sent: 23 August 2017 10:23
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
>> <JBeulich@suse.com>
>> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
>> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
>>
>> On 23/08/17 10:47, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Jan
>>>> Beulich
>>>> Sent: 23 August 2017 09:36
>>>> To: Juergen Gross <jgross@suse.com>
>>>> Cc: Tim (Xen.org) <tim@xen.org>; 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>; xen-devel@lists.xen.org
>>>> Subject: Re: [Xen-devel] [PATCH 0/5] xen: better grant v2 support
>>>>
>>>>>>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
>>>>> On 22/08/17 14:48, Jan Beulich wrote:
>>>>>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>>>>>> Currently Linux has no support for grant v2 as this would reduce the
>>>>>>> maximum number of active grants by a factor of 2 compared to v1,
>>>>>>> because the number of possible grants are limited by the allowed
>>>> number
>>>>>>> of grant frames and grant entries of v2 need twice as much bytes as
>>>>>>> those of v1.
>>>>>>>
>>>>>>> Unfortunately grant v2 is the only way to support either guests with
>>>>>>> more than 16TB memory size or PV guests with memory above the
>> 16TB
>>>>>>> border, as grant v1 limits the frame number to be 32 bits wide.
>>>>>>>
>>>>>>> In order to remove the disadvantage of grant v2 this patch series
>>>>>>> enables configuring different maximum grant frame numbers for v1
>> and
>>>>>>> v2.
>>>>>>
>>>>>> But that does imply higher memory footprint of such a guest in Xen,
>>>>>> doesn't it?
>>>>>
>>>>> With current defaults this would need up to 128kB more for a guest
>> using
>>>>> v2 grants.
>>>>
>>>> At least in an auto-ballooned setup this may make the difference
>>>> between a guest being able or failing to start.
>>>>
>>>>>> The limit, after all, is there to bound resource use of
>>>>>> DomU-s.  I wonder whether we shouldn't make any such increase
>>>>>> dependent on first putting in place proper accounting of the memory
>>>>>> used for individual domains.
>>>>>
>>>>> So you would want to have a way to count pages (or bytes?) allocated
>> for
>>>>> hypervisor internal needs on a per-domain basis, right?
>>>>>
>>>>> Would that be additional to struct domain -> xenheap_pages or would
>> you
>>>>> want to merge the new counter into it? I guess a new field would be
>>>>> required in order to avoid counting some data twice.
>>>>>
>>>>> Do you have an idea what to do with that value? Do you want to expose
>> it
>>>>> to the user (dom0 admin), or should it be used just inside the
>>>>> hypervisor and e.g. printed by a debug key handler?
>>>>>
>>>>> Do you want an additional set of allocating functions doing the
>>>>> accounting, or should the existing functions be used with an additional
>>>>> domain pointer, or should the caller be responsible doing the additional
>>>>> accounting?
>>>>>
>>>>> Do you want an all-or-nothing approach or a gradual move to add the
>> new
>>>>> accounting step by step?
>>>>
>>>> We've been vaguely discussing this in the past on a few occasions.
>>>> My personal thinking is that the "memory=" setting in a guest config
>>>> really ought to express all the memory associated with a guest. But
>>>> of course there'll be problems with us starting to do so, and that's
>>>> beyond people observing less memory in their guests. Switching to
>>>> such a full accounting model will require some careful thought (and
>>>> discussion up front). Hence I've only said "I wonder whether", i.e.
>>>> I don't mean to make this a strict prerequisite to the proposed
>>>> changes here. I'd be in particular interested to hear opinions of a
>>>> few other people.
>>>>
>>>
>>> Making a the number of grant frames a per-vm-configurable quantity
>> would seem like a reasonable first step. I'm not convinced of the need for
>> separate v1 and v2 limits if this were the case.
>>
>> Really? I don't think so. I believe the default should be to allow the
>> same number of grants regardless whether they are v1 or v2. Having to
>> modify the guest config to achieve this isn't good practice IMO.
>>
> 
> In that case I guess we should have per-vm config for the number of grants that the guest is allowed (since the admin should not really have to know about then nature of v1 or v2) and we'd always need to reserve sufficient pages to cover v2 even if v1 is being used (unless we also have a way to disallow use of v2 on a per-guest basis).

Its not about reserving pages or disallowing v2 IMO.

A guest needs to know whether it can use v2 without having disadvantages
due to that usage. With hosts having more than 16TB of memory v2 isn't
just a nice to have, but a mandatory feature to be able to run above the
16TB limit as a PV guest.

And I think this should be possible without any special command line
settings or guest config.


Juergen

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

Re: [PATCH 0/5] xen: better grant v2 support

[Jan Beulich]

>>> On 23.08.17 at 11:23, <jgross@suse.com> wrote:
> On 23/08/17 10:47, Paul Durrant wrote:
>> Making a the number of grant frames a per-vm-configurable quantity would 
> seem like a reasonable first step. I'm not convinced of the need for separate 
> v1 and v2 limits if this were the case.
> 
> Really? I don't think so. I believe the default should be to allow the
> same number of grants regardless whether they are v1 or v2. Having to
> modify the guest config to achieve this isn't good practice IMO.

I'm not convinced one is significantly better than the other. Both
have their upsides and downsides.

Jan


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

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 23/08/17 11:43, Jan Beulich wrote:
>>>> On 23.08.17 at 11:23, <jgross@suse.com> wrote:
>> On 23/08/17 10:47, Paul Durrant wrote:
>>> Making a the number of grant frames a per-vm-configurable quantity would 
>> seem like a reasonable first step. I'm not convinced of the need for separate 
>> v1 and v2 limits if this were the case.
>>
>> Really? I don't think so. I believe the default should be to allow the
>> same number of grants regardless whether they are v1 or v2. Having to
>> modify the guest config to achieve this isn't good practice IMO.
> 
> I'm not convinced one is significantly better than the other. Both
> have their upsides and downsides.

Again: the whole series is meant to not make a guest having to choose
whether it can either use the amount of grants it is needing or it can
run on a host with more than 16TB of memory.

And I believe the default should be not having to use special command
line parameters or guest config settings to achieve this.


Juergen

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

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 23/08/17 10:36, Jan Beulich wrote:
>>>> On 23.08.17 at 09:49, <jgross@suse.com> wrote:
>> On 22/08/17 14:48, Jan Beulich wrote:
>>>>>> On 21.08.17 at 20:05, <jgross@suse.com> wrote:
>>>> Currently Linux has no support for grant v2 as this would reduce the
>>>> maximum number of active grants by a factor of 2 compared to v1,
>>>> because the number of possible grants are limited by the allowed number
>>>> of grant frames and grant entries of v2 need twice as much bytes as
>>>> those of v1.
>>>>
>>>> Unfortunately grant v2 is the only way to support either guests with
>>>> more than 16TB memory size or PV guests with memory above the 16TB
>>>> border, as grant v1 limits the frame number to be 32 bits wide.
>>>>
>>>> In order to remove the disadvantage of grant v2 this patch series
>>>> enables configuring different maximum grant frame numbers for v1 and
>>>> v2.
>>>
>>> But that does imply higher memory footprint of such a guest in Xen,
>>> doesn't it?
>>
>> With current defaults this would need up to 128kB more for a guest using
>> v2 grants.
> 
> At least in an auto-ballooned setup this may make the difference
> between a guest being able or failing to start.
> 
>>> The limit, after all, is there to bound resource use of
>>> DomU-s.  I wonder whether we shouldn't make any such increase
>>> dependent on first putting in place proper accounting of the memory
>>> used for individual domains.
>>
>> So you would want to have a way to count pages (or bytes?) allocated for
>> hypervisor internal needs on a per-domain basis, right?
>>
>> Would that be additional to struct domain -> xenheap_pages or would you
>> want to merge the new counter into it? I guess a new field would be
>> required in order to avoid counting some data twice.
>>
>> Do you have an idea what to do with that value? Do you want to expose it
>> to the user (dom0 admin), or should it be used just inside the
>> hypervisor and e.g. printed by a debug key handler?
>>
>> Do you want an additional set of allocating functions doing the
>> accounting, or should the existing functions be used with an additional
>> domain pointer, or should the caller be responsible doing the additional
>> accounting?
>>
>> Do you want an all-or-nothing approach or a gradual move to add the new
>> accounting step by step?
> 
> We've been vaguely discussing this in the past on a few occasions.
> My personal thinking is that the "memory=" setting in a guest config
> really ought to express all the memory associated with a guest. But
> of course there'll be problems with us starting to do so, and that's
> beyond people observing less memory in their guests. Switching to
> such a full accounting model will require some careful thought (and
> discussion up front). Hence I've only said "I wonder whether", i.e.
> I don't mean to make this a strict prerequisite to the proposed
> changes here. I'd be in particular interested to hear opinions of a
> few other people.

Fair enough.

Just some thoughts on that topic from my side:

This approach should be fine IMO for memory allocated while creating a
domain. This is basically the same as a bare metal system where the BIOS
needs some memory.

Memory allocated during the lifetime of a domain is a different problem:
either you allow the domain to claim more memory as configured, or you
have to reserve a more or less arbitrary amount of memory to be
allocated later and eventually fail such allocations. I don't think this
approach is acceptable.


Juergen

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

Re: [PATCH 0/5] xen: better grant v2 support

[Jan Beulich]

>>> On 23.08.17 at 11:19, <jgross@suse.com> wrote:
> Just some thoughts on that topic from my side:
> 
> This approach should be fine IMO for memory allocated while creating a
> domain. This is basically the same as a bare metal system where the BIOS
> needs some memory.
> 
> Memory allocated during the lifetime of a domain is a different problem:
> either you allow the domain to claim more memory as configured, or you
> have to reserve a more or less arbitrary amount of memory to be
> allocated later and eventually fail such allocations. I don't think this
> approach is acceptable.

Or an interrupt gets sent to the guest telling it that it should
nominate (balloon out or something similar) a page in order for
another of its operations to succeed. Of course that would be
limited to paravirtual operations (i.e. mostly hypercalls);
allocations done while emulating something could not be
handled this way.

Jan


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

Re: [PATCH 0/5] xen: better grant v2 support

[Juergen Gross]

On 23/08/17 11:41, Jan Beulich wrote:
>>>> On 23.08.17 at 11:19, <jgross@suse.com> wrote:
>> Just some thoughts on that topic from my side:
>>
>> This approach should be fine IMO for memory allocated while creating a
>> domain. This is basically the same as a bare metal system where the BIOS
>> needs some memory.
>>
>> Memory allocated during the lifetime of a domain is a different problem:
>> either you allow the domain to claim more memory as configured, or you
>> have to reserve a more or less arbitrary amount of memory to be
>> allocated later and eventually fail such allocations. I don't think this
>> approach is acceptable.
> 
> Or an interrupt gets sent to the guest telling it that it should
> nominate (balloon out or something similar) a page in order for
> another of its operations to succeed. Of course that would be
> limited to paravirtual operations (i.e. mostly hypercalls);
> allocations done while emulating something could not be
> handled this way.

As this would probably break existing pv guests I don't think this
is an option we should consider.


Juergen

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