* [PATCH v21 0/2] guest resource mapping (reprise)
@ 2018-08-08 9:00 Paul Durrant
2018-08-08 9:00 ` [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
2018-08-08 9:00 ` [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
0 siblings, 2 replies; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 9:00 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant
These patches are the patches from my original resource mapping series
that did not make it into 4.11.
Paul Durrant (2):
common: add a new mappable resource type: XENMEM_resource_grant_table
tools/libxenctrl: use new xenforeignmemory API to seed grant table
tools/libxc/include/xc_dom.h | 8 +--
tools/libxc/xc_dom_boot.c | 114 +++++++++++++++++++++++++-----------
tools/libxc/xc_sr_restore_x86_hvm.c | 10 ++--
tools/libxc/xc_sr_restore_x86_pv.c | 2 +-
tools/libxl/libxl_dom.c | 1 -
tools/python/xen/lowlevel/xc/xc.c | 6 +-
xen/common/grant_table.c | 95 ++++++++++++++++++++++++++----
xen/common/memory.c | 55 ++++++++++++++++-
xen/include/public/memory.h | 6 ++
xen/include/xen/grant_table.h | 4 ++
10 files changed, 238 insertions(+), 63 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 9:00 [PATCH v21 0/2] guest resource mapping (reprise) Paul Durrant
@ 2018-08-08 9:00 ` Paul Durrant
2018-08-08 10:10 ` Andrew Cooper
2018-08-08 10:30 ` Jan Beulich
2018-08-08 9:00 ` [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
1 sibling, 2 replies; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 9:00 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Paul Durrant, Jan Beulich
This patch allows grant table frames to be mapped using the
XENMEM_acquire_resource memory op.
NOTE: This patch expands the on-stack mfn_list array in acquire_resource()
but it is still small enough to remain on-stack.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
v21:
- Prevent PVH/HVM tools domains from mapping any non-caller-owned resource
unless the tools domain is also the hardware domain.
- Grow the grant table appropriately whether it is a shared frame or
a status frame that is being mapped.
- Fix comment style breakage in memory.h.
- Move implicit version setting to gnttab_get_shared_frame().
v19:
- Add test to prevent PVH/HVM tools domains mapping grant status frames
this way as the mapping infrastructure in Xen does not yet implement the
necessary reference counting to make this safe.
- Make sure grant table version is set before any attempt to grow the table.
v18:
- Non-trivial re-base of grant table code.
- Dropped Jan's R-b because of the grant table changes.
v13:
- Re-work the internals to avoid using the XENMAPIDX_grant_table_status
hack.
v12:
- Dropped limit checks as requested by Jan.
v10:
- Addressed comments from Jan.
v8:
- The functionality was originally incorporated into the earlier patch
"x86/mm: add HYPERVISOR_memory_op to acquire guest resources".
---
xen/common/grant_table.c | 95 +++++++++++++++++++++++++++++++++++++------
xen/common/memory.c | 55 ++++++++++++++++++++++++-
xen/include/public/memory.h | 6 +++
xen/include/xen/grant_table.h | 4 ++
4 files changed, 146 insertions(+), 14 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d9ec711c73..8e623ea08e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -358,6 +358,12 @@ 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);
}
+/* Number of grant table entries. Caller must hold d's gr. table lock.*/
+static inline unsigned int status_to_grant_frames(unsigned int status_frames)
+{
+ return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, GRANT_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).
@@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
}
#endif
+/* caller must hold write lock */
+static int gnttab_get_status_frame_mfn(struct domain *d,
+ unsigned long idx, mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+
+ ASSERT(gt->gt_version == 2);
+
+ if ( idx >= nr_status_frames(gt) )
+ {
+ unsigned long nr = status_to_grant_frames(idx + 1);
+
+ if ( nr <= gt->max_grant_frames )
+ gnttab_grow_table(d, nr);
+
+ if ( idx >= nr_status_frames(gt) )
+ return -EINVAL;
+ }
+
+ *mfn = _mfn(virt_to_mfn(gt->status[idx]));
+ return 0;
+}
+
+/* caller must hold write lock */
+static int gnttab_get_shared_frame_mfn(struct domain *d,
+ unsigned long idx, mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+
+ ASSERT(gt->gt_version != 0);
+
+ if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
+ gnttab_grow_table(d, idx + 1);
+
+ if ( idx >= nr_grant_frames(gt) )
+ return -EINVAL;
+
+ *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
+ return 0;
+}
+
int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
mfn_t *mfn)
{
@@ -3877,21 +3924,11 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
{
idx &= ~XENMAPIDX_grant_table_status;
status = true;
- 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 < gt->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;
+ rc = gnttab_get_status_frame_mfn(d, idx, mfn);
}
+ else
+ rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
if ( !rc && paging_mode_translate(d) &&
!gfn_eq(gnttab_get_frame_gfn(gt, status, idx), INVALID_GFN) )
@@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
return rc;
}
+int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+ int rc;
+
+ grant_write_lock(gt);
+
+ if ( gt->gt_version == 0 )
+ gt->gt_version = 1;
+
+ rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
+
+ grant_write_unlock(gt);
+
+ return rc;
+}
+
+int gnttab_get_status_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+ int rc;
+
+ grant_write_lock(gt);
+ rc = (gt->gt_version == 2) ?
+ gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
+ grant_write_unlock(gt);
+
+ return rc;
+}
+
static void gnttab_usage_print(struct domain *rd)
{
int first = 1;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index e29d596727..427f65a5e1 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -23,6 +23,7 @@
#include <xen/numa.h>
#include <xen/mem_access.h>
#include <xen/trace.h>
+#include <xen/grant_table.h>
#include <asm/current.h>
#include <asm/hardirq.h>
#include <asm/p2m.h>
@@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
}
+static int acquire_grant_table(struct domain *d, unsigned int id,
+ unsigned long frame,
+ unsigned int nr_frames,
+ xen_pfn_t mfn_list[])
+{
+ unsigned int i = nr_frames;
+
+ /* Iterate backwards in case table needs to grow */
+ while ( i-- != 0 )
+ {
+ mfn_t mfn = INVALID_MFN;
+ int rc;
+
+ switch ( id )
+ {
+ case XENMEM_resource_grant_table_id_shared:
+ rc = gnttab_get_shared_frame(d, frame + i, &mfn);
+ break;
+
+ case XENMEM_resource_grant_table_id_status:
+ rc = gnttab_get_status_frame(d, frame + i, &mfn);
+ break;
+
+ default:
+ rc = -EINVAL;
+ break;
+ }
+
+ if ( rc )
+ return rc;
+
+ mfn_list[i] = mfn_x(mfn);
+ }
+
+ return 0;
+}
+
static int acquire_resource(
XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
{
@@ -992,7 +1030,7 @@ static int acquire_resource(
* moment since they are small, but if they need to grow in future
* use-cases then per-CPU arrays or heap allocations may be required.
*/
- xen_pfn_t mfn_list[2];
+ xen_pfn_t mfn_list[32];
int rc;
if ( copy_from_guest(&xmar, arg, 1) )
@@ -1027,6 +1065,11 @@ static int acquire_resource(
switch ( xmar.type )
{
+ case XENMEM_resource_grant_table:
+ rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
+ mfn_list);
+ break;
+
default:
rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame,
xmar.nr_frames, mfn_list, &xmar.flags);
@@ -1046,6 +1089,16 @@ static int acquire_resource(
xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
unsigned int i;
+ /*
+ * FIXME: until foreign pages inserted into the P2M are properly
+ * reference counted, it is unsafe to allow mapping of
+ * non-caller-owned resource pages unless the caller is
+ * the hardware domain.
+ */
+ if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
+ !is_hardware_domain(currd) )
+ return -EOPNOTSUPP;
+
if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
rc = -EFAULT;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index bf2f81faae..8fc27ceeab 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -611,14 +611,20 @@ struct xen_mem_acquire_resource {
uint16_t type;
#define XENMEM_resource_ioreq_server 0
+#define XENMEM_resource_grant_table 1
/*
* IN - a type-specific resource identifier, which must be zero
* unless stated otherwise.
*
* type == XENMEM_resource_ioreq_server -> id == ioreq server id
+ * type == XENMEM_resource_grant_table -> id defined below
*/
uint32_t id;
+
+#define XENMEM_resource_grant_table_id_shared 0
+#define XENMEM_resource_grant_table_id_status 1
+
/*
* IN/OUT - As an IN parameter number of frames of the resource
* to be mapped. However, if the specified value is 0 and
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 0286ba33dd..c881414e5b 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -58,6 +58,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
mfn_t *mfn);
+int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn);
+int gnttab_get_status_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn);
unsigned int gnttab_dom0_frames(void);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table
2018-08-08 9:00 [PATCH v21 0/2] guest resource mapping (reprise) Paul Durrant
2018-08-08 9:00 ` [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
@ 2018-08-08 9:00 ` Paul Durrant
2018-08-08 9:58 ` Andrew Cooper
1 sibling, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 9:00 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson
A previous patch added support for priv-mapping guest resources directly
(rather than having to foreign-map, which requires P2M modification for
HVM guests).
This patch makes use of the new API to seed the guest grant table unless
the underlying infrastructure (i.e. privcmd) doesn't support it, in which
case the old scheme is used.
NOTE: The call to xc_dom_gnttab_hvm_seed() in hvm_build_set_params() was
actually unnecessary, as the grant table has already been seeded
by a prior call to xc_dom_gnttab_init() made by libxl__build_dom().
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
v18:
- Trivial re-base.
v13:
- Re-base.
v10:
- Use new id constant for grant table.
v4:
- Minor cosmetic fix suggested by Roger.
v3:
- Introduced xc_dom_set_gnttab_entry() to avoid duplicated code.
---
tools/libxc/include/xc_dom.h | 8 +--
tools/libxc/xc_dom_boot.c | 114 +++++++++++++++++++++++++-----------
tools/libxc/xc_sr_restore_x86_hvm.c | 10 ++--
tools/libxc/xc_sr_restore_x86_pv.c | 2 +-
tools/libxl/libxl_dom.c | 1 -
tools/python/xen/lowlevel/xc/xc.c | 6 +-
6 files changed, 92 insertions(+), 49 deletions(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 8a66889c68..a8a0c0da66 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -337,12 +337,8 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
int xc_dom_boot_image(struct xc_dom_image *dom);
int xc_dom_compat_check(struct xc_dom_image *dom);
int xc_dom_gnttab_init(struct xc_dom_image *dom);
-int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
- xen_pfn_t console_gmfn,
- xen_pfn_t xenstore_gmfn,
- uint32_t console_domid,
- uint32_t xenstore_domid);
-int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
+int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
+ bool is_hvm,
xen_pfn_t console_gmfn,
xen_pfn_t xenstore_gmfn,
uint32_t console_domid,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 2e5681dc5d..8307ebeaf6 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -256,11 +256,29 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, uint32_t domid)
return gmfn;
}
-int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
- xen_pfn_t console_gmfn,
- xen_pfn_t xenstore_gmfn,
- uint32_t console_domid,
- uint32_t xenstore_domid)
+static void xc_dom_set_gnttab_entry(xc_interface *xch,
+ grant_entry_v1_t *gnttab,
+ unsigned int idx,
+ uint32_t guest_domid,
+ uint32_t backend_domid,
+ xen_pfn_t backend_gmfn)
+{
+ if ( guest_domid == backend_domid || backend_gmfn == -1)
+ return;
+
+ xc_dom_printf(xch, "%s: [%u] -> 0x%"PRI_xen_pfn,
+ __FUNCTION__, idx, backend_gmfn);
+
+ gnttab[idx].flags = GTF_permit_access;
+ gnttab[idx].domid = backend_domid;
+ gnttab[idx].frame = backend_gmfn;
+}
+
+static int compat_gnttab_seed(xc_interface *xch, uint32_t domid,
+ xen_pfn_t console_gmfn,
+ xen_pfn_t xenstore_gmfn,
+ uint32_t console_domid,
+ uint32_t xenstore_domid)
{
xen_pfn_t gnttab_gmfn;
@@ -284,18 +302,10 @@ int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
return -1;
}
- if ( domid != console_domid && console_gmfn != -1)
- {
- gnttab[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
- gnttab[GNTTAB_RESERVED_CONSOLE].domid = console_domid;
- gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gmfn;
- }
- if ( domid != xenstore_domid && xenstore_gmfn != -1)
- {
- gnttab[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
- gnttab[GNTTAB_RESERVED_XENSTORE].domid = xenstore_domid;
- gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gmfn;
- }
+ xc_dom_set_gnttab_entry(xch, gnttab, GNTTAB_RESERVED_CONSOLE,
+ domid, console_domid, console_gmfn);
+ xc_dom_set_gnttab_entry(xch, gnttab, GNTTAB_RESERVED_XENSTORE,
+ domid, xenstore_domid, xenstore_gmfn);
if ( munmap(gnttab, PAGE_SIZE) == -1 )
{
@@ -313,11 +323,11 @@ int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
return 0;
}
-int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
- xen_pfn_t console_gpfn,
- xen_pfn_t xenstore_gpfn,
- uint32_t console_domid,
- uint32_t xenstore_domid)
+static int compat_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
+ xen_pfn_t console_gpfn,
+ xen_pfn_t xenstore_gpfn,
+ uint32_t console_domid,
+ uint32_t xenstore_domid)
{
int rc;
xen_pfn_t scratch_gpfn;
@@ -356,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
return -1;
}
- rc = xc_dom_gnttab_seed(xch, domid,
+ rc = compat_gnttab_seed(xch, domid,
console_gpfn, xenstore_gpfn,
console_domid, xenstore_domid);
if (rc != 0)
@@ -381,18 +391,56 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
return 0;
}
-int xc_dom_gnttab_init(struct xc_dom_image *dom)
+int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
+ bool is_hvm, xen_pfn_t console_gmfn,
+ xen_pfn_t xenstore_gmfn, uint32_t console_domid,
+ uint32_t xenstore_domid)
{
- if ( xc_dom_translated(dom) ) {
- return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
- dom->console_pfn, dom->xenstore_pfn,
- dom->console_domid, dom->xenstore_domid);
- } else {
- return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
- xc_dom_p2m(dom, dom->console_pfn),
- xc_dom_p2m(dom, dom->xenstore_pfn),
- dom->console_domid, dom->xenstore_domid);
+ xenforeignmemory_handle* fmem = xch->fmem;
+ xenforeignmemory_resource_handle *fres;
+ void *addr = NULL;
+
+ fres = xenforeignmemory_map_resource(
+ fmem, guest_domid, XENMEM_resource_grant_table,
+ XENMEM_resource_grant_table_id_shared, 0, 1, &addr,
+ PROT_READ | PROT_WRITE, 0);
+ if ( !fres )
+ {
+ if ( errno == EOPNOTSUPP )
+ return is_hvm ?
+ compat_gnttab_hvm_seed(xch, guest_domid,
+ console_gmfn, xenstore_gmfn,
+ console_domid, xenstore_domid) :
+ compat_gnttab_seed(xch, guest_domid,
+ console_gmfn, xenstore_gmfn,
+ console_domid, xenstore_domid);
+
+ xc_dom_panic(xch, XC_INTERNAL_ERROR,
+ "%s: failed to acquire grant table "
+ "[errno=%d]\n",
+ __FUNCTION__, errno);
+ return -1;
}
+
+ xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_CONSOLE,
+ guest_domid, console_domid, console_gmfn);
+ xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_XENSTORE,
+ guest_domid, xenstore_domid, xenstore_gmfn);
+
+ xenforeignmemory_unmap_resource(fmem, fres);
+
+ return 0;
+}
+
+int xc_dom_gnttab_init(struct xc_dom_image *dom)
+{
+ bool is_hvm = xc_dom_translated(dom);
+ xen_pfn_t console_gmfn = xc_dom_p2m(dom, dom->console_pfn);
+ xen_pfn_t xenstore_gmfn = xc_dom_p2m(dom, dom->xenstore_pfn);
+
+ return xc_dom_gnttab_seed(dom->xch, dom->guest_domid, is_hvm,
+ console_gmfn, xenstore_gmfn,
+ dom->console_domid, dom->xenstore_domid);
}
/*
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 227c48553e..4765a52f33 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -216,11 +216,11 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
return rc;
}
- rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
- ctx->restore.console_gfn,
- ctx->restore.xenstore_gfn,
- ctx->restore.console_domid,
- ctx->restore.xenstore_domid);
+ rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
+ ctx->restore.console_gfn,
+ ctx->restore.xenstore_gfn,
+ ctx->restore.console_domid,
+ ctx->restore.xenstore_domid);
if ( rc )
{
PERROR("Failed to seed grant table");
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index d81dfdcca6..a2dbf85157 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -1105,7 +1105,7 @@ static int x86_pv_stream_complete(struct xc_sr_context *ctx)
if ( rc )
return rc;
- rc = xc_dom_gnttab_seed(xch, ctx->domid,
+ rc = xc_dom_gnttab_seed(xch, ctx->domid, false,
ctx->restore.console_gfn,
ctx->restore.xenstore_gfn,
ctx->restore.console_domid,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 3cfe0d4808..c8a1dc7fd5 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -905,7 +905,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
*store_mfn = str_mfn;
*console_mfn = cons_mfn;
- xc_dom_gnttab_hvm_seed(handle, domid, *console_mfn, *store_mfn, console_domid, store_domid);
return 0;
}
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index fc19ee0741..5ade12762a 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -800,9 +800,9 @@ static PyObject *pyxc_gnttab_hvm_seed(XcObject *self,
&console_domid, &xenstore_domid) )
return NULL;
- if ( xc_dom_gnttab_hvm_seed(self->xc_handle, dom,
- console_gmfn, xenstore_gmfn,
- console_domid, xenstore_domid) != 0 )
+ if ( xc_dom_gnttab_seed(self->xc_handle, dom, true,
+ console_gmfn, xenstore_gmfn,
+ console_domid, xenstore_domid) != 0 )
return pyxc_error_to_exception(self->xc_handle);
return Py_None;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table
2018-08-08 9:00 ` [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
@ 2018-08-08 9:58 ` Andrew Cooper
2018-08-08 13:00 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 9:58 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Ian Jackson
On 08/08/18 10:00, Paul Durrant wrote:
> A previous patch added support for priv-mapping guest resources directly
> (rather than having to foreign-map, which requires P2M modification for
> HVM guests).
>
> This patch makes use of the new API to seed the guest grant table unless
> the underlying infrastructure (i.e. privcmd) doesn't support it, in which
> case the old scheme is used.
>
> NOTE: The call to xc_dom_gnttab_hvm_seed() in hvm_build_set_params() was
> actually unnecessary, as the grant table has already been seeded
> by a prior call to xc_dom_gnttab_init() made by libxl__build_dom().
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
Some minor style issues, all of which can fixed on commit (probably).
Everything else looks fine.
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 8a66889c68..a8a0c0da66 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -337,12 +337,8 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
> int xc_dom_boot_image(struct xc_dom_image *dom);
> int xc_dom_compat_check(struct xc_dom_image *dom);
> int xc_dom_gnttab_init(struct xc_dom_image *dom);
> -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> - xen_pfn_t console_gmfn,
> - xen_pfn_t xenstore_gmfn,
> - uint32_t console_domid,
> - uint32_t xenstore_domid);
> -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> + bool is_hvm,
> xen_pfn_t console_gmfn,
> xen_pfn_t xenstore_gmfn,
As you're rewriting most of the functionality, can we switch to gfn to
correct the terminology?
> uint32_t console_domid,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 2e5681dc5d..8307ebeaf6 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -256,11 +256,29 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, uint32_t domid)
> return gmfn;
> }
>
> -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> - xen_pfn_t console_gmfn,
> - xen_pfn_t xenstore_gmfn,
> - uint32_t console_domid,
> - uint32_t xenstore_domid)
> +static void xc_dom_set_gnttab_entry(xc_interface *xch,
> + grant_entry_v1_t *gnttab,
> + unsigned int idx,
> + uint32_t guest_domid,
> + uint32_t backend_domid,
> + xen_pfn_t backend_gmfn)
gfn
> +{
> + if ( guest_domid == backend_domid || backend_gmfn == -1)
Space at the end.
> + return;
> +
> + xc_dom_printf(xch, "%s: [%u] -> 0x%"PRI_xen_pfn,
> + __FUNCTION__, idx, backend_gmfn);
__func__ rather than __FUNCTION__. Also, "[%u] => d%d 0x"PRI_xen_pfn
would be more helpful for diagnostics. (I do realise that backend
domid's were retrofitted without adjusting the printf().)
> +
> + gnttab[idx].flags = GTF_permit_access;
> + gnttab[idx].domid = backend_domid;
> + gnttab[idx].frame = backend_gmfn;
> +}
> +
> +static int compat_gnttab_seed(xc_interface *xch, uint32_t domid,
compat_gnttab_pv_seed() ?
> + xen_pfn_t console_gmfn,
> + xen_pfn_t xenstore_gmfn,
gfn
> + uint32_t console_domid,
> + uint32_t xenstore_domid)
> {
>
> xen_pfn_t gnttab_gmfn;
> @@ -313,11 +323,11 @@ int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> return 0;
> }
>
> -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> - xen_pfn_t console_gpfn,
> - xen_pfn_t xenstore_gpfn,
> - uint32_t console_domid,
> - uint32_t xenstore_domid)
> +static int compat_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> + xen_pfn_t console_gpfn,
> + xen_pfn_t xenstore_gpfn,
gfn.
> + uint32_t console_domid,
> + uint32_t xenstore_domid)
> {
> int rc;
> xen_pfn_t scratch_gpfn;
> @@ -356,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> return -1;
> }
>
> - rc = xc_dom_gnttab_seed(xch, domid,
> + rc = compat_gnttab_seed(xch, domid,
> console_gpfn, xenstore_gpfn,
> console_domid, xenstore_domid);
> if (rc != 0)
> @@ -381,18 +391,56 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> return 0;
> }
>
> -int xc_dom_gnttab_init(struct xc_dom_image *dom)
> +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> + bool is_hvm, xen_pfn_t console_gmfn,
> + xen_pfn_t xenstore_gmfn, uint32_t console_domid,
gfn.
> + uint32_t xenstore_domid)
> {
> - if ( xc_dom_translated(dom) ) {
> - return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
> - dom->console_pfn, dom->xenstore_pfn,
> - dom->console_domid, dom->xenstore_domid);
> - } else {
> - return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
> - xc_dom_p2m(dom, dom->console_pfn),
> - xc_dom_p2m(dom, dom->xenstore_pfn),
> - dom->console_domid, dom->xenstore_domid);
> + xenforeignmemory_handle* fmem = xch->fmem;
> + xenforeignmemory_resource_handle *fres;
> + void *addr = NULL;
> +
> + fres = xenforeignmemory_map_resource(
> + fmem, guest_domid, XENMEM_resource_grant_table,
> + XENMEM_resource_grant_table_id_shared, 0, 1, &addr,
> + PROT_READ | PROT_WRITE, 0);
> + if ( !fres )
> + {
> + if ( errno == EOPNOTSUPP )
> + return is_hvm ?
> + compat_gnttab_hvm_seed(xch, guest_domid,
> + console_gmfn, xenstore_gmfn,
> + console_domid, xenstore_domid) :
> + compat_gnttab_seed(xch, guest_domid,
> + console_gmfn, xenstore_gmfn,
> + console_domid, xenstore_domid);
> +
> + xc_dom_panic(xch, XC_INTERNAL_ERROR,
> + "%s: failed to acquire grant table "
> + "[errno=%d]\n",
> + __FUNCTION__, errno);
__func__, and guest domid in the printk()?
> + return -1;
> }
> +
> + xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_CONSOLE,
> + guest_domid, console_domid, console_gmfn);
> + xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_XENSTORE,
> + guest_domid, xenstore_domid, xenstore_gmfn);
> +
> + xenforeignmemory_unmap_resource(fmem, fres);
> +
> + return 0;
> +}
> +
> +int xc_dom_gnttab_init(struct xc_dom_image *dom)
> +{
> + bool is_hvm = xc_dom_translated(dom);
> + xen_pfn_t console_gmfn = xc_dom_p2m(dom, dom->console_pfn);
> + xen_pfn_t xenstore_gmfn = xc_dom_p2m(dom, dom->xenstore_pfn);
I've still got a pending fix to rename dom->*_pfn to gfn, but the local
variables should at least be correct.
> +
> + return xc_dom_gnttab_seed(dom->xch, dom->guest_domid, is_hvm,
> + console_gmfn, xenstore_gmfn,
> + dom->console_domid, dom->xenstore_domid);
> }
>
> /*
> diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
> index 227c48553e..4765a52f33 100644
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
> @@ -216,11 +216,11 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
> return rc;
> }
>
> - rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
> - ctx->restore.console_gfn,
> - ctx->restore.xenstore_gfn,
> - ctx->restore.console_domid,
> - ctx->restore.xenstore_domid);
> + rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
> + ctx->restore.console_gfn,
> + ctx->restore.xenstore_gfn,
> + ctx->restore.console_domid,
> + ctx->restore.xenstore_domid);
Hmm. This is now common with the pv side, and will similarly be wanted
on the ARM side eventually. This patch shouldn't change from how it is
now, but I'll try and find some copious free time to pull together a
common stream_complete() function.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 9:00 ` [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
@ 2018-08-08 10:10 ` Andrew Cooper
2018-08-08 10:19 ` Paul Durrant
2018-08-08 10:47 ` Jan Beulich
2018-08-08 10:30 ` Jan Beulich
1 sibling, 2 replies; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 10:10 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Ian Jackson,
Tim Deegan, Jan Beulich
On 08/08/18 10:00, Paul Durrant wrote:
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index d9ec711c73..8e623ea08e 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ 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);
> }
>
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
Really? this is some straight arithmetic on the integer parameter.
> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> + return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, GRANT_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).
> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> }
> #endif
>
> +/* caller must hold write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
> +
> + ASSERT(gt->gt_version == 2);
> +
> + if ( idx >= nr_status_frames(gt) )
> + {
> + unsigned long nr = status_to_grant_frames(idx + 1);
Why the +1 ? Won't that cause a failure if you attempt to map the
maximum valid index?
> +
> + if ( nr <= gt->max_grant_frames )
> + gnttab_grow_table(d, nr);
You want to capture the return value of grow_table(), which at least
distinguishes between ENODEV and ENOMEM.
> +
> + if ( idx >= nr_status_frames(gt) )
> + return -EINVAL;
This can probably(?) be asserted if gnttab_grow_table() returns
successfully.
> + }
> +
> + *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> + return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
> +
> + ASSERT(gt->gt_version != 0);
> +
> + if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> + gnttab_grow_table(d, idx + 1);
Similarly wrt rc.
> +
> + if ( idx >= nr_grant_frames(gt) )
> + return -EINVAL;
> +
> + *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> + return 0;
> +}
> +
> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> mfn_t *mfn)
> {
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e29d596727..427f65a5e1 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -23,6 +23,7 @@
> #include <xen/numa.h>
> #include <xen/mem_access.h>
> #include <xen/trace.h>
> +#include <xen/grant_table.h>
> #include <asm/current.h>
> #include <asm/hardirq.h>
> #include <asm/p2m.h>
> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> }
>
> +static int acquire_grant_table(struct domain *d, unsigned int id,
> + unsigned long frame,
> + unsigned int nr_frames,
> + xen_pfn_t mfn_list[])
> +{
> + unsigned int i = nr_frames;
> +
> + /* Iterate backwards in case table needs to grow */
> + while ( i-- != 0 )
> + {
> + mfn_t mfn = INVALID_MFN;
> + int rc;
> +
> + switch ( id )
> + {
> + case XENMEM_resource_grant_table_id_shared:
> + rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> + break;
> +
> + case XENMEM_resource_grant_table_id_status:
> + rc = gnttab_get_status_frame(d, frame + i, &mfn);
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> +
> + if ( rc )
Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN) here? I
don't think anything good will come of handing INVALID_MFN back to the
caller.
> + return rc;
> +
> + mfn_list[i] = mfn_x(mfn);
> + }
> +
> + return 0;
> +}
> +
> static int acquire_resource(
> XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> {
> @@ -1046,6 +1089,16 @@ static int acquire_resource(
> xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> unsigned int i;
>
> + /*
> + * FIXME: until foreign pages inserted into the P2M are properly
> + * reference counted, it is unsafe to allow mapping of
> + * non-caller-owned resource pages unless the caller is
> + * the hardware domain.
> + */
> + if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
Space.
> + !is_hardware_domain(currd) )
> + return -EOPNOTSUPP;
EPERM perhaps?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:10 ` Andrew Cooper
@ 2018-08-08 10:19 ` Paul Durrant
2018-08-08 10:45 ` Andrew Cooper
2018-08-08 10:47 ` Jan Beulich
1 sibling, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 10:19 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Wei Liu, Tim (Xen.org),
George Dunlap, Jan Beulich, Ian Jackson
> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 11:11
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu
> <wei.liu2@citrix.com>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> On 08/08/18 10:00, Paul Durrant wrote:
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index d9ec711c73..8e623ea08e 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -358,6 +358,12 @@ 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);
> > }
> >
> > +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
>
> Really? this is some straight arithmetic on the integer parameter.
>
Good point. I'd just adapted the comment from the reverse translation above it but the comment is indeed bogus.
> > +static inline unsigned int status_to_grant_frames(unsigned int
> status_frames)
> > +{
> > + return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_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).
> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> > }
> > #endif
> >
> > +/* caller must hold write lock */
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > + struct grant_table *gt = d->grant_table;
> > +
> > + ASSERT(gt->gt_version == 2);
> > +
> > + if ( idx >= nr_status_frames(gt) )
> > + {
> > + unsigned long nr = status_to_grant_frames(idx + 1);
>
> Why the +1 ? Won't that cause a failure if you attempt to map the
> maximum valid index?
That's kind of the idea...
>
> > +
> > + if ( nr <= gt->max_grant_frames )
> > + gnttab_grow_table(d, nr);
>
> You want to capture the return value of grow_table(), which at least
> distinguishes between ENODEV and ENOMEM.
>
> > +
> > + if ( idx >= nr_status_frames(gt) )
> > + return -EINVAL;
>
> This can probably(?) be asserted if gnttab_grow_table() returns
> successfully.
>
... which is why this is an if and not an ASSERT. I'm just following the analogy of the way the table is grown in gnttab_get_shared_frame_mfn() which is the way it was done before. If you'd rather I change things to use the return value from gnttab_grow_table() then I guess I could do that.
> > + }
> > +
> > + *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> > + return 0;
> > +}
> > +
> > +/* caller must hold write lock */
> > +static int gnttab_get_shared_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > + struct grant_table *gt = d->grant_table;
> > +
> > + ASSERT(gt->gt_version != 0);
> > +
> > + if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> > + gnttab_grow_table(d, idx + 1);
>
> Similarly wrt rc.
>
> > +
> > + if ( idx >= nr_grant_frames(gt) )
> > + return -EINVAL;
> > +
> > + *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> > + return 0;
> > +}
> > +
> > int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > mfn_t *mfn)
> > {
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index e29d596727..427f65a5e1 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -23,6 +23,7 @@
> > #include <xen/numa.h>
> > #include <xen/mem_access.h>
> > #include <xen/trace.h>
> > +#include <xen/grant_table.h>
> > #include <asm/current.h>
> > #include <asm/hardirq.h>
> > #include <asm/p2m.h>
> > @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> > return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> > }
> >
> > +static int acquire_grant_table(struct domain *d, unsigned int id,
> > + unsigned long frame,
> > + unsigned int nr_frames,
> > + xen_pfn_t mfn_list[])
> > +{
> > + unsigned int i = nr_frames;
> > +
> > + /* Iterate backwards in case table needs to grow */
> > + while ( i-- != 0 )
> > + {
> > + mfn_t mfn = INVALID_MFN;
> > + int rc;
> > +
> > + switch ( id )
> > + {
> > + case XENMEM_resource_grant_table_id_shared:
> > + rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> > + break;
> > +
> > + case XENMEM_resource_grant_table_id_status:
> > + rc = gnttab_get_status_frame(d, frame + i, &mfn);
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + break;
> > + }
> > +
> > + if ( rc )
>
> Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN)
> here? I
> don't think anything good will come of handing INVALID_MFN back to the
> caller.
Why? The value of mfn will be overwritten if rc == 0 and will be left as INVALID_MFN if rc != 0. I can ASSERT that if you'd like.
>
> > + return rc;
> > +
> > + mfn_list[i] = mfn_x(mfn);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int acquire_resource(
> > XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> > {
> > @@ -1046,6 +1089,16 @@ static int acquire_resource(
> > xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > unsigned int i;
> >
> > + /*
> > + * FIXME: until foreign pages inserted into the P2M are properly
> > + * reference counted, it is unsafe to allow mapping of
> > + * non-caller-owned resource pages unless the caller is
> > + * the hardware domain.
> > + */
> > + if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
>
> Space.
Oh yes.
>
> > + !is_hardware_domain(currd) )
> > + return -EOPNOTSUPP;
>
> EPERM perhaps?
>
I debated that. I'm really not sure which one would be best.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 9:00 ` [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
2018-08-08 10:10 ` Andrew Cooper
@ 2018-08-08 10:30 ` Jan Beulich
2018-08-08 10:38 ` Paul Durrant
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2018-08-08 10:30 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
>>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ 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);
> }
>
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> + return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, GRANT_PER_PAGE);
> +}
Seeing no use of the grant table (it's not even passed in) I'm confused
by the comment you add. I guess you've simply copied
grant_to_status_frames()'es, which looks similarly wrong.
> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> }
> #endif
>
> +/* caller must hold write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
const?
> + ASSERT(gt->gt_version == 2);
> +
> + if ( idx >= nr_status_frames(gt) )
> + {
> + unsigned long nr = status_to_grant_frames(idx + 1);
> +
> + if ( nr <= gt->max_grant_frames )
> + gnttab_grow_table(d, nr);
> +
> + if ( idx >= nr_status_frames(gt) )
> + return -EINVAL;
> + }
> +
> + *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> + return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> + unsigned long idx, mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
Again?
> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> return rc;
> }
>
> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> + mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
> + int rc;
> +
> + grant_write_lock(gt);
> +
> + if ( gt->gt_version == 0 )
> + gt->gt_version = 1;
Since you've moved this here instead of dropping it, what requirement
have you found for this to be set (other than the ASSERT() you put in
gnttab_get_shared_frame_mfn()?
> @@ -1046,6 +1089,16 @@ static int acquire_resource(
> xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> unsigned int i;
>
> + /*
> + * FIXME: until foreign pages inserted into the P2M are properly
> + * reference counted, it is unsafe to allow mapping of
> + * non-caller-owned resource pages unless the caller is
> + * the hardware domain.
> + */
> + if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> + !is_hardware_domain(currd) )
Missing blank and as a result improper indentation. Also the U in
"until" wants to be upper case I think.
The cosmetic aspects could of course be taken care of while
committing, but the version related question needs to be
clarified first.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:30 ` Jan Beulich
@ 2018-08-08 10:38 ` Paul Durrant
2018-08-08 10:43 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 10:38 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 August 2018 11:30
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -358,6 +358,12 @@ 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);
> > }
> >
> > +/* Number of grant table entries. Caller must hold d's gr. table lock.*/
> > +static inline unsigned int status_to_grant_frames(unsigned int
> status_frames)
> > +{
> > + return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
> GRANT_PER_PAGE);
> > +}
>
> Seeing no use of the grant table (it's not even passed in) I'm confused
> by the comment you add. I guess you've simply copied
> grant_to_status_frames()'es, which looks similarly wrong.
>
Indeed.
> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> > }
> > #endif
> >
> > +/* caller must hold write lock */
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > + struct grant_table *gt = d->grant_table;
>
> const?
>
IIRC that didn't work because gnttab_grow_table() modifies the content.
> > + ASSERT(gt->gt_version == 2);
> > +
> > + if ( idx >= nr_status_frames(gt) )
> > + {
> > + unsigned long nr = status_to_grant_frames(idx + 1);
> > +
> > + if ( nr <= gt->max_grant_frames )
> > + gnttab_grow_table(d, nr);
> > +
> > + if ( idx >= nr_status_frames(gt) )
> > + return -EINVAL;
> > + }
> > +
> > + *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> > + return 0;
> > +}
> > +
> > +/* caller must hold write lock */
> > +static int gnttab_get_shared_frame_mfn(struct domain *d,
> > + unsigned long idx, mfn_t *mfn)
> > +{
> > + struct grant_table *gt = d->grant_table;
>
> Again?
>
> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> > return rc;
> > }
> >
> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> > + mfn_t *mfn)
> > +{
> > + struct grant_table *gt = d->grant_table;
> > + int rc;
> > +
> > + grant_write_lock(gt);
> > +
> > + if ( gt->gt_version == 0 )
> > + gt->gt_version = 1;
>
> Since you've moved this here instead of dropping it, what requirement
> have you found for this to be set (other than the ASSERT() you put in
> gnttab_get_shared_frame_mfn()?
>
The code in patch #2 is executed before the grant table version is set. I could alternatively have libxl explicitly set the version to 1 before trying to seed the table.
> > @@ -1046,6 +1089,16 @@ static int acquire_resource(
> > xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > unsigned int i;
> >
> > + /*
> > + * FIXME: until foreign pages inserted into the P2M are properly
> > + * reference counted, it is unsafe to allow mapping of
> > + * non-caller-owned resource pages unless the caller is
> > + * the hardware domain.
> > + */
> > + if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
> > + !is_hardware_domain(currd) )
>
> Missing blank and as a result improper indentation. Also the U in
> "until" wants to be upper case I think.
>
Ok.
> The cosmetic aspects could of course be taken care of while
> committing, but the version related question needs to be
> clarified first.
>
It's ok, I'm happy to send v22. Just need to know whether you'd prefer explicit version setting in the toolstack.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:38 ` Paul Durrant
@ 2018-08-08 10:43 ` Jan Beulich
2018-08-08 10:46 ` Andrew Cooper
2018-08-08 11:32 ` Paul Durrant
0 siblings, 2 replies; 20+ messages in thread
From: Jan Beulich @ 2018-08-08 10:43 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Tim Deegan,
george.dunlap, Ian Jackson, xen-devel
>>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 08 August 2018 11:30
>>
>> >>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
>> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>> grant_table *gt, grant_ref_t ref,
>> > }
>> > #endif
>> >
>> > +/* caller must hold write lock */
>> > +static int gnttab_get_status_frame_mfn(struct domain *d,
>> > + unsigned long idx, mfn_t *mfn)
>> > +{
>> > + struct grant_table *gt = d->grant_table;
>>
>> const?
>>
>
> IIRC that didn't work because gnttab_grow_table() modifies the content.
But you don't pass gt to the function:
>> > + ASSERT(gt->gt_version == 2);
>> > +
>> > + if ( idx >= nr_status_frames(gt) )
>> > + {
>> > + unsigned long nr = status_to_grant_frames(idx + 1);
>> > +
>> > + if ( nr <= gt->max_grant_frames )
>> > + gnttab_grow_table(d, nr);
^^^
>> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>> unsigned long idx, gfn_t gfn,
>> > return rc;
>> > }
>> >
>> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>> > + mfn_t *mfn)
>> > +{
>> > + struct grant_table *gt = d->grant_table;
>> > + int rc;
>> > +
>> > + grant_write_lock(gt);
>> > +
>> > + if ( gt->gt_version == 0 )
>> > + gt->gt_version = 1;
>>
>> Since you've moved this here instead of dropping it, what requirement
>> have you found for this to be set (other than the ASSERT() you put in
>> gnttab_get_shared_frame_mfn()?
>>
>
> The code in patch #2 is executed before the grant table version is set. I
> could alternatively have libxl explicitly set the version to 1 before trying
> to seed the table.
But that's not my point. What's wrong with leaving it at zero?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:19 ` Paul Durrant
@ 2018-08-08 10:45 ` Andrew Cooper
0 siblings, 0 replies; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 10:45 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Tim (Xen.org),
George Dunlap, Jan Beulich, Ian Jackson
On 08/08/18 11:19, Paul Durrant wrote:
>
>>> +static inline unsigned int status_to_grant_frames(unsigned int
>> status_frames)
>>> +{
>>> + return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
>> GRANT_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).
>>> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>> grant_table *gt, grant_ref_t ref,
>>> }
>>> #endif
>>>
>>> +/* caller must hold write lock */
>>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>>> + unsigned long idx, mfn_t *mfn)
>>> +{
>>> + struct grant_table *gt = d->grant_table;
>>> +
>>> + ASSERT(gt->gt_version == 2);
>>> +
>>> + if ( idx >= nr_status_frames(gt) )
>>> + {
>>> + unsigned long nr = status_to_grant_frames(idx + 1);
>> Why the +1 ? Won't that cause a failure if you attempt to map the
>> maximum valid index?
> That's kind of the idea...
To force a failure when mapping a legitimate index? Whatever the old
code did, this smells like broken boundary case.
A caller is going to want to map frames 0 to N-1, based on some
knowledge of either how many frames the guest has, or what the total
number of expected frames is. nr_status_frames() OTOH, is 1-based,
which is why this looks wrong.
How about a comment along the lines of
/* idx is 0-based, nr_* is 1-based. */
which might help reduce the confusion?
>>> +
>>> + if ( idx >= nr_grant_frames(gt) )
>>> + return -EINVAL;
>>> +
>>> + *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>>> + return 0;
>>> +}
>>> +
>>> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>>> mfn_t *mfn)
>>> {
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index e29d596727..427f65a5e1 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -23,6 +23,7 @@
>>> #include <xen/numa.h>
>>> #include <xen/mem_access.h>
>>> #include <xen/trace.h>
>>> +#include <xen/grant_table.h>
>>> #include <asm/current.h>
>>> #include <asm/hardirq.h>
>>> #include <asm/p2m.h>
>>> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>>> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>>> }
>>>
>>> +static int acquire_grant_table(struct domain *d, unsigned int id,
>>> + unsigned long frame,
>>> + unsigned int nr_frames,
>>> + xen_pfn_t mfn_list[])
>>> +{
>>> + unsigned int i = nr_frames;
>>> +
>>> + /* Iterate backwards in case table needs to grow */
>>> + while ( i-- != 0 )
>>> + {
>>> + mfn_t mfn = INVALID_MFN;
>>> + int rc;
>>> +
>>> + switch ( id )
>>> + {
>>> + case XENMEM_resource_grant_table_id_shared:
>>> + rc = gnttab_get_shared_frame(d, frame + i, &mfn);
>>> + break;
>>> +
>>> + case XENMEM_resource_grant_table_id_status:
>>> + rc = gnttab_get_status_frame(d, frame + i, &mfn);
>>> + break;
>>> +
>>> + default:
>>> + rc = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + if ( rc )
>> Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN)
>> here? I
>> don't think anything good will come of handing INVALID_MFN back to the
>> caller.
> Why? The value of mfn will be overwritten if rc == 0 and will be left as INVALID_MFN if rc != 0. I can ASSERT that if you'd like.
Yeah - an ASSERT() would be nice.
>>> + !is_hardware_domain(currd) )
>>> + return -EOPNOTSUPP;
>> EPERM perhaps?
>>
> I debated that. I'm really not sure which one would be best.
Hmm, nor me. Lets leave it like that for now.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:43 ` Jan Beulich
@ 2018-08-08 10:46 ` Andrew Cooper
2018-08-08 10:55 ` Jan Beulich
2018-08-08 11:32 ` Paul Durrant
1 sibling, 1 reply; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 10:46 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Tim Deegan, george.dunlap,
xen-devel, Ian Jackson
On 08/08/18 11:43, Jan Beulich wrote:
>>>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 08 August 2018 11:30
>>>
>>>>>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
>>>> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>>> grant_table *gt, grant_ref_t ref,
>>>> }
>>>> #endif
>>>>
>>>> +/* caller must hold write lock */
>>>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>>>> + unsigned long idx, mfn_t *mfn)
>>>> +{
>>>> + struct grant_table *gt = d->grant_table;
>>> const?
>>>
>> IIRC that didn't work because gnttab_grow_table() modifies the content.
> But you don't pass gt to the function:
>
>>>> + ASSERT(gt->gt_version == 2);
>>>> +
>>>> + if ( idx >= nr_status_frames(gt) )
>>>> + {
>>>> + unsigned long nr = status_to_grant_frames(idx + 1);
>>>> +
>>>> + if ( nr <= gt->max_grant_frames )
>>>> + gnttab_grow_table(d, nr);
> ^^^
>
>>>> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>>> unsigned long idx, gfn_t gfn,
>>>> return rc;
>>>> }
>>>>
>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>>>> + mfn_t *mfn)
>>>> +{
>>>> + struct grant_table *gt = d->grant_table;
>>>> + int rc;
>>>> +
>>>> + grant_write_lock(gt);
>>>> +
>>>> + if ( gt->gt_version == 0 )
>>>> + gt->gt_version = 1;
>>> Since you've moved this here instead of dropping it, what requirement
>>> have you found for this to be set (other than the ASSERT() you put in
>>> gnttab_get_shared_frame_mfn()?
>>>
>> The code in patch #2 is executed before the grant table version is set. I
>> could alternatively have libxl explicitly set the version to 1 before trying
>> to seed the table.
> But that's not my point. What's wrong with leaving it at zero?
On a tangent, why does a gnttab version of 0 exist at all? Why don't we
explicitly initialise it to 1 in the hypervisor?
We've had plenty of XSAs to do with mishandling of a gnttab version of
0. Why not fix the problem at its source, and simplify the gnttab code
while we are at it.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:10 ` Andrew Cooper
2018-08-08 10:19 ` Paul Durrant
@ 2018-08-08 10:47 ` Jan Beulich
2018-08-08 11:24 ` Paul Durrant
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2018-08-08 10:47 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
Ian Jackson, xen-devel
>>> On 08.08.18 at 12:10, <andrew.cooper3@citrix.com> wrote:
> On 08/08/18 10:00, Paul Durrant wrote:
>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>> + unsigned long idx, mfn_t *mfn)
>> +{
>> + struct grant_table *gt = d->grant_table;
>> +
>> + ASSERT(gt->gt_version == 2);
>> +
>> + if ( idx >= nr_status_frames(gt) )
>> + {
>> + unsigned long nr = status_to_grant_frames(idx + 1);
>
> Why the +1 ? Won't that cause a failure if you attempt to map the
> maximum valid index?
That's the normal index-of-something to count-of-something
conversion (or else the table may get grown by too little). I've
instead been considering the badness of overflow here, but
decided to leave it uncommented as the check further down
would at least not make this insecure. However, with ...
>> +
>> + if ( nr <= gt->max_grant_frames )
>> + gnttab_grow_table(d, nr);
>
> You want to capture the return value of grow_table(), which at least
> distinguishes between ENODEV and ENOMEM.
>
>> +
>> + if ( idx >= nr_status_frames(gt) )
>> + return -EINVAL;
>
> This can probably(?) be asserted if gnttab_grow_table() returns
> successfully.
... these two a potential overflow above would then have a
chance of triggering the assertion you suggest to add.
As to the grow_table() return value check - I'd prefer if the
patch here didn't alter original behavior. If we want it altered,
better in a separate patch.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:46 ` Andrew Cooper
@ 2018-08-08 10:55 ` Jan Beulich
2018-08-08 11:34 ` Andrew Cooper
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2018-08-08 10:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, Tim Deegan, george.dunlap,
Paul Durrant, xen-devel, Ian Jackson
>>> On 08.08.18 at 12:46, <andrew.cooper3@citrix.com> wrote:
> On 08/08/18 11:43, Jan Beulich wrote:
>>>>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 08 August 2018 11:30
>>>>
>>>>>>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
>>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>>>>> + mfn_t *mfn)
>>>>> +{
>>>>> + struct grant_table *gt = d->grant_table;
>>>>> + int rc;
>>>>> +
>>>>> + grant_write_lock(gt);
>>>>> +
>>>>> + if ( gt->gt_version == 0 )
>>>>> + gt->gt_version = 1;
>>>> Since you've moved this here instead of dropping it, what requirement
>>>> have you found for this to be set (other than the ASSERT() you put in
>>>> gnttab_get_shared_frame_mfn()?
>>>>
>>> The code in patch #2 is executed before the grant table version is set. I
>>> could alternatively have libxl explicitly set the version to 1 before trying
>>> to seed the table.
>> But that's not my point. What's wrong with leaving it at zero?
>
> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
> explicitly initialise it to 1 in the hypervisor?
Fair question, which unfortunately I can't answer.
> We've had plenty of XSAs to do with mishandling of a gnttab version of
> 0. Why not fix the problem at its source, and simplify the gnttab code
> while we are at it.
I don't mind, unless a reason for the seemingly strange behavior can be
found.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:47 ` Jan Beulich
@ 2018-08-08 11:24 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 11:24 UTC (permalink / raw)
To: 'Jan Beulich', Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, Tim (Xen.org),
George Dunlap, xen-devel, Ian Jackson
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 August 2018 11:47
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 12:10, <andrew.cooper3@citrix.com> wrote:
> > On 08/08/18 10:00, Paul Durrant wrote:
> >> +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> + unsigned long idx, mfn_t *mfn)
> >> +{
> >> + struct grant_table *gt = d->grant_table;
> >> +
> >> + ASSERT(gt->gt_version == 2);
> >> +
> >> + if ( idx >= nr_status_frames(gt) )
> >> + {
> >> + unsigned long nr = status_to_grant_frames(idx + 1);
> >
> > Why the +1 ? Won't that cause a failure if you attempt to map the
> > maximum valid index?
>
> That's the normal index-of-something to count-of-something
> conversion (or else the table may get grown by too little). I've
> instead been considering the badness of overflow here, but
> decided to leave it uncommented as the check further down
> would at least not make this insecure. However, with ...
>
> >> +
> >> + if ( nr <= gt->max_grant_frames )
> >> + gnttab_grow_table(d, nr);
> >
> > You want to capture the return value of grow_table(), which at least
> > distinguishes between ENODEV and ENOMEM.
> >
> >> +
> >> + if ( idx >= nr_status_frames(gt) )
> >> + return -EINVAL;
> >
> > This can probably(?) be asserted if gnttab_grow_table() returns
> > successfully.
>
> ... these two a potential overflow above would then have a
> chance of triggering the assertion you suggest to add.
>
> As to the grow_table() return value check - I'd prefer if the
> patch here didn't alter original behavior. If we want it altered,
> better in a separate patch.
Ok. I'll leave the return value of gnttab_grow_table() unchecked as-is.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:43 ` Jan Beulich
2018-08-08 10:46 ` Andrew Cooper
@ 2018-08-08 11:32 ` Paul Durrant
2018-08-08 13:15 ` Paul Durrant
1 sibling, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 11:32 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 August 2018 11:43
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> >>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 08 August 2018 11:30
> >>
> >> >>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
> >> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> >> grant_table *gt, grant_ref_t ref,
> >> > }
> >> > #endif
> >> >
> >> > +/* caller must hold write lock */
> >> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> > + unsigned long idx, mfn_t *mfn)
> >> > +{
> >> > + struct grant_table *gt = d->grant_table;
> >>
> >> const?
> >>
> >
> > IIRC that didn't work because gnttab_grow_table() modifies the content.
>
> But you don't pass gt to the function:
>
> >> > + ASSERT(gt->gt_version == 2);
> >> > +
> >> > + if ( idx >= nr_status_frames(gt) )
> >> > + {
> >> > + unsigned long nr = status_to_grant_frames(idx + 1);
> >> > +
> >> > + if ( nr <= gt->max_grant_frames )
> >> > + gnttab_grow_table(d, nr);
>
> ^^^
I know, I was just remembering that the compiler complained when I tried this before. My memory could be wrong so I'll try it again.
>
> >> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> >> unsigned long idx, gfn_t gfn,
> >> > return rc;
> >> > }
> >> >
> >> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> >> > + mfn_t *mfn)
> >> > +{
> >> > + struct grant_table *gt = d->grant_table;
> >> > + int rc;
> >> > +
> >> > + grant_write_lock(gt);
> >> > +
> >> > + if ( gt->gt_version == 0 )
> >> > + gt->gt_version = 1;
> >>
> >> Since you've moved this here instead of dropping it, what requirement
> >> have you found for this to be set (other than the ASSERT() you put in
> >> gnttab_get_shared_frame_mfn()?
> >>
> >
> > The code in patch #2 is executed before the grant table version is set. I
> > could alternatively have libxl explicitly set the version to 1 before trying
> > to seed the table.
>
> But that's not my point. What's wrong with leaving it at zero?
>
I'm not particularly happy calling gnttab_grow_table() with version left at 0 but I can try it and see if it breaks.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 10:55 ` Jan Beulich
@ 2018-08-08 11:34 ` Andrew Cooper
2018-08-08 11:36 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 11:34 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Tim Deegan, george.dunlap,
Paul Durrant, xen-devel, Ian Jackson
On 08/08/18 11:55, Jan Beulich wrote:
>>>> On 08.08.18 at 12:46, <andrew.cooper3@citrix.com> wrote:
>> On 08/08/18 11:43, Jan Beulich wrote:
>>>>>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: 08 August 2018 11:30
>>>>>
>>>>>>>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
>>>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>>>>>> + mfn_t *mfn)
>>>>>> +{
>>>>>> + struct grant_table *gt = d->grant_table;
>>>>>> + int rc;
>>>>>> +
>>>>>> + grant_write_lock(gt);
>>>>>> +
>>>>>> + if ( gt->gt_version == 0 )
>>>>>> + gt->gt_version = 1;
>>>>> Since you've moved this here instead of dropping it, what requirement
>>>>> have you found for this to be set (other than the ASSERT() you put in
>>>>> gnttab_get_shared_frame_mfn()?
>>>>>
>>>> The code in patch #2 is executed before the grant table version is set. I
>>>> could alternatively have libxl explicitly set the version to 1 before trying
>>>> to seed the table.
>>> But that's not my point. What's wrong with leaving it at zero?
>> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
>> explicitly initialise it to 1 in the hypervisor?
> Fair question, which unfortunately I can't answer.
>
>> We've had plenty of XSAs to do with mishandling of a gnttab version of
>> 0. Why not fix the problem at its source, and simplify the gnttab code
>> while we are at it.
> I don't mind, unless a reason for the seemingly strange behavior can be
> found.
gt_version only came in with grant table v2, so the toolstack never
previously set a version. That ended up with cases where dom0 tries to
map the store/cons grants before the guest driver has chosen a version.
I expect its like this because grant table v2 was a giant pile of poorly
reviewed hacks, rather than for any better reason.
If noone else gets to it, I'll fold it into my series to mess thoroughly
with parameter handling in DOMCTL_createdomain.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 11:34 ` Andrew Cooper
@ 2018-08-08 11:36 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 11:36 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Tim (Xen.org),
George Dunlap, xen-devel, Ian Jackson
> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 12:34
> To: Jan Beulich <JBeulich@suse.com>
> Cc: George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
>
> On 08/08/18 11:55, Jan Beulich wrote:
> >>>> On 08.08.18 at 12:46, <andrew.cooper3@citrix.com> wrote:
> >> On 08/08/18 11:43, Jan Beulich wrote:
> >>>>>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
> >>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>>> Sent: 08 August 2018 11:30
> >>>>>
> >>>>>>>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
> >>>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> >>>>>> + mfn_t *mfn)
> >>>>>> +{
> >>>>>> + struct grant_table *gt = d->grant_table;
> >>>>>> + int rc;
> >>>>>> +
> >>>>>> + grant_write_lock(gt);
> >>>>>> +
> >>>>>> + if ( gt->gt_version == 0 )
> >>>>>> + gt->gt_version = 1;
> >>>>> Since you've moved this here instead of dropping it, what
> requirement
> >>>>> have you found for this to be set (other than the ASSERT() you put in
> >>>>> gnttab_get_shared_frame_mfn()?
> >>>>>
> >>>> The code in patch #2 is executed before the grant table version is set. I
> >>>> could alternatively have libxl explicitly set the version to 1 before trying
> >>>> to seed the table.
> >>> But that's not my point. What's wrong with leaving it at zero?
> >> On a tangent, why does a gnttab version of 0 exist at all? Why don't we
> >> explicitly initialise it to 1 in the hypervisor?
> > Fair question, which unfortunately I can't answer.
> >
> >> We've had plenty of XSAs to do with mishandling of a gnttab version of
> >> 0. Why not fix the problem at its source, and simplify the gnttab code
> >> while we are at it.
> > I don't mind, unless a reason for the seemingly strange behavior can be
> > found.
>
> gt_version only came in with grant table v2, so the toolstack never
> previously set a version. That ended up with cases where dom0 tries to
> map the store/cons grants before the guest driver has chosen a version.
>
> I expect its like this because grant table v2 was a giant pile of poorly
> reviewed hacks, rather than for any better reason.
>
> If noone else gets to it, I'll fold it into my series to mess thoroughly
> with parameter handling in DOMCTL_createdomain.
If that's going to take a while then I can add the explicit version set in patch #2.
Paul
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table
2018-08-08 9:58 ` Andrew Cooper
@ 2018-08-08 13:00 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 13:00 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Ian Jackson
> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 10:59
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Ian Jackson <Ian.Jackson@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v21 2/2] tools/libxenctrl: use new
> xenforeignmemory API to seed grant table
>
> On 08/08/18 10:00, Paul Durrant wrote:
> > A previous patch added support for priv-mapping guest resources directly
> > (rather than having to foreign-map, which requires P2M modification for
> > HVM guests).
> >
> > This patch makes use of the new API to seed the guest grant table unless
> > the underlying infrastructure (i.e. privcmd) doesn't support it, in which
> > case the old scheme is used.
> >
> > NOTE: The call to xc_dom_gnttab_hvm_seed() in hvm_build_set_params()
> was
> > actually unnecessary, as the grant table has already been seeded
> > by a prior call to xc_dom_gnttab_init() made by libxl__build_dom().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Acked-by: Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> Some minor style issues, all of which can fixed on commit (probably).
> Everything else looks fine.
Cool.
>
> > diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> > index 8a66889c68..a8a0c0da66 100644
> > --- a/tools/libxc/include/xc_dom.h
> > +++ b/tools/libxc/include/xc_dom.h
> > @@ -337,12 +337,8 @@ void *xc_dom_boot_domU_map(struct
> xc_dom_image *dom, xen_pfn_t pfn,
> > int xc_dom_boot_image(struct xc_dom_image *dom);
> > int xc_dom_compat_check(struct xc_dom_image *dom);
> > int xc_dom_gnttab_init(struct xc_dom_image *dom);
> > -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> > - xen_pfn_t console_gmfn,
> > - xen_pfn_t xenstore_gmfn,
> > - uint32_t console_domid,
> > - uint32_t xenstore_domid);
> > -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> > +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> > + bool is_hvm,
> > xen_pfn_t console_gmfn,
> > xen_pfn_t xenstore_gmfn,
>
> As you're rewriting most of the functionality, can we switch to gfn to
> correct the terminology?
Sure. I'll restrict mods to the bits I'm touching though.
>
> > uint32_t console_domid,
> > diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> > index 2e5681dc5d..8307ebeaf6 100644
> > --- a/tools/libxc/xc_dom_boot.c
> > +++ b/tools/libxc/xc_dom_boot.c
> > @@ -256,11 +256,29 @@ static xen_pfn_t
> xc_dom_gnttab_setup(xc_interface *xch, uint32_t domid)
> > return gmfn;
> > }
> >
> > -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> > - xen_pfn_t console_gmfn,
> > - xen_pfn_t xenstore_gmfn,
> > - uint32_t console_domid,
> > - uint32_t xenstore_domid)
> > +static void xc_dom_set_gnttab_entry(xc_interface *xch,
> > + grant_entry_v1_t *gnttab,
> > + unsigned int idx,
> > + uint32_t guest_domid,
> > + uint32_t backend_domid,
> > + xen_pfn_t backend_gmfn)
>
> gfn
>
> > +{
> > + if ( guest_domid == backend_domid || backend_gmfn == -1)
>
> Space at the end.
>
> > + return;
> > +
> > + xc_dom_printf(xch, "%s: [%u] -> 0x%"PRI_xen_pfn,
> > + __FUNCTION__, idx, backend_gmfn);
>
> __func__ rather than __FUNCTION__. Also, "[%u] => d%d 0x"PRI_xen_pfn
> would be more helpful for diagnostics. (I do realise that backend
> domid's were retrofitted without adjusting the printf().)
>
> > +
> > + gnttab[idx].flags = GTF_permit_access;
> > + gnttab[idx].domid = backend_domid;
> > + gnttab[idx].frame = backend_gmfn;
> > +}
> > +
> > +static int compat_gnttab_seed(xc_interface *xch, uint32_t domid,
>
> compat_gnttab_pv_seed() ?
>
This is actually used in the HVM compat case too. I'll deal with the other stylistic issues and add explicit version setting as discussed on the other patch thread.
Paul
> > + xen_pfn_t console_gmfn,
> > + xen_pfn_t xenstore_gmfn,
>
> gfn
>
> > + uint32_t console_domid,
> > + uint32_t xenstore_domid)
> > {
> >
> > xen_pfn_t gnttab_gmfn;
> > @@ -313,11 +323,11 @@ int xc_dom_gnttab_seed(xc_interface *xch,
> uint32_t domid,
> > return 0;
> > }
> >
> > -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> > - xen_pfn_t console_gpfn,
> > - xen_pfn_t xenstore_gpfn,
> > - uint32_t console_domid,
> > - uint32_t xenstore_domid)
> > +static int compat_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> > + xen_pfn_t console_gpfn,
> > + xen_pfn_t xenstore_gpfn,
>
> gfn.
>
> > + uint32_t console_domid,
> > + uint32_t xenstore_domid)
> > {
> > int rc;
> > xen_pfn_t scratch_gpfn;
> > @@ -356,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch,
> uint32_t domid,
> > return -1;
> > }
> >
> > - rc = xc_dom_gnttab_seed(xch, domid,
> > + rc = compat_gnttab_seed(xch, domid,
> > console_gpfn, xenstore_gpfn,
> > console_domid, xenstore_domid);
> > if (rc != 0)
> > @@ -381,18 +391,56 @@ int xc_dom_gnttab_hvm_seed(xc_interface
> *xch, uint32_t domid,
> > return 0;
> > }
> >
> > -int xc_dom_gnttab_init(struct xc_dom_image *dom)
> > +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> > + bool is_hvm, xen_pfn_t console_gmfn,
> > + xen_pfn_t xenstore_gmfn, uint32_t console_domid,
>
> gfn.
>
> > + uint32_t xenstore_domid)
> > {
> > - if ( xc_dom_translated(dom) ) {
> > - return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
> > - dom->console_pfn, dom->xenstore_pfn,
> > - dom->console_domid, dom->xenstore_domid);
> > - } else {
> > - return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
> > - xc_dom_p2m(dom, dom->console_pfn),
> > - xc_dom_p2m(dom, dom->xenstore_pfn),
> > - dom->console_domid, dom->xenstore_domid);
> > + xenforeignmemory_handle* fmem = xch->fmem;
> > + xenforeignmemory_resource_handle *fres;
> > + void *addr = NULL;
> > +
> > + fres = xenforeignmemory_map_resource(
> > + fmem, guest_domid, XENMEM_resource_grant_table,
> > + XENMEM_resource_grant_table_id_shared, 0, 1, &addr,
> > + PROT_READ | PROT_WRITE, 0);
> > + if ( !fres )
> > + {
> > + if ( errno == EOPNOTSUPP )
> > + return is_hvm ?
> > + compat_gnttab_hvm_seed(xch, guest_domid,
> > + console_gmfn, xenstore_gmfn,
> > + console_domid, xenstore_domid) :
> > + compat_gnttab_seed(xch, guest_domid,
> > + console_gmfn, xenstore_gmfn,
> > + console_domid, xenstore_domid);
> > +
> > + xc_dom_panic(xch, XC_INTERNAL_ERROR,
> > + "%s: failed to acquire grant table "
> > + "[errno=%d]\n",
> > + __FUNCTION__, errno);
>
> __func__, and guest domid in the printk()?
>
> > + return -1;
> > }
> > +
> > + xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_CONSOLE,
> > + guest_domid, console_domid, console_gmfn);
> > + xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_XENSTORE,
> > + guest_domid, xenstore_domid, xenstore_gmfn);
> > +
> > + xenforeignmemory_unmap_resource(fmem, fres);
> > +
> > + return 0;
> > +}
> > +
> > +int xc_dom_gnttab_init(struct xc_dom_image *dom)
> > +{
> > + bool is_hvm = xc_dom_translated(dom);
> > + xen_pfn_t console_gmfn = xc_dom_p2m(dom, dom->console_pfn);
> > + xen_pfn_t xenstore_gmfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>
> I've still got a pending fix to rename dom->*_pfn to gfn, but the local
> variables should at least be correct.
>
> > +
> > + return xc_dom_gnttab_seed(dom->xch, dom->guest_domid, is_hvm,
> > + console_gmfn, xenstore_gmfn,
> > + dom->console_domid, dom->xenstore_domid);
> > }
> >
> > /*
> > diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c
> b/tools/libxc/xc_sr_restore_x86_hvm.c
> > index 227c48553e..4765a52f33 100644
> > --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> > +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
> > @@ -216,11 +216,11 @@ static int x86_hvm_stream_complete(struct
> xc_sr_context *ctx)
> > return rc;
> > }
> >
> > - rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
> > - ctx->restore.console_gfn,
> > - ctx->restore.xenstore_gfn,
> > - ctx->restore.console_domid,
> > - ctx->restore.xenstore_domid);
> > + rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
> > + ctx->restore.console_gfn,
> > + ctx->restore.xenstore_gfn,
> > + ctx->restore.console_domid,
> > + ctx->restore.xenstore_domid);
>
> Hmm. This is now common with the pv side, and will similarly be wanted
> on the ARM side eventually. This patch shouldn't change from how it is
> now, but I'll try and find some copious free time to pull together a
> common stream_complete() function.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 11:32 ` Paul Durrant
@ 2018-08-08 13:15 ` Paul Durrant
2018-08-08 13:17 ` Andrew Cooper
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2018-08-08 13:15 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Tim (Xen.org),
George Dunlap, xen-devel, Ian Jackson
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 08 August 2018 12:33
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable
> resource type: XENMEM_resource_grant_table
>
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 08 August 2018 11:43
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei
> Liu
> > <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> > devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> > Subject: RE: [PATCH v21 1/2] common: add a new mappable resource type:
> > XENMEM_resource_grant_table
> >
> > >>> On 08.08.18 at 12:38, <Paul.Durrant@citrix.com> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: 08 August 2018 11:30
> > >>
> > >> >>> On 08.08.18 at 11:00, <paul.durrant@citrix.com> wrote:
> > >> > @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
> > >> grant_table *gt, grant_ref_t ref,
> > >> > }
> > >> > #endif
> > >> >
> > >> > +/* caller must hold write lock */
> > >> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > >> > + unsigned long idx, mfn_t *mfn)
> > >> > +{
> > >> > + struct grant_table *gt = d->grant_table;
> > >>
> > >> const?
> > >>
> > >
> > > IIRC that didn't work because gnttab_grow_table() modifies the content.
> >
> > But you don't pass gt to the function:
> >
> > >> > + ASSERT(gt->gt_version == 2);
> > >> > +
> > >> > + if ( idx >= nr_status_frames(gt) )
> > >> > + {
> > >> > + unsigned long nr = status_to_grant_frames(idx + 1);
> > >> > +
> > >> > + if ( nr <= gt->max_grant_frames )
> > >> > + gnttab_grow_table(d, nr);
> >
> > ^^^
>
> I know, I was just remembering that the compiler complained when I tried
> this before. My memory could be wrong so I'll try it again.
>
> >
> > >> > @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
> > >> unsigned long idx, gfn_t gfn,
> > >> > return rc;
> > >> > }
> > >> >
> > >> > +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> > >> > + mfn_t *mfn)
> > >> > +{
> > >> > + struct grant_table *gt = d->grant_table;
> > >> > + int rc;
> > >> > +
> > >> > + grant_write_lock(gt);
> > >> > +
> > >> > + if ( gt->gt_version == 0 )
> > >> > + gt->gt_version = 1;
> > >>
> > >> Since you've moved this here instead of dropping it, what requirement
> > >> have you found for this to be set (other than the ASSERT() you put in
> > >> gnttab_get_shared_frame_mfn()?
> > >>
> > >
> > > The code in patch #2 is executed before the grant table version is set. I
> > > could alternatively have libxl explicitly set the version to 1 before trying
> > > to seed the table.
> >
> > But that's not my point. What's wrong with leaving it at zero?
> >
>
> I'm not particularly happy calling gnttab_grow_table() with version left at 0
> but I can try it and see if it breaks.
Actually, no. There is nowhere else that leaves it at 0 and I find that I can't set the version explicitly from the toolstack as gnttab_set_version doesn't take a domid as a parameter. I'll leave the version setting as-is.
Paul
>
> Paul
>
> > Jan
> >
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table
2018-08-08 13:15 ` Paul Durrant
@ 2018-08-08 13:17 ` Andrew Cooper
0 siblings, 0 replies; 20+ messages in thread
From: Andrew Cooper @ 2018-08-08 13:17 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel
On 08/08/18 14:15, Paul Durrant wrote:
>>
>>>>>> @@ -3906,6 +3943,38 @@ int gnttab_map_frame(struct domain *d,
>>>>> unsigned long idx, gfn_t gfn,
>>>>>> return rc;
>>>>>> }
>>>>>>
>>>>>> +int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
>>>>>> + mfn_t *mfn)
>>>>>> +{
>>>>>> + struct grant_table *gt = d->grant_table;
>>>>>> + int rc;
>>>>>> +
>>>>>> + grant_write_lock(gt);
>>>>>> +
>>>>>> + if ( gt->gt_version == 0 )
>>>>>> + gt->gt_version = 1;
>>>>> Since you've moved this here instead of dropping it, what requirement
>>>>> have you found for this to be set (other than the ASSERT() you put in
>>>>> gnttab_get_shared_frame_mfn()?
>>>>>
>>>> The code in patch #2 is executed before the grant table version is set. I
>>>> could alternatively have libxl explicitly set the version to 1 before trying
>>>> to seed the table.
>>> But that's not my point. What's wrong with leaving it at zero?
>>>
>> I'm not particularly happy calling gnttab_grow_table() with version left at 0
>> but I can try it and see if it breaks.
> Actually, no. There is nowhere else that leaves it at 0 and I find that I can't set the version explicitly from the toolstack as gnttab_set_version doesn't take a domid as a parameter. I'll leave the version setting as-is.
Yeah - this looks like the best option for now, and I'll fix the
defaulting-to-1 in due course.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-08-08 13:17 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 9:00 [PATCH v21 0/2] guest resource mapping (reprise) Paul Durrant
2018-08-08 9:00 ` [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
2018-08-08 10:10 ` Andrew Cooper
2018-08-08 10:19 ` Paul Durrant
2018-08-08 10:45 ` Andrew Cooper
2018-08-08 10:47 ` Jan Beulich
2018-08-08 11:24 ` Paul Durrant
2018-08-08 10:30 ` Jan Beulich
2018-08-08 10:38 ` Paul Durrant
2018-08-08 10:43 ` Jan Beulich
2018-08-08 10:46 ` Andrew Cooper
2018-08-08 10:55 ` Jan Beulich
2018-08-08 11:34 ` Andrew Cooper
2018-08-08 11:36 ` Paul Durrant
2018-08-08 11:32 ` Paul Durrant
2018-08-08 13:15 ` Paul Durrant
2018-08-08 13:17 ` Andrew Cooper
2018-08-08 9:00 ` [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
2018-08-08 9:58 ` Andrew Cooper
2018-08-08 13:00 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.