* [PATCH v8 01/15] xen: move XENMAPSPACE_grant_table code into grant_table.c
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 12:40 ` Julien Grall
2017-09-20 6:34 ` [PATCH v8 02/15] xen: clean up grant_table.h Juergen Gross
` (13 subsequent siblings)
14 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
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.
Switch to mfn_t in order to be more type safe.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
V8:
- make gnttab_grow_table() static instead doing so in next patch
(Jan Beulich)
V7:
- only call gnttab_set_frame_gfn() if no error (Julien Grall)
V6:
- test rc of gnttab_map_frame() (Jan Beulich)
V3:
- update commit message
V2:
- rebased to staging
---
xen/arch/arm/mm.c | 36 ++++----------------------------
xen/arch/x86/mm.c | 43 +++++++++++----------------------------
xen/common/grant_table.c | 41 ++++++++++++++++++++++++++++++++++++-
xen/include/asm-arm/grant_table.h | 7 +++++++
xen/include/asm-x86/grant_table.h | 5 +++++
xen/include/xen/grant_table.h | 9 +++-----
6 files changed, 71 insertions(+), 70 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 5716ef1123..e68facb0f4 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1233,39 +1233,11 @@ int xenmem_add_to_physmap_one(
switch ( space )
{
case XENMAPSPACE_grant_table:
- grant_write_lock(d->grant_table);
-
- if ( d->grant_table->gt_version == 0 )
- d->grant_table->gt_version = 1;
-
- if ( d->grant_table->gt_version == 2 &&
- (idx & XENMAPIDX_grant_table_status) )
- {
- idx &= ~XENMAPIDX_grant_table_status;
- if ( idx < nr_status_frames(d->grant_table) )
- mfn = virt_to_mfn(d->grant_table->status[idx]);
- }
- else
- {
- if ( (idx >= nr_grant_frames(d->grant_table)) &&
- (idx < max_grant_frames) )
- gnttab_grow_table(d, idx + 1);
-
- if ( idx < nr_grant_frames(d->grant_table) )
- mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
- }
-
- if ( !mfn_eq(mfn, INVALID_MFN) )
- {
- d->arch.grant_table_gfn[idx] = gfn;
-
- t = p2m_ram_rw;
- }
-
- grant_write_unlock(d->grant_table);
+ rc = gnttab_map_frame(d, idx, gfn, &mfn);
+ if ( rc )
+ return rc;
- if ( mfn_eq(mfn, INVALID_MFN) )
- return -EINVAL;
+ t = p2m_ram_rw;
break;
case XENMAPSPACE_shared_info:
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2abec67f6a..67f583e3a7 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4549,40 +4549,21 @@ 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);
+ rc = gnttab_map_frame(d, idx, gpfn, &mfn);
+ if ( rc )
+ return rc;
break;
case XENMAPSPACE_gmfn_range:
case XENMAPSPACE_gmfn:
@@ -4599,8 +4580,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:
@@ -4609,7 +4590,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;
@@ -4633,16 +4614,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 00ff075bd9..1b18e5f9cc 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1580,7 +1580,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;
@@ -3608,6 +3608,45 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
}
#endif
+int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
+ mfn_t *mfn)
+{
+ int rc = 0;
+ struct grant_table *gt = d->grant_table;
+
+ grant_write_lock(gt);
+
+ if ( gt->gt_version == 0 )
+ gt->gt_version = 1;
+
+ if ( gt->gt_version == 2 &&
+ (idx & XENMAPIDX_grant_table_status) )
+ {
+ idx &= ~XENMAPIDX_grant_table_status;
+ if ( idx < nr_status_frames(gt) )
+ *mfn = _mfn(virt_to_mfn(gt->status[idx]));
+ else
+ rc = -EINVAL;
+ }
+ else
+ {
+ if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
+ gnttab_grow_table(d, idx + 1);
+
+ if ( idx < nr_grant_frames(gt) )
+ *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
+ else
+ rc = -EINVAL;
+ }
+
+ if ( !rc )
+ gnttab_set_frame_gfn(d, idx, gfn);
+
+ grant_write_unlock(gt);
+
+ return rc;
+}
+
static void gnttab_usage_print(struct domain *rd)
{
int first = 1;
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index bc4d61a940..0a248a765a 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -2,6 +2,7 @@
#define __ASM_GRANT_TABLE_H__
#include <xen/grant_table.h>
+#include <xen/sched.h>
#define INITIAL_NR_GRANT_FRAMES 4
@@ -21,6 +22,12 @@ static inline int replace_grant_supported(void)
return 1;
}
+static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long idx,
+ gfn_t gfn)
+{
+ d->arch.grant_table_gfn[idx] = gfn;
+}
+
#define gnttab_create_shared_page(d, t, i) \
do { \
share_xen_page_with_guest( \
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 33b2f88b96..c865999a33 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -75,6 +75,11 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st)
asm volatile ("lock btrw %w1,%0" : "=m" (*st) : "Ir" (nr), "m" (*st));
}
+static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long idx,
+ gfn_t gfn)
+{
+}
+
/* Foreign mappings of HHVM-guest pages do not modify the type count. */
#define gnttab_host_mapping_get_page_type(ro, ld, rd) \
(!(ro) && (((ld) == (rd)) || !paging_mode_external(rd)))
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index af269a108d..fc058abfec 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -106,12 +106,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)
{
@@ -136,4 +130,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
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 01/15] xen: move XENMAPSPACE_grant_table code into grant_table.c
2017-09-20 6:34 ` [PATCH v8 01/15] xen: move XENMAPSPACE_grant_table code into grant_table.c Juergen Gross
@ 2017-09-20 12:40 ` Julien Grall
0 siblings, 0 replies; 69+ messages in thread
From: Julien Grall @ 2017-09-20 12:40 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 07:34, 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.
>
> Switch to mfn_t in order to be more type safe.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 02/15] xen: clean up grant_table.h
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
2017-09-20 6:34 ` [PATCH v8 01/15] xen: move XENMAPSPACE_grant_table code into grant_table.c Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits Juergen Gross
` (12 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
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.
Rearrange the elements of struct grant_table to minimize holes due to
alignment of elements.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
V6:
- coding style (Jan Beulich)
- rearrange struct grant_table to minimize holes (Jan Beulich)
---
xen/common/grant_table.c | 81 ++++++++++++++++++++++++++++++++++++++++++-
xen/include/xen/grant_table.h | 78 -----------------------------------------
2 files changed, 80 insertions(+), 79 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 1b18e5f9cc..ac845dbb35 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -40,6 +40,45 @@
#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;
+ /* 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 int gt_version;
+ /* Table size. Number of frames shared with guest */
+ unsigned int nr_grant_frames;
+ /* Number of grant status frames shared with guest (for version 2) */
+ unsigned int nr_status_frames;
+ /* Number of available maptrack entries. */
+ unsigned int maptrack_limit;
+ /* 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;
+ };
+ /* 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;
+};
+
+#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 +157,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(const 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(const 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 +248,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, >->lock);
+}
+
+static inline void grant_read_unlock(struct grant_table *gt)
+{
+ percpu_read_unlock(grant_rwlock, >->lock);
+}
+
+static inline void grant_write_lock(struct grant_table *gt)
+{
+ percpu_write_lock(grant_rwlock, >->lock);
+}
+
+static inline void grant_write_unlock(struct grant_table *gt)
+{
+ percpu_write_unlock(grant_rwlock, >->lock);
+}
static inline void gnttab_flush_tlb(const struct domain *d)
{
@@ -250,6 +321,14 @@ 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(unsigned int grant_frames)
+{
+ return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE, 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).
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index fc058abfec..43b07e60c5 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -29,66 +29,9 @@
#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, >->lock);
-}
-
-static inline void grant_read_unlock(struct grant_table *gt)
-{
- percpu_read_unlock(grant_rwlock, >->lock);
-}
-
-static inline void grant_write_lock(struct grant_table *gt)
-{
- percpu_write_lock(grant_rwlock, >->lock);
-}
-
-static inline void grant_write_unlock(struct grant_table *gt)
-{
- percpu_write_unlock(grant_rwlock, >->lock);
-}
-
/* Create/destroy per-domain grant table context. */
int grant_table_create(
struct domain *d);
@@ -106,27 +49,6 @@ void
gnttab_release_mappings(
struct domain *d);
-/* 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);
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
2017-09-20 6:34 ` [PATCH v8 01/15] xen: move XENMAPSPACE_grant_table code into grant_table.c Juergen Gross
2017-09-20 6:34 ` [PATCH v8 02/15] xen: clean up grant_table.h Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 8:26 ` Paul Durrant
2017-09-20 6:34 ` [PATCH v8 04/15] xen: add function for obtaining highest possible memory address Juergen Gross
` (11 subsequent siblings)
14 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add a domctl hypercall to set the domain's resource limits regarding
grant tables.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
V8:
- remove stale #if 0, adjust comments (Paul Durrant)
V6:
- moved earlier in series to support set_gnttab_limits being
mandatory for domain creation
V5:
- add set_gnttab_limits to create_domain_common in xen.if
(Daniel De Graaf)
V3:
- rename *gnttbl* to *gnttab* (Paul Durrant)
---
tools/flask/policy/modules/dom0.te | 2 +-
tools/flask/policy/modules/xen.if | 2 +-
xen/common/domctl.c | 6 ++++++
xen/common/grant_table.c | 19 +++++++++++++++++++
xen/include/public/domctl.h | 7 +++++++
xen/include/xen/grant_table.h | 2 ++
xen/xsm/flask/hooks.c | 3 +++
xen/xsm/flask/policy/access_vectors | 2 ++
8 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 338caaf41e..1643b400f0 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -39,7 +39,7 @@ allow dom0_t dom0_t:domain {
};
allow dom0_t dom0_t:domain2 {
set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
- get_vnumainfo psr_cmt_op psr_cat_op
+ get_vnumainfo psr_cmt_op psr_cat_op set_gnttab_limits
};
allow dom0_t dom0_t:resource { add remove };
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 912640002e..55437496f6 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -52,7 +52,7 @@ define(`create_domain_common', `
settime setdomainhandle getvcpucontext set_misc_info };
allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
set_max_evtchn set_vnumainfo get_vnumainfo cacheflush
- psr_cmt_op psr_cat_op soft_reset };
+ psr_cmt_op psr_cat_op soft_reset set_gnttab_limits };
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 42658e5744..58381f8fe9 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -14,6 +14,7 @@
#include <xen/sched-if.h>
#include <xen/domain.h>
#include <xen/event.h>
+#include <xen/grant_table.h>
#include <xen/domain_page.h>
#include <xen/trace.h>
#include <xen/console.h>
@@ -1149,6 +1150,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
copyback = 1;
break;
+ case XEN_DOMCTL_set_gnttab_limits:
+ ret = grant_table_set_limits(d, op->u.set_gnttab_limits.grant_frames,
+ op->u.set_gnttab_limits.maptrack_frames);
+ break;
+
default:
ret = arch_do_domctl(op, d, u_domctl);
break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ac845dbb35..f48eeff7ad 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3640,6 +3640,25 @@ void grant_table_init_vcpu(struct vcpu *v)
v->maptrack_tail = MAPTRACK_TAIL;
}
+int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
+ unsigned int maptrack_frames)
+{
+ struct grant_table *gt = d->grant_table;
+ int ret = -EBUSY;
+
+ if ( !gt )
+ return -ENOENT;
+
+ grant_write_lock(gt);
+
+ ret = 0;
+ /* Set limits, alloc needed arrays. */
+
+ grant_write_unlock(gt);
+
+ return ret;
+}
+
#ifdef CONFIG_HAS_MEM_SHARING
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/public/domctl.h b/xen/include/public/domctl.h
index 50ff58f5b9..167502c60b 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1163,6 +1163,11 @@ struct xen_domctl_psr_cat_op {
typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
+struct xen_domctl_set_gnttab_limits {
+ uint32_t grant_frames; /* IN */
+ uint32_t maptrack_frames; /* IN */
+};
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1240,6 +1245,7 @@ struct xen_domctl {
#define XEN_DOMCTL_monitor_op 77
#define XEN_DOMCTL_psr_cat_op 78
#define XEN_DOMCTL_soft_reset 79
+#define XEN_DOMCTL_set_gnttab_limits 80
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1302,6 +1308,7 @@ struct xen_domctl {
struct xen_domctl_psr_cmt_op psr_cmt_op;
struct xen_domctl_monitor_op monitor_op;
struct xen_domctl_psr_cat_op psr_cat_op;
+ struct xen_domctl_set_gnttab_limits set_gnttab_limits;
uint8_t pad[128];
} u;
};
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 43b07e60c5..df11b31264 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -38,6 +38,8 @@ int grant_table_create(
void grant_table_destroy(
struct domain *d);
void grant_table_init_vcpu(struct vcpu *v);
+int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
+ unsigned int maptrack_frames);
/*
* Check if domain has active grants and log first 10 of them.
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 56dc5b0ab9..7b005af834 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -749,6 +749,9 @@ static int flask_domctl(struct domain *d, int cmd)
case XEN_DOMCTL_soft_reset:
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET);
+ case XEN_DOMCTL_set_gnttab_limits:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_GNTTAB_LIMITS);
+
default:
return avc_unknown_permission("domctl", cmd);
}
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index da9f3dfb2e..3a2d863b8f 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -248,6 +248,8 @@ class domain2
mem_sharing
# XEN_DOMCTL_psr_cat_op
psr_cat_op
+# XEN_DOMCTL_set_gnttab_limits
+ set_gnttab_limits
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits
2017-09-20 6:34 ` [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits Juergen Gross
@ 2017-09-20 8:26 ` Paul Durrant
2017-09-20 8:49 ` Jan Beulich
0 siblings, 1 reply; 69+ messages in thread
From: Paul Durrant @ 2017-09-20 8:26 UTC (permalink / raw)
To: 'Juergen Gross', xen-devel
Cc: sstabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
George Dunlap, julien.grall, jbeulich, Ian Jackson, dgdegra
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 20 September 2017 07:34
> To: xen-devel@lists.xenproject.org
> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> julien.grall@arm.com; jbeulich@suse.com; dgdegra@tycho.nsa.gov
> Subject: [Xen-devel] [PATCH v8 03/15] xen: add new domctl hypercall to set
> grant table resource limits
>
> Add a domctl hypercall to set the domain's resource limits regarding
> grant tables.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> V8:
> - remove stale #if 0, adjust comments (Paul Durrant)
>
> V6:
> - moved earlier in series to support set_gnttab_limits being
> mandatory for domain creation
>
> V5:
> - add set_gnttab_limits to create_domain_common in xen.if
> (Daniel De Graaf)
>
> V3:
> - rename *gnttbl* to *gnttab* (Paul Durrant)
> ---
> tools/flask/policy/modules/dom0.te | 2 +-
> tools/flask/policy/modules/xen.if | 2 +-
> xen/common/domctl.c | 6 ++++++
> xen/common/grant_table.c | 19 +++++++++++++++++++
> xen/include/public/domctl.h | 7 +++++++
> xen/include/xen/grant_table.h | 2 ++
> xen/xsm/flask/hooks.c | 3 +++
> xen/xsm/flask/policy/access_vectors | 2 ++
> 8 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/tools/flask/policy/modules/dom0.te
> b/tools/flask/policy/modules/dom0.te
> index 338caaf41e..1643b400f0 100644
> --- a/tools/flask/policy/modules/dom0.te
> +++ b/tools/flask/policy/modules/dom0.te
> @@ -39,7 +39,7 @@ allow dom0_t dom0_t:domain {
> };
> allow dom0_t dom0_t:domain2 {
> set_cpuid gettsc settsc setscheduler set_max_evtchn
> set_vnumainfo
> - get_vnumainfo psr_cmt_op psr_cat_op
> + get_vnumainfo psr_cmt_op psr_cat_op set_gnttab_limits
> };
> allow dom0_t dom0_t:resource { add remove };
>
> diff --git a/tools/flask/policy/modules/xen.if
> b/tools/flask/policy/modules/xen.if
> index 912640002e..55437496f6 100644
> --- a/tools/flask/policy/modules/xen.if
> +++ b/tools/flask/policy/modules/xen.if
> @@ -52,7 +52,7 @@ define(`create_domain_common', `
> settime setdomainhandle getvcpucontext
> set_misc_info };
> allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
> set_max_evtchn set_vnumainfo get_vnumainfo
> cacheflush
> - psr_cmt_op psr_cat_op soft_reset };
> + psr_cmt_op psr_cat_op soft_reset set_gnttab_limits
> };
> allow $1 $2:security check_context;
> allow $1 $2:shadow enable;
> allow $1 $2:mmu { map_read map_write adjust memorymap
> physmap pinpage mmuext_op updatemp };
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 42658e5744..58381f8fe9 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -14,6 +14,7 @@
> #include <xen/sched-if.h>
> #include <xen/domain.h>
> #include <xen/event.h>
> +#include <xen/grant_table.h>
> #include <xen/domain_page.h>
> #include <xen/trace.h>
> #include <xen/console.h>
> @@ -1149,6 +1150,11 @@ long
> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> copyback = 1;
> break;
>
> + case XEN_DOMCTL_set_gnttab_limits:
> + ret = grant_table_set_limits(d, op->u.set_gnttab_limits.grant_frames,
> + op->u.set_gnttab_limits.maptrack_frames);
> + break;
> +
> default:
> ret = arch_do_domctl(op, d, u_domctl);
> break;
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index ac845dbb35..f48eeff7ad 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3640,6 +3640,25 @@ void grant_table_init_vcpu(struct vcpu *v)
> v->maptrack_tail = MAPTRACK_TAIL;
> }
>
> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
> + unsigned int maptrack_frames)
> +{
> + struct grant_table *gt = d->grant_table;
> + int ret = -EBUSY;
> +
> + if ( !gt )
> + return -ENOENT;
> +
> + grant_write_lock(gt);
> +
> + ret = 0;
> + /* Set limits, alloc needed arrays. */
> +
> + grant_write_unlock(gt);
> +
> + return ret;
> +}
> +
> #ifdef CONFIG_HAS_MEM_SHARING
> 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/public/domctl.h b/xen/include/public/domctl.h
> index 50ff58f5b9..167502c60b 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1163,6 +1163,11 @@ struct xen_domctl_psr_cat_op {
> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>
> +struct xen_domctl_set_gnttab_limits {
> + uint32_t grant_frames; /* IN */
> + uint32_t maptrack_frames; /* IN */
> +};
> +
> struct xen_domctl {
> uint32_t cmd;
> #define XEN_DOMCTL_createdomain 1
> @@ -1240,6 +1245,7 @@ struct xen_domctl {
> #define XEN_DOMCTL_monitor_op 77
> #define XEN_DOMCTL_psr_cat_op 78
> #define XEN_DOMCTL_soft_reset 79
> +#define XEN_DOMCTL_set_gnttab_limits 80
> #define XEN_DOMCTL_gdbsx_guestmemio 1000
> #define XEN_DOMCTL_gdbsx_pausevcpu 1001
> #define XEN_DOMCTL_gdbsx_unpausevcpu 1002
> @@ -1302,6 +1308,7 @@ struct xen_domctl {
> struct xen_domctl_psr_cmt_op psr_cmt_op;
> struct xen_domctl_monitor_op monitor_op;
> struct xen_domctl_psr_cat_op psr_cat_op;
> + struct xen_domctl_set_gnttab_limits set_gnttab_limits;
> uint8_t pad[128];
> } u;
> };
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 43b07e60c5..df11b31264 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -38,6 +38,8 @@ int grant_table_create(
> void grant_table_destroy(
> struct domain *d);
> void grant_table_init_vcpu(struct vcpu *v);
> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
> + unsigned int maptrack_frames);
>
> /*
> * Check if domain has active grants and log first 10 of them.
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 56dc5b0ab9..7b005af834 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -749,6 +749,9 @@ static int flask_domctl(struct domain *d, int cmd)
> case XEN_DOMCTL_soft_reset:
> return current_has_perm(d, SECCLASS_DOMAIN2,
> DOMAIN2__SOFT_RESET);
>
> + case XEN_DOMCTL_set_gnttab_limits:
> + return current_has_perm(d, SECCLASS_DOMAIN2,
> DOMAIN2__SET_GNTTAB_LIMITS);
> +
> default:
> return avc_unknown_permission("domctl", cmd);
> }
> diff --git a/xen/xsm/flask/policy/access_vectors
> b/xen/xsm/flask/policy/access_vectors
> index da9f3dfb2e..3a2d863b8f 100644
> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -248,6 +248,8 @@ class domain2
> mem_sharing
> # XEN_DOMCTL_psr_cat_op
> psr_cat_op
> +# XEN_DOMCTL_set_gnttab_limits
> + set_gnttab_limits
> }
>
> # Similar to class domain, but primarily contains domctls related to HVM
> domains
> --
> 2.12.3
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits
2017-09-20 8:26 ` Paul Durrant
@ 2017-09-20 8:49 ` Jan Beulich
0 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 8:49 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, Wei Liu, AndrewCooper, Tim (Xen.org),
George Dunlap, julien.grall, Paul Durrant, xen-devel,
Ian Jackson, dgdegra
>>> On 20.09.17 at 10:26, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Juergen Gross
>> Sent: 20 September 2017 07:34
>> To: xen-devel@lists.xenproject.org
>> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu
>> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
>> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>;
>> julien.grall@arm.com; jbeulich@suse.com; dgdegra@tycho.nsa.gov
>> Subject: [Xen-devel] [PATCH v8 03/15] xen: add new domctl hypercall to set
>> grant table resource limits
>>
>> Add a domctl hypercall to set the domain's resource limits regarding
>> grant tables.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
despite ...
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3640,6 +3640,25 @@ void grant_table_init_vcpu(struct vcpu *v)
>> v->maptrack_tail = MAPTRACK_TAIL;
>> }
>>
>> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>> + unsigned int maptrack_frames)
>> +{
>> + struct grant_table *gt = d->grant_table;
>> + int ret = -EBUSY;
... the initializer being pointless here (it becomes relevant only in
patch 11).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (2 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 03/15] xen: add new domctl hypercall to set grant table resource limits Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 8:57 ` Jan Beulich
` (2 more replies)
2017-09-20 6:34 ` [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo Juergen Gross
` (10 subsequent siblings)
14 siblings, 3 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add a function for obtaining the highest possible physical memory
address of the system. This value is influenced by:
- hypervisor configuration (CONFIG_BIGMEM)
- processor capability (max. addressable physical memory)
- memory map at boot time
- memory hotplug capability
The value is especially needed for dom0 to decide sizing of grant frame
limits of guests and for pv domains for selecting the grant interface
version to use.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/mm.c | 11 +++++++++++
xen/common/memory.c | 8 ++++++++
xen/include/asm-arm/mm.h | 5 +++++
xen/include/asm-x86/mm.h | 2 ++
xen/include/xen/mm.h | 3 +++
5 files changed, 29 insertions(+)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 67f583e3a7..31d96a3920 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6312,6 +6312,17 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs)
return 0;
}
+unsigned long arch_get_upper_mfn_bound(void)
+{
+ unsigned long max_mfn;
+
+ max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
+#ifndef CONFIG_BIGMEM
+ max_mfn = min(max_mfn, 1UL << 32);
+#endif
+ return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT));
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/memory.c b/xen/common/memory.c
index a2abf554e3..27e39cf45c 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1494,6 +1494,14 @@ int prepare_ring_for_helper(
return 0;
}
+unsigned long get_upper_mfn_bound(void)
+{
+ unsigned long max_mfn;
+
+ max_mfn = arch_get_upper_mfn_bound();
+ return max(max_mfn, max_page);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index cd6dfb54b9..6aa8cba5e0 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -376,6 +376,11 @@ static inline void put_page_and_type(struct page_info *page)
void clear_and_clean_page(struct page_info *page);
+static inline unsigned long arch_get_upper_mfn_bound(void)
+{
+ return 0;
+}
+
#endif /* __ARCH_ARM_MM__ */
/*
* Local variables:
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index bef45e8e9f..91b62b111e 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -608,4 +608,6 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
return mfn <= (virt_to_mfn(eva - 1) + 1);
}
+unsigned long arch_get_upper_mfn_bound(void);
+
#endif /* __ASM_X86_MM_H__ */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index f8b6177c32..e813c07b22 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -599,6 +599,9 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
struct page_info **_page, void **_va);
void destroy_ring_for_helper(void **_va, struct page_info *page);
+/* Return the upper bound of MFNs, including hotplug memory. */
+unsigned long get_upper_mfn_bound(void);
+
#include <asm/flushtlb.h>
static inline void accumulate_tlbflush(bool *need_tlbflush,
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 6:34 ` [PATCH v8 04/15] xen: add function for obtaining highest possible memory address Juergen Gross
@ 2017-09-20 8:57 ` Jan Beulich
[not found] ` <59C2499A020000780017D412@suse.com>
2017-09-20 12:51 ` Julien Grall
2 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 8:57 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6312,6 +6312,17 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs)
> return 0;
> }
>
> +unsigned long arch_get_upper_mfn_bound(void)
> +{
> + unsigned long max_mfn;
> +
> + max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
Taking into account the code in the caller of this function as well
as the ARM counterpart I find the use of max_page here odd. I'd
prefer if get_upper_mfn_bound() went away altogether, and it's
sole caller (which strangely enough doesn't get introduced here)
called the arch function directly. Additionally, with the caller being
a sysctl, how is that supposed to help a PV DomU kernel in their
choice of grant table version?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C2499A020000780017D412@suse.com>]
* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
[not found] ` <59C2499A020000780017D412@suse.com>
@ 2017-09-20 8:58 ` Juergen Gross
2017-09-20 9:32 ` Jan Beulich
[not found] ` <59C251C8020000780017D4D7@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 8:58 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 10:57, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -6312,6 +6312,17 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs)
>> return 0;
>> }
>>
>> +unsigned long arch_get_upper_mfn_bound(void)
>> +{
>> + unsigned long max_mfn;
>> +
>> + max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
>
> Taking into account the code in the caller of this function as well
> as the ARM counterpart I find the use of max_page here odd. I'd
> prefer if get_upper_mfn_bound() went away altogether, and it's
> sole caller (which strangely enough doesn't get introduced here)
> called the arch function directly. Additionally, with the caller being
> a sysctl, how is that supposed to help a PV DomU kernel in their
> choice of grant table version?
Did you look at patch 15?
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 8:58 ` Juergen Gross
@ 2017-09-20 9:32 ` Jan Beulich
[not found] ` <59C251C8020000780017D4D7@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 9:32 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 10:58, <jgross@suse.com> wrote:
> On 20/09/17 10:57, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -6312,6 +6312,17 @@ int pv_ro_page_fault(unsigned long addr, struct
> cpu_user_regs *regs)
>>> return 0;
>>> }
>>>
>>> +unsigned long arch_get_upper_mfn_bound(void)
>>> +{
>>> + unsigned long max_mfn;
>>> +
>>> + max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
>>
>> Taking into account the code in the caller of this function as well
>> as the ARM counterpart I find the use of max_page here odd. I'd
>> prefer if get_upper_mfn_bound() went away altogether, and it's
>> sole caller (which strangely enough doesn't get introduced here)
>> called the arch function directly. Additionally, with the caller being
>> a sysctl, how is that supposed to help a PV DomU kernel in their
>> choice of grant table version?
>
> Did you look at patch 15?
Not yet, no (I had looked over the titles, but this one's didn't
make me make the connection). So yes, that addresses the PV
DomU concern. Still I'd like to get away without the thin common
wrapper around the arch specific actual implementation (and I
don't care much whether the resulting function has an arch_
prefix).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C251C8020000780017D4D7@suse.com>]
* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
[not found] ` <59C251C8020000780017D4D7@suse.com>
@ 2017-09-20 9:39 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 9:39 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 11:32, Jan Beulich wrote:
>>>> On 20.09.17 at 10:58, <jgross@suse.com> wrote:
>> On 20/09/17 10:57, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -6312,6 +6312,17 @@ int pv_ro_page_fault(unsigned long addr, struct
>> cpu_user_regs *regs)
>>>> return 0;
>>>> }
>>>>
>>>> +unsigned long arch_get_upper_mfn_bound(void)
>>>> +{
>>>> + unsigned long max_mfn;
>>>> +
>>>> + max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
>>>
>>> Taking into account the code in the caller of this function as well
>>> as the ARM counterpart I find the use of max_page here odd. I'd
>>> prefer if get_upper_mfn_bound() went away altogether, and it's
>>> sole caller (which strangely enough doesn't get introduced here)
>>> called the arch function directly. Additionally, with the caller being
>>> a sysctl, how is that supposed to help a PV DomU kernel in their
>>> choice of grant table version?
>>
>> Did you look at patch 15?
>
> Not yet, no (I had looked over the titles, but this one's didn't
> make me make the connection). So yes, that addresses the PV
> DomU concern. Still I'd like to get away without the thin common
> wrapper around the arch specific actual implementation (and I
> don't care much whether the resulting function has an arch_
> prefix).
Okay, I'll remove the common wrapper and drop the arch_ from the x86 and
arm variants.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 6:34 ` [PATCH v8 04/15] xen: add function for obtaining highest possible memory address Juergen Gross
2017-09-20 8:57 ` Jan Beulich
[not found] ` <59C2499A020000780017D412@suse.com>
@ 2017-09-20 12:51 ` Julien Grall
2017-09-20 13:08 ` Juergen Gross
2 siblings, 1 reply; 69+ messages in thread
From: Julien Grall @ 2017-09-20 12:51 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
Sorry for the late comment.
On 20/09/17 07:34, Juergen Gross wrote:
> Add a function for obtaining the highest possible physical memory
> address of the system. This value is influenced by:
>
> - hypervisor configuration (CONFIG_BIGMEM)
> - processor capability (max. addressable physical memory)
> - memory map at boot time
> - memory hotplug capability
>
> The value is especially needed for dom0 to decide sizing of grant frame
> limits of guests and for pv domains for selecting the grant interface
Why limiting to PV domain? Arm domain may also need to switch to another
interface between v1 only support 32-bit GFN.
> version to use.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
[...]
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index cd6dfb54b9..6aa8cba5e0 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct page_info *page)
>
> void clear_and_clean_page(struct page_info *page);
>
> +static inline unsigned long arch_get_upper_mfn_bound(void)
> +{
> + return 0;
> +}
I am not sure to understand the Arm implementation given the description
of the commit message.
The guest layout is completely separate from the host layout. It might
be possible to have all the memory below 40 bits on the host, but this
does not preclude the guest to have all memory below 40 bits (the
hardware might support, for instance, up to 48 bits).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 12:51 ` Julien Grall
@ 2017-09-20 13:08 ` Juergen Gross
2017-09-20 14:24 ` Julien Grall
0 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 13:08 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
On 20/09/17 14:51, Julien Grall wrote:
> Hi Juergen,
>
> Sorry for the late comment.
>
> On 20/09/17 07:34, Juergen Gross wrote:
>> Add a function for obtaining the highest possible physical memory
>> address of the system. This value is influenced by:
>>
>> - hypervisor configuration (CONFIG_BIGMEM)
>> - processor capability (max. addressable physical memory)
>> - memory map at boot time
>> - memory hotplug capability
>>
>> The value is especially needed for dom0 to decide sizing of grant frame
>> limits of guests and for pv domains for selecting the grant interface
>
> Why limiting to PV domain? Arm domain may also need to switch to another
> interface between v1 only support 32-bit GFN.
Right. And I just used that reasoning for an answer to Jan. :-)
>
>> version to use.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>
> [...]
>
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index cd6dfb54b9..6aa8cba5e0 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct
>> page_info *page)
>> void clear_and_clean_page(struct page_info *page);
>> +static inline unsigned long arch_get_upper_mfn_bound(void)
>> +{
>> + return 0;
>> +}
>
> I am not sure to understand the Arm implementation given the description
> of the commit message.
>
> The guest layout is completely separate from the host layout. It might
> be possible to have all the memory below 40 bits on the host, but this
> does not preclude the guest to have all memory below 40 bits (the
> hardware might support, for instance, up to 48 bits).
Who is setting up the memory map for the guest then?
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 13:08 ` Juergen Gross
@ 2017-09-20 14:24 ` Julien Grall
2017-09-20 14:33 ` Juergen Gross
0 siblings, 1 reply; 69+ messages in thread
From: Julien Grall @ 2017-09-20 14:24 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 14:08, Juergen Gross wrote:
> On 20/09/17 14:51, Julien Grall wrote:
>> Hi Juergen,
>>
>> Sorry for the late comment.
>>
>> On 20/09/17 07:34, Juergen Gross wrote:
>>> Add a function for obtaining the highest possible physical memory
>>> address of the system. This value is influenced by:
>>>
>>> - hypervisor configuration (CONFIG_BIGMEM)
>>> - processor capability (max. addressable physical memory)
>>> - memory map at boot time
>>> - memory hotplug capability
>>>
>>> The value is especially needed for dom0 to decide sizing of grant frame
>>> limits of guests and for pv domains for selecting the grant interface
>>
>> Why limiting to PV domain? Arm domain may also need to switch to another
>> interface between v1 only support 32-bit GFN.
>
> Right. And I just used that reasoning for an answer to Jan. :-)
>
>>
>>> version to use.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> [...]
>>
>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>> index cd6dfb54b9..6aa8cba5e0 100644
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct
>>> page_info *page)
>>> void clear_and_clean_page(struct page_info *page);
>>> +static inline unsigned long arch_get_upper_mfn_bound(void)
>>> +{
>>> + return 0;
>>> +}
>>
>> I am not sure to understand the Arm implementation given the description
>> of the commit message.
>>
>> The guest layout is completely separate from the host layout. It might
>> be possible to have all the memory below 40 bits on the host, but this
>> does not preclude the guest to have all memory below 40 bits (the
>> hardware might support, for instance, up to 48 bits).
>
> Who is setting up the memory map for the guest then?
The memory map is at the moment static and described in
public/arch-arm.h. The guest is not allowed to assume it and should
discover it through ACPI/DT.
There are 2 banks of memory for the guest (it depends on the amount of
memory requested by the user):
- 3GB @ 1GB
- 1016GB @ 8GB
But the guest would be free to use the populate memory hypercall to
allocate memory anywhere in the address space.
For Arm32, the maximum IPA (Intermediate Physical Address aka guest
physical address on Xen) we currently support is always 40 bits.
For Arm64, this range from 32 bits to 48 bits. New hardware can support
up to 52 bits.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 14:24 ` Julien Grall
@ 2017-09-20 14:33 ` Juergen Gross
2017-09-20 17:15 ` Julien Grall
0 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 14:33 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
On 20/09/17 16:24, Julien Grall wrote:
> Hi Juergen,
>
> On 20/09/17 14:08, Juergen Gross wrote:
>> On 20/09/17 14:51, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> Sorry for the late comment.
>>>
>>> On 20/09/17 07:34, Juergen Gross wrote:
>>>> Add a function for obtaining the highest possible physical memory
>>>> address of the system. This value is influenced by:
>>>>
>>>> - hypervisor configuration (CONFIG_BIGMEM)
>>>> - processor capability (max. addressable physical memory)
>>>> - memory map at boot time
>>>> - memory hotplug capability
>>>>
>>>> The value is especially needed for dom0 to decide sizing of grant frame
>>>> limits of guests and for pv domains for selecting the grant interface
>>>
>>> Why limiting to PV domain? Arm domain may also need to switch to another
>>> interface between v1 only support 32-bit GFN.
>>
>> Right. And I just used that reasoning for an answer to Jan. :-)
>>
>>>
>>>> version to use.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> [...]
>>>
>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>> index cd6dfb54b9..6aa8cba5e0 100644
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct
>>>> page_info *page)
>>>> void clear_and_clean_page(struct page_info *page);
>>>> +static inline unsigned long arch_get_upper_mfn_bound(void)
>>>> +{
>>>> + return 0;
>>>> +}
>>>
>>> I am not sure to understand the Arm implementation given the description
>>> of the commit message.
>>>
>>> The guest layout is completely separate from the host layout. It might
>>> be possible to have all the memory below 40 bits on the host, but this
>>> does not preclude the guest to have all memory below 40 bits (the
>>> hardware might support, for instance, up to 48 bits).
>>
>> Who is setting up the memory map for the guest then?
>
> The memory map is at the moment static and described in
> public/arch-arm.h. The guest is not allowed to assume it and should
> discover it through ACPI/DT.
Is there any memory hotplug possible (host level, guest level)?
> There are 2 banks of memory for the guest (it depends on the amount of
> memory requested by the user):
> - 3GB @ 1GB
> - 1016GB @ 8GB
>
> But the guest would be free to use the populate memory hypercall to
> allocate memory anywhere in the address space.
Okay, so this is similar to x86 HVM then.
> For Arm32, the maximum IPA (Intermediate Physical Address aka guest
> physical address on Xen) we currently support is always 40 bits.
>
> For Arm64, this range from 32 bits to 48 bits. New hardware can support
> up to 52 bits.
I guess this information is included in some tables like ACPI or DT?
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 14:33 ` Juergen Gross
@ 2017-09-20 17:15 ` Julien Grall
2017-09-21 4:18 ` Juergen Gross
0 siblings, 1 reply; 69+ messages in thread
From: Julien Grall @ 2017-09-20 17:15 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 15:33, Juergen Gross wrote:
> On 20/09/17 16:24, Julien Grall wrote:
>> On 20/09/17 14:08, Juergen Gross wrote:
>>> On 20/09/17 14:51, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> Sorry for the late comment.
>>>>
>>>> On 20/09/17 07:34, Juergen Gross wrote:
>>>>> Add a function for obtaining the highest possible physical memory
>>>>> address of the system. This value is influenced by:
>>>>>
>>>>> - hypervisor configuration (CONFIG_BIGMEM)
>>>>> - processor capability (max. addressable physical memory)
>>>>> - memory map at boot time
>>>>> - memory hotplug capability
>>>>>
>>>>> The value is especially needed for dom0 to decide sizing of grant frame
>>>>> limits of guests and for pv domains for selecting the grant interface
>>>>
>>>> Why limiting to PV domain? Arm domain may also need to switch to another
>>>> interface between v1 only support 32-bit GFN.
>>>
>>> Right. And I just used that reasoning for an answer to Jan. :-)
>>>
>>>>
>>>>> version to use.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>>> index cd6dfb54b9..6aa8cba5e0 100644
>>>>> --- a/xen/include/asm-arm/mm.h
>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct
>>>>> page_info *page)
>>>>> void clear_and_clean_page(struct page_info *page);
>>>>> +static inline unsigned long arch_get_upper_mfn_bound(void)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>
>>>> I am not sure to understand the Arm implementation given the description
>>>> of the commit message.
>>>>
>>>> The guest layout is completely separate from the host layout. It might
>>>> be possible to have all the memory below 40 bits on the host, but this
>>>> does not preclude the guest to have all memory below 40 bits (the
>>>> hardware might support, for instance, up to 48 bits).
>>>
>>> Who is setting up the memory map for the guest then?
>>
>> The memory map is at the moment static and described in
>> public/arch-arm.h. The guest is not allowed to assume it and should
>> discover it through ACPI/DT.
>
> Is there any memory hotplug possible (host level, guest level)?
It is not implemented at the moment.
>
>> There are 2 banks of memory for the guest (it depends on the amount of
>> memory requested by the user):
>> - 3GB @ 1GB
>> - 1016GB @ 8GB
>>
>> But the guest would be free to use the populate memory hypercall to
>> allocate memory anywhere in the address space.
>
> Okay, so this is similar to x86 HVM then.
You could compare Arm guest to PVH.
>
>> For Arm32, the maximum IPA (Intermediate Physical Address aka guest
>> physical address on Xen) we currently support is always 40 bits.
>>
>> For Arm64, this range from 32 bits to 48 bits. New hardware can support
>> up to 52 bits.
>
> I guess this information is included in some tables like ACPI or DT?
No. On Arm64, you can deduce the maximum size from the ID_AA64MMFR0_EL1.
But, the hypervisor would be free to limit the number of guest physical
bits. Although, it could never be higher than the Physical Address range
supported.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 04/15] xen: add function for obtaining highest possible memory address
2017-09-20 17:15 ` Julien Grall
@ 2017-09-21 4:18 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 4:18 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
On 20/09/17 19:15, Julien Grall wrote:
> Hi Juergen,
>
> On 20/09/17 15:33, Juergen Gross wrote:
>> On 20/09/17 16:24, Julien Grall wrote:
>>> On 20/09/17 14:08, Juergen Gross wrote:
>>>> On 20/09/17 14:51, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> Sorry for the late comment.
>>>>>
>>>>> On 20/09/17 07:34, Juergen Gross wrote:
>>>>>> Add a function for obtaining the highest possible physical memory
>>>>>> address of the system. This value is influenced by:
>>>>>>
>>>>>> - hypervisor configuration (CONFIG_BIGMEM)
>>>>>> - processor capability (max. addressable physical memory)
>>>>>> - memory map at boot time
>>>>>> - memory hotplug capability
>>>>>>
>>>>>> The value is especially needed for dom0 to decide sizing of grant
>>>>>> frame
>>>>>> limits of guests and for pv domains for selecting the grant interface
>>>>>
>>>>> Why limiting to PV domain? Arm domain may also need to switch to
>>>>> another
>>>>> interface between v1 only support 32-bit GFN.
>>>>
>>>> Right. And I just used that reasoning for an answer to Jan. :-)
>>>>
>>>>>
>>>>>> version to use.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>>>> index cd6dfb54b9..6aa8cba5e0 100644
>>>>>> --- a/xen/include/asm-arm/mm.h
>>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>>> @@ -376,6 +376,11 @@ static inline void put_page_and_type(struct
>>>>>> page_info *page)
>>>>>> void clear_and_clean_page(struct page_info *page);
>>>>>> +static inline unsigned long arch_get_upper_mfn_bound(void)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>
>>>>> I am not sure to understand the Arm implementation given the
>>>>> description
>>>>> of the commit message.
>>>>>
>>>>> The guest layout is completely separate from the host layout. It might
>>>>> be possible to have all the memory below 40 bits on the host, but this
>>>>> does not preclude the guest to have all memory below 40 bits (the
>>>>> hardware might support, for instance, up to 48 bits).
>>>>
>>>> Who is setting up the memory map for the guest then?
>>>
>>> The memory map is at the moment static and described in
>>> public/arch-arm.h. The guest is not allowed to assume it and should
>>> discover it through ACPI/DT.
>>
>> Is there any memory hotplug possible (host level, guest level)?
>
> It is not implemented at the moment.
>
>>
>>> There are 2 banks of memory for the guest (it depends on the amount of
>>> memory requested by the user):
>>> - 3GB @ 1GB
>>> - 1016GB @ 8GB
>>>
>>> But the guest would be free to use the populate memory hypercall to
>>> allocate memory anywhere in the address space.
>>
>> Okay, so this is similar to x86 HVM then.
> You could compare Arm guest to PVH.
>
>>
>>> For Arm32, the maximum IPA (Intermediate Physical Address aka guest
>>> physical address on Xen) we currently support is always 40 bits.
>>>
>>> For Arm64, this range from 32 bits to 48 bits. New hardware can support
>>> up to 52 bits.
>>
>> I guess this information is included in some tables like ACPI or DT?
>
> No. On Arm64, you can deduce the maximum size from the ID_AA64MMFR0_EL1.
> But, the hypervisor would be free to limit the number of guest physical
> bits. Although, it could never be higher than the Physical Address range
> supported.
Okay, so we have no need for an additional interface on ARM, right?
It can all be handled via the existing interfaces.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (3 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 04/15] xen: add function for obtaining highest possible memory address Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 8:58 ` Jan Beulich
` (2 more replies)
2017-09-20 6:34 ` [PATCH v8 06/15] libxc: add libxc support for setting grant table resource limits Juergen Gross
` (9 subsequent siblings)
14 siblings, 3 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add the maximum possible mfn to struct xen_sysctl_physinfo in order to
enable Xen tools to size the grant table frame limits for a domU.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/sysctl.c | 1 +
xen/include/public/sysctl.h | 2 ++
2 files changed, 3 insertions(+)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index a6882d1c9d..22f5d991f6 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -266,6 +266,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
get_outstanding_claims(&pi->free_pages, &pi->outstanding_pages);
pi->scrub_pages = 0;
pi->cpu_khz = cpu_khz;
+ pi->max_mfn = get_upper_mfn_bound() - 1;
arch_do_physinfo(pi);
if ( copy_to_guest(u_sysctl, op, 1) )
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 7830b987da..86b9ced86b 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -108,6 +108,8 @@ struct xen_sysctl_physinfo {
/* XEN_SYSCTL_PHYSCAP_??? */
uint32_t capabilities;
+
+ uint64_t max_mfn; /* Largest possible MFN on this host */
};
typedef struct xen_sysctl_physinfo xen_sysctl_physinfo_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_physinfo_t);
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo
2017-09-20 6:34 ` [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo Juergen Gross
@ 2017-09-20 8:58 ` Jan Beulich
[not found] ` <59C249F3020000780017D415@suse.com>
2017-09-20 12:53 ` Julien Grall
2 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 8:58 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> Add the maximum possible mfn to struct xen_sysctl_physinfo in order to
> enable Xen tools to size the grant table frame limits for a domU.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> xen/common/sysctl.c | 1 +
> xen/include/public/sysctl.h | 2 ++
> 2 files changed, 3 insertions(+)
As indicated in reply to patch 4, I think the on here should be folded
into that one, in order to not transiently leave around a dead function.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C249F3020000780017D415@suse.com>]
* Re: [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo
[not found] ` <59C249F3020000780017D415@suse.com>
@ 2017-09-20 9:00 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 9:00 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 10:58, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> Add the maximum possible mfn to struct xen_sysctl_physinfo in order to
>> enable Xen tools to size the grant table frame limits for a domU.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> xen/common/sysctl.c | 1 +
>> xen/include/public/sysctl.h | 2 ++
>> 2 files changed, 3 insertions(+)
>
> As indicated in reply to patch 4, I think the on here should be folded
> into that one, in order to not transiently leave around a dead function.
Okay, I can do it.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo
2017-09-20 6:34 ` [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo Juergen Gross
2017-09-20 8:58 ` Jan Beulich
[not found] ` <59C249F3020000780017D415@suse.com>
@ 2017-09-20 12:53 ` Julien Grall
2017-09-20 13:06 ` Juergen Gross
2 siblings, 1 reply; 69+ messages in thread
From: Julien Grall @ 2017-09-20 12:53 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 07:34, Juergen Gross wrote:
> Add the maximum possible mfn to struct xen_sysctl_physinfo in order to
> enable Xen tools to size the grant table frame limits for a domU
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> xen/common/sysctl.c | 1 +
> xen/include/public/sysctl.h | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index a6882d1c9d..22f5d991f6 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -266,6 +266,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> get_outstanding_claims(&pi->free_pages, &pi->outstanding_pages);
> pi->scrub_pages = 0;
> pi->cpu_khz = cpu_khz;
> + pi->max_mfn = get_upper_mfn_bound() - 1;
> arch_do_physinfo(pi);
>
> if ( copy_to_guest(u_sysctl, op, 1) )
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 7830b987da..86b9ced86b 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -108,6 +108,8 @@ struct xen_sysctl_physinfo {
>
> /* XEN_SYSCTL_PHYSCAP_??? */
> uint32_t capabilities;
> +
> + uint64_t max_mfn; /* Largest possible MFN on this host */
Don't you need to bump XEN_SYSCTL_INTERFACE_VERSION because of this change?
> };
> typedef struct xen_sysctl_physinfo xen_sysctl_physinfo_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_physinfo_t);
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo
2017-09-20 12:53 ` Julien Grall
@ 2017-09-20 13:06 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 13:06 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
On 20/09/17 14:53, Julien Grall wrote:
> Hi Juergen,
>
> On 20/09/17 07:34, Juergen Gross wrote:
>> Add the maximum possible mfn to struct xen_sysctl_physinfo in order to
>> enable Xen tools to size the grant table frame limits for a domU
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> xen/common/sysctl.c | 1 +
>> xen/include/public/sysctl.h | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index a6882d1c9d..22f5d991f6 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -266,6 +266,7 @@ long
>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>> get_outstanding_claims(&pi->free_pages,
>> &pi->outstanding_pages);
>> pi->scrub_pages = 0;
>> pi->cpu_khz = cpu_khz;
>> + pi->max_mfn = get_upper_mfn_bound() - 1;
>> arch_do_physinfo(pi);
>> if ( copy_to_guest(u_sysctl, op, 1) )
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 7830b987da..86b9ced86b 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -108,6 +108,8 @@ struct xen_sysctl_physinfo {
>> /* XEN_SYSCTL_PHYSCAP_??? */
>> uint32_t capabilities;
>> +
>> + uint64_t max_mfn; /* Largest possible MFN on this host */
>
> Don't you need to bump XEN_SYSCTL_INTERFACE_VERSION because of this change?
Thanks for noticing.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 06/15] libxc: add libxc support for setting grant table resource limits
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (4 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 05/15] xen: add max possible mfn to struct xen_sysctl_physinfo Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 07/15] tools: set grant limits for xenstore stubdom Juergen Gross
` (8 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add a new libxc function xc_domain_set_gnttbl_limits() setting the
limits for the maximum numbers of grant table frames and maptrack
frames of a domain.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
V4:
- use domid_t (Wei Liu)
---
tools/libxc/include/xenctrl.h | 14 ++++++++++++++
tools/libxc/xc_domain.c | 13 +++++++++++++
2 files changed, 27 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 43151cb415..ab34fb4f70 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1064,6 +1064,20 @@ int xc_domain_set_virq_handler(xc_interface *xch, uint32_t domid, int virq);
int xc_domain_set_max_evtchn(xc_interface *xch, uint32_t domid,
uint32_t max_port);
+/**
+ * Set the maximum number of grant frames and/or maptrack frames a domain
+ * can have. Can only be used at domain setup time. A zero value means
+ * no change.
+ *
+ * @param xch a handle to an open hypervisor interface
+ * @param domid the domain id
+ * @param grant_frames max. number of grant frames
+ * @param maptrack_frames max. number of maptrack frames
+ */
+int xc_domain_set_gnttab_limits(xc_interface *xch, domid_t domid,
+ uint32_t grant_frames,
+ uint32_t maptrack_frames);
+
/*
* CPUPOOL MANAGEMENT FUNCTIONS
*/
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 3bab4e8bab..41b42d6637 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2268,6 +2268,19 @@ int xc_domain_set_max_evtchn(xc_interface *xch, uint32_t domid,
return do_domctl(xch, &domctl);
}
+int xc_domain_set_gnttab_limits(xc_interface *xch, domid_t domid,
+ uint32_t grant_frames,
+ uint32_t maptrack_frames)
+{
+ DECLARE_DOMCTL;
+
+ domctl.cmd = XEN_DOMCTL_set_gnttab_limits;
+ domctl.domain = domid;
+ domctl.u.set_gnttab_limits.grant_frames = grant_frames;
+ domctl.u.set_gnttab_limits.maptrack_frames = maptrack_frames;
+ return do_domctl(xch, &domctl);
+}
+
/* Plumbing Xen with vNUMA topology */
int xc_domain_setvnuma(xc_interface *xch,
uint32_t domid,
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 07/15] tools: set grant limits for xenstore stubdom
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (5 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 06/15] libxc: add libxc support for setting grant table resource limits Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 08/15] libxl: add max possible mfn to libxl_physinfo Juergen Gross
` (7 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
When creating a Xenstore stubdom set the grant limits: the stubdom
will need to setup a very limited amount of grants only, so 4 grant
frames are enough. For being able to support up to 32768 domains it
will need 128 maptrack frames (1 mapping per domain, 256 maptrack
entries per maptrack frame).
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
tools/helpers/init-xenstore-domain.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 8a41ee7d3a..047ad0cb1d 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -105,6 +105,17 @@ static int build(xc_interface *xch)
fprintf(stderr, "xc_domain_setmaxmem failed\n");
goto err;
}
+ /*
+ * 1 grant frame is enough: we don't need many grants.
+ * Mini-OS doesn't like less than 4, though, so use 4.
+ * 128 maptrack frames: 256 entries per frame, enough for 32768 domains.
+ */
+ rv = xc_domain_set_gnttab_limits(xch, domid, 4, 128);
+ if ( rv )
+ {
+ fprintf(stderr, "xc_domain_set_gnttab_limits failed\n");
+ goto err;
+ }
rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
if ( rv )
{
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 08/15] libxl: add max possible mfn to libxl_physinfo
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (6 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 07/15] tools: set grant limits for xenstore stubdom Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 09/15] xl: add global grant limit config items Juergen Gross
` (6 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add the maximum possible mfn of the host to the libxl_physinfo
data structure.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 1 +
tools/libxl/libxl.h | 9 +++++++++
tools/libxl/libxl_types.idl | 1 +
3 files changed, 11 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 247c56cf83..b41ade9fda 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -373,6 +373,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
physinfo->free_pages = xcphysinfo.free_pages;
physinfo->scrub_pages = xcphysinfo.scrub_pages;
physinfo->outstanding_pages = xcphysinfo.outstanding_pages;
+ physinfo->max_possible_mfn = xcphysinfo.max_mfn;
l = xc_sharing_freed_pages(ctx->xch);
if (l < 0 && errno == ENOSYS) {
l = 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7d853ca924..fb960debee 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -644,6 +644,15 @@ typedef struct libxl__ctx libxl_ctx;
#define LIBXL_HAVE_PHYSINFO_OUTSTANDING_PAGES 1
/*
+ * LIBXL_HAVE_PHYSINFO_MAX_POSSIBLE_MFN
+ *
+ * If this is defined, libxl_physinfo structure will contain an uint64 field
+ * called max_possible_mfn, containing the highest possible mfn on this host,
+ * possibly taking memory hotplug into account.
+ */
+#define LIBXL_HAVE_PHYSINFO_MAX_POSSIBLE_MFN 1
+
+/*
* LIBXL_HAVE_DOMINFO_OUTSTANDING_MEMKB 1
*
* If this is defined, libxl_dominfo will contain a MemKB type field called
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 756e120ad7..5d9e7aabba 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -895,6 +895,7 @@ libxl_physinfo = Struct("physinfo", [
("outstanding_pages", uint64),
("sharing_freed_pages", uint64),
("sharing_used_frames", uint64),
+ ("max_possible_mfn", uint64),
("nr_nodes", uint32),
("hw_cap", libxl_hwcap),
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 09/15] xl: add global grant limit config items
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (7 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 08/15] libxl: add max possible mfn to libxl_physinfo Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 10/15] libxl: add libxl support for setting grant table resource limits Juergen Gross
` (5 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add xl.conf config items for default values of grant limits:
max_grant_frames will set the default for the maximum number of grant
frames for a domain which will take effect if the domain's config file
doesn't specify a value. If max_grant_frames isn't set in xl.conf it
will default to 32 for hosts with all memory below 16TB and to 64 for
hosts with memory above 16TB.
max_maptrack_frames will set the default for the maximum number of
maptrack frames for a domain. If max_maptrack_frames isn't set in
xl.conf it will default to 0, as normally only backend domains need
maptrack frames.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
V7:
- don't use xc_maximum_ram_page() but max_possible_mfn from physinfo
---
docs/man/xl.conf.pod.5 | 12 ++++++++++++
tools/xl/xl.c | 15 +++++++++++++++
tools/xl/xl.h | 2 ++
3 files changed, 29 insertions(+)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 88ab506609..fe2cf27ea4 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -77,6 +77,18 @@ operations (primarily domain creation).
Default: C</var/lock/xl>
+=item B<max_grant_frames=NUMBER>
+
+Sets the default value for the C<max_grant_frames> domain config value.
+
+Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
+
+=item B<max_maptrack_frames=NUMBER>
+
+Sets the default value for the C<max_maptrack_frames> domain config value.
+
+Default: C<0>
+
=item B<vif.default.script="PATH">
Configures the default hotplug script used by virtual network devices.
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 02179a6229..c1bbb4b939 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -45,6 +45,8 @@ char *default_colo_proxy_script = NULL;
enum output_format default_output_format = OUTPUT_FORMAT_JSON;
int claim_mode = 1;
bool progress_use_cr = 0;
+int max_grant_frames = -1;
+int max_maptrack_frames = 0;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -88,6 +90,7 @@ static void parse_global_config(const char *configfile,
XLU_Config *config;
int e;
const char *buf;
+ libxl_physinfo physinfo;
config = xlu_cfg_init(stderr, configfile);
if (!config) {
@@ -188,6 +191,18 @@ static void parse_global_config(const char *configfile,
xlu_cfg_replace_string (config, "colo.default.proxyscript",
&default_colo_proxy_script, 0);
+ if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
+ max_grant_frames = l;
+ else {
+ libxl_physinfo_init(&physinfo);
+ max_grant_frames = (libxl_get_physinfo(ctx, &physinfo) != 0 ||
+ !(physinfo.max_possible_mfn >> 32))
+ ? 32 : 64;
+ libxl_physinfo_dispose(&physinfo);
+ }
+ if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
+ max_maptrack_frames = l;
+
xlu_cfg_destroy(config);
}
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 31d660b89a..6b60d1db50 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -275,6 +275,8 @@ extern char *default_vifbackend;
extern char *default_remus_netbufscript;
extern char *default_colo_proxy_script;
extern char *blkdev_start;
+extern int max_grant_frames;
+extern int max_maptrack_frames;
enum output_format {
OUTPUT_FORMAT_JSON,
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 10/15] libxl: add libxl support for setting grant table resource limits
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (8 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 09/15] xl: add global grant limit config items Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 6:34 ` [PATCH v8 11/15] xen: delay allocation of grant table sub structures Juergen Gross
` (4 subsequent siblings)
14 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Add new domain config items for setting the limits for the maximum
numbers of grant table frames and maptrack frames of a domain.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
V6:
- made set_gnttab_limits hypercall mandatory, taking defaults from
xl.conf
V4:
- rename configuration items to use max_ prefixes (Wei Liu)
---
docs/man/xl.cfg.pod.5.in | 16 ++++++++++++++++
tools/libxl/libxl.h | 6 ++++++
tools/libxl/libxl_dm.c | 3 +++
tools/libxl/libxl_dom.c | 6 ++++++
tools/libxl/libxl_types.idl | 3 +++
tools/xl/xl_parse.c | 9 +++++++++
tools/xl/xl_sxp.c | 2 ++
7 files changed, 45 insertions(+)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 247ae99ca7..e7ab67395b 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -444,6 +444,20 @@ unpausing the domain. With a properly constructed security policy (such
as nomigrate_t in the example policy), this can be used to build a
domain whose memory is not accessible to the toolstack domain.
+=item B<max_grant_frames=NUMBER>
+
+Specify the maximum number of grant frames the domain is allowed to have.
+This value controls how many pages the domain is able to grant access to for
+other domains, needed e.g. for the operation of paravirtualized devices.
+The default is settable via L<xl.conf(5)>.
+
+=item B<max_maptrack_frames=NUMBER>
+
+Specify the maximum number of grant maptrack frames the domain is allowed
+to have. This value controls how many pages of foreign domains can be accessed
+via the grant mechanism by this domain. The default value is settable via
+L<xl.conf(5)>.
+
=item B<nomigrate=BOOLEAN>
Disable migration of this domain. This enables certain other features
@@ -2252,6 +2266,8 @@ No MCA capabilities in above list are enabled.
=item L<xl(1)>
+=item L<xl.conf(5)>
+
=item L<xlcpupool.cfg(5)>
=item L<xl-disk-configuration(5)>
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index fb960debee..c6f42945de 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -311,6 +311,12 @@
#define LIBXL_HAVE_P9S 1
/*
+ * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
+ * has the max_grant_frames and max_maptrack_frames fields.
+ */
+#define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 98f89a95ce..bf651006b4 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1845,6 +1845,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
guest_config->b_info.video_memkb;
dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
+ dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
+ dm_config->b_info.max_maptrack_frames = 0;
+
dm_config->b_info.u.pv.features = "";
dm_config->b_info.device_model_version =
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f54fd49a73..b420738adf 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -322,6 +322,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
return ERROR_FAIL;
}
+ if (xc_domain_set_gnttab_limits(ctx->xch, domid, info->max_grant_frames,
+ info->max_maptrack_frames) != 0) {
+ LOG(ERROR, "Couldn't set grant table limits");
+ return ERROR_FAIL;
+ }
+
/*
* Check if the domain has any CPU or node affinity already. If not, try
* to build up the latter via automatic NUMA placement. In fact, in case
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5d9e7aabba..dc3544873c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -472,6 +472,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
("blkdev_start", string),
("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
+
+ ("max_grant_frames", uint32),
+ ("max_maptrack_frames", uint32),
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 0678fbc1b0..5ba18e9a15 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -990,6 +990,15 @@ void parse_config_data(const char *config_source,
!xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
+ if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
+ b_info->max_grant_frames = l;
+ else
+ b_info->max_grant_frames = max_grant_frames;
+ if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
+ b_info->max_maptrack_frames = l;
+ else
+ b_info->max_maptrack_frames = max_maptrack_frames;
+
libxl_defbool_set(&b_info->claim_mode, claim_mode);
if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
index e738bf2465..e264cf2023 100644
--- a/tools/xl/xl_sxp.c
+++ b/tools/xl/xl_sxp.c
@@ -64,6 +64,8 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh)
fprintf(fh, "\t(build_info)\n");
fprintf(fh, "\t(max_vcpus %d)\n", b_info->max_vcpus);
+ fprintf(fh, "\t(max_grant_frames %d)\n", b_info->max_grant_frames);
+ fprintf(fh, "\t(max_maptrack_frames %d)\n", b_info->max_maptrack_frames);
fprintf(fh, "\t(tsc_mode %s)\n", libxl_tsc_mode_to_string(b_info->tsc_mode));
fprintf(fh, "\t(max_memkb %"PRId64")\n", b_info->max_memkb);
fprintf(fh, "\t(target_memkb %"PRId64")\n", b_info->target_memkb);
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v8 11/15] xen: delay allocation of grant table sub structures
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (9 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 10/15] libxl: add libxl support for setting grant table resource limits Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 9:22 ` Jan Beulich
[not found] ` <59C24F80020000780017D473@suse.com>
2017-09-20 6:34 ` [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c Juergen Gross
` (3 subsequent siblings)
14 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Delay the allocation of the grant table sub structures in order to
allow modifying parameters needed for sizing of these structures at a
per domain basis. Allocate the structures from gnttab_setup_table()
and the table frames only from grant_table_set_limits() (dom0: from
grant_table_create()).
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
V6:
- move call of grant_table_init() for dom0 to grant_table_create()
(Jan Beulich)
- move frame allocations to gnttab_grow_table() (Jan Beulich)
- several other changes due to new patch order
V4:
- make ret more local (Wei Liu)
V3:
- move call of grant_table_init() from gnttab_setup_table() to
gnttab_grow_table() (Paul Durrant)
---
xen/common/grant_table.c | 113 ++++++++++++++++++++++-------------------------
1 file changed, 52 insertions(+), 61 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f48eeff7ad..f66940551e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1655,6 +1655,46 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
gt->nr_status_frames = 0;
}
+static int
+grant_table_init(struct grant_table *gt)
+{
+ if ( gt->active )
+ return -EBUSY;
+
+ /* Active grant table. */
+ gt->active = xzalloc_array(struct active_grant_entry *,
+ max_nr_active_grant_frames);
+ if ( gt->active == NULL )
+ goto no_mem;
+
+ /* Tracking of mapped foreign frames table */
+ gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
+ if ( gt->maptrack == NULL )
+ goto no_mem;
+
+ /* Shared grant table. */
+ gt->shared_raw = xzalloc_array(void *, max_grant_frames);
+ if ( gt->shared_raw == NULL )
+ goto no_mem;
+
+ /* Status pages for grant table - for version 2 */
+ gt->status = xzalloc_array(grant_status_t *,
+ grant_to_status_frames(max_grant_frames));
+ if ( gt->status == NULL )
+ goto no_mem;
+
+ return 0;
+
+ no_mem:
+ xfree(gt->shared_raw);
+ gt->shared_raw = NULL;
+ vfree(gt->maptrack);
+ gt->maptrack = NULL;
+ xfree(gt->active);
+ gt->active = NULL;
+ return -ENOMEM;
+}
+
/*
* Grow the grant table. The caller must hold the grant table's
* write lock before calling this function.
@@ -1665,6 +1705,10 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
struct grant_table *gt = d->grant_table;
unsigned int i, j;
+ ASSERT(gt->active);
+
+ if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
+ req_nr_frames = INITIAL_NR_GRANT_FRAMES;
ASSERT(req_nr_frames <= max_grant_frames);
gdprintk(XENLOG_INFO,
@@ -3381,75 +3425,21 @@ grant_table_create(
struct domain *d)
{
struct grant_table *t;
- unsigned int i, j;
if ( (t = xzalloc(struct grant_table)) == NULL )
- goto no_mem_0;
+ return -ENOMEM;
/* Simple stuff. */
percpu_rwlock_resource_init(&t->lock, grant_rwlock);
spin_lock_init(&t->maptrack_lock);
- t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
-
- /* Active grant table. */
- if ( (t->active = xzalloc_array(struct active_grant_entry *,
- max_nr_active_grant_frames)) == NULL )
- goto no_mem_1;
- for ( i = 0;
- i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
- {
- if ( (t->active[i] = alloc_xenheap_page()) == NULL )
- goto no_mem_2;
- clear_page(t->active[i]);
- for ( j = 0; j < ACGNT_PER_PAGE; j++ )
- spin_lock_init(&t->active[i][j].lock);
- }
-
- /* Tracking of mapped foreign frames table */
- t->maptrack = vzalloc(max_maptrack_frames * sizeof(*t->maptrack));
- if ( t->maptrack == NULL )
- goto no_mem_2;
-
- /* Shared grant table. */
- if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
- goto no_mem_3;
- for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
- {
- if ( (t->shared_raw[i] = alloc_xenheap_page()) == NULL )
- goto no_mem_4;
- clear_page(t->shared_raw[i]);
- }
-
- /* Status pages for grant table - for version 2 */
- t->status = xzalloc_array(grant_status_t *,
- grant_to_status_frames(max_grant_frames));
- if ( t->status == NULL )
- goto no_mem_4;
-
- for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
- gnttab_create_shared_page(d, t, i);
-
- t->nr_status_frames = 0;
/* Okay, install the structure. */
d->grant_table = t;
- return 0;
- no_mem_4:
- for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
- free_xenheap_page(t->shared_raw[i]);
- xfree(t->shared_raw);
- no_mem_3:
- vfree(t->maptrack);
- no_mem_2:
- for ( i = 0;
- i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
- free_xenheap_page(t->active[i]);
- xfree(t->active);
- no_mem_1:
- xfree(t);
- no_mem_0:
- return -ENOMEM;
+ if ( d->domain_id == 0 )
+ return grant_table_init(t);
+
+ return 0;
}
void
@@ -3651,8 +3641,9 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
grant_write_lock(gt);
- ret = 0;
- /* Set limits, alloc needed arrays. */
+ /* Set limits. */
+ if ( !gt->active )
+ ret = grant_table_init(gt);
grant_write_unlock(gt);
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 11/15] xen: delay allocation of grant table sub structures
2017-09-20 6:34 ` [PATCH v8 11/15] xen: delay allocation of grant table sub structures Juergen Gross
@ 2017-09-20 9:22 ` Jan Beulich
[not found] ` <59C24F80020000780017D473@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 9:22 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> @@ -3381,75 +3425,21 @@ grant_table_create(
> struct domain *d)
> {
> struct grant_table *t;
> - unsigned int i, j;
>
> if ( (t = xzalloc(struct grant_table)) == NULL )
> - goto no_mem_0;
> + return -ENOMEM;
>
> /* Simple stuff. */
> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
> spin_lock_init(&t->maptrack_lock);
> - t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
> -
> - /* Active grant table. */
> - if ( (t->active = xzalloc_array(struct active_grant_entry *,
> - max_nr_active_grant_frames)) == NULL )
> - goto no_mem_1;
> - for ( i = 0;
> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
> - {
> - if ( (t->active[i] = alloc_xenheap_page()) == NULL )
> - goto no_mem_2;
> - clear_page(t->active[i]);
> - for ( j = 0; j < ACGNT_PER_PAGE; j++ )
> - spin_lock_init(&t->active[i][j].lock);
> - }
> -
> - /* Tracking of mapped foreign frames table */
> - t->maptrack = vzalloc(max_maptrack_frames * sizeof(*t->maptrack));
> - if ( t->maptrack == NULL )
> - goto no_mem_2;
> -
> - /* Shared grant table. */
> - if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
> - goto no_mem_3;
> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> - {
> - if ( (t->shared_raw[i] = alloc_xenheap_page()) == NULL )
> - goto no_mem_4;
> - clear_page(t->shared_raw[i]);
> - }
> -
> - /* Status pages for grant table - for version 2 */
> - t->status = xzalloc_array(grant_status_t *,
> - grant_to_status_frames(max_grant_frames));
> - if ( t->status == NULL )
> - goto no_mem_4;
> -
> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> - gnttab_create_shared_page(d, t, i);
> -
> - t->nr_status_frames = 0;
>
> /* Okay, install the structure. */
> d->grant_table = t;
> - return 0;
>
> - no_mem_4:
> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
> - free_xenheap_page(t->shared_raw[i]);
> - xfree(t->shared_raw);
> - no_mem_3:
> - vfree(t->maptrack);
> - no_mem_2:
> - for ( i = 0;
> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
> - free_xenheap_page(t->active[i]);
> - xfree(t->active);
> - no_mem_1:
> - xfree(t);
> - no_mem_0:
> - return -ENOMEM;
> + if ( d->domain_id == 0 )
> + return grant_table_init(t);
> +
> + return 0;
> }
>
> void
> @@ -3651,8 +3641,9 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>
> grant_write_lock(gt);
>
> - ret = 0;
> - /* Set limits, alloc needed arrays. */
> + /* Set limits. */
> + if ( !gt->active )
> + ret = grant_table_init(gt);
>
> grant_write_unlock(gt);
These changes don't leave the domains in a state similar to that
before the change - I'm missing calls to gnttab_grow_table() to
establish the minimal sizes. Aiui so far there has been no
requirement for a domain to invoke GNTTABOP_setup_table if it
is happy with v1 and the minimum size.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C24F80020000780017D473@suse.com>]
* Re: [PATCH v8 11/15] xen: delay allocation of grant table sub structures
[not found] ` <59C24F80020000780017D473@suse.com>
@ 2017-09-20 9:44 ` Juergen Gross
2017-09-20 10:02 ` Jan Beulich
[not found] ` <59C258D4020000780017D521@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 9:44 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 11:22, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> @@ -3381,75 +3425,21 @@ grant_table_create(
>> struct domain *d)
>> {
>> struct grant_table *t;
>> - unsigned int i, j;
>>
>> if ( (t = xzalloc(struct grant_table)) == NULL )
>> - goto no_mem_0;
>> + return -ENOMEM;
>>
>> /* Simple stuff. */
>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>> spin_lock_init(&t->maptrack_lock);
>> - t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
>> -
>> - /* Active grant table. */
>> - if ( (t->active = xzalloc_array(struct active_grant_entry *,
>> - max_nr_active_grant_frames)) == NULL )
>> - goto no_mem_1;
>> - for ( i = 0;
>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>> - {
>> - if ( (t->active[i] = alloc_xenheap_page()) == NULL )
>> - goto no_mem_2;
>> - clear_page(t->active[i]);
>> - for ( j = 0; j < ACGNT_PER_PAGE; j++ )
>> - spin_lock_init(&t->active[i][j].lock);
>> - }
>> -
>> - /* Tracking of mapped foreign frames table */
>> - t->maptrack = vzalloc(max_maptrack_frames * sizeof(*t->maptrack));
>> - if ( t->maptrack == NULL )
>> - goto no_mem_2;
>> -
>> - /* Shared grant table. */
>> - if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
>> - goto no_mem_3;
>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>> - {
>> - if ( (t->shared_raw[i] = alloc_xenheap_page()) == NULL )
>> - goto no_mem_4;
>> - clear_page(t->shared_raw[i]);
>> - }
>> -
>> - /* Status pages for grant table - for version 2 */
>> - t->status = xzalloc_array(grant_status_t *,
>> - grant_to_status_frames(max_grant_frames));
>> - if ( t->status == NULL )
>> - goto no_mem_4;
>> -
>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>> - gnttab_create_shared_page(d, t, i);
>> -
>> - t->nr_status_frames = 0;
>>
>> /* Okay, install the structure. */
>> d->grant_table = t;
>> - return 0;
>>
>> - no_mem_4:
>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>> - free_xenheap_page(t->shared_raw[i]);
>> - xfree(t->shared_raw);
>> - no_mem_3:
>> - vfree(t->maptrack);
>> - no_mem_2:
>> - for ( i = 0;
>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>> - free_xenheap_page(t->active[i]);
>> - xfree(t->active);
>> - no_mem_1:
>> - xfree(t);
>> - no_mem_0:
>> - return -ENOMEM;
>> + if ( d->domain_id == 0 )
>> + return grant_table_init(t);
>> +
>> + return 0;
>> }
>>
>> void
>> @@ -3651,8 +3641,9 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>
>> grant_write_lock(gt);
>>
>> - ret = 0;
>> - /* Set limits, alloc needed arrays. */
>> + /* Set limits. */
>> + if ( !gt->active )
>> + ret = grant_table_init(gt);
>>
>> grant_write_unlock(gt);
>
> These changes don't leave the domains in a state similar to that
> before the change - I'm missing calls to gnttab_grow_table() to
> establish the minimal sizes. Aiui so far there has been no
> requirement for a domain to invoke GNTTABOP_setup_table if it
> is happy with v1 and the minimum size.
So you don't like gnttab_grow_table() being called when the guest tries
to map the grant frames in this case?
I can add the call of gnttab_grow_table() to grant_table_set_limits() in
case you like that better.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 11/15] xen: delay allocation of grant table sub structures
2017-09-20 9:44 ` Juergen Gross
@ 2017-09-20 10:02 ` Jan Beulich
[not found] ` <59C258D4020000780017D521@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 10:02 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 11:44, <jgross@suse.com> wrote:
> On 20/09/17 11:22, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> @@ -3381,75 +3425,21 @@ grant_table_create(
>>> struct domain *d)
>>> {
>>> struct grant_table *t;
>>> - unsigned int i, j;
>>>
>>> if ( (t = xzalloc(struct grant_table)) == NULL )
>>> - goto no_mem_0;
>>> + return -ENOMEM;
>>>
>>> /* Simple stuff. */
>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>> spin_lock_init(&t->maptrack_lock);
>>> - t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
>>> -
>>> - /* Active grant table. */
>>> - if ( (t->active = xzalloc_array(struct active_grant_entry *,
>>> - max_nr_active_grant_frames)) == NULL )
>>> - goto no_mem_1;
>>> - for ( i = 0;
>>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>>> - {
>>> - if ( (t->active[i] = alloc_xenheap_page()) == NULL )
>>> - goto no_mem_2;
>>> - clear_page(t->active[i]);
>>> - for ( j = 0; j < ACGNT_PER_PAGE; j++ )
>>> - spin_lock_init(&t->active[i][j].lock);
>>> - }
>>> -
>>> - /* Tracking of mapped foreign frames table */
>>> - t->maptrack = vzalloc(max_maptrack_frames * sizeof(*t->maptrack));
>>> - if ( t->maptrack == NULL )
>>> - goto no_mem_2;
>>> -
>>> - /* Shared grant table. */
>>> - if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
>>> - goto no_mem_3;
>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> - {
>>> - if ( (t->shared_raw[i] = alloc_xenheap_page()) == NULL )
>>> - goto no_mem_4;
>>> - clear_page(t->shared_raw[i]);
>>> - }
>>> -
>>> - /* Status pages for grant table - for version 2 */
>>> - t->status = xzalloc_array(grant_status_t *,
>>> - grant_to_status_frames(max_grant_frames));
>>> - if ( t->status == NULL )
>>> - goto no_mem_4;
>>> -
>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> - gnttab_create_shared_page(d, t, i);
>>> -
>>> - t->nr_status_frames = 0;
>>>
>>> /* Okay, install the structure. */
>>> d->grant_table = t;
>>> - return 0;
>>>
>>> - no_mem_4:
>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> - free_xenheap_page(t->shared_raw[i]);
>>> - xfree(t->shared_raw);
>>> - no_mem_3:
>>> - vfree(t->maptrack);
>>> - no_mem_2:
>>> - for ( i = 0;
>>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>>> - free_xenheap_page(t->active[i]);
>>> - xfree(t->active);
>>> - no_mem_1:
>>> - xfree(t);
>>> - no_mem_0:
>>> - return -ENOMEM;
>>> + if ( d->domain_id == 0 )
>>> + return grant_table_init(t);
>>> +
>>> + return 0;
>>> }
>>>
>>> void
>>> @@ -3651,8 +3641,9 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>
>>> grant_write_lock(gt);
>>>
>>> - ret = 0;
>>> - /* Set limits, alloc needed arrays. */
>>> + /* Set limits. */
>>> + if ( !gt->active )
>>> + ret = grant_table_init(gt);
>>>
>>> grant_write_unlock(gt);
>>
>> These changes don't leave the domains in a state similar to that
>> before the change - I'm missing calls to gnttab_grow_table() to
>> establish the minimal sizes. Aiui so far there has been no
>> requirement for a domain to invoke GNTTABOP_setup_table if it
>> is happy with v1 and the minimum size.
>
> So you don't like gnttab_grow_table() being called when the guest tries
> to map the grant frames in this case?
I don't like it being called _only_ in that case.
> I can add the call of gnttab_grow_table() to grant_table_set_limits() in
> case you like that better.
And to grant_table_create() for the Dom0 case.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C258D4020000780017D521@suse.com>]
* Re: [PATCH v8 11/15] xen: delay allocation of grant table sub structures
[not found] ` <59C258D4020000780017D521@suse.com>
@ 2017-09-20 10:05 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 10:05 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 12:02, Jan Beulich wrote:
>>>> On 20.09.17 at 11:44, <jgross@suse.com> wrote:
>> On 20/09/17 11:22, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> @@ -3381,75 +3425,21 @@ grant_table_create(
>>>> struct domain *d)
>>>> {
>>>> struct grant_table *t;
>>>> - unsigned int i, j;
>>>>
>>>> if ( (t = xzalloc(struct grant_table)) == NULL )
>>>> - goto no_mem_0;
>>>> + return -ENOMEM;
>>>>
>>>> /* Simple stuff. */
>>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>>> spin_lock_init(&t->maptrack_lock);
>>>> - t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
>>>> -
>>>> - /* Active grant table. */
>>>> - if ( (t->active = xzalloc_array(struct active_grant_entry *,
>>>> - max_nr_active_grant_frames)) == NULL )
>>>> - goto no_mem_1;
>>>> - for ( i = 0;
>>>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>>>> - {
>>>> - if ( (t->active[i] = alloc_xenheap_page()) == NULL )
>>>> - goto no_mem_2;
>>>> - clear_page(t->active[i]);
>>>> - for ( j = 0; j < ACGNT_PER_PAGE; j++ )
>>>> - spin_lock_init(&t->active[i][j].lock);
>>>> - }
>>>> -
>>>> - /* Tracking of mapped foreign frames table */
>>>> - t->maptrack = vzalloc(max_maptrack_frames * sizeof(*t->maptrack));
>>>> - if ( t->maptrack == NULL )
>>>> - goto no_mem_2;
>>>> -
>>>> - /* Shared grant table. */
>>>> - if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
>>>> - goto no_mem_3;
>>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>>> - {
>>>> - if ( (t->shared_raw[i] = alloc_xenheap_page()) == NULL )
>>>> - goto no_mem_4;
>>>> - clear_page(t->shared_raw[i]);
>>>> - }
>>>> -
>>>> - /* Status pages for grant table - for version 2 */
>>>> - t->status = xzalloc_array(grant_status_t *,
>>>> - grant_to_status_frames(max_grant_frames));
>>>> - if ( t->status == NULL )
>>>> - goto no_mem_4;
>>>> -
>>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>>> - gnttab_create_shared_page(d, t, i);
>>>> -
>>>> - t->nr_status_frames = 0;
>>>>
>>>> /* Okay, install the structure. */
>>>> d->grant_table = t;
>>>> - return 0;
>>>>
>>>> - no_mem_4:
>>>> - for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>>> - free_xenheap_page(t->shared_raw[i]);
>>>> - xfree(t->shared_raw);
>>>> - no_mem_3:
>>>> - vfree(t->maptrack);
>>>> - no_mem_2:
>>>> - for ( i = 0;
>>>> - i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
>>>> - free_xenheap_page(t->active[i]);
>>>> - xfree(t->active);
>>>> - no_mem_1:
>>>> - xfree(t);
>>>> - no_mem_0:
>>>> - return -ENOMEM;
>>>> + if ( d->domain_id == 0 )
>>>> + return grant_table_init(t);
>>>> +
>>>> + return 0;
>>>> }
>>>>
>>>> void
>>>> @@ -3651,8 +3641,9 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>>
>>>> grant_write_lock(gt);
>>>>
>>>> - ret = 0;
>>>> - /* Set limits, alloc needed arrays. */
>>>> + /* Set limits. */
>>>> + if ( !gt->active )
>>>> + ret = grant_table_init(gt);
>>>>
>>>> grant_write_unlock(gt);
>>>
>>> These changes don't leave the domains in a state similar to that
>>> before the change - I'm missing calls to gnttab_grow_table() to
>>> establish the minimal sizes. Aiui so far there has been no
>>> requirement for a domain to invoke GNTTABOP_setup_table if it
>>> is happy with v1 and the minimum size.
>>
>> So you don't like gnttab_grow_table() being called when the guest tries
>> to map the grant frames in this case?
>
> I don't like it being called _only_ in that case.
>
>> I can add the call of gnttab_grow_table() to grant_table_set_limits() in
>> case you like that better.
>
> And to grant_table_create() for the Dom0 case.
So in the end: to grant_table_init().
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (10 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 11/15] xen: delay allocation of grant table sub structures Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 9:25 ` Jan Beulich
2017-09-20 14:34 ` Julien Grall
2017-09-20 6:34 ` [PATCH v8 13/15] xen: make grant resource limits per domain Juergen Gross
` (2 subsequent siblings)
14 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Instead of attaching the ARM specific grant table data to the domain
structure add it to struct grant_table. Add the needed arch functions
to the asm-*/grant_table.h includes.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
V7:
- re-add #include <asm/grant-table.h> in grant_table.h (Julien Grall)
---
xen/arch/arm/domain.c | 2 --
xen/common/grant_table.c | 26 +++++++++++++++++++-------
xen/include/asm-arm/domain.h | 1 -
xen/include/asm-arm/grant_table.h | 26 +++++++++++++++++++-------
xen/include/asm-x86/grant_table.h | 12 +++++++-----
xen/include/xen/grant_table.h | 2 ++
6 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 784ae392cf..e39a79885c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -486,13 +486,11 @@ struct domain *alloc_domain_struct(void)
return NULL;
clear_page(d);
- d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames);
return d;
}
void free_domain_struct(struct domain *d)
{
- xfree(d->arch.grant_table_gfn);
free_xenheap_page(d);
}
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f66940551e..26f9a32656 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -72,6 +72,8 @@ struct grant_table {
struct active_grant_entry **active;
/* Mapping tracking table per vcpu. */
struct grant_mapping **maptrack;
+
+ struct grant_table_arch arch;
};
#ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
@@ -1658,6 +1660,8 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
static int
grant_table_init(struct grant_table *gt)
{
+ int ret = -ENOMEM;
+
if ( gt->active )
return -EBUSY;
@@ -1665,34 +1669,40 @@ grant_table_init(struct grant_table *gt)
gt->active = xzalloc_array(struct active_grant_entry *,
max_nr_active_grant_frames);
if ( gt->active == NULL )
- goto no_mem;
+ goto out;
/* Tracking of mapped foreign frames table */
gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
if ( gt->maptrack == NULL )
- goto no_mem;
+ goto out;
/* Shared grant table. */
gt->shared_raw = xzalloc_array(void *, max_grant_frames);
if ( gt->shared_raw == NULL )
- goto no_mem;
+ goto out;
/* Status pages for grant table - for version 2 */
gt->status = xzalloc_array(grant_status_t *,
grant_to_status_frames(max_grant_frames));
if ( gt->status == NULL )
- goto no_mem;
+ goto out;
+
+ ret = gnttab_init_arch(gt);
+ if ( ret )
+ goto out;
return 0;
- no_mem:
+ out:
+ xfree(gt->status);
+ gt->status = NULL;
xfree(gt->shared_raw);
gt->shared_raw = NULL;
vfree(gt->maptrack);
gt->maptrack = NULL;
xfree(gt->active);
gt->active = NULL;
- return -ENOMEM;
+ return ret;
}
/*
@@ -3603,6 +3613,8 @@ grant_table_destroy(
if ( t == NULL )
return;
+ gnttab_destroy_arch(t);
+
for ( i = 0; i < nr_grant_frames(t); i++ )
free_xenheap_page(t->shared_raw[i]);
xfree(t->shared_raw);
@@ -3729,7 +3741,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
}
if ( !rc )
- gnttab_set_frame_gfn(d, idx, gfn);
+ gnttab_set_frame_gfn(gt, idx, gfn);
grant_write_unlock(gt);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index b174c65080..ce9b6a4032 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -50,7 +50,6 @@ struct arch_domain
struct p2m_domain p2m;
struct hvm_domain hvm_domain;
- gfn_t *grant_table_gfn;
struct vmmio vmmio;
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 0a248a765a..0870b5b782 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -6,6 +6,10 @@
#define INITIAL_NR_GRANT_FRAMES 4
+struct grant_table_arch {
+ gfn_t *gfn;
+};
+
void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
int create_grant_host_mapping(unsigned long gpaddr,
unsigned long mfn, unsigned int flags, unsigned int
@@ -22,11 +26,19 @@ 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_init_arch(gt) \
+ ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
+ ? 0 : -ENOMEM )
+
+#define gnttab_destroy_arch(gt) \
+ do { \
+ xfree((gt)->arch.gfn); \
+ } while ( 0 )
+
+#define gnttab_set_frame_gfn(gt, idx, gfn) \
+ do { \
+ (gt)->arch.gfn[idx] = gfn; \
+ } while ( 0 )
#define gnttab_create_shared_page(d, t, i) \
do { \
@@ -36,8 +48,8 @@ static inline void gnttab_set_frame_gfn(struct domain *d, unsigned long idx,
} while ( 0 )
#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 >= nr_grant_frames(t)) && \
+ (i < max_grant_frames)) ? 0 : gfn_x(t->arch.gfn[i]))
#define gnttab_need_iommu_mapping(d) \
(is_domain_direct_mapped(d) && need_iommu(d))
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index c865999a33..1b93c5720d 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -14,6 +14,9 @@
#define INITIAL_NR_GRANT_FRAMES 4
+struct grant_table_arch {
+};
+
/*
* Caller must own caller's BIGLOCK, is responsible for flushing the TLB, and
* must hold a reference to the page.
@@ -36,6 +39,10 @@ static inline int replace_grant_host_mapping(uint64_t addr, unsigned long frame,
return replace_grant_pv_mapping(addr, frame, new_addr, flags);
}
+#define gnttab_init_arch(gt) 0
+#define gnttab_destroy_arch(gt) do {} while ( 0 )
+#define gnttab_set_frame_gfn(gt, idx, gfn) do {} while ( 0 )
+
#define gnttab_create_shared_page(d, t, i) \
do { \
share_xen_page_with_guest( \
@@ -75,11 +82,6 @@ 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 df11b31264..d2bd2416c4 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -29,6 +29,8 @@
#include <asm/page.h>
#include <asm/grant_table.h>
+struct grant_table;
+
/* The maximum size of a grant table. */
extern unsigned int max_grant_frames;
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c
2017-09-20 6:34 ` [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c Juergen Gross
@ 2017-09-20 9:25 ` Jan Beulich
2017-09-20 14:34 ` Julien Grall
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 9:25 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> Instead of attaching the ARM specific grant table data to the domain
> structure add it to struct grant_table. Add the needed arch functions
> to the asm-*/grant_table.h includes.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Non-ARM parts
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c
2017-09-20 6:34 ` [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c Juergen Gross
2017-09-20 9:25 ` Jan Beulich
@ 2017-09-20 14:34 ` Julien Grall
2017-09-21 4:36 ` Juergen Gross
1 sibling, 1 reply; 69+ messages in thread
From: Julien Grall @ 2017-09-20 14:34 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 07:34, Juergen Gross wrote:
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 0a248a765a..0870b5b782 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -6,6 +6,10 @@
>
> #define INITIAL_NR_GRANT_FRAMES 4
>
> +struct grant_table_arch {
> + gfn_t *gfn;
> +};
> +
> void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
> int create_grant_host_mapping(unsigned long gpaddr,
> unsigned long mfn, unsigned int flags, unsigned int
> @@ -22,11 +26,19 @@ 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_init_arch(gt)
\
> + ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
I am not sure to understand the 0 here. Don't you check the return of
xzalloc_array? If so it should be NULL if it failed. And therefore the
return error looks inverted below.
> + ? 0 : -ENOMEM )
I admit I would much prefer to see static inline rather than define.
More typesafe and usually easier to read. You cannot do that because the
type of gt is only defined in grant-table?
Nonetheless, I think you could clarify this code by doing:
({
(gt)->arch.gfn = xzalloc_array(....);
( g->arch.gfn ? 0 : -ENOMEM );
)}
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c
2017-09-20 14:34 ` Julien Grall
@ 2017-09-21 4:36 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 4:36 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
On 20/09/17 16:34, Julien Grall wrote:
> Hi Juergen,
>
> On 20/09/17 07:34, Juergen Gross wrote:
>> diff --git a/xen/include/asm-arm/grant_table.h
>> b/xen/include/asm-arm/grant_table.h
>> index 0a248a765a..0870b5b782 100644
>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -6,6 +6,10 @@
>> #define INITIAL_NR_GRANT_FRAMES 4
>> +struct grant_table_arch {
>> + gfn_t *gfn;
>> +};
>> +
>> void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
>> int create_grant_host_mapping(unsigned long gpaddr,
>> unsigned long mfn, unsigned int flags, unsigned int
>> @@ -22,11 +26,19 @@ 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_init_arch(gt)
> \
>> + ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) ==
>> 0 \
>
> I am not sure to understand the 0 here. Don't you check the return of
> xzalloc_array? If so it should be NULL if it failed. And therefore the
> return error looks inverted below.
Indeed.
>
>> + ? 0 : -ENOMEM )
>
> I admit I would much prefer to see static inline rather than define.
> More typesafe and usually easier to read. You cannot do that because the
> type of gt is only defined in grant-table?
Right.
> Nonetheless, I think you could clarify this code by doing:
>
> ({
> (gt)->arch.gfn = xzalloc_array(....);
> ( g->arch.gfn ? 0 : -ENOMEM );
> )}
Yes, this is better.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (11 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 10:34 ` Jan Beulich
[not found] ` <59C2603C020000780017D561@suse.com>
2017-09-20 6:34 ` [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only Juergen Gross
2017-09-20 6:34 ` [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info Juergen Gross
14 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
Instead of using the same global resource limits of grant tables (max.
number of grant frames, max. number of maptrack frames) for all domains
make these limits per domain. Set those per-domain limits in
grant_table_set_limits().
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
V6:
- several changes due to new patch order
V3:
- correct error message (Paul Durrant)
---
xen/common/compat/grant_table.c | 31 +++-------
xen/common/grant_table.c | 121 ++++++++++++++++++++++++--------------
xen/include/asm-arm/grant_table.h | 6 +-
3 files changed, 88 insertions(+), 70 deletions(-)
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index cce3ff0b9a..ff1d678f01 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
unsigned int max_frame_list_size_in_page =
(COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
sizeof(*nat.setup->frame_list.p);
- if ( max_frame_list_size_in_page < max_grant_frames )
- {
- gdprintk(XENLOG_WARNING,
- "max_grant_frames is too large (%u,%u)\n",
- max_grant_frames, max_frame_list_size_in_page);
- rc = -EINVAL;
- }
- else
- {
+
#define XLAT_gnttab_setup_table_HNDL_frame_list(_d_, _s_) \
- set_xen_guest_handle((_d_)->frame_list, (unsigned long *)(nat.setup + 1))
- XLAT_gnttab_setup_table(nat.setup, &cmp.setup);
+ set_xen_guest_handle((_d_)->frame_list, (unsigned long *)(nat.setup + 1))
+ XLAT_gnttab_setup_table(nat.setup, &cmp.setup);
#undef XLAT_gnttab_setup_table_HNDL_frame_list
- rc = gnttab_setup_table(guest_handle_cast(nat.uop, gnttab_setup_table_t), 1);
- }
+ rc = gnttab_setup_table(guest_handle_cast(nat.uop,
+ gnttab_setup_table_t),
+ 1, max_frame_list_size_in_page);
}
ASSERT(rc <= 0);
if ( rc == 0 )
@@ -294,16 +287,6 @@ int compat_grant_table_op(unsigned int cmd,
rc = -EFAULT;
break;
}
- if ( max_frame_list_size_in_pages <
- grant_to_status_frames(max_grant_frames) )
- {
- gdprintk(XENLOG_WARNING,
- "grant_to_status_frames(max_grant_frames) is too large (%u,%u)\n",
- grant_to_status_frames(max_grant_frames),
- max_frame_list_size_in_pages);
- rc = -EINVAL;
- break;
- }
#define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
set_xen_guest_handle((_d_)->frame_list, (uint64_t *)(nat.get_status + 1))
@@ -312,7 +295,7 @@ int compat_grant_table_op(unsigned int cmd,
rc = gnttab_get_status_frames(
guest_handle_cast(nat.uop, gnttab_get_status_frames_t),
- count);
+ count, max_frame_list_size_in_pages);
if ( rc >= 0 )
{
#define XLAT_gnttab_get_status_frames_HNDL_frame_list(_d_, _s_) \
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 26f9a32656..a0d8f32869 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -54,6 +54,9 @@ struct grant_table {
* what version to use yet.
*/
unsigned int gt_version;
+ /* Resource limits of the domain. */
+ unsigned int max_grant_frames;
+ unsigned int max_maptrack_frames;
/* Table size. Number of frames shared with guest */
unsigned int nr_grant_frames;
/* Number of grant status frames shared with guest (for version 2) */
@@ -290,8 +293,8 @@ num_act_frames_from_sha_frames(const unsigned int num)
return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
}
-#define max_nr_active_grant_frames \
- num_act_frames_from_sha_frames(max_grant_frames)
+#define max_nr_active_grant_frames(gt) \
+ num_act_frames_from_sha_frames(gt->max_grant_frames)
static inline unsigned int
nr_active_grant_frames(struct grant_table *gt)
@@ -528,7 +531,7 @@ get_maptrack_handle(
* out of memory, try stealing an entry from another VCPU (in case the
* guest isn't mapping across its VCPUs evenly).
*/
- if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+ if ( nr_maptrack_frames(lgt) < lgt->max_maptrack_frames )
new_mt = alloc_xenheap_page();
if ( !new_mt )
@@ -1667,23 +1670,26 @@ grant_table_init(struct grant_table *gt)
/* Active grant table. */
gt->active = xzalloc_array(struct active_grant_entry *,
- max_nr_active_grant_frames);
+ max_nr_active_grant_frames(gt));
if ( gt->active == NULL )
goto out;
/* Tracking of mapped foreign frames table */
- gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
- if ( gt->maptrack == NULL )
- goto out;
+ if ( gt->max_maptrack_frames )
+ {
+ gt->maptrack = vzalloc(gt->max_maptrack_frames * sizeof(*gt->maptrack));
+ if ( gt->maptrack == NULL )
+ goto out;
+ }
/* Shared grant table. */
- gt->shared_raw = xzalloc_array(void *, max_grant_frames);
+ gt->shared_raw = xzalloc_array(void *, gt->max_grant_frames);
if ( gt->shared_raw == NULL )
goto out;
/* Status pages for grant table - for version 2 */
gt->status = xzalloc_array(grant_status_t *,
- grant_to_status_frames(max_grant_frames));
+ grant_to_status_frames(gt->max_grant_frames));
if ( gt->status == NULL )
goto out;
@@ -1718,8 +1724,9 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
ASSERT(gt->active);
if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
- req_nr_frames = INITIAL_NR_GRANT_FRAMES;
- ASSERT(req_nr_frames <= max_grant_frames);
+ req_nr_frames = min_t(unsigned int, INITIAL_NR_GRANT_FRAMES,
+ gt->max_grant_frames);
+ ASSERT(req_nr_frames <= gt->max_grant_frames);
gdprintk(XENLOG_INFO,
"Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1777,13 +1784,15 @@ active_alloc_failed:
static long
gnttab_setup_table(
- XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
+ XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
+ unsigned int limit_max)
{
struct vcpu *curr = current;
struct gnttab_setup_table op;
struct domain *d = NULL;
struct grant_table *gt;
unsigned int i;
+ long ret = 0;
if ( count != 1 )
return -EINVAL;
@@ -1791,15 +1800,6 @@ gnttab_setup_table(
if ( unlikely(copy_from_guest(&op, uop, 1)) )
return -EFAULT;
- if ( unlikely(op.nr_frames > max_grant_frames) )
- {
- gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
- " per domain.\n",
- max_grant_frames);
- op.status = GNTST_general_error;
- goto out;
- }
-
if ( !guest_handle_okay(op.frame_list, op.nr_frames) )
return -EFAULT;
@@ -1819,6 +1819,21 @@ gnttab_setup_table(
gt = d->grant_table;
grant_write_lock(gt);
+ if ( unlikely(op.nr_frames > gt->max_grant_frames) )
+ {
+ gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table frames.\n",
+ gt->max_grant_frames);
+ op.status = GNTST_general_error;
+ goto unlock;
+ }
+ if ( unlikely(limit_max < gt->max_grant_frames) )
+ {
+ gdprintk(XENLOG_WARNING, "max_grant_frames is too large (%u,%u)\n",
+ gt->max_grant_frames, limit_max);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
if ( gt->gt_version == 0 )
gt->gt_version = 1;
@@ -1829,7 +1844,7 @@ gnttab_setup_table(
{
gdprintk(XENLOG_INFO,
"Expand grant table to %u failed. Current: %u Max: %u\n",
- op.nr_frames, nr_grant_frames(gt), max_grant_frames);
+ op.nr_frames, nr_grant_frames(gt), gt->max_grant_frames);
op.status = GNTST_general_error;
goto unlock;
}
@@ -1852,10 +1867,10 @@ gnttab_setup_table(
if ( d )
rcu_unlock_domain(d);
- if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
+ if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
return -EFAULT;
- return 0;
+ return ret;
}
static long
@@ -1864,6 +1879,7 @@ gnttab_query_size(
{
struct gnttab_query_size op;
struct domain *d;
+ struct grant_table *gt;
if ( count != 1 )
return -EINVAL;
@@ -1884,13 +1900,15 @@ gnttab_query_size(
goto out;
}
- grant_read_lock(d->grant_table);
+ gt = d->grant_table;
- op.nr_frames = nr_grant_frames(d->grant_table);
- op.max_nr_frames = max_grant_frames;
+ grant_read_lock(gt);
+
+ op.nr_frames = nr_grant_frames(gt);
+ op.max_nr_frames = gt->max_grant_frames;
op.status = GNTST_okay;
- grant_read_unlock(d->grant_table);
+ grant_read_unlock(gt);
out:
if ( d )
@@ -2965,14 +2983,14 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
static long
gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
- int count)
+ int count, unsigned int limit_max)
{
gnttab_get_status_frames_t op;
struct domain *d;
struct grant_table *gt;
uint64_t gmfn;
int i;
- int rc;
+ int rc, ret = 0;
if ( count != 1 )
return -EINVAL;
@@ -3012,6 +3030,15 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
goto unlock;
}
+ if ( unlikely(limit_max < grant_to_status_frames(gt->max_grant_frames)) )
+ {
+ gdprintk(XENLOG_WARNING,
+ "grant_to_status_frames(max_grant_frames) is too large (%u,%u)\n",
+ grant_to_status_frames(gt->max_grant_frames), limit_max);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
for ( i = 0; i < op.nr_frames; i++ )
{
gmfn = gnttab_status_gmfn(d, gt, i);
@@ -3024,10 +3051,10 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
out2:
rcu_unlock_domain(d);
out1:
- if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
+ if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
return -EFAULT;
- return 0;
+ return ret;
}
static long
@@ -3320,7 +3347,7 @@ do_grant_table_op(
case GNTTABOP_setup_table:
rc = gnttab_setup_table(
- guest_handle_cast(uop, gnttab_setup_table_t), count);
+ guest_handle_cast(uop, gnttab_setup_table_t), count, ~0);
ASSERT(rc <= 0);
break;
@@ -3369,7 +3396,7 @@ do_grant_table_op(
case GNTTABOP_get_status_frames:
rc = gnttab_get_status_frames(
- guest_handle_cast(uop, gnttab_get_status_frames_t), count);
+ guest_handle_cast(uop, gnttab_get_status_frames_t), count, ~0);
break;
case GNTTABOP_get_version:
@@ -3442,6 +3469,8 @@ grant_table_create(
/* Simple stuff. */
percpu_rwlock_resource_init(&t->lock, grant_rwlock);
spin_lock_init(&t->maptrack_lock);
+ t->max_grant_frames = max_grant_frames;
+ t->max_maptrack_frames = max_maptrack_frames;
/* Okay, install the structure. */
d->grant_table = t;
@@ -3648,6 +3677,8 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
struct grant_table *gt = d->grant_table;
int ret = -EBUSY;
+ if ( !grant_frames )
+ return -EINVAL;
if ( !gt )
return -ENOENT;
@@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
/* Set limits. */
if ( !gt->active )
+ {
+ gt->max_grant_frames = grant_frames;
+ gt->max_maptrack_frames = maptrack_frames;
ret = grant_table_init(gt);
+ }
grant_write_unlock(gt);
@@ -3731,7 +3766,7 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
}
else
{
- if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) )
+ if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
gnttab_grow_table(d, idx + 1);
if ( idx < nr_grant_frames(gt) )
@@ -3759,6 +3794,12 @@ static void gnttab_usage_print(struct domain *rd)
grant_read_lock(gt);
+ printk("grant-table for remote domain:%5d (v%d)\n"
+ " %d frames (%d max), %d maptrack frames (%d max)\n",
+ rd->domain_id, gt->gt_version,
+ nr_grant_frames(gt), gt->max_grant_frames,
+ nr_maptrack_frames(gt), gt->max_maptrack_frames);
+
for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
{
struct active_grant_entry *act;
@@ -3786,12 +3827,7 @@ static void gnttab_usage_print(struct domain *rd)
status = status_entry(gt, ref);
}
- if ( first )
- {
- printk("grant-table for remote domain:%5d (v%d)\n",
- rd->domain_id, gt->gt_version);
- first = 0;
- }
+ first = 0;
/* [0xXXX] ddddd 0xXXXXXX 0xXXXXXXXX ddddd 0xXXXXXX 0xXX */
printk("[0x%03x] %5d 0x%06lx 0x%08x %5d 0x%06"PRIx64" 0x%02x\n",
@@ -3803,8 +3839,7 @@ static void gnttab_usage_print(struct domain *rd)
grant_read_unlock(gt);
if ( first )
- printk("grant-table for remote domain:%5d ... "
- "no active grant table entries\n", rd->domain_id);
+ printk("no active grant table entries\n");
}
static void gnttab_usage_print_all(unsigned char key)
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 0870b5b782..9a60e4e614 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
return 1;
}
-#define gnttab_init_arch(gt) \
- ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
+#define gnttab_init_arch(gt) \
+ ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
? 0 : -ENOMEM )
#define gnttab_destroy_arch(gt) \
@@ -49,7 +49,7 @@ static inline int replace_grant_supported(void)
#define gnttab_shared_gmfn(d, t, i) \
( ((i >= nr_grant_frames(t)) && \
- (i < max_grant_frames)) ? 0 : gfn_x(t->arch.gfn[i]))
+ (i < (t)->max_grant_frames))? 0 : gfn_x((t)->arch.gfn[i]))
#define gnttab_need_iommu_mapping(d) \
(is_domain_direct_mapped(d) && need_iommu(d))
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-20 6:34 ` [PATCH v8 13/15] xen: make grant resource limits per domain Juergen Gross
@ 2017-09-20 10:34 ` Jan Beulich
2017-09-20 14:37 ` Julien Grall
[not found] ` <59C2603C020000780017D561@suse.com>
1 sibling, 1 reply; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 10:34 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
> unsigned int max_frame_list_size_in_page =
> (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
> sizeof(*nat.setup->frame_list.p);
> - if ( max_frame_list_size_in_page < max_grant_frames )
The latest here, but perhaps even earlier I think max_grant_frames
should become static, so one can be reasonably certain that all other
references are gone.
> @@ -290,8 +293,8 @@ num_act_frames_from_sha_frames(const unsigned int num)
> return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
> }
>
> -#define max_nr_active_grant_frames \
> - num_act_frames_from_sha_frames(max_grant_frames)
> +#define max_nr_active_grant_frames(gt) \
> + num_act_frames_from_sha_frames(gt->max_grant_frames)
Parentheses around gt please.
> @@ -1718,8 +1724,9 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
> ASSERT(gt->active);
>
> if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
> - req_nr_frames = INITIAL_NR_GRANT_FRAMES;
> - ASSERT(req_nr_frames <= max_grant_frames);
> + req_nr_frames = min_t(unsigned int, INITIAL_NR_GRANT_FRAMES,
> + gt->max_grant_frames);
Perhaps give the INITIAL_NR_GRANT_FRAMES value a U suffix
instead of using min_t() here?
> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>
> static long
> gnttab_setup_table(
> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
> + unsigned int limit_max)
> {
> struct vcpu *curr = current;
> struct gnttab_setup_table op;
> struct domain *d = NULL;
> struct grant_table *gt;
> unsigned int i;
> + long ret = 0;
Wouldn't int suffice here?
> @@ -1819,6 +1819,21 @@ gnttab_setup_table(
> gt = d->grant_table;
> grant_write_lock(gt);
>
> + if ( unlikely(op.nr_frames > gt->max_grant_frames) )
> + {
> + gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table frames.\n",
%u and you also want to log the subject domain ID (which may not
be current's; same for the other log message below as well as the
similar one in gnttab_get_status_frames()).
> + gt->max_grant_frames);
> + op.status = GNTST_general_error;
> + goto unlock;
> + }
> + if ( unlikely(limit_max < gt->max_grant_frames) )
With the check moved here it can be relaxed afaict: Code below
only writes op.nr_frames entries (same then again for
gnttab_get_status_frames()).
> @@ -1852,10 +1867,10 @@ gnttab_setup_table(
> if ( d )
> rcu_unlock_domain(d);
>
> - if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
> + if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
I wonder whether it wouldn't be better to switch that check
producing -EINVAL to also report the failure in op.status, now
that it lives here (same then for gnttab_get_status_frames()
once again).
> static long
> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
> - int count)
> + int count, unsigned int limit_max)
Take the opportunity and switch count to unsigned int?
> @@ -3320,7 +3347,7 @@ do_grant_table_op(
>
> case GNTTABOP_setup_table:
> rc = gnttab_setup_table(
> - guest_handle_cast(uop, gnttab_setup_table_t), count);
> + guest_handle_cast(uop, gnttab_setup_table_t), count, ~0);
UINT_MAX?
> @@ -3442,6 +3469,8 @@ grant_table_create(
> /* Simple stuff. */
> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
> spin_lock_init(&t->maptrack_lock);
> + t->max_grant_frames = max_grant_frames;
> + t->max_maptrack_frames = max_maptrack_frames;
This together with ...
> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>
> /* Set limits. */
> if ( !gt->active )
> + {
> + gt->max_grant_frames = grant_frames;
> + gt->max_maptrack_frames = maptrack_frames;
> ret = grant_table_init(gt);
> + }
.. this raises the question of whether it is legal to decrease the
limits. There may be code depending on it only ever growing.
Additionally to take the input values without applying some
upper cap - while we have XSA-77, we still shouldn't introduce
new issues making disaggregation more unsafe. Perhaps the
global limits could serve as a cap here?
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
> return 1;
> }
>
> -#define gnttab_init_arch(gt) \
> - ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
> +#define gnttab_init_arch(gt) \
> + ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
Mind switching to use NULL at this occasion?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-20 10:34 ` Jan Beulich
@ 2017-09-20 14:37 ` Julien Grall
0 siblings, 0 replies; 69+ messages in thread
From: Julien Grall @ 2017-09-20 14:37 UTC (permalink / raw)
To: Jan Beulich, Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, xen-devel, dgdegra
Hi Jan,
On 20/09/17 11:34, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
>> return 1;
>> }
>>
>> -#define gnttab_init_arch(gt) \
>> - ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
>> +#define gnttab_init_arch(gt) \
>> + ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
>
> Mind switching to use NULL at this occasion?
This would belong to the patch #12 where it as been introduced.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
[parent not found: <59C2603C020000780017D561@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C2603C020000780017D561@suse.com>
@ 2017-09-20 11:10 ` Juergen Gross
2017-09-20 11:48 ` Jan Beulich
[not found] ` <59C271B9020000780017D620@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 11:10 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 12:34, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
>> unsigned int max_frame_list_size_in_page =
>> (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
>> sizeof(*nat.setup->frame_list.p);
>> - if ( max_frame_list_size_in_page < max_grant_frames )
>
> The latest here, but perhaps even earlier I think max_grant_frames
> should become static, so one can be reasonably certain that all other
> references are gone.
Patch 14 removes the last references, so I can't do it earlier.
>> @@ -290,8 +293,8 @@ num_act_frames_from_sha_frames(const unsigned int num)
>> return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
>> }
>>
>> -#define max_nr_active_grant_frames \
>> - num_act_frames_from_sha_frames(max_grant_frames)
>> +#define max_nr_active_grant_frames(gt) \
>> + num_act_frames_from_sha_frames(gt->max_grant_frames)
>
> Parentheses around gt please.
Okay.
>> @@ -1718,8 +1724,9 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>> ASSERT(gt->active);
>>
>> if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
>> - req_nr_frames = INITIAL_NR_GRANT_FRAMES;
>> - ASSERT(req_nr_frames <= max_grant_frames);
>> + req_nr_frames = min_t(unsigned int, INITIAL_NR_GRANT_FRAMES,
>> + gt->max_grant_frames);
>
> Perhaps give the INITIAL_NR_GRANT_FRAMES value a U suffix
> instead of using min_t() here?
Okay.
>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>
>> static long
>> gnttab_setup_table(
>> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>> + unsigned int limit_max)
>> {
>> struct vcpu *curr = current;
>> struct gnttab_setup_table op;
>> struct domain *d = NULL;
>> struct grant_table *gt;
>> unsigned int i;
>> + long ret = 0;
>
> Wouldn't int suffice here?
I just followed the return type of the function. I think this way is
cleaner, but in case you like int better I can change it.
>> @@ -1819,6 +1819,21 @@ gnttab_setup_table(
>> gt = d->grant_table;
>> grant_write_lock(gt);
>>
>> + if ( unlikely(op.nr_frames > gt->max_grant_frames) )
>> + {
>> + gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table frames.\n",
>
> %u and you also want to log the subject domain ID (which may not
> be current's; same for the other log message below as well as the
> similar one in gnttab_get_status_frames()).
Okay.
>> + gt->max_grant_frames);
>> + op.status = GNTST_general_error;
>> + goto unlock;
>> + }
>> + if ( unlikely(limit_max < gt->max_grant_frames) )
>
> With the check moved here it can be relaxed afaict: Code below
> only writes op.nr_frames entries (same then again for
> gnttab_get_status_frames()).
Okay.
>> @@ -1852,10 +1867,10 @@ gnttab_setup_table(
>> if ( d )
>> rcu_unlock_domain(d);
>>
>> - if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
>> + if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
>
> I wonder whether it wouldn't be better to switch that check
> producing -EINVAL to also report the failure in op.status, now
> that it lives here (same then for gnttab_get_status_frames()
> once again).
Okay.
>> static long
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
>> - int count)
>> + int count, unsigned int limit_max)
>
> Take the opportunity and switch count to unsigned int?
Okay.
>> @@ -3320,7 +3347,7 @@ do_grant_table_op(
>>
>> case GNTTABOP_setup_table:
>> rc = gnttab_setup_table(
>> - guest_handle_cast(uop, gnttab_setup_table_t), count);
>> + guest_handle_cast(uop, gnttab_setup_table_t), count, ~0);
>
> UINT_MAX?
Yes.
>> @@ -3442,6 +3469,8 @@ grant_table_create(
>> /* Simple stuff. */
>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>> spin_lock_init(&t->maptrack_lock);
>> + t->max_grant_frames = max_grant_frames;
>> + t->max_maptrack_frames = max_maptrack_frames;
>
> This together with ...
>
>> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>
>> /* Set limits. */
>> if ( !gt->active )
>> + {
>> + gt->max_grant_frames = grant_frames;
>> + gt->max_maptrack_frames = maptrack_frames;
>> ret = grant_table_init(gt);
>> + }
>
> .. this raises the question of whether it is legal to decrease the
> limits. There may be code depending on it only ever growing.
Before grant_table_init() has been called there is no problem
decreasing the limits, as nothing depending on them has been setup
yet.
> Additionally to take the input values without applying some
> upper cap - while we have XSA-77, we still shouldn't introduce
> new issues making disaggregation more unsafe. Perhaps the
> global limits could serve as a cap here?
I thought about a cap and TBH I'm not sure which would be sane to
apply. The global limits seem wrong, especially looking at patch 14:
those limits will be for dom0 only then. And dom0 won't need many
grant frames in the normal case...
So I could make up a cap in form of either a configurable constant
(CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
preferences here?
>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
>> return 1;
>> }
>>
>> -#define gnttab_init_arch(gt) \
>> - ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0 \
>> +#define gnttab_init_arch(gt) \
>> + ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
>
> Mind switching to use NULL at this occasion?
I'll change it in patch 12 then.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-20 11:10 ` Juergen Gross
@ 2017-09-20 11:48 ` Jan Beulich
[not found] ` <59C271B9020000780017D620@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 11:48 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
> On 20/09/17 12:34, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> --- a/xen/common/compat/grant_table.c
>>> +++ b/xen/common/compat/grant_table.c
>>> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
>>> unsigned int max_frame_list_size_in_page =
>>> (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
>>> sizeof(*nat.setup->frame_list.p);
>>> - if ( max_frame_list_size_in_page < max_grant_frames )
>>
>> The latest here, but perhaps even earlier I think max_grant_frames
>> should become static, so one can be reasonably certain that all other
>> references are gone.
>
> Patch 14 removes the last references, so I can't do it earlier.
That's unfortunate, but okay.
>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>
>>> static long
>>> gnttab_setup_table(
>>> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>> + unsigned int limit_max)
>>> {
>>> struct vcpu *curr = current;
>>> struct gnttab_setup_table op;
>>> struct domain *d = NULL;
>>> struct grant_table *gt;
>>> unsigned int i;
>>> + long ret = 0;
>>
>> Wouldn't int suffice here?
>
> I just followed the return type of the function. I think this way is
> cleaner, but in case you like int better I can change it.
I sort of expected this reply, but that's not in line with what you
did in gnttab_get_status_frames() then. I think we will want to
eventually change all function return types to int where the wider
type isn't needed.
>>> @@ -3442,6 +3469,8 @@ grant_table_create(
>>> /* Simple stuff. */
>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>> spin_lock_init(&t->maptrack_lock);
>>> + t->max_grant_frames = max_grant_frames;
>>> + t->max_maptrack_frames = max_maptrack_frames;
>>
>> This together with ...
>>
>>> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>
>>> /* Set limits. */
>>> if ( !gt->active )
>>> + {
>>> + gt->max_grant_frames = grant_frames;
>>> + gt->max_maptrack_frames = maptrack_frames;
>>> ret = grant_table_init(gt);
>>> + }
>>
>> .. this raises the question of whether it is legal to decrease the
>> limits. There may be code depending on it only ever growing.
>
> Before grant_table_init() has been called there is no problem
> decreasing the limits, as nothing depending on them has been setup
> yet.
Oh, right, I didn't pay attention to this being a one-time action.
>> Additionally to take the input values without applying some
>> upper cap - while we have XSA-77, we still shouldn't introduce
>> new issues making disaggregation more unsafe. Perhaps the
>> global limits could serve as a cap here?
>
> I thought about a cap and TBH I'm not sure which would be sane to
> apply. The global limits seem wrong, especially looking at patch 14:
> those limits will be for dom0 only then. And dom0 won't need many
> grant frames in the normal case...
I've been thinking about this Dom0 aspect too over lunch. What
about allowing the hardware domain to set its limit (only upwards
of course) in setup_table(), without any upper bound enforced?
This would free up the globals to be used as system wide limits
again.
> So I could make up a cap in form of either a configurable constant
> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
> preferences here?
A config constant as well as a fraction of domain memory might lock
out special purpose guests. Which would leave command line options
- as per above perhaps the ones we already have.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C271B9020000780017D620@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C271B9020000780017D620@suse.com>
@ 2017-09-20 12:44 ` Juergen Gross
2017-09-20 15:35 ` Jan Beulich
[not found] ` <59C2A6DE020000780017D874@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 12:44 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 13:48, Jan Beulich wrote:
>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>> On 20/09/17 12:34, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> --- a/xen/common/compat/grant_table.c
>>>> +++ b/xen/common/compat/grant_table.c
>>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>>
>>>> static long
>>>> gnttab_setup_table(
>>>> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>>> + unsigned int limit_max)
>>>> {
>>>> struct vcpu *curr = current;
>>>> struct gnttab_setup_table op;
>>>> struct domain *d = NULL;
>>>> struct grant_table *gt;
>>>> unsigned int i;
>>>> + long ret = 0;
>>>
>>> Wouldn't int suffice here?
>>
>> I just followed the return type of the function. I think this way is
>> cleaner, but in case you like int better I can change it.
>
> I sort of expected this reply, but that's not in line with what you
> did in gnttab_get_status_frames() then. I think we will want to
> eventually change all function return types to int where the wider
> type isn't needed.
Okay. Should I include a patch doing that in this series or would you
prefer this cleanup being delayed to 4.11?
>>>> @@ -3442,6 +3469,8 @@ grant_table_create(
>>>> /* Simple stuff. */
>>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>>> spin_lock_init(&t->maptrack_lock);
>>>> + t->max_grant_frames = max_grant_frames;
>>>> + t->max_maptrack_frames = max_maptrack_frames;
>>>
>>> This together with ...
>>>
>>>> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>>
>>>> /* Set limits. */
>>>> if ( !gt->active )
>>>> + {
>>>> + gt->max_grant_frames = grant_frames;
>>>> + gt->max_maptrack_frames = maptrack_frames;
>>>> ret = grant_table_init(gt);
>>>> + }
>>>
>>> .. this raises the question of whether it is legal to decrease the
>>> limits. There may be code depending on it only ever growing.
>>
>> Before grant_table_init() has been called there is no problem
>> decreasing the limits, as nothing depending on them has been setup
>> yet.
>
> Oh, right, I didn't pay attention to this being a one-time action.
>
>>> Additionally to take the input values without applying some
>>> upper cap - while we have XSA-77, we still shouldn't introduce
>>> new issues making disaggregation more unsafe. Perhaps the
>>> global limits could serve as a cap here?
Thinking more about it: what can happen in worst case when no cap
is being enforced?
A domain with the privilege to create another domain with arbitrary
amounts of memory (we have no cap here) might go crazy and give the
created domain an arbitrary amount of grant frames or maptrack
frames. So what is the difference whether the memory is spent directly
for the domain or via grant frames? In both cases there will be no
memory left for other purposes. I can't see how this would be worse
than allocating the same amount of memory directly for the new domain.
>> I thought about a cap and TBH I'm not sure which would be sane to
>> apply. The global limits seem wrong, especially looking at patch 14:
>> those limits will be for dom0 only then. And dom0 won't need many
>> grant frames in the normal case...
>
> I've been thinking about this Dom0 aspect too over lunch. What
> about allowing the hardware domain to set its limit (only upwards
> of course) in setup_table(), without any upper bound enforced?
> This would free up the globals to be used as system wide limits
> again.
This would be possible, of course.
The question is whether the need to re-allocate the frame pointer arrays
is it worth.
>> So I could make up a cap in form of either a configurable constant
>> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
>> preferences here?
>
> A config constant as well as a fraction of domain memory might lock
> out special purpose guests. Which would leave command line options
> - as per above perhaps the ones we already have.
In case we really need the cap parameters I'd prefer distinct ones from
the dom0 initial values.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-20 12:44 ` Juergen Gross
@ 2017-09-20 15:35 ` Jan Beulich
[not found] ` <59C2A6DE020000780017D874@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 15:35 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
> On 20/09/17 13:48, Jan Beulich wrote:
>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>> On 20/09/17 12:34, Jan Beulich wrote:
>>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>>> --- a/xen/common/compat/grant_table.c
>>>>> +++ b/xen/common/compat/grant_table.c
>>>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>>>
>>>>> static long
>>>>> gnttab_setup_table(
>>>>> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>>> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>>>> + unsigned int limit_max)
>>>>> {
>>>>> struct vcpu *curr = current;
>>>>> struct gnttab_setup_table op;
>>>>> struct domain *d = NULL;
>>>>> struct grant_table *gt;
>>>>> unsigned int i;
>>>>> + long ret = 0;
>>>>
>>>> Wouldn't int suffice here?
>>>
>>> I just followed the return type of the function. I think this way is
>>> cleaner, but in case you like int better I can change it.
>>
>> I sort of expected this reply, but that's not in line with what you
>> did in gnttab_get_status_frames() then. I think we will want to
>> eventually change all function return types to int where the wider
>> type isn't needed.
>
> Okay. Should I include a patch doing that in this series or would you
> prefer this cleanup being delayed to 4.11?
This should be delayed imo - we're past the date where new
non-bug-fix patches should be accepted.
>>>> Additionally to take the input values without applying some
>>>> upper cap - while we have XSA-77, we still shouldn't introduce
>>>> new issues making disaggregation more unsafe. Perhaps the
>>>> global limits could serve as a cap here?
>
> Thinking more about it: what can happen in worst case when no cap
> is being enforced?
>
> A domain with the privilege to create another domain with arbitrary
> amounts of memory (we have no cap here) might go crazy and give the
> created domain an arbitrary amount of grant frames or maptrack
> frames. So what is the difference whether the memory is spent directly
> for the domain or via grant frames? In both cases there will be no
> memory left for other purposes. I can't see how this would be worse
> than allocating the same amount of memory directly for the new domain.
Oh, memory exhaustion wasn't my primary worry, as that's at
least immediately visible. I was rather thinking about long lasting
loop or misbehavior because of arithmetic overflowing somewhere.
>>> I thought about a cap and TBH I'm not sure which would be sane to
>>> apply. The global limits seem wrong, especially looking at patch 14:
>>> those limits will be for dom0 only then. And dom0 won't need many
>>> grant frames in the normal case...
>>
>> I've been thinking about this Dom0 aspect too over lunch. What
>> about allowing the hardware domain to set its limit (only upwards
>> of course) in setup_table(), without any upper bound enforced?
>> This would free up the globals to be used as system wide limits
>> again.
>
> This would be possible, of course.
>
> The question is whether the need to re-allocate the frame pointer arrays
> is it worth.
Input by others would be helpful...
>>> So I could make up a cap in form of either a configurable constant
>>> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
>>> preferences here?
>>
>> A config constant as well as a fraction of domain memory might lock
>> out special purpose guests. Which would leave command line options
>> - as per above perhaps the ones we already have.
>
> In case we really need the cap parameters I'd prefer distinct ones from
> the dom0 initial values.
I agree - reusing what we have would be an option primarily
when we don't need anything for Dom0. But if we need two
sets of options, perhaps keeping the current ones for global
limits and introducing new sub-options to "dom0=" would
perhaps be better.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C2A6DE020000780017D874@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C2A6DE020000780017D874@suse.com>
@ 2017-09-21 4:35 ` Juergen Gross
2017-09-21 6:15 ` Jan Beulich
[not found] ` <59C3751D020000780017DCB6@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 4:35 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 17:35, Jan Beulich wrote:
>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>> On 20/09/17 12:34, Jan Beulich wrote:
>>>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>>>> --- a/xen/common/compat/grant_table.c
>>>>>> +++ b/xen/common/compat/grant_table.c
>>>>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>>>>
>>>>>> static long
>>>>>> gnttab_setup_table(
>>>>>> - XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>>>> + XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>>>>> + unsigned int limit_max)
>>>>>> {
>>>>>> struct vcpu *curr = current;
>>>>>> struct gnttab_setup_table op;
>>>>>> struct domain *d = NULL;
>>>>>> struct grant_table *gt;
>>>>>> unsigned int i;
>>>>>> + long ret = 0;
>>>>>
>>>>> Wouldn't int suffice here?
>>>>
>>>> I just followed the return type of the function. I think this way is
>>>> cleaner, but in case you like int better I can change it.
>>>
>>> I sort of expected this reply, but that's not in line with what you
>>> did in gnttab_get_status_frames() then. I think we will want to
>>> eventually change all function return types to int where the wider
>>> type isn't needed.
>>
>> Okay. Should I include a patch doing that in this series or would you
>> prefer this cleanup being delayed to 4.11?
>
> This should be delayed imo - we're past the date where new
> non-bug-fix patches should be accepted.
>
>>>>> Additionally to take the input values without applying some
>>>>> upper cap - while we have XSA-77, we still shouldn't introduce
>>>>> new issues making disaggregation more unsafe. Perhaps the
>>>>> global limits could serve as a cap here?
>>
>> Thinking more about it: what can happen in worst case when no cap
>> is being enforced?
>>
>> A domain with the privilege to create another domain with arbitrary
>> amounts of memory (we have no cap here) might go crazy and give the
>> created domain an arbitrary amount of grant frames or maptrack
>> frames. So what is the difference whether the memory is spent directly
>> for the domain or via grant frames? In both cases there will be no
>> memory left for other purposes. I can't see how this would be worse
>> than allocating the same amount of memory directly for the new domain.
>
> Oh, memory exhaustion wasn't my primary worry, as that's at
> least immediately visible. I was rather thinking about long lasting
> loop or misbehavior because of arithmetic overflowing somewhere.
Okay, this makes a static cap configurable via boot parameters an
appropriate solution.
>
>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>> grant frames in the normal case...
>>>
>>> I've been thinking about this Dom0 aspect too over lunch. What
>>> about allowing the hardware domain to set its limit (only upwards
>>> of course) in setup_table(), without any upper bound enforced?
>>> This would free up the globals to be used as system wide limits
>>> again.
>>
>> This would be possible, of course.
>>
>> The question is whether the need to re-allocate the frame pointer arrays
>> is it worth.
>
> Input by others would be helpful...
I think I'll go with additional cap boot parameters, so I don't think
we need dom0 to modify its own limits.
>
>>>> So I could make up a cap in form of either a configurable constant
>>>> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
>>>> preferences here?
>>>
>>> A config constant as well as a fraction of domain memory might lock
>>> out special purpose guests. Which would leave command line options
>>> - as per above perhaps the ones we already have.
>>
>> In case we really need the cap parameters I'd prefer distinct ones from
>> the dom0 initial values.
>
> I agree - reusing what we have would be an option primarily
> when we don't need anything for Dom0. But if we need two
> sets of options, perhaps keeping the current ones for global
> limits and introducing new sub-options to "dom0=" would
> perhaps be better.
I agree.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-21 4:35 ` Juergen Gross
@ 2017-09-21 6:15 ` Jan Beulich
[not found] ` <59C3751D020000780017DCB6@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-21 6:15 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
> On 20/09/17 17:35, Jan Beulich wrote:
>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>> grant frames in the normal case...
>>>>
>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>> about allowing the hardware domain to set its limit (only upwards
>>>> of course) in setup_table(), without any upper bound enforced?
>>>> This would free up the globals to be used as system wide limits
>>>> again.
>>>
>>> This would be possible, of course.
>>>
>>> The question is whether the need to re-allocate the frame pointer arrays
>>> is it worth.
>>
>> Input by others would be helpful...
>
> I think I'll go with additional cap boot parameters, so I don't think
> we need dom0 to modify its own limits.
So are we in agreement then that no new command line options
are needed, and that hence the cap will be applicable to all
domains (with Dom0 simply not having any other limit enforced
on it)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C3751D020000780017DCB6@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C3751D020000780017DCB6@suse.com>
@ 2017-09-21 7:53 ` Juergen Gross
2017-09-21 11:31 ` Jan Beulich
[not found] ` <59C3BF28020000780017DE68@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 7:53 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 21/09/17 08:15, Jan Beulich wrote:
>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>> grant frames in the normal case...
>>>>>
>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>> This would free up the globals to be used as system wide limits
>>>>> again.
>>>>
>>>> This would be possible, of course.
>>>>
>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>> is it worth.
>>>
>>> Input by others would be helpful...
>>
>> I think I'll go with additional cap boot parameters, so I don't think
>> we need dom0 to modify its own limits.
>
> So are we in agreement then that no new command line options
> are needed, and that hence the cap will be applicable to all
> domains (with Dom0 simply not having any other limit enforced
> on it)?
Hmm, I meant this to be the other way round: having distinct parameters
for dom0 and the cap.
In case you like it much better to merge them I won't argue over it.
In this case annotating the variables with __init would be moot, of
course.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-21 7:53 ` Juergen Gross
@ 2017-09-21 11:31 ` Jan Beulich
[not found] ` <59C3BF28020000780017DE68@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-21 11:31 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
> On 21/09/17 08:15, Jan Beulich wrote:
>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>> grant frames in the normal case...
>>>>>>
>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>> This would free up the globals to be used as system wide limits
>>>>>> again.
>>>>>
>>>>> This would be possible, of course.
>>>>>
>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>> is it worth.
>>>>
>>>> Input by others would be helpful...
>>>
>>> I think I'll go with additional cap boot parameters, so I don't think
>>> we need dom0 to modify its own limits.
>>
>> So are we in agreement then that no new command line options
>> are needed, and that hence the cap will be applicable to all
>> domains (with Dom0 simply not having any other limit enforced
>> on it)?
>
> Hmm, I meant this to be the other way round: having distinct parameters
> for dom0 and the cap.
>
> In case you like it much better to merge them I won't argue over it.
Well, late yesterday evening it occurred to me that it would
only be consistent to apply the same cap to all domains. That's
in particular to not penalize a non-Dom0 hardware domain in
comparison with Dom0 itself.
> In this case annotating the variables with __init would be moot, of
> course.
Of course.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C3BF28020000780017DE68@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C3BF28020000780017DE68@suse.com>
@ 2017-09-21 11:39 ` Juergen Gross
2017-09-21 11:48 ` Jan Beulich
[not found] ` <59C3C33E020000780017DEC3@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 11:39 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 21/09/17 13:31, Jan Beulich wrote:
>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>> grant frames in the normal case...
>>>>>>>
>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>> again.
>>>>>>
>>>>>> This would be possible, of course.
>>>>>>
>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>> is it worth.
>>>>>
>>>>> Input by others would be helpful...
>>>>
>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>> we need dom0 to modify its own limits.
>>>
>>> So are we in agreement then that no new command line options
>>> are needed, and that hence the cap will be applicable to all
>>> domains (with Dom0 simply not having any other limit enforced
>>> on it)?
>>
>> Hmm, I meant this to be the other way round: having distinct parameters
>> for dom0 and the cap.
>>
>> In case you like it much better to merge them I won't argue over it.
>
> Well, late yesterday evening it occurred to me that it would
> only be consistent to apply the same cap to all domains. That's
> in particular to not penalize a non-Dom0 hardware domain in
> comparison with Dom0 itself.
That's correct.
OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
4 only (why would it need more?) would make sense: the grant frame array
for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
if the cap would be the configuration value for Dom0.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-21 11:39 ` Juergen Gross
@ 2017-09-21 11:48 ` Jan Beulich
[not found] ` <59C3C33E020000780017DEC3@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-21 11:48 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
> On 21/09/17 13:31, Jan Beulich wrote:
>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>> grant frames in the normal case...
>>>>>>>>
>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>> again.
>>>>>>>
>>>>>>> This would be possible, of course.
>>>>>>>
>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>> is it worth.
>>>>>>
>>>>>> Input by others would be helpful...
>>>>>
>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>> we need dom0 to modify its own limits.
>>>>
>>>> So are we in agreement then that no new command line options
>>>> are needed, and that hence the cap will be applicable to all
>>>> domains (with Dom0 simply not having any other limit enforced
>>>> on it)?
>>>
>>> Hmm, I meant this to be the other way round: having distinct parameters
>>> for dom0 and the cap.
>>>
>>> In case you like it much better to merge them I won't argue over it.
>>
>> Well, late yesterday evening it occurred to me that it would
>> only be consistent to apply the same cap to all domains. That's
>> in particular to not penalize a non-Dom0 hardware domain in
>> comparison with Dom0 itself.
>
> That's correct.
>
> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
> 4 only (why would it need more?) would make sense: the grant frame array
> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
> if the cap would be the configuration value for Dom0.
May I suggest that for now we use the simpler variant without
extra Dom0 command line options, and later (post 4.10), if you or
anyone else really feels like it, Dom0 specific options be introduced?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C3C33E020000780017DEC3@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C3C33E020000780017DEC3@suse.com>
@ 2017-09-21 11:51 ` Juergen Gross
2017-09-22 6:19 ` Juergen Gross
1 sibling, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 11:51 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 21/09/17 13:48, Jan Beulich wrote:
>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>
>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>> again.
>>>>>>>>
>>>>>>>> This would be possible, of course.
>>>>>>>>
>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>> is it worth.
>>>>>>>
>>>>>>> Input by others would be helpful...
>>>>>>
>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>> we need dom0 to modify its own limits.
>>>>>
>>>>> So are we in agreement then that no new command line options
>>>>> are needed, and that hence the cap will be applicable to all
>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>> on it)?
>>>>
>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>> for dom0 and the cap.
>>>>
>>>> In case you like it much better to merge them I won't argue over it.
>>>
>>> Well, late yesterday evening it occurred to me that it would
>>> only be consistent to apply the same cap to all domains. That's
>>> in particular to not penalize a non-Dom0 hardware domain in
>>> comparison with Dom0 itself.
>>
>> That's correct.
>>
>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>> 4 only (why would it need more?) would make sense: the grant frame array
>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>> if the cap would be the configuration value for Dom0.
>
> May I suggest that for now we use the simpler variant without
> extra Dom0 command line options, and later (post 4.10), if you or
> anyone else really feels like it, Dom0 specific options be introduced?
NP. I just wanted to point it out.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C3C33E020000780017DEC3@suse.com>
2017-09-21 11:51 ` Juergen Gross
@ 2017-09-22 6:19 ` Juergen Gross
2017-09-22 7:53 ` Jan Beulich
[not found] ` <59C4DD9B020000780017E575@suse.com>
1 sibling, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-22 6:19 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 21/09/17 13:48, Jan Beulich wrote:
>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>
>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>> again.
>>>>>>>>
>>>>>>>> This would be possible, of course.
>>>>>>>>
>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>> is it worth.
>>>>>>>
>>>>>>> Input by others would be helpful...
>>>>>>
>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>> we need dom0 to modify its own limits.
>>>>>
>>>>> So are we in agreement then that no new command line options
>>>>> are needed, and that hence the cap will be applicable to all
>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>> on it)?
>>>>
>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>> for dom0 and the cap.
>>>>
>>>> In case you like it much better to merge them I won't argue over it.
>>>
>>> Well, late yesterday evening it occurred to me that it would
>>> only be consistent to apply the same cap to all domains. That's
>>> in particular to not penalize a non-Dom0 hardware domain in
>>> comparison with Dom0 itself.
>>
>> That's correct.
>>
>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>> 4 only (why would it need more?) would make sense: the grant frame array
>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>> if the cap would be the configuration value for Dom0.
>
> May I suggest that for now we use the simpler variant without
> extra Dom0 command line options, and later (post 4.10), if you or
> anyone else really feels like it, Dom0 specific options be introduced?
While applying these changes to my series I realized this might be a bad
choice for ARM: the dom0 grant table is here limited to about 100 pages.
If there is some need to have a domU with more grant frames the system
wouldn't be able to boot as the high cap would be used for the dom0
grant frame number.
And for being able to cover the xl defaults of the grant frame limits of
huge hosts I need to set the cap to at least 64 now. I'd like to raise
it even more, lets say to 256 or 512, in order to have some headroom.
So I think I'm going with the dom0 sub-option for the max_grant_frames
parameter. I don't think it is needed for max_maptrack_frames, as dom0
will be normally the domain needing the highest value.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-22 6:19 ` Juergen Gross
@ 2017-09-22 7:53 ` Jan Beulich
[not found] ` <59C4DD9B020000780017E575@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-22 7:53 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 22.09.17 at 08:19, <jgross@suse.com> wrote:
> On 21/09/17 13:48, Jan Beulich wrote:
>>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>>
>>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>>> again.
>>>>>>>>>
>>>>>>>>> This would be possible, of course.
>>>>>>>>>
>>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>>> is it worth.
>>>>>>>>
>>>>>>>> Input by others would be helpful...
>>>>>>>
>>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>>> we need dom0 to modify its own limits.
>>>>>>
>>>>>> So are we in agreement then that no new command line options
>>>>>> are needed, and that hence the cap will be applicable to all
>>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>>> on it)?
>>>>>
>>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>>> for dom0 and the cap.
>>>>>
>>>>> In case you like it much better to merge them I won't argue over it.
>>>>
>>>> Well, late yesterday evening it occurred to me that it would
>>>> only be consistent to apply the same cap to all domains. That's
>>>> in particular to not penalize a non-Dom0 hardware domain in
>>>> comparison with Dom0 itself.
>>>
>>> That's correct.
>>>
>>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>>> 4 only (why would it need more?) would make sense: the grant frame array
>>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>>> if the cap would be the configuration value for Dom0.
>>
>> May I suggest that for now we use the simpler variant without
>> extra Dom0 command line options, and later (post 4.10), if you or
>> anyone else really feels like it, Dom0 specific options be introduced?
>
> While applying these changes to my series I realized this might be a bad
> choice for ARM: the dom0 grant table is here limited to about 100 pages.
> If there is some need to have a domU with more grant frames the system
> wouldn't be able to boot as the high cap would be used for the dom0
> grant frame number.
Why can't ARM code lower the Dom0 values without lowering the
caps?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C4DD9B020000780017E575@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C4DD9B020000780017E575@suse.com>
@ 2017-09-22 8:27 ` Juergen Gross
2017-09-22 8:35 ` Jan Beulich
[not found] ` <59C4E78B020000780017E67D@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-22 8:27 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 22/09/17 09:53, Jan Beulich wrote:
>>>> On 22.09.17 at 08:19, <jgross@suse.com> wrote:
>> On 21/09/17 13:48, Jan Beulich wrote:
>>>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>>>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>>>
>>>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>>>> again.
>>>>>>>>>>
>>>>>>>>>> This would be possible, of course.
>>>>>>>>>>
>>>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>>>> is it worth.
>>>>>>>>>
>>>>>>>>> Input by others would be helpful...
>>>>>>>>
>>>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>>>> we need dom0 to modify its own limits.
>>>>>>>
>>>>>>> So are we in agreement then that no new command line options
>>>>>>> are needed, and that hence the cap will be applicable to all
>>>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>>>> on it)?
>>>>>>
>>>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>>>> for dom0 and the cap.
>>>>>>
>>>>>> In case you like it much better to merge them I won't argue over it.
>>>>>
>>>>> Well, late yesterday evening it occurred to me that it would
>>>>> only be consistent to apply the same cap to all domains. That's
>>>>> in particular to not penalize a non-Dom0 hardware domain in
>>>>> comparison with Dom0 itself.
>>>>
>>>> That's correct.
>>>>
>>>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>>>> 4 only (why would it need more?) would make sense: the grant frame array
>>>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>>>> if the cap would be the configuration value for Dom0.
>>>
>>> May I suggest that for now we use the simpler variant without
>>> extra Dom0 command line options, and later (post 4.10), if you or
>>> anyone else really feels like it, Dom0 specific options be introduced?
>>
>> While applying these changes to my series I realized this might be a bad
>> choice for ARM: the dom0 grant table is here limited to about 100 pages.
>> If there is some need to have a domU with more grant frames the system
>> wouldn't be able to boot as the high cap would be used for the dom0
>> grant frame number.
>
> Why can't ARM code lower the Dom0 values without lowering the
> caps?
So either we let control the max_grant_frames value the cap _and_ the
dom0 value or not. We could handle this differently on ARM, of course,
but this would mean that the dom0 value on ARM wouldn't be adjustable
other than as a compile time option. Or we could do that on x86, too.
For setting a compile time value of dom0 I'd go with a rather low value
like INITIAL_NR_GRANT_FRAMES.
In the end having a sub-option for dom0 isn't that complicated, IMO.
I'm open for all possibilities mentioned above.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-22 8:27 ` Juergen Gross
@ 2017-09-22 8:35 ` Jan Beulich
[not found] ` <59C4E78B020000780017E67D@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-22 8:35 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 22.09.17 at 10:27, <jgross@suse.com> wrote:
> On 22/09/17 09:53, Jan Beulich wrote:
>>>>> On 22.09.17 at 08:19, <jgross@suse.com> wrote:
>>> On 21/09/17 13:48, Jan Beulich wrote:
>>>>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>>>>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>>>>
>>>>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>>>>> again.
>>>>>>>>>>>
>>>>>>>>>>> This would be possible, of course.
>>>>>>>>>>>
>>>>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>>>>> is it worth.
>>>>>>>>>>
>>>>>>>>>> Input by others would be helpful...
>>>>>>>>>
>>>>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>>>>> we need dom0 to modify its own limits.
>>>>>>>>
>>>>>>>> So are we in agreement then that no new command line options
>>>>>>>> are needed, and that hence the cap will be applicable to all
>>>>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>>>>> on it)?
>>>>>>>
>>>>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>>>>> for dom0 and the cap.
>>>>>>>
>>>>>>> In case you like it much better to merge them I won't argue over it.
>>>>>>
>>>>>> Well, late yesterday evening it occurred to me that it would
>>>>>> only be consistent to apply the same cap to all domains. That's
>>>>>> in particular to not penalize a non-Dom0 hardware domain in
>>>>>> comparison with Dom0 itself.
>>>>>
>>>>> That's correct.
>>>>>
>>>>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>>>>> 4 only (why would it need more?) would make sense: the grant frame array
>>>>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>>>>> if the cap would be the configuration value for Dom0.
>>>>
>>>> May I suggest that for now we use the simpler variant without
>>>> extra Dom0 command line options, and later (post 4.10), if you or
>>>> anyone else really feels like it, Dom0 specific options be introduced?
>>>
>>> While applying these changes to my series I realized this might be a bad
>>> choice for ARM: the dom0 grant table is here limited to about 100 pages.
>>> If there is some need to have a domU with more grant frames the system
>>> wouldn't be able to boot as the high cap would be used for the dom0
>>> grant frame number.
>>
>> Why can't ARM code lower the Dom0 values without lowering the
>> caps?
>
> So either we let control the max_grant_frames value the cap _and_ the
> dom0 value or not. We could handle this differently on ARM, of course,
> but this would mean that the dom0 value on ARM wouldn't be adjustable
> other than as a compile time option.
Why? If the specified value is lower than the about 100 pages
allow for, it could still take effect.
> Or we could do that on x86, too.
Not without an actual need to, I would say.
> For setting a compile time value of dom0 I'd go with a rather low value
> like INITIAL_NR_GRANT_FRAMES.
>
> In the end having a sub-option for dom0 isn't that complicated, IMO.
That's true, but the inflation of command line options is by itself
worrying to me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C4E78B020000780017E67D@suse.com>]
* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
[not found] ` <59C4E78B020000780017E67D@suse.com>
@ 2017-09-22 8:44 ` Juergen Gross
2017-09-22 8:51 ` Jan Beulich
0 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-22 8:44 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 22/09/17 10:35, Jan Beulich wrote:
>>>> On 22.09.17 at 10:27, <jgross@suse.com> wrote:
>> On 22/09/17 09:53, Jan Beulich wrote:
>>>>>> On 22.09.17 at 08:19, <jgross@suse.com> wrote:
>>>> On 21/09/17 13:48, Jan Beulich wrote:
>>>>>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>>>>>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>>>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>>>>>> again.
>>>>>>>>>>>>
>>>>>>>>>>>> This would be possible, of course.
>>>>>>>>>>>>
>>>>>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>>>>>> is it worth.
>>>>>>>>>>>
>>>>>>>>>>> Input by others would be helpful...
>>>>>>>>>>
>>>>>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>>>>>> we need dom0 to modify its own limits.
>>>>>>>>>
>>>>>>>>> So are we in agreement then that no new command line options
>>>>>>>>> are needed, and that hence the cap will be applicable to all
>>>>>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>>>>>> on it)?
>>>>>>>>
>>>>>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>>>>>> for dom0 and the cap.
>>>>>>>>
>>>>>>>> In case you like it much better to merge them I won't argue over it.
>>>>>>>
>>>>>>> Well, late yesterday evening it occurred to me that it would
>>>>>>> only be consistent to apply the same cap to all domains. That's
>>>>>>> in particular to not penalize a non-Dom0 hardware domain in
>>>>>>> comparison with Dom0 itself.
>>>>>>
>>>>>> That's correct.
>>>>>>
>>>>>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>>>>>> 4 only (why would it need more?) would make sense: the grant frame array
>>>>>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>>>>>> if the cap would be the configuration value for Dom0.
>>>>>
>>>>> May I suggest that for now we use the simpler variant without
>>>>> extra Dom0 command line options, and later (post 4.10), if you or
>>>>> anyone else really feels like it, Dom0 specific options be introduced?
>>>>
>>>> While applying these changes to my series I realized this might be a bad
>>>> choice for ARM: the dom0 grant table is here limited to about 100 pages.
>>>> If there is some need to have a domU with more grant frames the system
>>>> wouldn't be able to boot as the high cap would be used for the dom0
>>>> grant frame number.
>>>
>>> Why can't ARM code lower the Dom0 values without lowering the
>>> caps?
>>
>> So either we let control the max_grant_frames value the cap _and_ the
>> dom0 value or not. We could handle this differently on ARM, of course,
>> but this would mean that the dom0 value on ARM wouldn't be adjustable
>> other than as a compile time option.
>
> Why? If the specified value is lower than the about 100 pages
> allow for, it could still take effect.
So you would like to use the lower value of max_grant_frames and the
maximum possible value on ARM for dom0?
Doesn't seem to be the worst option: instead of refusing to boot like
today in case someone entered a value too high it would just use a
sane value.
>
>> Or we could do that on x86, too.
>
> Not without an actual need to, I would say.
>
>> For setting a compile time value of dom0 I'd go with a rather low value
>> like INITIAL_NR_GRANT_FRAMES.
>>
>> In the end having a sub-option for dom0 isn't that complicated, IMO.
>
> That's true, but the inflation of command line options is by itself
> worrying to me.
Okay.
BTW: would you mind me making the cap values modifiable at runtime? I
think this could be a nice feature requiring just to use
integer_runtime_param() instead of integer_param().
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 13/15] xen: make grant resource limits per domain
2017-09-22 8:44 ` Juergen Gross
@ 2017-09-22 8:51 ` Jan Beulich
0 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-22 8:51 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 22.09.17 at 10:44, <jgross@suse.com> wrote:
> On 22/09/17 10:35, Jan Beulich wrote:
>>>>> On 22.09.17 at 10:27, <jgross@suse.com> wrote:
>>> On 22/09/17 09:53, Jan Beulich wrote:
>>>>>>> On 22.09.17 at 08:19, <jgross@suse.com> wrote:
>>>>> On 21/09/17 13:48, Jan Beulich wrote:
>>>>>>>>> On 21.09.17 at 13:39, <jgross@suse.com> wrote:
>>>>>>> On 21/09/17 13:31, Jan Beulich wrote:
>>>>>>>>>>> On 21.09.17 at 09:53, <jgross@suse.com> wrote:
>>>>>>>>> On 21/09/17 08:15, Jan Beulich wrote:
>>>>>>>>>>>>> On 21.09.17 at 06:35, <jgross@suse.com> wrote:
>>>>>>>>>>> On 20/09/17 17:35, Jan Beulich wrote:
>>>>>>>>>>>>>>> On 20.09.17 at 14:44, <jgross@suse.com> wrote:
>>>>>>>>>>>>> On 20/09/17 13:48, Jan Beulich wrote:
>>>>>>>>>>>>>>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>>>>>>>>>>>>>>> I thought about a cap and TBH I'm not sure which would be sane to
>>>>>>>>>>>>>>> apply. The global limits seem wrong, especially looking at patch 14:
>>>>>>>>>>>>>>> those limits will be for dom0 only then. And dom0 won't need many
>>>>>>>>>>>>>>> grant frames in the normal case...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I've been thinking about this Dom0 aspect too over lunch. What
>>>>>>>>>>>>>> about allowing the hardware domain to set its limit (only upwards
>>>>>>>>>>>>>> of course) in setup_table(), without any upper bound enforced?
>>>>>>>>>>>>>> This would free up the globals to be used as system wide limits
>>>>>>>>>>>>>> again.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This would be possible, of course.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The question is whether the need to re-allocate the frame pointer arrays
>>>>>>>>>>>>> is it worth.
>>>>>>>>>>>>
>>>>>>>>>>>> Input by others would be helpful...
>>>>>>>>>>>
>>>>>>>>>>> I think I'll go with additional cap boot parameters, so I don't think
>>>>>>>>>>> we need dom0 to modify its own limits.
>>>>>>>>>>
>>>>>>>>>> So are we in agreement then that no new command line options
>>>>>>>>>> are needed, and that hence the cap will be applicable to all
>>>>>>>>>> domains (with Dom0 simply not having any other limit enforced
>>>>>>>>>> on it)?
>>>>>>>>>
>>>>>>>>> Hmm, I meant this to be the other way round: having distinct parameters
>>>>>>>>> for dom0 and the cap.
>>>>>>>>>
>>>>>>>>> In case you like it much better to merge them I won't argue over it.
>>>>>>>>
>>>>>>>> Well, late yesterday evening it occurred to me that it would
>>>>>>>> only be consistent to apply the same cap to all domains. That's
>>>>>>>> in particular to not penalize a non-Dom0 hardware domain in
>>>>>>>> comparison with Dom0 itself.
>>>>>>>
>>>>>>> That's correct.
>>>>>>>
>>>>>>> OTOH e.g. a cap of lets say 1024 grant frames but Dom0 configured to
>>>>>>> 4 only (why would it need more?) would make sense: the grant frame array
>>>>>>> for Dom0 would need 32 bytes only instead of the 8kB for the 1024 frames
>>>>>>> if the cap would be the configuration value for Dom0.
>>>>>>
>>>>>> May I suggest that for now we use the simpler variant without
>>>>>> extra Dom0 command line options, and later (post 4.10), if you or
>>>>>> anyone else really feels like it, Dom0 specific options be introduced?
>>>>>
>>>>> While applying these changes to my series I realized this might be a bad
>>>>> choice for ARM: the dom0 grant table is here limited to about 100 pages.
>>>>> If there is some need to have a domU with more grant frames the system
>>>>> wouldn't be able to boot as the high cap would be used for the dom0
>>>>> grant frame number.
>>>>
>>>> Why can't ARM code lower the Dom0 values without lowering the
>>>> caps?
>>>
>>> So either we let control the max_grant_frames value the cap _and_ the
>>> dom0 value or not. We could handle this differently on ARM, of course,
>>> but this would mean that the dom0 value on ARM wouldn't be adjustable
>>> other than as a compile time option.
>>
>> Why? If the specified value is lower than the about 100 pages
>> allow for, it could still take effect.
>
> So you would like to use the lower value of max_grant_frames and the
> maximum possible value on ARM for dom0?
>
> Doesn't seem to be the worst option: instead of refusing to boot like
> today in case someone entered a value too high it would just use a
> sane value.
Indeed.
>>> Or we could do that on x86, too.
>>
>> Not without an actual need to, I would say.
>>
>>> For setting a compile time value of dom0 I'd go with a rather low value
>>> like INITIAL_NR_GRANT_FRAMES.
>>>
>>> In the end having a sub-option for dom0 isn't that complicated, IMO.
>>
>> That's true, but the inflation of command line options is by itself
>> worrying to me.
>
> Okay.
>
> BTW: would you mind me making the cap values modifiable at runtime? I
> think this could be a nice feature requiring just to use
> integer_runtime_param() instead of integer_param().
That's a good idea - go for it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (12 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 13/15] xen: make grant resource limits per domain Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 12:07 ` Jan Beulich
[not found] ` <59C27619020000780017D66F@suse.com>
2017-09-20 6:34 ` [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info Juergen Gross
14 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
The boot parameters gnttab_max_frames and gnttab_max_maptrack_frames
are used for dom0 only now, as all other domains require a
XEN_DOMCTL_set_gnttab_limits call now. So make that explicit by
setting the boot values for dom0 only.
While updating the documentation regarding new scope of the boot
parameters remove the documentation of gnttab_max_nr_frames as it
isn't existing any longer.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V7:
- add boot parameter documentation changes
---
docs/misc/xen-command-line.markdown | 15 +++------------
xen/arch/arm/domain_build.c | 2 +-
xen/common/grant_table.c | 35 ++++++++++++++---------------------
xen/include/xen/grant_table.h | 4 +---
4 files changed, 19 insertions(+), 37 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9797c8db2d..7bdc4119a1 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -877,26 +877,17 @@ Specify which console gdbstub should use. See **console**.
> Default: `32`
-Specify the maximum number of frames which any domain may use as part
+Specify the maximum number of frames which dom0 may use as part
of its grant table.
### gnttab\_max\_maptrack\_frames
> `= <integer>`
-> Default: `8 * gnttab_max_frames`
+> Default: `1024`
-Specify the maximum number of frames to use as part of a domains
+Specify the maximum number of frames to use as part of dom0's
maptrack array.
-### gnttab\_max\_nr\_frames
-> `= <integer>`
-
-*Deprecated*
-Use **gnttab\_max\_frames** and **gnttab\_max\_maptrack\_frames** instead.
-
-Specify the maximum number of frames per grant table operation and the
-maximum number of maptrack frames domain.
-
### guest\_loglvl
> `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c34238ec1b..295a539780 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2098,7 +2098,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 ( grant_table_verify_size(d, kinfo->gnttab_size >> PAGE_SHIFT) )
panic("Cannot find a space for the grant table region\n");
#ifdef CONFIG_ARM_32
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a0d8f32869..65084d7f5a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -84,21 +84,8 @@ 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);
-
-/* The maximum number of grant mappings is defined as a multiplier of the
- * maximum number of grant table entries. This defines the multiplier used.
- * Pretty arbitrary. [POLICY]
- * As gnttab_max_nr_frames has been deprecated, this multiplier is deprecated too.
- * New options allow to set max_maptrack_frames and
- * map_grant_table_frames independently.
- */
#define DEFAULT_MAX_MAPTRACK_FRAMES 1024
-static unsigned int __read_mostly max_maptrack_frames;
-integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
-
/*
* Note that the three values below are effectively part of the ABI, even if
* we don't need to make them a formal part of it: A guest suspended for
@@ -3462,6 +3449,10 @@ grant_table_create(
struct domain *d)
{
struct grant_table *t;
+ static unsigned int max_grant_frames;
+ static unsigned int max_maptrack_frames;
+ integer_param("gnttab_max_frames", max_grant_frames);
+ integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
if ( (t = xzalloc(struct grant_table)) == NULL )
return -ENOMEM;
@@ -3469,14 +3460,17 @@ grant_table_create(
/* Simple stuff. */
percpu_rwlock_resource_init(&t->lock, grant_rwlock);
spin_lock_init(&t->maptrack_lock);
- t->max_grant_frames = max_grant_frames;
- t->max_maptrack_frames = max_maptrack_frames;
/* Okay, install the structure. */
d->grant_table = t;
if ( d->domain_id == 0 )
+ {
+ t->max_grant_frames = max_grant_frames ? : DEFAULT_MAX_NR_GRANT_FRAMES;
+ t->max_maptrack_frames =
+ max_maptrack_frames ? : DEFAULT_MAX_MAPTRACK_FRAMES;
return grant_table_init(t);
+ }
return 0;
}
@@ -3855,18 +3849,17 @@ 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_maptrack_frames )
- max_maptrack_frames = DEFAULT_MAX_MAPTRACK_FRAMES;
-
register_keyhandler('g', gnttab_usage_print_all,
"print grant table usage", 1);
return 0;
}
__initcall(gnttab_usage_init);
+bool __init grant_table_verify_size(struct domain *d, unsigned int frames)
+{
+ return d->grant_table->max_grant_frames > frames;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index d2bd2416c4..04a4d82e71 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -31,9 +31,6 @@
struct grant_table;
-/* The maximum size of a grant table. */
-extern unsigned int max_grant_frames;
-
/* Create/destroy per-domain grant table context. */
int grant_table_create(
struct domain *d);
@@ -42,6 +39,7 @@ void grant_table_destroy(
void grant_table_init_vcpu(struct vcpu *v);
int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
unsigned int maptrack_frames);
+bool grant_table_verify_size(struct domain *d, unsigned int frames);
/*
* Check if domain has active grants and log first 10 of them.
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
2017-09-20 6:34 ` [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only Juergen Gross
@ 2017-09-20 12:07 ` Jan Beulich
[not found] ` <59C27619020000780017D66F@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 12:07 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2098,7 +2098,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 ( grant_table_verify_size(d, kinfo->gnttab_size >> PAGE_SHIFT) )
> panic("Cannot find a space for the grant table region\n");
I can see how this may cause complications with my alternative
proposal for the meaning of the command line options, but that's
solvable for sure.
> @@ -3462,6 +3449,10 @@ grant_table_create(
> struct domain *d)
> {
> struct grant_table *t;
> + static unsigned int max_grant_frames;
> + static unsigned int max_maptrack_frames;
> + integer_param("gnttab_max_frames", max_grant_frames);
> + integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
>
> if ( (t = xzalloc(struct grant_table)) == NULL )
> return -ENOMEM;
> @@ -3469,14 +3460,17 @@ grant_table_create(
> /* Simple stuff. */
> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
> spin_lock_init(&t->maptrack_lock);
> - t->max_grant_frames = max_grant_frames;
> - t->max_maptrack_frames = max_maptrack_frames;
>
> /* Okay, install the structure. */
> d->grant_table = t;
>
> if ( d->domain_id == 0 )
> + {
> + t->max_grant_frames = max_grant_frames ? : DEFAULT_MAX_NR_GRANT_FRAMES;
> + t->max_maptrack_frames =
> + max_maptrack_frames ? : DEFAULT_MAX_MAPTRACK_FRAMES;
> return grant_table_init(t);
> + }
The use here makes it that in effect the two variables could then
become __initdata. Maybe their definition should then even move
here (if the other proposal would be discarded).
> +bool __init grant_table_verify_size(struct domain *d, unsigned int frames)
const
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C27619020000780017D66F@suse.com>]
* Re: [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
[not found] ` <59C27619020000780017D66F@suse.com>
@ 2017-09-20 12:48 ` Juergen Gross
2017-09-20 15:36 ` Jan Beulich
[not found] ` <59C2A72D020000780017D881@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 12:48 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 14:07, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2098,7 +2098,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 ( grant_table_verify_size(d, kinfo->gnttab_size >> PAGE_SHIFT) )
>> panic("Cannot find a space for the grant table region\n");
>
> I can see how this may cause complications with my alternative
> proposal for the meaning of the command line options, but that's
> solvable for sure.
>
>> @@ -3462,6 +3449,10 @@ grant_table_create(
>> struct domain *d)
>> {
>> struct grant_table *t;
>> + static unsigned int max_grant_frames;
>> + static unsigned int max_maptrack_frames;
>> + integer_param("gnttab_max_frames", max_grant_frames);
>> + integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
>>
>> if ( (t = xzalloc(struct grant_table)) == NULL )
>> return -ENOMEM;
>> @@ -3469,14 +3460,17 @@ grant_table_create(
>> /* Simple stuff. */
>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>> spin_lock_init(&t->maptrack_lock);
>> - t->max_grant_frames = max_grant_frames;
>> - t->max_maptrack_frames = max_maptrack_frames;
>>
>> /* Okay, install the structure. */
>> d->grant_table = t;
>>
>> if ( d->domain_id == 0 )
>> + {
>> + t->max_grant_frames = max_grant_frames ? : DEFAULT_MAX_NR_GRANT_FRAMES;
>> + t->max_maptrack_frames =
>> + max_maptrack_frames ? : DEFAULT_MAX_MAPTRACK_FRAMES;
>> return grant_table_init(t);
>> + }
>
> The use here makes it that in effect the two variables could then
> become __initdata. Maybe their definition should then even move
> here (if the other proposal would be discarded).
Is it possible in the hypervisor to access __initdata variables from non
__init functions? In the Linux kernel this is forbidden.
>> +bool __init grant_table_verify_size(struct domain *d, unsigned int frames)
>
> const
Yes.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
2017-09-20 12:48 ` Juergen Gross
@ 2017-09-20 15:36 ` Jan Beulich
[not found] ` <59C2A72D020000780017D881@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 15:36 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 14:48, <jgross@suse.com> wrote:
> On 20/09/17 14:07, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> @@ -3469,14 +3460,17 @@ grant_table_create(
>>> /* Simple stuff. */
>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>> spin_lock_init(&t->maptrack_lock);
>>> - t->max_grant_frames = max_grant_frames;
>>> - t->max_maptrack_frames = max_maptrack_frames;
>>>
>>> /* Okay, install the structure. */
>>> d->grant_table = t;
>>>
>>> if ( d->domain_id == 0 )
>>> + {
>>> + t->max_grant_frames = max_grant_frames ? : DEFAULT_MAX_NR_GRANT_FRAMES;
>>> + t->max_maptrack_frames =
>>> + max_maptrack_frames ? : DEFAULT_MAX_MAPTRACK_FRAMES;
>>> return grant_table_init(t);
>>> + }
>>
>> The use here makes it that in effect the two variables could then
>> become __initdata. Maybe their definition should then even move
>> here (if the other proposal would be discarded).
>
> Is it possible in the hypervisor to access __initdata variables from non
> __init functions? In the Linux kernel this is forbidden.
In Xen this is possible, and there are examples. Iirc in Linux this
breaking the build is also just optional.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C2A72D020000780017D881@suse.com>]
* Re: [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
[not found] ` <59C2A72D020000780017D881@suse.com>
@ 2017-09-21 4:27 ` Juergen Gross
2017-09-21 6:16 ` Jan Beulich
0 siblings, 1 reply; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 4:27 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 17:36, Jan Beulich wrote:
>>>> On 20.09.17 at 14:48, <jgross@suse.com> wrote:
>> On 20/09/17 14:07, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> @@ -3469,14 +3460,17 @@ grant_table_create(
>>>> /* Simple stuff. */
>>>> percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>>> spin_lock_init(&t->maptrack_lock);
>>>> - t->max_grant_frames = max_grant_frames;
>>>> - t->max_maptrack_frames = max_maptrack_frames;
>>>>
>>>> /* Okay, install the structure. */
>>>> d->grant_table = t;
>>>>
>>>> if ( d->domain_id == 0 )
>>>> + {
>>>> + t->max_grant_frames = max_grant_frames ? : DEFAULT_MAX_NR_GRANT_FRAMES;
>>>> + t->max_maptrack_frames =
>>>> + max_maptrack_frames ? : DEFAULT_MAX_MAPTRACK_FRAMES;
>>>> return grant_table_init(t);
>>>> + }
>>>
>>> The use here makes it that in effect the two variables could then
>>> become __initdata. Maybe their definition should then even move
>>> here (if the other proposal would be discarded).
>>
>> Is it possible in the hypervisor to access __initdata variables from non
>> __init functions? In the Linux kernel this is forbidden.
>
> In Xen this is possible, and there are examples. Iirc in Linux this
> breaking the build is also just optional.
Yes, there you have the __ref annotation for __init stuff accessible
from non-__init areas.
IMHO it would be better to add:
static int __init grant_table_init_dom0()
hiding the two variables. This makes it more obvious that path is taken
only at boot time.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only
2017-09-21 4:27 ` Juergen Gross
@ 2017-09-21 6:16 ` Jan Beulich
0 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-21 6:16 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 21.09.17 at 06:27, <jgross@suse.com> wrote:
> IMHO it would be better to add:
>
> static int __init grant_table_init_dom0()
>
> hiding the two variables. This makes it more obvious that path is taken
> only at boot time.
Oh, that's fine of course, and hence appreciated (but it obviously
won't make the init -> non-init reference go away).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
2017-09-20 6:34 [PATCH v8 00/15] xen: better grant v2 support Juergen Gross
` (13 preceding siblings ...)
2017-09-20 6:34 ` [PATCH v8 14/15] xen: make grant table limits boot parameters dom0 only Juergen Gross
@ 2017-09-20 6:34 ` Juergen Gross
2017-09-20 12:18 ` Jan Beulich
` (2 more replies)
14 siblings, 3 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 6:34 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
andrew.cooper3, ian.jackson, tim, julien.grall, jbeulich,
dgdegra
On very large hosts a guest needs to know whether it will have to
handle frame numbers larger than 32 bits in order to select the
appropriate grant interface version.
Add a new Xen specific CPUID node to contain the maximum guest address
width similar to the x86 CPUID node 0x80000008 containing the maximum
physical address width. The maximum frame width needs to take memory
hotplug into account.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/traps.c | 4 ++++
xen/include/public/arch-x86/cpuid.h | 11 ++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6091f239ce..7d1e0a872c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -929,6 +929,10 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
res->b = v->vcpu_id;
break;
+ case 5: /* Host specific parameters */
+ res->a = generic_flsl(get_upper_mfn_bound() - 1) + PAGE_SHIFT;
+ break;
+
default:
ASSERT_UNREACHABLE();
}
diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index d709340f18..11eaa4ebfd 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -85,6 +85,15 @@
#define XEN_HVM_CPUID_IOMMU_MAPPINGS (1u << 2)
#define XEN_HVM_CPUID_VCPU_ID_PRESENT (1u << 3) /* vcpu id is present in EBX */
-#define XEN_CPUID_MAX_NUM_LEAVES 4
+/*
+ * Leaf 6 (0x40000x05)
+ * Host specific parameters
+ * EAX: bits 0-7: max guest address width
+ */
+
+/* Max. address width in bits taking memory hotplug into account. */
+#define XEN_CPUID_GUEST_ADDRESS_WIDTH_MASK (255u << 0)
+
+#define XEN_CPUID_MAX_NUM_LEAVES 5
#endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
2017-09-20 6:34 ` [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info Juergen Gross
@ 2017-09-20 12:18 ` Jan Beulich
[not found] ` <59C278A3020000780017D689@suse.com>
2017-09-20 13:00 ` Julien Grall
2 siblings, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 12:18 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
> On very large hosts a guest needs to know whether it will have to
... a PV guest ...
> handle frame numbers larger than 32 bits in order to select the
> appropriate grant interface version.
>
> Add a new Xen specific CPUID node to contain the maximum guest address
> width
"guest address width" is ambiguous here, the more when looking at
what you actually return. We should no longer allow ourselves to
mix up the different address spaces. The limit you want to report
here is that in MFN space, which ought to be of no relevance to
HVM guests. Therefore I'm against uniformly exposing this (as much
as almost no other host property should have any relevance for
HVM guests), and would instead like to see a PV-only leaf just like
we already have a HVM only one.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -929,6 +929,10 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> res->b = v->vcpu_id;
> break;
>
> + case 5: /* Host specific parameters */
> + res->a = generic_flsl(get_upper_mfn_bound() - 1) + PAGE_SHIFT;
> + break;
Already when looking at the patch introducing the function I was
wondering whether the function wouldn't better return the highest
frame number instead of the first invalid one. From an abstract
perspective this would allow an arch (e.g. ARM32) to report that
all addresses are valid.
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -85,6 +85,15 @@
> #define XEN_HVM_CPUID_IOMMU_MAPPINGS (1u << 2)
> #define XEN_HVM_CPUID_VCPU_ID_PRESENT (1u << 3) /* vcpu id is present in EBX
> */
>
> -#define XEN_CPUID_MAX_NUM_LEAVES 4
> +/*
> + * Leaf 6 (0x40000x05)
> + * Host specific parameters
> + * EAX: bits 0-7: max guest address width
> + */
> +
> +/* Max. address width in bits taking memory hotplug into account. */
> +#define XEN_CPUID_GUEST_ADDRESS_WIDTH_MASK (255u << 0)
Please use hex numbers for multi-digit masks, and please make
explicit (at least in the comment, perhaps also in the constant's
name) which "address" is being talked about here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C278A3020000780017D689@suse.com>]
* Re: [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
[not found] ` <59C278A3020000780017D689@suse.com>
@ 2017-09-20 12:58 ` Juergen Gross
2017-09-20 15:42 ` Jan Beulich
[not found] ` <59C2A880020000780017D8A2@suse.com>
0 siblings, 2 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-20 12:58 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 14:18, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>> On very large hosts a guest needs to know whether it will have to
>
> ... a PV guest ...
What about a HVM guest with (potentially) more than 16TB?
>> handle frame numbers larger than 32 bits in order to select the
>> appropriate grant interface version.
>>
>> Add a new Xen specific CPUID node to contain the maximum guest address
>> width
>
> "guest address width" is ambiguous here, the more when looking at
> what you actually return. We should no longer allow ourselves to
> mix up the different address spaces.
I've chosen "guest address width" similar to the "guest frame number"
we already have: it is MFN based for PV and PFN based for HVM (and ARM).
I'm open for a better name.
> The limit you want to report
> here is that in MFN space, which ought to be of no relevance to
> HVM guests. Therefore I'm against uniformly exposing this (as much
> as almost no other host property should have any relevance for
> HVM guests), and would instead like to see a PV-only leaf just like
> we already have a HVM only one.
As said above: a HVM guest needs to know whether it will have to deal
with frame numbers >32 bits, too.
For HVM guests this would just be a hint that the host might be large
enough for this to happen, as even today a HVM guest could in theory
reorganize its memory map to have parts of the memory above the 16TB
border even with only rather small amounts of memory. But this would
be the problem of the guest then.
For the future I could envision a per-domain parameter setting the
upper limit where a guest could put its memory. This parameter would
then influence the new CPUID value fro HVM domains, of course.
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -929,6 +929,10 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>> res->b = v->vcpu_id;
>> break;
>>
>> + case 5: /* Host specific parameters */
>> + res->a = generic_flsl(get_upper_mfn_bound() - 1) + PAGE_SHIFT;
>> + break;
>
> Already when looking at the patch introducing the function I was
> wondering whether the function wouldn't better return the highest
> frame number instead of the first invalid one. From an abstract
> perspective this would allow an arch (e.g. ARM32) to report that
> all addresses are valid.
That's fine with me.
>> --- a/xen/include/public/arch-x86/cpuid.h
>> +++ b/xen/include/public/arch-x86/cpuid.h
>> @@ -85,6 +85,15 @@
>> #define XEN_HVM_CPUID_IOMMU_MAPPINGS (1u << 2)
>> #define XEN_HVM_CPUID_VCPU_ID_PRESENT (1u << 3) /* vcpu id is present in EBX
>> */
>>
>> -#define XEN_CPUID_MAX_NUM_LEAVES 4
>> +/*
>> + * Leaf 6 (0x40000x05)
>> + * Host specific parameters
>> + * EAX: bits 0-7: max guest address width
>> + */
>> +
>> +/* Max. address width in bits taking memory hotplug into account. */
>> +#define XEN_CPUID_GUEST_ADDRESS_WIDTH_MASK (255u << 0)
>
> Please use hex numbers for multi-digit masks, and please make
> explicit (at least in the comment, perhaps also in the constant's
> name) which "address" is being talked about here.
Okay.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
2017-09-20 12:58 ` Juergen Gross
@ 2017-09-20 15:42 ` Jan Beulich
[not found] ` <59C2A880020000780017D8A2@suse.com>
1 sibling, 0 replies; 69+ messages in thread
From: Jan Beulich @ 2017-09-20 15:42 UTC (permalink / raw)
To: Juergen Gross
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
>>> On 20.09.17 at 14:58, <jgross@suse.com> wrote:
> On 20/09/17 14:18, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> On very large hosts a guest needs to know whether it will have to
>>
>> ... a PV guest ...
>
> What about a HVM guest with (potentially) more than 16TB?
Such a guest knows how much memory it has without querying Xen.
>>> handle frame numbers larger than 32 bits in order to select the
>>> appropriate grant interface version.
>>>
>>> Add a new Xen specific CPUID node to contain the maximum guest address
>>> width
>>
>> "guest address width" is ambiguous here, the more when looking at
>> what you actually return. We should no longer allow ourselves to
>> mix up the different address spaces.
>
> I've chosen "guest address width" similar to the "guest frame number"
> we already have: it is MFN based for PV and PFN based for HVM (and ARM).
> I'm open for a better name.
If the interface is needed for other than PV, then the term likely
is fine. But for a PV only interface I'd prefer it to be "machine
address".
>> The limit you want to report
>> here is that in MFN space, which ought to be of no relevance to
>> HVM guests. Therefore I'm against uniformly exposing this (as much
>> as almost no other host property should have any relevance for
>> HVM guests), and would instead like to see a PV-only leaf just like
>> we already have a HVM only one.
>
> As said above: a HVM guest needs to know whether it will have to deal
> with frame numbers >32 bits, too.
>
> For HVM guests this would just be a hint that the host might be large
> enough for this to happen, as even today a HVM guest could in theory
> reorganize its memory map to have parts of the memory above the 16TB
> border even with only rather small amounts of memory. But this would
> be the problem of the guest then.
A HVM guest booted with less than 16Tb and then being pushed
up beyond that boundary would still know in advance that this
could happen - the SRAT table would tell it what hotplug regions
there are.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread[parent not found: <59C2A880020000780017D8A2@suse.com>]
* Re: [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
[not found] ` <59C2A880020000780017D8A2@suse.com>
@ 2017-09-21 4:21 ` Juergen Gross
0 siblings, 0 replies; 69+ messages in thread
From: Juergen Gross @ 2017-09-21 4:21 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, julien.grall, xen-devel, dgdegra
On 20/09/17 17:42, Jan Beulich wrote:
>>>> On 20.09.17 at 14:58, <jgross@suse.com> wrote:
>> On 20/09/17 14:18, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> On very large hosts a guest needs to know whether it will have to
>>>
>>> ... a PV guest ...
>>
>> What about a HVM guest with (potentially) more than 16TB?
>
> Such a guest knows how much memory it has without querying Xen.
>
>>>> handle frame numbers larger than 32 bits in order to select the
>>>> appropriate grant interface version.
>>>>
>>>> Add a new Xen specific CPUID node to contain the maximum guest address
>>>> width
>>>
>>> "guest address width" is ambiguous here, the more when looking at
>>> what you actually return. We should no longer allow ourselves to
>>> mix up the different address spaces.
>>
>> I've chosen "guest address width" similar to the "guest frame number"
>> we already have: it is MFN based for PV and PFN based for HVM (and ARM).
>> I'm open for a better name.
>
> If the interface is needed for other than PV, then the term likely
> is fine. But for a PV only interface I'd prefer it to be "machine
> address".
>
>>> The limit you want to report
>>> here is that in MFN space, which ought to be of no relevance to
>>> HVM guests. Therefore I'm against uniformly exposing this (as much
>>> as almost no other host property should have any relevance for
>>> HVM guests), and would instead like to see a PV-only leaf just like
>>> we already have a HVM only one.
>>
>> As said above: a HVM guest needs to know whether it will have to deal
>> with frame numbers >32 bits, too.
>>
>> For HVM guests this would just be a hint that the host might be large
>> enough for this to happen, as even today a HVM guest could in theory
>> reorganize its memory map to have parts of the memory above the 16TB
>> border even with only rather small amounts of memory. But this would
>> be the problem of the guest then.
>
> A HVM guest booted with less than 16Tb and then being pushed
> up beyond that boundary would still know in advance that this
> could happen - the SRAT table would tell it what hotplug regions
> there are.
Okay, lets go that route then.
HVM guests need to query current memory map and/or the SRAT table in
order to decide which grant interface to use, while PV guests have to
use the CPUID leaf, which will be a PV specific one.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info
2017-09-20 6:34 ` [PATCH v8 15/15] xen: add new Xen cpuid node for max address width info Juergen Gross
2017-09-20 12:18 ` Jan Beulich
[not found] ` <59C278A3020000780017D689@suse.com>
@ 2017-09-20 13:00 ` Julien Grall
2 siblings, 0 replies; 69+ messages in thread
From: Julien Grall @ 2017-09-20 13:00 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
ian.jackson, tim, jbeulich, dgdegra
Hi Juergen,
On 20/09/17 07:34, Juergen Gross wrote:
> On very large hosts a guest needs to know whether it will have to
> handle frame numbers larger than 32 bits in order to select the
> appropriate grant interface version.
Can you explain why you decided to have an arch specific interface
rather than generic one?
On Arm, we would likely need to make same decision for large hosts.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 69+ messages in thread