* [PATCH v12 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 02/11] x86/hvm/ioreq: simplify code and use consistent naming Paul Durrant
` (9 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
A subsequent patch will remove the current implicit limitation on creation
of ioreq servers which is due to the allocation of gfns for the ioreq
structures and buffered ioreq ring.
It will therefore be necessary to introduce an explicit limit and, since
this limit should be small, it simplifies the code to maintain an array of
that size rather than using a list.
Also, by reserving an array slot for the default server and populating
array slots early in create, the need to pass an 'is_default' boolean
to sub-functions can be avoided.
Some function return values are changed by this patch: Specifically, in
the case where the id of the default ioreq server is passed in, -EOPNOTSUPP
is now returned rather than -ENOENT.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v10:
- modified FOR_EACH... macro as suggested by Jan.
- check for NULL in IS_DEFAULT macro as suggested by Jan.
v9:
- modified FOR_EACH... macro as requested by Andrew.
v8:
- Addressed various comments from Jan.
v7:
- Fixed assertion failure found in testing.
v6:
- Updated according to comments made by Roger on v4 that I'd missed.
v5:
- Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
functions to avoid possible double-evaluation issues.
v4:
- Introduced more helper macros and relocated them to the top of the
code.
v3:
- New patch (replacing "move is_default into struct hvm_ioreq_server") in
response to review comments.
---
xen/arch/x86/hvm/ioreq.c | 502 +++++++++++++++++++--------------------
xen/include/asm-x86/hvm/domain.h | 10 +-
2 files changed, 245 insertions(+), 267 deletions(-)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index f2e0b3f74a..e6ccc7572a 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -33,6 +33,37 @@
#include <public/hvm/ioreq.h>
+static void set_ioreq_server(struct domain *d, unsigned int id,
+ struct hvm_ioreq_server *s)
+{
+ ASSERT(id < MAX_NR_IOREQ_SERVERS);
+ ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
+
+ d->arch.hvm_domain.ioreq_server.server[id] = s;
+}
+
+#define GET_IOREQ_SERVER(d, id) \
+ (d)->arch.hvm_domain.ioreq_server.server[id]
+
+static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
+ unsigned int id)
+{
+ if ( id >= MAX_NR_IOREQ_SERVERS )
+ return NULL;
+
+ return GET_IOREQ_SERVER(d, id);
+}
+
+#define IS_DEFAULT(s) \
+ ((s) && (s) == GET_IOREQ_SERVER((s)->domain, DEFAULT_IOSERVID))
+
+/* Iterate over all possible ioreq servers */
+#define FOR_EACH_IOREQ_SERVER(d, id, s) \
+ for ( (id) = 0; (id) < MAX_NR_IOREQ_SERVERS; (id)++ ) \
+ if ( !(s = GET_IOREQ_SERVER(d, id)) ) \
+ continue; \
+ else
+
static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
{
shared_iopage_t *p = s->ioreq.va;
@@ -47,10 +78,9 @@ bool hvm_io_pending(struct vcpu *v)
{
struct domain *d = v->domain;
struct hvm_ioreq_server *s;
+ unsigned int id;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
struct hvm_ioreq_vcpu *sv;
@@ -127,10 +157,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
struct hvm_vcpu_io *vio = &v->arch.hvm_vcpu.hvm_io;
struct hvm_ioreq_server *s;
enum hvm_io_completion io_completion;
+ unsigned int id;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
struct hvm_ioreq_vcpu *sv;
@@ -243,13 +272,12 @@ static int hvm_map_ioreq_page(
bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
{
const struct hvm_ioreq_server *s;
+ unsigned int id;
bool found = false;
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
if ( (s->ioreq.va && s->ioreq.page == page) ||
(s->bufioreq.va && s->bufioreq.page == page) )
@@ -302,7 +330,7 @@ static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
}
static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
- bool is_default, struct vcpu *v)
+ struct vcpu *v)
{
struct hvm_ioreq_vcpu *sv;
int rc;
@@ -331,7 +359,7 @@ static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
goto fail3;
s->bufioreq_evtchn = rc;
- if ( is_default )
+ if ( IS_DEFAULT(s) )
d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN] =
s->bufioreq_evtchn;
}
@@ -431,7 +459,6 @@ static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
}
static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
- bool is_default,
bool handle_bufioreq)
{
struct domain *d = s->domain;
@@ -439,7 +466,7 @@ static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
unsigned long bufioreq_gfn = gfn_x(INVALID_GFN);
int rc;
- if ( is_default )
+ if ( IS_DEFAULT(s) )
{
/*
* The default ioreq server must handle buffered ioreqs, for
@@ -468,8 +495,7 @@ static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
return rc;
}
-static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
- bool is_default)
+static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
{
struct domain *d = s->domain;
bool handle_bufioreq = !!s->bufioreq.va;
@@ -479,7 +505,7 @@ static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
hvm_unmap_ioreq_page(s, false);
- if ( !is_default )
+ if ( !IS_DEFAULT(s) )
{
if ( handle_bufioreq )
hvm_free_ioreq_gfn(d, s->bufioreq.gfn);
@@ -488,12 +514,11 @@ static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
}
}
-static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
- bool is_default)
+static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
{
unsigned int i;
- if ( is_default )
+ if ( IS_DEFAULT(s) )
return;
for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
@@ -501,19 +526,21 @@ static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
}
static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
- bool is_default)
+ ioservid_t id)
{
unsigned int i;
int rc;
- if ( is_default )
+ if ( id == DEFAULT_IOSERVID )
goto done;
+ ASSERT(!IS_DEFAULT(s));
+
for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
{
char *name;
- rc = asprintf(&name, "ioreq_server %d %s", s->id,
+ rc = asprintf(&name, "ioreq_server %d %s", id,
(i == XEN_DMOP_IO_RANGE_PORT) ? "port" :
(i == XEN_DMOP_IO_RANGE_MEMORY) ? "memory" :
(i == XEN_DMOP_IO_RANGE_PCI) ? "pci" :
@@ -537,13 +564,12 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
return 0;
fail:
- hvm_ioreq_server_free_rangesets(s, false);
+ hvm_ioreq_server_free_rangesets(s);
return rc;
}
-static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
- bool is_default)
+static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
{
struct domain *d = s->domain;
struct hvm_ioreq_vcpu *sv;
@@ -554,7 +580,7 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
if ( s->enabled )
goto done;
- if ( !is_default )
+ if ( !IS_DEFAULT(s) )
{
hvm_remove_ioreq_gfn(d, &s->ioreq);
@@ -573,8 +599,7 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
spin_unlock(&s->lock);
}
-static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
- bool is_default)
+static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
{
struct domain *d = s->domain;
bool handle_bufioreq = !!s->bufioreq.va;
@@ -584,7 +609,7 @@ static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
if ( !s->enabled )
goto done;
- if ( !is_default )
+ if ( !IS_DEFAULT(s) )
{
if ( handle_bufioreq )
hvm_add_ioreq_gfn(d, &s->bufioreq);
@@ -600,13 +625,11 @@ static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
struct domain *d, domid_t domid,
- bool is_default, int bufioreq_handling,
- ioservid_t id)
+ int bufioreq_handling, ioservid_t id)
{
struct vcpu *v;
int rc;
- s->id = id;
s->domain = d;
s->domid = domid;
@@ -614,7 +637,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
INIT_LIST_HEAD(&s->ioreq_vcpu_list);
spin_lock_init(&s->bufioreq_lock);
- rc = hvm_ioreq_server_alloc_rangesets(s, is_default);
+ rc = hvm_ioreq_server_alloc_rangesets(s, id);
if ( rc )
return rc;
@@ -622,13 +645,13 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
s->bufioreq_atomic = true;
rc = hvm_ioreq_server_setup_pages(
- s, is_default, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
+ s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
if ( rc )
goto fail_map;
for_each_vcpu ( d, v )
{
- rc = hvm_ioreq_server_add_vcpu(s, is_default, v);
+ rc = hvm_ioreq_server_add_vcpu(s, v);
if ( rc )
goto fail_add;
}
@@ -637,47 +660,20 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
fail_add:
hvm_ioreq_server_remove_all_vcpus(s);
- hvm_ioreq_server_unmap_pages(s, is_default);
+ hvm_ioreq_server_unmap_pages(s);
fail_map:
- hvm_ioreq_server_free_rangesets(s, is_default);
+ hvm_ioreq_server_free_rangesets(s);
return rc;
}
-static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s,
- bool is_default)
+static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
{
ASSERT(!s->enabled);
hvm_ioreq_server_remove_all_vcpus(s);
- hvm_ioreq_server_unmap_pages(s, is_default);
- hvm_ioreq_server_free_rangesets(s, is_default);
-}
-
-static ioservid_t next_ioservid(struct domain *d)
-{
- struct hvm_ioreq_server *s;
- ioservid_t id;
-
- ASSERT(spin_is_locked(&d->arch.hvm_domain.ioreq_server.lock));
-
- id = d->arch.hvm_domain.ioreq_server.id;
-
- again:
- id++;
-
- /* Check for uniqueness */
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( id == s->id )
- goto again;
- }
-
- d->arch.hvm_domain.ioreq_server.id = id;
-
- return id;
+ hvm_ioreq_server_unmap_pages(s);
+ hvm_ioreq_server_free_rangesets(s);
}
int hvm_create_ioreq_server(struct domain *d, domid_t domid,
@@ -685,52 +681,64 @@ int hvm_create_ioreq_server(struct domain *d, domid_t domid,
ioservid_t *id)
{
struct hvm_ioreq_server *s;
+ unsigned int i;
int rc;
if ( bufioreq_handling > HVM_IOREQSRV_BUFIOREQ_ATOMIC )
return -EINVAL;
- rc = -ENOMEM;
s = xzalloc(struct hvm_ioreq_server);
if ( !s )
- goto fail1;
+ return -ENOMEM;
domain_pause(d);
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- rc = -EEXIST;
- if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
- goto fail2;
-
- rc = hvm_ioreq_server_init(s, d, domid, is_default, bufioreq_handling,
- next_ioservid(d));
- if ( rc )
- goto fail3;
-
- list_add(&s->list_entry,
- &d->arch.hvm_domain.ioreq_server.list);
-
if ( is_default )
{
- d->arch.hvm_domain.default_ioreq_server = s;
- hvm_ioreq_server_enable(s, true);
+ i = DEFAULT_IOSERVID;
+
+ rc = -EEXIST;
+ if ( GET_IOREQ_SERVER(d, i) )
+ goto fail;
+ }
+ else
+ {
+ for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
+ {
+ if ( i != DEFAULT_IOSERVID && !GET_IOREQ_SERVER(d, i) )
+ break;
+ }
+
+ rc = -ENOSPC;
+ if ( i >= MAX_NR_IOREQ_SERVERS )
+ goto fail;
}
+ set_ioreq_server(d, i, s);
+
+ rc = hvm_ioreq_server_init(s, d, domid, bufioreq_handling, i);
+ if ( rc )
+ goto fail;
+
+ if ( i == DEFAULT_IOSERVID )
+ hvm_ioreq_server_enable(s);
+
if ( id )
- *id = s->id;
+ *id = i;
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
domain_unpause(d);
return 0;
- fail3:
- fail2:
+ fail:
+ set_ioreq_server(d, i, NULL);
+
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
domain_unpause(d);
xfree(s);
- fail1:
return rc;
}
@@ -739,37 +747,34 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
struct hvm_ioreq_server *s;
int rc;
- spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
+ if ( id == DEFAULT_IOSERVID )
+ return -EPERM;
- rc = -ENOENT;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- if ( s->id != id )
- continue;
+ s = get_ioreq_server(d, id);
- domain_pause(d);
+ rc = -ENOENT;
+ if ( !s )
+ goto out;
- p2m_set_ioreq_server(d, 0, s);
+ ASSERT(!IS_DEFAULT(s));
- hvm_ioreq_server_disable(s, false);
+ domain_pause(d);
- list_del(&s->list_entry);
+ p2m_set_ioreq_server(d, 0, s);
- hvm_ioreq_server_deinit(s, false);
+ hvm_ioreq_server_disable(s);
+ hvm_ioreq_server_deinit(s);
- domain_unpause(d);
+ domain_unpause(d);
- xfree(s);
+ set_ioreq_server(d, id, NULL);
+ xfree(s);
- rc = 0;
- break;
- }
+ rc = 0;
+ out:
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
return rc;
@@ -783,31 +788,30 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
struct hvm_ioreq_server *s;
int rc;
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- rc = -ENOENT;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ s = get_ioreq_server(d, id);
- if ( s->id != id )
- continue;
+ rc = -ENOENT;
+ if ( !s )
+ goto out;
- *ioreq_gfn = s->ioreq.gfn;
+ ASSERT(!IS_DEFAULT(s));
- if ( s->bufioreq.va != NULL )
- {
- *bufioreq_gfn = s->bufioreq.gfn;
- *bufioreq_port = s->bufioreq_evtchn;
- }
+ *ioreq_gfn = s->ioreq.gfn;
- rc = 0;
- break;
+ if ( s->bufioreq.va != NULL )
+ {
+ *bufioreq_gfn = s->bufioreq.gfn;
+ *bufioreq_port = s->bufioreq_evtchn;
}
+ rc = 0;
+
+ out:
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
return rc;
@@ -818,48 +822,46 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
uint64_t end)
{
struct hvm_ioreq_server *s;
+ struct rangeset *r;
int rc;
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
+ s = get_ioreq_server(d, id);
+
rc = -ENOENT;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ if ( !s )
+ goto out;
- if ( s->id == id )
- {
- struct rangeset *r;
+ ASSERT(!IS_DEFAULT(s));
- switch ( type )
- {
- case XEN_DMOP_IO_RANGE_PORT:
- case XEN_DMOP_IO_RANGE_MEMORY:
- case XEN_DMOP_IO_RANGE_PCI:
- r = s->range[type];
- break;
+ switch ( type )
+ {
+ case XEN_DMOP_IO_RANGE_PORT:
+ case XEN_DMOP_IO_RANGE_MEMORY:
+ case XEN_DMOP_IO_RANGE_PCI:
+ r = s->range[type];
+ break;
- default:
- r = NULL;
- break;
- }
+ default:
+ r = NULL;
+ break;
+ }
- rc = -EINVAL;
- if ( !r )
- break;
+ rc = -EINVAL;
+ if ( !r )
+ goto out;
- rc = -EEXIST;
- if ( rangeset_overlaps_range(r, start, end) )
- break;
+ rc = -EEXIST;
+ if ( rangeset_overlaps_range(r, start, end) )
+ goto out;
- rc = rangeset_add_range(r, start, end);
- break;
- }
- }
+ rc = rangeset_add_range(r, start, end);
+ out:
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
return rc;
@@ -870,48 +872,46 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
uint64_t end)
{
struct hvm_ioreq_server *s;
+ struct rangeset *r;
int rc;
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
+ s = get_ioreq_server(d, id);
+
rc = -ENOENT;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ if ( !s )
+ goto out;
- if ( s->id == id )
- {
- struct rangeset *r;
+ ASSERT(!IS_DEFAULT(s));
- switch ( type )
- {
- case XEN_DMOP_IO_RANGE_PORT:
- case XEN_DMOP_IO_RANGE_MEMORY:
- case XEN_DMOP_IO_RANGE_PCI:
- r = s->range[type];
- break;
+ switch ( type )
+ {
+ case XEN_DMOP_IO_RANGE_PORT:
+ case XEN_DMOP_IO_RANGE_MEMORY:
+ case XEN_DMOP_IO_RANGE_PCI:
+ r = s->range[type];
+ break;
- default:
- r = NULL;
- break;
- }
+ default:
+ r = NULL;
+ break;
+ }
- rc = -EINVAL;
- if ( !r )
- break;
+ rc = -EINVAL;
+ if ( !r )
+ goto out;
- rc = -ENOENT;
- if ( !rangeset_contains_range(r, start, end) )
- break;
+ rc = -ENOENT;
+ if ( !rangeset_contains_range(r, start, end) )
+ goto out;
- rc = rangeset_remove_range(r, start, end);
- break;
- }
- }
+ rc = rangeset_remove_range(r, start, end);
+ out:
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
return rc;
@@ -931,6 +931,9 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
struct hvm_ioreq_server *s;
int rc;
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
if ( type != HVMMEM_ioreq_server )
return -EINVAL;
@@ -939,19 +942,14 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- rc = -ENOENT;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
- {
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ s = get_ioreq_server(d, id);
- if ( s->id == id )
- {
- rc = p2m_set_ioreq_server(d, flags, s);
- break;
- }
+ if ( !s )
+ rc = -ENOENT;
+ else
+ {
+ ASSERT(!IS_DEFAULT(s));
+ rc = p2m_set_ioreq_server(d, flags, s);
}
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
@@ -970,38 +968,34 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
bool enabled)
{
- struct list_head *entry;
+ struct hvm_ioreq_server *s;
int rc;
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- rc = -ENOENT;
- list_for_each ( entry,
- &d->arch.hvm_domain.ioreq_server.list )
- {
- struct hvm_ioreq_server *s = list_entry(entry,
- struct hvm_ioreq_server,
- list_entry);
+ s = get_ioreq_server(d, id);
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
+ rc = -ENOENT;
+ if ( !s )
+ goto out;
- if ( s->id != id )
- continue;
+ ASSERT(!IS_DEFAULT(s));
- domain_pause(d);
+ domain_pause(d);
- if ( enabled )
- hvm_ioreq_server_enable(s, false);
- else
- hvm_ioreq_server_disable(s, false);
+ if ( enabled )
+ hvm_ioreq_server_enable(s);
+ else
+ hvm_ioreq_server_disable(s);
- domain_unpause(d);
+ domain_unpause(d);
- rc = 0;
- break;
- }
+ rc = 0;
+ out:
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
return rc;
}
@@ -1009,17 +1003,14 @@ int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
{
struct hvm_ioreq_server *s;
+ unsigned int id;
int rc;
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
- bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
-
- rc = hvm_ioreq_server_add_vcpu(s, is_default, v);
+ rc = hvm_ioreq_server_add_vcpu(s, v);
if ( rc )
goto fail;
}
@@ -1029,10 +1020,15 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
return 0;
fail:
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ while ( id-- != 0 )
+ {
+ s = GET_IOREQ_SERVER(d, id);
+
+ if ( !s )
+ continue;
+
hvm_ioreq_server_remove_vcpu(s, v);
+ }
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
@@ -1042,12 +1038,11 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v)
{
struct hvm_ioreq_server *s;
+ unsigned int id;
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
hvm_ioreq_server_remove_vcpu(s, v);
spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
@@ -1055,28 +1050,19 @@ void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v)
void hvm_destroy_all_ioreq_servers(struct domain *d)
{
- struct hvm_ioreq_server *s, *next;
+ struct hvm_ioreq_server *s;
+ unsigned int id;
spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
/* No need to domain_pause() as the domain is being torn down */
- list_for_each_entry_safe ( s,
- next,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
- bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
-
- hvm_ioreq_server_disable(s, is_default);
-
- if ( is_default )
- d->arch.hvm_domain.default_ioreq_server = NULL;
-
- list_del(&s->list_entry);
-
- hvm_ioreq_server_deinit(s, is_default);
+ hvm_ioreq_server_disable(s);
+ hvm_ioreq_server_deinit(s);
+ set_ioreq_server(d, id, NULL);
xfree(s);
}
@@ -1111,7 +1097,7 @@ int hvm_set_dm_domain(struct domain *d, domid_t domid)
* still be set and thus, when the server is created, it will have
* the correct domid.
*/
- s = d->arch.hvm_domain.default_ioreq_server;
+ s = GET_IOREQ_SERVER(d, DEFAULT_IOSERVID);
if ( !s )
goto done;
@@ -1164,12 +1150,10 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
uint32_t cf8;
uint8_t type;
uint64_t addr;
-
- if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
- return NULL;
+ unsigned int id;
if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
- return d->arch.hvm_domain.default_ioreq_server;
+ return GET_IOREQ_SERVER(d, DEFAULT_IOSERVID);
cf8 = d->arch.hvm_domain.pci_cf8;
@@ -1209,16 +1193,11 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
addr = p->addr;
}
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
{
struct rangeset *r;
- if ( s == d->arch.hvm_domain.default_ioreq_server )
- continue;
-
- if ( !s->enabled )
+ if ( IS_DEFAULT(s) )
continue;
r = s->range[type];
@@ -1251,7 +1230,7 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
}
}
- return d->arch.hvm_domain.default_ioreq_server;
+ return GET_IOREQ_SERVER(d, DEFAULT_IOSERVID);
}
static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
@@ -1410,13 +1389,13 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
{
struct domain *d = current->domain;
struct hvm_ioreq_server *s;
- unsigned int failed = 0;
+ unsigned int id, failed = 0;
- list_for_each_entry ( s,
- &d->arch.hvm_domain.ioreq_server.list,
- list_entry )
+ FOR_EACH_IOREQ_SERVER(d, id, s)
+ {
if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE )
failed++;
+ }
return failed;
}
@@ -1436,7 +1415,6 @@ static int hvm_access_cf8(
void hvm_ioreq_init(struct domain *d)
{
spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
- INIT_LIST_HEAD(&d->arch.hvm_domain.ioreq_server.list);
register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
}
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 7f128c05ff..e17bbe4004 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -60,7 +60,6 @@ struct hvm_ioreq_server {
/* Domain id of emulating domain */
domid_t domid;
- ioservid_t id;
struct hvm_ioreq_page ioreq;
struct list_head ioreq_vcpu_list;
struct hvm_ioreq_page bufioreq;
@@ -100,6 +99,9 @@ struct hvm_pi_ops {
void (*do_resume)(struct vcpu *v);
};
+#define MAX_NR_IOREQ_SERVERS 8
+#define DEFAULT_IOSERVID 0
+
struct hvm_domain {
/* Guest page range used for non-default ioreq servers */
struct {
@@ -109,11 +111,9 @@ struct hvm_domain {
/* Lock protects all other values in the sub-struct and the default */
struct {
- spinlock_t lock;
- ioservid_t id;
- struct list_head list;
+ spinlock_t lock;
+ struct hvm_ioreq_server *server[MAX_NR_IOREQ_SERVERS];
} ioreq_server;
- struct hvm_ioreq_server *default_ioreq_server;
/* Cached CF8 for guest PCI config cycles */
uint32_t pci_cf8;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 02/11] x86/hvm/ioreq: simplify code and use consistent naming
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
2017-10-17 13:24 ` [PATCH v12 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 03/11] x86/hvm/ioreq: use gfn_t in struct hvm_ioreq_page Paul Durrant
` (8 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
This patch re-works much of the ioreq server initialization and teardown
code:
- The hvm_map/unmap_ioreq_gfn() functions are expanded to call through
to hvm_alloc/free_ioreq_gfn() rather than expecting them to be called
separately by outer functions.
- Several functions now test the validity of the hvm_ioreq_page gfn value
to determine whether they need to act. This means can be safely called
for the bufioreq page even when it is not used.
- hvm_add/remove_ioreq_gfn() simply return in the case of the default
IOREQ server so callers no longer need to test before calling.
- hvm_ioreq_server_setup_pages() is renamed to hvm_ioreq_server_map_pages()
to mirror the existing hvm_ioreq_server_unmap_pages().
All of this significantly shortens the code.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v3:
- Rebased on top of 's->is_default' to 'IS_DEFAULT(s)' changes.
- Minor updates in response to review comments from Roger.
---
xen/arch/x86/hvm/ioreq.c | 182 ++++++++++++++++++-----------------------------
1 file changed, 69 insertions(+), 113 deletions(-)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index e6ccc7572a..6d81018369 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -210,63 +210,75 @@ bool handle_hvm_io_completion(struct vcpu *v)
return true;
}
-static int hvm_alloc_ioreq_gfn(struct domain *d, unsigned long *gfn)
+static unsigned long hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
{
+ struct domain *d = s->domain;
unsigned int i;
- int rc;
- rc = -ENOMEM;
+ ASSERT(!IS_DEFAULT(s));
+
for ( i = 0; i < sizeof(d->arch.hvm_domain.ioreq_gfn.mask) * 8; i++ )
{
if ( test_and_clear_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask) )
- {
- *gfn = d->arch.hvm_domain.ioreq_gfn.base + i;
- rc = 0;
- break;
- }
+ return d->arch.hvm_domain.ioreq_gfn.base + i;
}
- return rc;
+ return gfn_x(INVALID_GFN);
}
-static void hvm_free_ioreq_gfn(struct domain *d, unsigned long gfn)
+static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s,
+ unsigned long gfn)
{
+ struct domain *d = s->domain;
unsigned int i = gfn - d->arch.hvm_domain.ioreq_gfn.base;
- if ( gfn != gfn_x(INVALID_GFN) )
- set_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask);
+ ASSERT(!IS_DEFAULT(s));
+ ASSERT(gfn != gfn_x(INVALID_GFN));
+
+ set_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask);
}
-static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool buf)
+static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
{
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+ if ( iorp->gfn == gfn_x(INVALID_GFN) )
+ return;
+
destroy_ring_for_helper(&iorp->va, iorp->page);
+ iorp->page = NULL;
+
+ if ( !IS_DEFAULT(s) )
+ hvm_free_ioreq_gfn(s, iorp->gfn);
+
+ iorp->gfn = gfn_x(INVALID_GFN);
}
-static int hvm_map_ioreq_page(
- struct hvm_ioreq_server *s, bool buf, unsigned long gfn)
+static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
{
struct domain *d = s->domain;
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
- struct page_info *page;
- void *va;
int rc;
- if ( (rc = prepare_ring_for_helper(d, gfn, &page, &va)) )
- return rc;
-
- if ( (iorp->va != NULL) || d->is_dying )
- {
- destroy_ring_for_helper(&va, page);
+ if ( d->is_dying )
return -EINVAL;
- }
- iorp->va = va;
- iorp->page = page;
- iorp->gfn = gfn;
+ if ( IS_DEFAULT(s) )
+ iorp->gfn = buf ?
+ d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN] :
+ d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN];
+ else
+ iorp->gfn = hvm_alloc_ioreq_gfn(s);
- return 0;
+ if ( iorp->gfn == gfn_x(INVALID_GFN) )
+ return -ENOMEM;
+
+ rc = prepare_ring_for_helper(d, iorp->gfn, &iorp->page, &iorp->va);
+
+ if ( rc )
+ hvm_unmap_ioreq_gfn(s, buf);
+
+ return rc;
}
bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
@@ -279,8 +291,7 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
FOR_EACH_IOREQ_SERVER(d, id, s)
{
- if ( (s->ioreq.va && s->ioreq.page == page) ||
- (s->bufioreq.va && s->bufioreq.page == page) )
+ if ( (s->ioreq.page == page) || (s->bufioreq.page == page) )
{
found = true;
break;
@@ -292,20 +303,30 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
return found;
}
-static void hvm_remove_ioreq_gfn(
- struct domain *d, struct hvm_ioreq_page *iorp)
+static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
+
{
+ struct domain *d = s->domain;
+ struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+
+ if ( IS_DEFAULT(s) || iorp->gfn == gfn_x(INVALID_GFN) )
+ return;
+
if ( guest_physmap_remove_page(d, _gfn(iorp->gfn),
_mfn(page_to_mfn(iorp->page)), 0) )
domain_crash(d);
clear_page(iorp->va);
}
-static int hvm_add_ioreq_gfn(
- struct domain *d, struct hvm_ioreq_page *iorp)
+static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
{
+ struct domain *d = s->domain;
+ struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
int rc;
+ if ( IS_DEFAULT(s) || iorp->gfn == gfn_x(INVALID_GFN) )
+ return 0;
+
clear_page(iorp->va);
rc = guest_physmap_add_page(d, _gfn(iorp->gfn),
@@ -440,78 +461,25 @@ static void hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
}
static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
- unsigned long ioreq_gfn,
- unsigned long bufioreq_gfn)
+ bool handle_bufioreq)
{
int rc;
- rc = hvm_map_ioreq_page(s, false, ioreq_gfn);
- if ( rc )
- return rc;
-
- if ( bufioreq_gfn != gfn_x(INVALID_GFN) )
- rc = hvm_map_ioreq_page(s, true, bufioreq_gfn);
-
- if ( rc )
- hvm_unmap_ioreq_page(s, false);
-
- return rc;
-}
-
-static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
- bool handle_bufioreq)
-{
- struct domain *d = s->domain;
- unsigned long ioreq_gfn = gfn_x(INVALID_GFN);
- unsigned long bufioreq_gfn = gfn_x(INVALID_GFN);
- int rc;
-
- if ( IS_DEFAULT(s) )
- {
- /*
- * The default ioreq server must handle buffered ioreqs, for
- * backwards compatibility.
- */
- ASSERT(handle_bufioreq);
- return hvm_ioreq_server_map_pages(s,
- d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN],
- d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN]);
- }
-
- rc = hvm_alloc_ioreq_gfn(d, &ioreq_gfn);
+ rc = hvm_map_ioreq_gfn(s, false);
if ( !rc && handle_bufioreq )
- rc = hvm_alloc_ioreq_gfn(d, &bufioreq_gfn);
-
- if ( !rc )
- rc = hvm_ioreq_server_map_pages(s, ioreq_gfn, bufioreq_gfn);
+ rc = hvm_map_ioreq_gfn(s, true);
if ( rc )
- {
- hvm_free_ioreq_gfn(d, ioreq_gfn);
- hvm_free_ioreq_gfn(d, bufioreq_gfn);
- }
+ hvm_unmap_ioreq_gfn(s, false);
return rc;
}
static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
{
- struct domain *d = s->domain;
- bool handle_bufioreq = !!s->bufioreq.va;
-
- if ( handle_bufioreq )
- hvm_unmap_ioreq_page(s, true);
-
- hvm_unmap_ioreq_page(s, false);
-
- if ( !IS_DEFAULT(s) )
- {
- if ( handle_bufioreq )
- hvm_free_ioreq_gfn(d, s->bufioreq.gfn);
-
- hvm_free_ioreq_gfn(d, s->ioreq.gfn);
- }
+ hvm_unmap_ioreq_gfn(s, true);
+ hvm_unmap_ioreq_gfn(s, false);
}
static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
@@ -571,22 +539,15 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
{
- struct domain *d = s->domain;
struct hvm_ioreq_vcpu *sv;
- bool handle_bufioreq = !!s->bufioreq.va;
spin_lock(&s->lock);
if ( s->enabled )
goto done;
- if ( !IS_DEFAULT(s) )
- {
- hvm_remove_ioreq_gfn(d, &s->ioreq);
-
- if ( handle_bufioreq )
- hvm_remove_ioreq_gfn(d, &s->bufioreq);
- }
+ hvm_remove_ioreq_gfn(s, false);
+ hvm_remove_ioreq_gfn(s, true);
s->enabled = true;
@@ -601,21 +562,13 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
{
- struct domain *d = s->domain;
- bool handle_bufioreq = !!s->bufioreq.va;
-
spin_lock(&s->lock);
if ( !s->enabled )
goto done;
- if ( !IS_DEFAULT(s) )
- {
- if ( handle_bufioreq )
- hvm_add_ioreq_gfn(d, &s->bufioreq);
-
- hvm_add_ioreq_gfn(d, &s->ioreq);
- }
+ hvm_add_ioreq_gfn(s, true);
+ hvm_add_ioreq_gfn(s, false);
s->enabled = false;
@@ -637,6 +590,9 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
INIT_LIST_HEAD(&s->ioreq_vcpu_list);
spin_lock_init(&s->bufioreq_lock);
+ s->ioreq.gfn = gfn_x(INVALID_GFN);
+ s->bufioreq.gfn = gfn_x(INVALID_GFN);
+
rc = hvm_ioreq_server_alloc_rangesets(s, id);
if ( rc )
return rc;
@@ -644,7 +600,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
if ( bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_ATOMIC )
s->bufioreq_atomic = true;
- rc = hvm_ioreq_server_setup_pages(
+ rc = hvm_ioreq_server_map_pages(
s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
if ( rc )
goto fail_map;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 03/11] x86/hvm/ioreq: use gfn_t in struct hvm_ioreq_page
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
2017-10-17 13:24 ` [PATCH v12 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list Paul Durrant
2017-10-17 13:24 ` [PATCH v12 02/11] x86/hvm/ioreq: simplify code and use consistent naming Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 04/11] x86/hvm/ioreq: defer mapping gfns until they are actually requsted Paul Durrant
` (7 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
This patch adjusts the ioreq server code to use type-safe gfn_t values
where possible. No functional change.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/ioreq.c | 44 ++++++++++++++++++++--------------------
xen/include/asm-x86/hvm/domain.h | 2 +-
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 6d81018369..64bb13cec9 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -210,7 +210,7 @@ bool handle_hvm_io_completion(struct vcpu *v)
return true;
}
-static unsigned long hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
+static gfn_t hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
{
struct domain *d = s->domain;
unsigned int i;
@@ -220,20 +220,19 @@ static unsigned long hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
for ( i = 0; i < sizeof(d->arch.hvm_domain.ioreq_gfn.mask) * 8; i++ )
{
if ( test_and_clear_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask) )
- return d->arch.hvm_domain.ioreq_gfn.base + i;
+ return _gfn(d->arch.hvm_domain.ioreq_gfn.base + i);
}
- return gfn_x(INVALID_GFN);
+ return INVALID_GFN;
}
-static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s,
- unsigned long gfn)
+static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s, gfn_t gfn)
{
struct domain *d = s->domain;
- unsigned int i = gfn - d->arch.hvm_domain.ioreq_gfn.base;
+ unsigned int i = gfn_x(gfn) - d->arch.hvm_domain.ioreq_gfn.base;
ASSERT(!IS_DEFAULT(s));
- ASSERT(gfn != gfn_x(INVALID_GFN));
+ ASSERT(!gfn_eq(gfn, INVALID_GFN));
set_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask);
}
@@ -242,7 +241,7 @@ static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
{
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
- if ( iorp->gfn == gfn_x(INVALID_GFN) )
+ if ( gfn_eq(iorp->gfn, INVALID_GFN) )
return;
destroy_ring_for_helper(&iorp->va, iorp->page);
@@ -251,7 +250,7 @@ static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
if ( !IS_DEFAULT(s) )
hvm_free_ioreq_gfn(s, iorp->gfn);
- iorp->gfn = gfn_x(INVALID_GFN);
+ iorp->gfn = INVALID_GFN;
}
static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
@@ -264,16 +263,17 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
return -EINVAL;
if ( IS_DEFAULT(s) )
- iorp->gfn = buf ?
- d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN] :
- d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN];
+ iorp->gfn = _gfn(buf ?
+ d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN] :
+ d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN]);
else
iorp->gfn = hvm_alloc_ioreq_gfn(s);
- if ( iorp->gfn == gfn_x(INVALID_GFN) )
+ if ( gfn_eq(iorp->gfn, INVALID_GFN) )
return -ENOMEM;
- rc = prepare_ring_for_helper(d, iorp->gfn, &iorp->page, &iorp->va);
+ rc = prepare_ring_for_helper(d, gfn_x(iorp->gfn), &iorp->page,
+ &iorp->va);
if ( rc )
hvm_unmap_ioreq_gfn(s, buf);
@@ -309,10 +309,10 @@ static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
struct domain *d = s->domain;
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
- if ( IS_DEFAULT(s) || iorp->gfn == gfn_x(INVALID_GFN) )
+ if ( IS_DEFAULT(s) || gfn_eq(iorp->gfn, INVALID_GFN) )
return;
- if ( guest_physmap_remove_page(d, _gfn(iorp->gfn),
+ if ( guest_physmap_remove_page(d, iorp->gfn,
_mfn(page_to_mfn(iorp->page)), 0) )
domain_crash(d);
clear_page(iorp->va);
@@ -324,12 +324,12 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
int rc;
- if ( IS_DEFAULT(s) || iorp->gfn == gfn_x(INVALID_GFN) )
+ if ( IS_DEFAULT(s) || gfn_eq(iorp->gfn, INVALID_GFN) )
return 0;
clear_page(iorp->va);
- rc = guest_physmap_add_page(d, _gfn(iorp->gfn),
+ rc = guest_physmap_add_page(d, iorp->gfn,
_mfn(page_to_mfn(iorp->page)), 0);
if ( rc == 0 )
paging_mark_dirty(d, _mfn(page_to_mfn(iorp->page)));
@@ -590,8 +590,8 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
INIT_LIST_HEAD(&s->ioreq_vcpu_list);
spin_lock_init(&s->bufioreq_lock);
- s->ioreq.gfn = gfn_x(INVALID_GFN);
- s->bufioreq.gfn = gfn_x(INVALID_GFN);
+ s->ioreq.gfn = INVALID_GFN;
+ s->bufioreq.gfn = INVALID_GFN;
rc = hvm_ioreq_server_alloc_rangesets(s, id);
if ( rc )
@@ -757,11 +757,11 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
ASSERT(!IS_DEFAULT(s));
- *ioreq_gfn = s->ioreq.gfn;
+ *ioreq_gfn = gfn_x(s->ioreq.gfn);
if ( s->bufioreq.va != NULL )
{
- *bufioreq_gfn = s->bufioreq.gfn;
+ *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
*bufioreq_port = s->bufioreq_evtchn;
}
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index e17bbe4004..3bd9c5d7c0 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -36,7 +36,7 @@
#include <public/hvm/dm_op.h>
struct hvm_ioreq_page {
- unsigned long gfn;
+ gfn_t gfn;
struct page_info *page;
void *va;
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 04/11] x86/hvm/ioreq: defer mapping gfns until they are actually requsted
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (2 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 03/11] x86/hvm/ioreq: use gfn_t in struct hvm_ioreq_page Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources Paul Durrant
` (6 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Paul Durrant
A subsequent patch will introduce a new scheme to allow an emulator to
map ioreq server pages directly from Xen rather than the guest P2M.
This patch lays the groundwork for that change by deferring mapping of
gfns until their values are requested by an emulator. To that end, the
pad field of the xen_dm_op_get_ioreq_server_info structure is re-purposed
to a flags field and new flag, XEN_DMOP_no_gfns, defined which modifies the
behaviour of XEN_DMOP_get_ioreq_server_info to allow the caller to avoid
requesting the gfn values.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
v8:
- For safety make all of the pointers passed to
hvm_get_ioreq_server_info() optional.
- Shrink bufioreq_handling down to a uint8_t.
v3:
- Updated in response to review comments from Wei and Roger.
- Added a HANDLE_BUFIOREQ macro to make the code neater.
- This patch no longer introduces a security vulnerability since there
is now an explicit limit on the number of ioreq servers that may be
created for any one domain.
---
tools/libs/devicemodel/core.c | 8 +++++
tools/libs/devicemodel/include/xendevicemodel.h | 6 ++--
xen/arch/x86/hvm/dm.c | 9 +++--
xen/arch/x86/hvm/ioreq.c | 47 ++++++++++++++-----------
xen/include/asm-x86/hvm/domain.h | 2 +-
xen/include/public/hvm/dm_op.h | 32 ++++++++++-------
6 files changed, 63 insertions(+), 41 deletions(-)
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 0f2c1a791f..91c69d103b 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -188,6 +188,14 @@ int xendevicemodel_get_ioreq_server_info(
data->id = id;
+ /*
+ * If the caller is not requesting gfn values then instruct the
+ * hypercall not to retrieve them as this may cause them to be
+ * mapped.
+ */
+ if (!ioreq_gfn && !bufioreq_gfn)
+ data->flags |= XEN_DMOP_no_gfns;
+
rc = xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
if (rc)
return rc;
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
index 13216db04a..d73a76da35 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -61,11 +61,11 @@ int xendevicemodel_create_ioreq_server(
* @parm domid the domain id to be serviced
* @parm id the IOREQ Server id.
* @parm ioreq_gfn pointer to a xen_pfn_t to receive the synchronous ioreq
- * gfn
+ * gfn. (May be NULL if not required)
* @parm bufioreq_gfn pointer to a xen_pfn_t to receive the buffered ioreq
- * gfn
+ * gfn. (May be NULL if not required)
* @parm bufioreq_port pointer to a evtchn_port_t to receive the buffered
- * ioreq event channel
+ * ioreq event channel. (May be NULL if not required)
* @return 0 on success, -1 on failure.
*/
int xendevicemodel_get_ioreq_server_info(
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 9cf53b551c..22fa5b51e3 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -416,16 +416,19 @@ static int dm_op(const struct dmop_args *op_args)
{
struct xen_dm_op_get_ioreq_server_info *data =
&op.u.get_ioreq_server_info;
+ const uint16_t valid_flags = XEN_DMOP_no_gfns;
const_op = false;
rc = -EINVAL;
- if ( data->pad )
+ if ( data->flags & ~valid_flags )
break;
rc = hvm_get_ioreq_server_info(d, data->id,
- &data->ioreq_gfn,
- &data->bufioreq_gfn,
+ (data->flags & XEN_DMOP_no_gfns) ?
+ NULL : &data->ioreq_gfn,
+ (data->flags & XEN_DMOP_no_gfns) ?
+ NULL : &data->bufioreq_gfn,
&data->bufioreq_port);
break;
}
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 64bb13cec9..f654e7796c 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -350,6 +350,9 @@ static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
}
}
+#define HANDLE_BUFIOREQ(s) \
+ ((s)->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF)
+
static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
struct vcpu *v)
{
@@ -371,7 +374,7 @@ static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
sv->ioreq_evtchn = rc;
- if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+ if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
{
struct domain *d = s->domain;
@@ -422,7 +425,7 @@ static void hvm_ioreq_server_remove_vcpu(struct hvm_ioreq_server *s,
list_del(&sv->list_entry);
- if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+ if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
free_xen_event_channel(v->domain, s->bufioreq_evtchn);
free_xen_event_channel(v->domain, sv->ioreq_evtchn);
@@ -449,7 +452,7 @@ static void hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
list_del(&sv->list_entry);
- if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+ if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
free_xen_event_channel(v->domain, s->bufioreq_evtchn);
free_xen_event_channel(v->domain, sv->ioreq_evtchn);
@@ -460,14 +463,13 @@ static void hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
spin_unlock(&s->lock);
}
-static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
- bool handle_bufioreq)
+static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s)
{
int rc;
rc = hvm_map_ioreq_gfn(s, false);
- if ( !rc && handle_bufioreq )
+ if ( !rc && HANDLE_BUFIOREQ(s) )
rc = hvm_map_ioreq_gfn(s, true);
if ( rc )
@@ -597,13 +599,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
if ( rc )
return rc;
- if ( bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_ATOMIC )
- s->bufioreq_atomic = true;
-
- rc = hvm_ioreq_server_map_pages(
- s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
- if ( rc )
- goto fail_map;
+ s->bufioreq_handling = bufioreq_handling;
for_each_vcpu ( d, v )
{
@@ -618,9 +614,6 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
hvm_ioreq_server_remove_all_vcpus(s);
hvm_ioreq_server_unmap_pages(s);
- fail_map:
- hvm_ioreq_server_free_rangesets(s);
-
return rc;
}
@@ -757,12 +750,23 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
ASSERT(!IS_DEFAULT(s));
- *ioreq_gfn = gfn_x(s->ioreq.gfn);
+ if ( ioreq_gfn || bufioreq_gfn )
+ {
+ rc = hvm_ioreq_server_map_pages(s);
+ if ( rc )
+ goto out;
+ }
- if ( s->bufioreq.va != NULL )
+ if ( ioreq_gfn )
+ *ioreq_gfn = gfn_x(s->ioreq.gfn);
+
+ if ( HANDLE_BUFIOREQ(s) )
{
- *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
- *bufioreq_port = s->bufioreq_evtchn;
+ if ( bufioreq_gfn )
+ *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
+
+ if ( bufioreq_port )
+ *bufioreq_port = s->bufioreq_evtchn;
}
rc = 0;
@@ -1264,7 +1268,8 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
pg->ptrs.write_pointer += qw ? 2 : 1;
/* Canonicalize read/write pointers to prevent their overflow. */
- while ( s->bufioreq_atomic && qw++ < IOREQ_BUFFER_SLOT_NUM &&
+ while ( (s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_ATOMIC) &&
+ qw++ < IOREQ_BUFFER_SLOT_NUM &&
pg->ptrs.read_pointer >= IOREQ_BUFFER_SLOT_NUM )
{
union bufioreq_pointers old = pg->ptrs, new;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 3bd9c5d7c0..8b798ee4e9 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -69,7 +69,7 @@ struct hvm_ioreq_server {
evtchn_port_t bufioreq_evtchn;
struct rangeset *range[NR_IO_RANGE_TYPES];
bool enabled;
- bool bufioreq_atomic;
+ uint8_t bufioreq_handling;
};
/*
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 6bbab5fca3..9677bd74e7 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -79,28 +79,34 @@ struct xen_dm_op_create_ioreq_server {
* XEN_DMOP_get_ioreq_server_info: Get all the information necessary to
* access IOREQ Server <id>.
*
- * The emulator needs to map the synchronous ioreq structures and buffered
- * ioreq ring (if it exists) that Xen uses to request emulation. These are
- * hosted in the target domain's gmfns <ioreq_gfn> and <bufioreq_gfn>
- * respectively. In addition, if the IOREQ Server is handling buffered
- * emulation requests, the emulator needs to bind to event channel
- * <bufioreq_port> to listen for them. (The event channels used for
- * synchronous emulation requests are specified in the per-CPU ioreq
- * structures in <ioreq_gfn>).
- * If the IOREQ Server is not handling buffered emulation requests then the
- * values handed back in <bufioreq_gfn> and <bufioreq_port> will both be 0.
+ * If the IOREQ Server is handling buffered emulation requests, the
+ * emulator needs to bind to event channel <bufioreq_port> to listen for
+ * them. (The event channels used for synchronous emulation requests are
+ * specified in the per-CPU ioreq structures).
+ * In addition, if the XENMEM_acquire_resource memory op cannot be used,
+ * the emulator will need to map the synchronous ioreq structures and
+ * buffered ioreq ring (if it exists) from guest memory. If <flags> does
+ * not contain XEN_DMOP_no_gfns then these pages will be made available and
+ * the frame numbers passed back in gfns <ioreq_gfn> and <bufioreq_gfn>
+ * respectively. (If the IOREQ Server is not handling buffered emulation
+ * only <ioreq_gfn> will be valid).
*/
#define XEN_DMOP_get_ioreq_server_info 2
struct xen_dm_op_get_ioreq_server_info {
/* IN - server id */
ioservid_t id;
- uint16_t pad;
+ /* IN - flags */
+ uint16_t flags;
+
+#define _XEN_DMOP_no_gfns 0
+#define XEN_DMOP_no_gfns (1u << _XEN_DMOP_no_gfns)
+
/* OUT - buffered ioreq port */
evtchn_port_t bufioreq_port;
- /* OUT - sync ioreq gfn */
+ /* OUT - sync ioreq gfn (see block comment above) */
uint64_aligned_t ioreq_gfn;
- /* OUT - buffered ioreq gfn */
+ /* OUT - buffered ioreq gfn (see block comment above)*/
uint64_aligned_t bufioreq_gfn;
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (3 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 04/11] x86/hvm/ioreq: defer mapping gfns until they are actually requsted Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 14:45 ` Daniel De Graaf
` (2 more replies)
2017-10-17 13:24 ` [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type Paul Durrant
` (5 subsequent siblings)
10 siblings, 3 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Jan Beulich, Daniel De Graaf
Certain memory resources associated with a guest are not necessarily
present in the guest P2M.
This patch adds the boilerplate for new memory op to allow such a resource
to be priv-mapped directly, by either a PV or HVM tools domain.
NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
I have no means to test it on an ARM platform and so cannot verify
that it functions correctly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.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>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Julien Grall <julien.grall@arm.com>
v12:
- Addressed more comments form Jan.
- Removed #ifdef CONFIG_X86 from common code and instead introduced a
stub set_foreign_p2m_entry() in asm-arm/p2m.h returning -EOPNOTSUPP.
- Restricted mechanism for querying implementation limit on nr_frames
and simplified compat code.
v11:
- Addressed more comments from Jan.
v9:
- Addressed more comments from Jan.
v8:
- Move the code into common as requested by Jan.
- Make the gmfn_list handle a 64-bit type to avoid limiting the MFN
range for a 32-bit tools domain.
- Add missing pad.
- Add compat code.
- Make this patch deal with purely boilerplate.
- Drop George's A-b and Wei's R-b because the changes are non-trivial,
and update Cc list now the boilerplate is common.
v5:
- Switched __copy_to/from_guest_offset() to copy_to/from_guest_offset().
---
tools/flask/policy/modules/xen.if | 4 +-
xen/arch/x86/mm/p2m.c | 3 +-
xen/common/compat/memory.c | 95 +++++++++++++++++++++++++++++++++++++
xen/common/memory.c | 94 ++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 6 +++
xen/include/asm-x86/p2m.h | 3 ++
xen/include/public/memory.h | 43 ++++++++++++++++-
xen/include/xlat.lst | 1 +
xen/include/xsm/dummy.h | 6 +++
xen/include/xsm/xsm.h | 6 +++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 6 +++
xen/xsm/flask/policy/access_vectors | 2 +
13 files changed, 266 insertions(+), 4 deletions(-)
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 55437496f6..07cba8a15d 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -52,7 +52,8 @@ 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 set_gnttab_limits };
+ psr_cmt_op psr_cat_op soft_reset set_gnttab_limits
+ resource_map };
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 };
@@ -152,6 +153,7 @@ define(`device_model', `
allow $1 $2_target:domain { getdomaininfo shutdown };
allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack };
allow $1 $2_target:hvm { getparam setparam hvmctl cacheattr dm };
+ allow $1 $2_target:domain2 resource_map;
')
# make_device_model(priv, dm_dom, hvm_dom)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c72a3cdebb..71bb9b4f93 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1132,8 +1132,7 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
}
/* Set foreign mfn in the given guest's p2m table. */
-static int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
- mfn_t mfn)
+int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
{
return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign,
p2m_get_hostp2m(d)->default_access);
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 35bb259808..7f2e2e3107 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -71,6 +71,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
struct xen_remove_from_physmap *xrfp;
struct xen_vnuma_topology_info *vnuma;
struct xen_mem_access_op *mao;
+ struct xen_mem_acquire_resource *mar;
} nat;
union {
struct compat_memory_reservation rsrv;
@@ -79,6 +80,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
struct compat_add_to_physmap_batch atpb;
struct compat_vnuma_topology_info vnuma;
struct compat_mem_access_op mao;
+ struct compat_mem_acquire_resource mar;
} cmp;
set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
@@ -395,6 +397,57 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
}
#endif
+ case XENMEM_acquire_resource:
+ {
+ xen_ulong_t *xen_frame_list;
+ unsigned int max_nr_frames;
+
+ if ( copy_from_guest(&cmp.mar, compat, 1) )
+ return -EFAULT;
+
+ /*
+ * The number of frames handled is currently limited to a
+ * small number by the underlying implementation, so the
+ * scratch space should be sufficient for bouncing the
+ * frame addresses.
+ */
+ max_nr_frames = (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
+ sizeof(*xen_frame_list);
+
+ if ( cmp.mar.nr_frames > max_nr_frames )
+ return -E2BIG;
+
+ if ( compat_handle_is_null(cmp.mar.frame_list) )
+ xen_frame_list = NULL;
+ else
+ {
+ xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
+
+ if ( !compat_handle_okay(cmp.mar.frame_list,
+ cmp.mar.nr_frames) )
+ return -EFAULT;
+
+ for ( i = 0; i < cmp.mar.nr_frames; i++ )
+ {
+ compat_ulong_t frame;
+
+ if ( __copy_from_compat_offset(
+ &frame, cmp.mar.frame_list, i, 1) )
+ return -EFAULT;
+
+ xen_frame_list[i] = frame;
+ }
+ }
+
+#define XLAT_mem_acquire_resource_HNDL_frame_list(_d_, _s_) \
+ set_xen_guest_handle((_d_)->frame_list, xen_frame_list)
+
+ XLAT_mem_acquire_resource(nat.mar, &cmp.mar);
+
+#undef XLAT_mem_acquire_resource_HNDL_frame_list
+
+ break;
+ }
default:
return compat_arch_memory_op(cmd, compat);
}
@@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
rc = -EFAULT;
break;
+ case XENMEM_acquire_resource:
+ {
+ const xen_ulong_t *xen_frame_list =
+ (xen_ulong_t *)(nat.mar + 1);
+ compat_ulong_t *compat_frame_list =
+ (compat_ulong_t *)(nat.mar + 1);
+
+ if ( cmp.mar.nr_frames == 0 )
+ {
+ DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
+
+ if ( __copy_field_to_guest(
+ guest_handle_cast(compat,
+ compat_mem_acquire_resource_t),
+ &cmp.mar, nr_frames) )
+ return -EFAULT;
+ }
+ else
+ {
+ /*
+ * NOTE: the smaller compat array overwrites the native
+ * array.
+ */
+ for ( i = 0; i < cmp.mar.nr_frames; i++ )
+ {
+ compat_ulong_t frame = xen_frame_list[i];
+
+ if ( frame != xen_frame_list[i] )
+ return -ERANGE;
+
+ compat_frame_list[i] = frame;
+ }
+
+ if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
+ compat_frame_list,
+ cmp.mar.nr_frames) )
+ return -EFAULT;
+ }
+
+ break;
+ }
+
default:
domain_crash(current->domain);
split = 0;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad987e0f29..cdd2e030cf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
}
+static int acquire_resource(
+ XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
+{
+ struct domain *d, *currd = current->domain;
+ xen_mem_acquire_resource_t xmar;
+ unsigned long mfn_list[2];
+ int rc;
+
+ if ( copy_from_guest(&xmar, arg, 1) )
+ return -EFAULT;
+
+ if ( xmar.pad != 0 )
+ return -EINVAL;
+
+ if ( guest_handle_is_null(xmar.frame_list) )
+ {
+ /* Special case for querying implementation limit */
+ if ( xmar.nr_frames == 0 )
+ {
+ xmar.nr_frames = ARRAY_SIZE(mfn_list);
+
+ if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
+ return -EFAULT;
+
+ return 0;
+ }
+
+ return -EINVAL;
+ }
+
+ if ( xmar.nr_frames == 0 )
+ return -EINVAL;
+
+ if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
+ return -E2BIG;
+
+ d = rcu_lock_domain_by_any_id(xmar.domid);
+ if ( d == NULL )
+ return -ESRCH;
+
+ rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
+ if ( rc )
+ goto out;
+
+ switch ( xmar.type )
+ {
+ default:
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
+ if ( rc )
+ goto out;
+
+ if ( !paging_mode_translate(currd) )
+ {
+ if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
+ rc = -EFAULT;
+ }
+ else
+ {
+ xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
+ unsigned int i;
+
+ rc = -EFAULT;
+ if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
+ goto out;
+
+ for ( i = 0; i < xmar.nr_frames; i++ )
+ {
+ rc = set_foreign_p2m_entry(currd, gfn_list[i],
+ _mfn(mfn_list[i]));
+ if ( rc )
+ {
+ /*
+ * Make sure rc is -EIO for any interation other than
+ * the first.
+ */
+ rc = (i != 0) ? -EIO : rc;
+ break;
+ }
+ }
+ }
+
+ out:
+ rcu_unlock_domain(d);
+ return rc;
+}
+
long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
struct domain *d, *curr_d = current->domain;
@@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
}
#endif
+ case XENMEM_acquire_resource:
+ rc = acquire_resource(
+ guest_handle_cast(arg, xen_mem_acquire_resource_t));
+ break;
+
default:
rc = arch_memory_op(cmd, arg);
break;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index faadcfe8fe..a5caa747ce 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -346,6 +346,12 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
return gfn_add(gfn, 1UL << order);
}
+static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
+ mfn_t mfn)
+{
+ return -EOPNOTSUPP;
+}
+
#endif /* _XEN_P2M_H */
/*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 17b1d0c8d3..44f7ec088c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -620,6 +620,9 @@ void p2m_memory_type_changed(struct domain *d);
int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
unsigned long end);
+/* Set foreign entry in the p2m table (for priv-mapping) */
+int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+
/* Set mmio addresses in the p2m table (for pass-through) */
int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
unsigned int order, p2m_access_t access);
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 29386df98b..18118ea5c6 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
+/*
+ * Get the pages for a particular guest resource, so that they can be
+ * mapped directly by a tools domain.
+ */
+#define XENMEM_acquire_resource 28
+struct xen_mem_acquire_resource {
+ /* IN - the domain whose resource is to be mapped */
+ domid_t domid;
+ /* IN - the type of resource */
+ uint16_t type;
+ /*
+ * IN - a type-specific resource identifier, which must be zero
+ * unless stated otherwise.
+ */
+ uint32_t id;
+ /* IN/OUT - As an IN parameter number of frames of the resource
+ * to be mapped. However, if the specified value is 0 and
+ * frame_list is NULL then this field will be set to the
+ * maximum value supported by the implementation on return.
+ */
+ uint32_t nr_frames;
+ uint32_t pad;
+ /* IN - the index of the initial frame to be mapped. This parameter
+ * is ignored if nr_frames is 0.
+ */
+ uint64_aligned_t frame;
+ /* IN/OUT - If the tools domain is PV then, upon return, frame_list
+ * will be populated with the MFNs of the resource.
+ * If the tools domain is HVM then it is expected that, on
+ * entry, frame_list will be populated with a list of GFNs
+ * that will be mapped to the MFNs of the resource.
+ * If -EIO is returned then the frame_list has only been
+ * partially mapped and it is up to the caller to unmap all
+ * the GFNs.
+ * This parameter may be NULL if nr_frames is 0.
+ */
+ XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
+};
+typedef struct xen_mem_acquire_resource xen_mem_acquire_resource_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
+
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
/*
@@ -650,7 +691,7 @@ struct xen_vnuma_topology_info {
typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
-/* Next available subop number is 28 */
+/* Next available subop number is 29 */
#endif /* __XEN_PUBLIC_MEMORY_H__ */
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 0f17000ea7..5835872334 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -83,6 +83,7 @@
! memory_map memory.h
! memory_reservation memory.h
! mem_access_op memory.h
+! mem_acquire_resource memory.h
! pod_target memory.h
! remove_from_physmap memory.h
! reserved_device_memory_map memory.h
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index ba89ea4bc1..8f04d59a3e 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -724,3 +724,9 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
return xsm_default_action(XSM_PRIV, current->domain, NULL);
}
}
+
+static XSM_INLINE int xsm_domain_resource_map(XSM_DEFAULT_ARG struct domain *d)
+{
+ XSM_ASSERT_ACTION(XSM_DM_PRIV);
+ return xsm_default_action(action, current->domain, d);
+}
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 7f7feffc68..d0db860ae0 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -180,6 +180,7 @@ struct xsm_operations {
int (*dm_op) (struct domain *d);
#endif
int (*xen_version) (uint32_t cmd);
+ int (*domain_resource_map) (struct domain *d);
};
#ifdef CONFIG_XSM
@@ -692,6 +693,11 @@ static inline int xsm_xen_version (xsm_default_t def, uint32_t op)
return xsm_ops->xen_version(op);
}
+static inline int xsm_domain_resource_map(xsm_default_t def, struct domain *d)
+{
+ return xsm_ops->domain_resource_map(d);
+}
+
#endif /* XSM_NO_WRAPPERS */
#ifdef CONFIG_MULTIBOOT
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 479b103614..6e751199ee 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -157,4 +157,5 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, dm_op);
#endif
set_to_dummy_if_null(ops, xen_version);
+ set_to_dummy_if_null(ops, domain_resource_map);
}
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 7b005af834..7e9efcd865 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1718,6 +1718,11 @@ static int flask_xen_version (uint32_t op)
}
}
+static int flask_domain_resource_map(struct domain *d)
+{
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__RESOURCE_MAP);
+}
+
long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
int compat_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
@@ -1851,6 +1856,7 @@ static struct xsm_operations flask_ops = {
.dm_op = flask_dm_op,
#endif
.xen_version = flask_xen_version,
+ .domain_resource_map = flask_domain_resource_map,
};
void __init flask_init(const void *policy_buffer, size_t policy_size)
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 3a2d863b8f..341ade1f7d 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -250,6 +250,8 @@ class domain2
psr_cat_op
# XEN_DOMCTL_set_gnttab_limits
set_gnttab_limits
+# XENMEM_resource_map
+ resource_map
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-17 13:24 ` [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources Paul Durrant
@ 2017-10-17 14:45 ` Daniel De Graaf
2017-10-19 12:22 ` Julien Grall
2017-10-26 15:26 ` Jan Beulich
2 siblings, 0 replies; 45+ messages in thread
From: Daniel De Graaf @ 2017-10-17 14:45 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich
On 10/17/2017 09:24 AM, Paul Durrant wrote:
> Certain memory resources associated with a guest are not necessarily
> present in the guest P2M.
>
> This patch adds the boilerplate for new memory op to allow such a resource
> to be priv-mapped directly, by either a PV or HVM tools domain.
>
> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
> I have no means to test it on an ARM platform and so cannot verify
> that it functions correctly.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-17 13:24 ` [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources Paul Durrant
2017-10-17 14:45 ` Daniel De Graaf
@ 2017-10-19 12:22 ` Julien Grall
2017-10-19 12:57 ` Paul Durrant
2017-10-26 15:26 ` Jan Beulich
2 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 12:22 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Daniel De Graaf, Roger Pau Monné
Hi,
On 17/10/17 14:24, Paul Durrant wrote:
> Certain memory resources associated with a guest are not necessarily
> present in the guest P2M.
>
> This patch adds the boilerplate for new memory op to allow such a resource
> to be priv-mapped directly, by either a PV or HVM tools domain.
>
> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
> I have no means to test it on an ARM platform and so cannot verify
> that it functions correctly.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
[...]
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index ad987e0f29..cdd2e030cf 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
[...]
> + if ( rc )
> + goto out;
> +
> + if ( !paging_mode_translate(currd) )
> + {
> + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> + rc = -EFAULT;
> + }
> + else
> + {
> + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> + unsigned int i;
> +
> + rc = -EFAULT;
> + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> + goto out;
> +
> + for ( i = 0; i < xmar.nr_frames; i++ )
> + {
> + rc = set_foreign_p2m_entry(currd, gfn_list[i],
> + _mfn(mfn_list[i]));
Something looks a bit odd to me here. When I read foreign mapping, I
directly associate to mapping from a foreign domain.
On Arm, we will always get a reference on that page to prevent it
disappearing if the foreign domain is destroyed but the mapping is still
present.
This reference will either be put with an unmapped hypercall or while
teardown the domain.
Per my understanding, this MFN does not belong to any domain (or at
least currd). Right? So there is no way to get/put a reference on that
page. So I am unconvinced that this is very safe.
Also looking at the x86 side, I can't find such reference in the foreign
path in p2m_add_foreign. Did I miss anything?
Note that x86 does not handle p2m teardown with foreign map at the
moment (see p2m_add_foreign).
You are by-passing this check and I can't see how this would be safe for
the x86 side too.
> + if ( rc )
> + {
> + /*
> + * Make sure rc is -EIO for any interation other than
> + * the first.
> + */
> + rc = (i != 0) ? -EIO : rc;
> + break;
> + }
> + }
> + }
> +
> + out:
> + rcu_unlock_domain(d);
> + return rc;
> +}
> +
> long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> struct domain *d, *curr_d = current->domain;
> @@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> }
> #endif
>
> + case XENMEM_acquire_resource:
> + rc = acquire_resource(
> + guest_handle_cast(arg, xen_mem_acquire_resource_t));
> + break;
> +
> default:
> rc = arch_memory_op(cmd, arg);
> break;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index faadcfe8fe..a5caa747ce 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -346,6 +346,12 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
> return gfn_add(gfn, 1UL << order);
> }
>
> +static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
Please modifify the prototype to use gfn_t.
> + mfn_t mfn)
> +{
> + return -EOPNOTSUPP;
> +} > +
> #endif /* _XEN_P2M_H */
>
[...]
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 29386df98b..18118ea5c6 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
> typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
> DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>
> +/*
> + * Get the pages for a particular guest resource, so that they can be
> + * mapped directly by a tools domain.
> + */
> +#define XENMEM_acquire_resource 28
> +struct xen_mem_acquire_resource {
> + /* IN - the domain whose resource is to be mapped */
> + domid_t domid;
> + /* IN - the type of resource */
> + uint16_t type;
> + /*
> + * IN - a type-specific resource identifier, which must be zero
> + * unless stated otherwise.
> + */
> + uint32_t id;
> + /* IN/OUT - As an IN parameter number of frames of the resource
Coding style:
/*
*
> + * to be mapped. However, if the specified value is 0 and
> + * frame_list is NULL then this field will be set to the
> + * maximum value supported by the implementation on return.
> + */
> + uint32_t nr_frames;
> + uint32_t pad;
> + /* IN - the index of the initial frame to be mapped. This parameter
Ditto
> + * is ignored if nr_frames is 0.
> + */
> + uint64_aligned_t frame;
> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
Ditto
> + * will be populated with the MFNs of the resource.
> + * If the tools domain is HVM then it is expected that, on
> + * entry, frame_list will be populated with a list of GFNs
> + * that will be mapped to the MFNs of the resource.
> + * If -EIO is returned then the frame_list has only been
> + * partially mapped and it is up to the caller to unmap all
> + * the GFNs.
> + * This parameter may be NULL if nr_frames is 0.
> + */
> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> +};
> +typedef struct xen_mem_acquire_resource xen_mem_acquire_resource_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
> +
> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>
> /*
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 12:22 ` Julien Grall
@ 2017-10-19 12:57 ` Paul Durrant
2017-10-19 13:29 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-19 12:57 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 19 October 2017 13:23
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
> Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> Hi,
>
> On 17/10/17 14:24, Paul Durrant wrote:
> > Certain memory resources associated with a guest are not necessarily
> > present in the guest P2M.
> >
> > This patch adds the boilerplate for new memory op to allow such a
> resource
> > to be priv-mapped directly, by either a PV or HVM tools domain.
> >
> > NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
> > I have no means to test it on an ARM platform and so cannot verify
> > that it functions correctly.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
>
> [...]
>
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index ad987e0f29..cdd2e030cf 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
>
> [...]
>
> > + if ( rc )
> > + goto out;
> > +
> > + if ( !paging_mode_translate(currd) )
> > + {
> > + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> > + rc = -EFAULT;
> > + }
> > + else
> > + {
> > + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > + unsigned int i;
> > +
> > + rc = -EFAULT;
> > + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> > + goto out;
> > +
> > + for ( i = 0; i < xmar.nr_frames; i++ )
> > + {
> > + rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > + _mfn(mfn_list[i]));
>
> Something looks a bit odd to me here. When I read foreign mapping, I
> directly associate to mapping from a foreign domain.
>
> On Arm, we will always get a reference on that page to prevent it
> disappearing if the foreign domain is destroyed but the mapping is still
> present.
>
> This reference will either be put with an unmapped hypercall or while
> teardown the domain.
>
> Per my understanding, this MFN does not belong to any domain (or at
> least currd). Right?
No. The mfns do belong to the target domain.
> So there is no way to get/put a reference on that
> page. So I am unconvinced that this is very safe.
>
> Also looking at the x86 side, I can't find such reference in the foreign
> path in p2m_add_foreign. Did I miss anything?
No, I don't think there is any reference counting there... but this is no different to priv mapping. I'm not trying to fix the mapping infrastructure at this point.
>
> Note that x86 does not handle p2m teardown with foreign map at the
> moment (see p2m_add_foreign).
>
> You are by-passing this check and I can't see how this would be safe for
> the x86 side too.
>
I don't follow. What check am I by-passing that is covered when priv mapping?
> > + if ( rc )
> > + {
> > + /*
> > + * Make sure rc is -EIO for any interation other than
> > + * the first.
> > + */
> > + rc = (i != 0) ? -EIO : rc;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + out:
> > + rcu_unlock_domain(d);
> > + return rc;
> > +}
> > +
> > long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> > {
> > struct domain *d, *curr_d = current->domain;
> > @@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> > }
> > #endif
> >
> > + case XENMEM_acquire_resource:
> > + rc = acquire_resource(
> > + guest_handle_cast(arg, xen_mem_acquire_resource_t));
> > + break;
> > +
> > default:
> > rc = arch_memory_op(cmd, arg);
> > break;
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index faadcfe8fe..a5caa747ce 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -346,6 +346,12 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn,
> unsigned int order)
> > return gfn_add(gfn, 1UL << order);
> > }
> >
> > +static inline int set_foreign_p2m_entry(struct domain *d, unsigned long
> gfn,
>
> Please modifify the prototype to use gfn_t.
>
<sigh> I only put this stub in to match x86 so I can avoid the #ifdefs that Jan objects to. Which other bits of the universe do I need to fix?
> > + mfn_t mfn)
> > +{
> > + return -EOPNOTSUPP;
> > +} > +
> > #endif /* _XEN_P2M_H */
> >
>
> [...]
>
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 29386df98b..18118ea5c6 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
> > typedef struct xen_reserved_device_memory_map
> xen_reserved_device_memory_map_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
> >
> > +/*
> > + * Get the pages for a particular guest resource, so that they can be
> > + * mapped directly by a tools domain.
> > + */
> > +#define XENMEM_acquire_resource 28
> > +struct xen_mem_acquire_resource {
> > + /* IN - the domain whose resource is to be mapped */
> > + domid_t domid;
> > + /* IN - the type of resource */
> > + uint16_t type;
> > + /*
> > + * IN - a type-specific resource identifier, which must be zero
> > + * unless stated otherwise.
> > + */
> > + uint32_t id;
> > + /* IN/OUT - As an IN parameter number of frames of the resource
>
> Coding style:
>
> /*
> *
Ok.
>
> > + * to be mapped. However, if the specified value is 0 and
> > + * frame_list is NULL then this field will be set to the
> > + * maximum value supported by the implementation on return.
> > + */
> > + uint32_t nr_frames;
> > + uint32_t pad;
> > + /* IN - the index of the initial frame to be mapped. This parameter
>
> Ditto
>
Ok.
> > + * is ignored if nr_frames is 0.
> > + */
> > + uint64_aligned_t frame;
> > + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>
> Ditto
> > + * will be populated with the MFNs of the resource.
> > + * If the tools domain is HVM then it is expected that, on
> > + * entry, frame_list will be populated with a list of GFNs
> > + * that will be mapped to the MFNs of the resource.
> > + * If -EIO is returned then the frame_list has only been
> > + * partially mapped and it is up to the caller to unmap all
> > + * the GFNs.
> > + * This parameter may be NULL if nr_frames is 0.
> > + */
> > + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> > +};
> > +typedef struct xen_mem_acquire_resource
> xen_mem_acquire_resource_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
> > +
> > #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> >
> > /*
>
Sorry to be getting frustrated with this, but I'm wondering how many more colours I need to paint this bike-shed.
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 12:57 ` Paul Durrant
@ 2017-10-19 13:29 ` Julien Grall
2017-10-19 13:35 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 13:29 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
Hi,
On 10/19/2017 01:57 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 19 October 2017 13:23
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan Beulich
>> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
>> Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi,
>>
>> On 17/10/17 14:24, Paul Durrant wrote:
>>> Certain memory resources associated with a guest are not necessarily
>>> present in the guest P2M.
>>>
>>> This patch adds the boilerplate for new memory op to allow such a
>> resource
>>> to be priv-mapped directly, by either a PV or HVM tools domain.
>>>
>>> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
>>> I have no means to test it on an ARM platform and so cannot verify
>>> that it functions correctly.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> ---
>>
>> [...]
>>
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index ad987e0f29..cdd2e030cf 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>>
>> [...]
>>
>>> + if ( rc )
>>> + goto out;
>>> +
>>> + if ( !paging_mode_translate(currd) )
>>> + {
>>> + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
>>> + rc = -EFAULT;
>>> + }
>>> + else
>>> + {
>>> + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>>> + unsigned int i;
>>> +
>>> + rc = -EFAULT;
>>> + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
>>> + goto out;
>>> +
>>> + for ( i = 0; i < xmar.nr_frames; i++ )
>>> + {
>>> + rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>> + _mfn(mfn_list[i]));
>>
>> Something looks a bit odd to me here. When I read foreign mapping, I
>> directly associate to mapping from a foreign domain.
>>
>> On Arm, we will always get a reference on that page to prevent it
>> disappearing if the foreign domain is destroyed but the mapping is still
>> present.
>>
>> This reference will either be put with an unmapped hypercall or while
>> teardown the domain.
>>
>> Per my understanding, this MFN does not belong to any domain (or at
>> least currd). Right?
>
> No. The mfns do belong to the target domain.
To be fully safe, you need to take a reference on each page you mapped.
So who is going to get a reference on them? Who is going to drop that?
>
>> So there is no way to get/put a reference on that
>> page. So I am unconvinced that this is very safe.
>>
>> Also looking at the x86 side, I can't find such reference in the foreign
>> path in p2m_add_foreign. Did I miss anything?
>
> No, I don't think there is any reference counting there... but this is no different to priv mapping. I'm not trying to fix the mapping infrastructure at this point.
>
>>
>> Note that x86 does not handle p2m teardown with foreign map at the
>> moment (see p2m_add_foreign).
>>
>> You are by-passing this check and I can't see how this would be safe for
>> the x86 side too.
>>
>
> I don't follow. What check am I by-passing that is covered when priv mapping?
/*
* hvm fixme: until support is added to p2m teardown code to
cleanup any
* foreign entries, limit this to hardware domain only.
*/
How this is safe with your new solution? That looks like a regression...
[...]
>>> + * will be populated with the MFNs of the resource.
>>> + * If the tools domain is HVM then it is expected that, on
>>> + * entry, frame_list will be populated with a list of GFNs
>>> + * that will be mapped to the MFNs of the resource.
>>> + * If -EIO is returned then the frame_list has only been
>>> + * partially mapped and it is up to the caller to unmap all
>>> + * the GFNs.
>>> + * This parameter may be NULL if nr_frames is 0.
>>> + */
>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>> +};
>>> +typedef struct xen_mem_acquire_resource
>> xen_mem_acquire_resource_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
>>> +
>>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>>
>>> /*
>>
>
> Sorry to be getting frustrated with this, but I'm wondering how many more colours I need to paint this bike-shed.
I don't know how x86 looks like and maybe this is fine for Andrew and
Jan. But for Arm, it does not look correct.
To give you an idea, my first thought to implement your newly wrongly
named function was to just call p2m_set_entry with p2m_map_foreign. But
from this discussion it would look plain wrong.
So this means the interface is not clear enough.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 13:29 ` Julien Grall
@ 2017-10-19 13:35 ` Paul Durrant
2017-10-19 14:12 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-19 13:35 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 19 October 2017 14:29
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
> Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> Hi,
>
> On 10/19/2017 01:57 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@linaro.org]
> >> Sent: 19 October 2017 13:23
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan
> Beulich
> >> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
> >> Pau Monne <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi,
> >>
> >> On 17/10/17 14:24, Paul Durrant wrote:
> >>> Certain memory resources associated with a guest are not necessarily
> >>> present in the guest P2M.
> >>>
> >>> This patch adds the boilerplate for new memory op to allow such a
> >> resource
> >>> to be priv-mapped directly, by either a PV or HVM tools domain.
> >>>
> >>> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
> >>> I have no means to test it on an ARM platform and so cannot verify
> >>> that it functions correctly.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >>> ---
> >>
> >> [...]
> >>
> >>> diff --git a/xen/common/memory.c b/xen/common/memory.c
> >>> index ad987e0f29..cdd2e030cf 100644
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct
> domain
> >> *d, unsigned int space)
> >>
> >> [...]
> >>
> >>> + if ( rc )
> >>> + goto out;
> >>> +
> >>> + if ( !paging_mode_translate(currd) )
> >>> + {
> >>> + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> >>> + rc = -EFAULT;
> >>> + }
> >>> + else
> >>> + {
> >>> + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> >>> + unsigned int i;
> >>> +
> >>> + rc = -EFAULT;
> >>> + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> >>> + goto out;
> >>> +
> >>> + for ( i = 0; i < xmar.nr_frames; i++ )
> >>> + {
> >>> + rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>> + _mfn(mfn_list[i]));
> >>
> >> Something looks a bit odd to me here. When I read foreign mapping, I
> >> directly associate to mapping from a foreign domain.
> >>
> >> On Arm, we will always get a reference on that page to prevent it
> >> disappearing if the foreign domain is destroyed but the mapping is still
> >> present.
> >>
> >> This reference will either be put with an unmapped hypercall or while
> >> teardown the domain.
> >>
> >> Per my understanding, this MFN does not belong to any domain (or at
> >> least currd). Right?
> >
> > No. The mfns do belong to the target domain.
>
> To be fully safe, you need to take a reference on each page you mapped.
> So who is going to get a reference on them? Who is going to drop that?
>
Yes, that's true but it's also true of priv mapping AIUI. I think the correct fix is to deal with this in set_p2m_foreign_entry() so that it is fixed for both cases. I don't think it is something that ought to be addressed here... unless I'm missing something.
> >
> >> So there is no way to get/put a reference on that
> >> page. So I am unconvinced that this is very safe.
> >>
> >> Also looking at the x86 side, I can't find such reference in the foreign
> >> path in p2m_add_foreign. Did I miss anything?
> >
> > No, I don't think there is any reference counting there... but this is no
> different to priv mapping. I'm not trying to fix the mapping infrastructure at
> this point.
> >
> >>
> >> Note that x86 does not handle p2m teardown with foreign map at the
> >> moment (see p2m_add_foreign).
> >>
> >> You are by-passing this check and I can't see how this would be safe for
> >> the x86 side too.
> >>
> >
> > I don't follow. What check am I by-passing that is covered when priv
> mapping?
>
> /*
> * hvm fixme: until support is added to p2m teardown code to
> cleanup any
> * foreign entries, limit this to hardware domain only.
> */
>
> How this is safe with your new solution? That looks like a regression...
Well, the new hypercall is tools-only but I can add the extra check for the hardware domain although it's probably redundant in practice.
>
> [...]
>
> >>> + * will be populated with the MFNs of the resource.
> >>> + * If the tools domain is HVM then it is expected that, on
> >>> + * entry, frame_list will be populated with a list of GFNs
> >>> + * that will be mapped to the MFNs of the resource.
> >>> + * If -EIO is returned then the frame_list has only been
> >>> + * partially mapped and it is up to the caller to unmap all
> >>> + * the GFNs.
> >>> + * This parameter may be NULL if nr_frames is 0.
> >>> + */
> >>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>> +};
> >>> +typedef struct xen_mem_acquire_resource
> >> xen_mem_acquire_resource_t;
> >>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
> >>> +
> >>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> >>>
> >>> /*
> >>
> >
> > Sorry to be getting frustrated with this, but I'm wondering how many more
> colours I need to paint this bike-shed.
>
> I don't know how x86 looks like and maybe this is fine for Andrew and
> Jan. But for Arm, it does not look correct.
>
> To give you an idea, my first thought to implement your newly wrongly
> named function was to just call p2m_set_entry with p2m_map_foreign. But
> from this discussion it would look plain wrong.
>
> So this means the interface is not clear enough.
I'd prefer to make the whole thing x86-only since that's the only platform on which I can test it, and indeed the code used to be x86-only. Jan objected to this so all I'm trying to achieve is that it builds for ARM. Please can you and Jan reach agreement on where the code should live and how, if at all, it should be #ifdef-ed?
Paul
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 13:35 ` Paul Durrant
@ 2017-10-19 14:12 ` Julien Grall
2017-10-19 14:49 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 14:12 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
Hi,
On 19/10/17 14:35, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 19 October 2017 14:29
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan Beulich
>> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
>> Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi,
>>
>> On 10/19/2017 01:57 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall [mailto:julien.grall@linaro.org]
>>>> Sent: 19 October 2017 13:23
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
>> devel@lists.xenproject.org
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>>>> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.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 <julien.grall@arm.com>; Jan
>> Beulich
>>>> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
>>>> Pau Monne <roger.pau@citrix.com>
>>>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>>>> HYPERVISOR_memory_op to acquire guest resources
>>>>
>>>> Hi,
>>>>
>>>> On 17/10/17 14:24, Paul Durrant wrote:
>>>>> Certain memory resources associated with a guest are not necessarily
>>>>> present in the guest P2M.
>>>>>
>>>>> This patch adds the boilerplate for new memory op to allow such a
>>>> resource
>>>>> to be priv-mapped directly, by either a PV or HVM tools domain.
>>>>>
>>>>> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
>>>>> I have no means to test it on an ARM platform and so cannot verify
>>>>> that it functions correctly.
>>>>>
>>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>>>> index ad987e0f29..cdd2e030cf 100644
>>>>> --- a/xen/common/memory.c
>>>>> +++ b/xen/common/memory.c
>>>>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct
>> domain
>>>> *d, unsigned int space)
>>>>
>>>> [...]
>>>>
>>>>> + if ( rc )
>>>>> + goto out;
>>>>> +
>>>>> + if ( !paging_mode_translate(currd) )
>>>>> + {
>>>>> + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
>>>>> + rc = -EFAULT;
>>>>> + }
>>>>> + else
>>>>> + {
>>>>> + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>>>>> + unsigned int i;
>>>>> +
>>>>> + rc = -EFAULT;
>>>>> + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
>>>>> + goto out;
>>>>> +
>>>>> + for ( i = 0; i < xmar.nr_frames; i++ )
>>>>> + {
>>>>> + rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>>>> + _mfn(mfn_list[i]));
>>>>
>>>> Something looks a bit odd to me here. When I read foreign mapping, I
>>>> directly associate to mapping from a foreign domain.
>>>>
>>>> On Arm, we will always get a reference on that page to prevent it
>>>> disappearing if the foreign domain is destroyed but the mapping is still
>>>> present.
>>>>
>>>> This reference will either be put with an unmapped hypercall or while
>>>> teardown the domain.
>>>>
>>>> Per my understanding, this MFN does not belong to any domain (or at
>>>> least currd). Right?
>>>
>>> No. The mfns do belong to the target domain.
>>
>> To be fully safe, you need to take a reference on each page you mapped.
>> So who is going to get a reference on them? Who is going to drop that?
>>
>
> Yes, that's true but it's also true of priv mapping AIUI. I think the correct fix is to deal with this in set_p2m_foreign_entry() so that it is fixed for both cases. I don't think it is something that ought to be addressed here... unless I'm missing something.
For x86 maybe. For Arm, foreign mapping are also exposed to guest and we
get a reference every time. It is probably an oversight on the x86 side.
>
>>>
>>>> So there is no way to get/put a reference on that
>>>> page. So I am unconvinced that this is very safe.
>>>>
>>>> Also looking at the x86 side, I can't find such reference in the foreign
>>>> path in p2m_add_foreign. Did I miss anything?
>>>
>>> No, I don't think there is any reference counting there... but this is no
>> different to priv mapping. I'm not trying to fix the mapping infrastructure at
>> this point.
>>>
>>>>
>>>> Note that x86 does not handle p2m teardown with foreign map at the
>>>> moment (see p2m_add_foreign).
>>>>
>>>> You are by-passing this check and I can't see how this would be safe for
>>>> the x86 side too.
>>>>
>>>
>>> I don't follow. What check am I by-passing that is covered when priv
>> mapping?
>>
>> /*
>> * hvm fixme: until support is added to p2m teardown code to
>> cleanup any
>> * foreign entries, limit this to hardware domain only.
>> */
>>
>> How this is safe with your new solution? That looks like a regression...
>
> Well, the new hypercall is tools-only but I can add the extra check for the hardware domain although it's probably redundant in practice.
Well a foreign domain can be nasty with you. Like removing the mapping
from itself resulting to free the memory... So you may end up
writing/read wrong mapping or even worth in another domain.
But that's may not impact this new hypercall.
>
>>
>> [...]
>>
>>>>> + * will be populated with the MFNs of the resource.
>>>>> + * If the tools domain is HVM then it is expected that, on
>>>>> + * entry, frame_list will be populated with a list of GFNs
>>>>> + * that will be mapped to the MFNs of the resource.
>>>>> + * If -EIO is returned then the frame_list has only been
>>>>> + * partially mapped and it is up to the caller to unmap all
>>>>> + * the GFNs.
>>>>> + * This parameter may be NULL if nr_frames is 0.
>>>>> + */
>>>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>>>> +};
>>>>> +typedef struct xen_mem_acquire_resource
>>>> xen_mem_acquire_resource_t;
>>>>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
>>>>> +
>>>>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>>>>
>>>>> /*
>>>>
>>>
>>> Sorry to be getting frustrated with this, but I'm wondering how many more
>> colours I need to paint this bike-shed.
>>
>> I don't know how x86 looks like and maybe this is fine for Andrew and
>> Jan. But for Arm, it does not look correct.
>>
>> To give you an idea, my first thought to implement your newly wrongly
>> named function was to just call p2m_set_entry with p2m_map_foreign. But
>> from this discussion it would look plain wrong.
>>
>> So this means the interface is not clear enough.
>
> I'd prefer to make the whole thing x86-only since that's the only platform on which I can test it, and indeed the code used to be x86-only. Jan objected to this so all I'm trying to achieve is that it builds for ARM. Please can you and Jan reach agreement on where the code should live and how, if at all, it should be #ifdef-ed?
I am quite surprised of "it is tools-only" so it is fine to not protect
it even if it is x86 only. That's probably going to bite us in the future.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 14:12 ` Julien Grall
@ 2017-10-19 14:49 ` Paul Durrant
2017-10-19 15:11 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-19 14:49 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
[snip]
> >
> > I'd prefer to make the whole thing x86-only since that's the only platform
> on which I can test it, and indeed the code used to be x86-only. Jan objected
> to this so all I'm trying to achieve is that it builds for ARM. Please can you and
> Jan reach agreement on where the code should live and how, if at all, it
> should be #ifdef-ed?
>
> I am quite surprised of "it is tools-only" so it is fine to not protect
> it even if it is x86 only. That's probably going to bite us in the future.
>
So, this appears to have reached an impasse. I don't know how to proceed without having to also fix priv mapping for x86, which is a yak far too large for me to shave at the moment.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 14:49 ` Paul Durrant
@ 2017-10-19 15:11 ` Jan Beulich
2017-10-19 15:37 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2017-10-19 15:11 UTC (permalink / raw)
To: 'Julien Grall'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
>>> On 19.10.17 at 16:49, <Paul.Durrant@citrix.com> wrote:
>> > I'd prefer to make the whole thing x86-only since that's the only platform
>> on which I can test it, and indeed the code used to be x86-only. Jan objected
>> to this so all I'm trying to achieve is that it builds for ARM. Please can you and
>> Jan reach agreement on where the code should live and how, if at all, it
>> should be #ifdef-ed?
>>
>> I am quite surprised of "it is tools-only" so it is fine to not protect
>> it even if it is x86 only. That's probably going to bite us in the future.
>>
>
> So, this appears to have reached an impasse. I don't know how to proceed
> without having to also fix priv mapping for x86, which is a yak far too large
> for me to shave at the moment.
Julien,
why is it that you are making refcounting on p2m insertion / removal
a requirement for this series? We all know it's not there right now
(and similarly also not for the IOMMU, which might affect ARM as well
unless you always use shared page tables), and it's - as Paul validly
says - unrelated to his series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 15:11 ` Jan Beulich
@ 2017-10-19 15:37 ` Julien Grall
2017-10-19 15:47 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 15:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
Hi,
On 19/10/17 16:11, Jan Beulich wrote:
>>>> On 19.10.17 at 16:49, <Paul.Durrant@citrix.com> wrote:
>>>> I'd prefer to make the whole thing x86-only since that's the only platform
>>> on which I can test it, and indeed the code used to be x86-only. Jan objected
>>> to this so all I'm trying to achieve is that it builds for ARM. Please can you and
>>> Jan reach agreement on where the code should live and how, if at all, it
>>> should be #ifdef-ed?
>>>
>>> I am quite surprised of "it is tools-only" so it is fine to not protect
>>> it even if it is x86 only. That's probably going to bite us in the future.
>>>
>>
>> So, this appears to have reached an impasse. I don't know how to proceed
>> without having to also fix priv mapping for x86, which is a yak far too large
>> for me to shave at the moment.
>
> Julien,
>
> why is it that you are making refcounting on p2m insertion / removal
> a requirement for this series? We all know it's not there right now
> (and similarly also not for the IOMMU, which might affect ARM as well
> unless you always use shared page tables), and it's - as Paul validly
> says - unrelated to his series.
Well, we do at least have refcounting for foreign mapping on Arm. So if
a foreign domain remove the mapping, the page will stay allocated until
the last mapping is removed. For IOMMU, page tables are for the moment
always shared.
If you don't want to fix x86 now, then it is fine. But I would
appreciate if you don't spread that on Arm.
To give you a bit more context, I was ready to implement the Arm version
of set_foreign_p2m_entry (it is quite trivial) to append at the end of
this series. But given that refcounting is not done, I am more reluctant
to do that.
Anyway, I don't plan to block common code. But I will block any
implementation of set_foreign_p2m_entry (other than returning not
implemented) on Arm until someone step up and fix the refcounting.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 15:37 ` Julien Grall
@ 2017-10-19 15:47 ` Jan Beulich
2017-10-19 16:06 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2017-10-19 15:47 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
>>> On 19.10.17 at 17:37, <julien.grall@linaro.org> wrote:
> Hi,
>
> On 19/10/17 16:11, Jan Beulich wrote:
>>>>> On 19.10.17 at 16:49, <Paul.Durrant@citrix.com> wrote:
>>>>> I'd prefer to make the whole thing x86-only since that's the only platform
>>>> on which I can test it, and indeed the code used to be x86-only. Jan
> objected
>>>> to this so all I'm trying to achieve is that it builds for ARM. Please can
> you and
>>>> Jan reach agreement on where the code should live and how, if at all, it
>>>> should be #ifdef-ed?
>>>>
>>>> I am quite surprised of "it is tools-only" so it is fine to not protect
>>>> it even if it is x86 only. That's probably going to bite us in the future.
>>>>
>>>
>>> So, this appears to have reached an impasse. I don't know how to proceed
>>> without having to also fix priv mapping for x86, which is a yak far too
> large
>>> for me to shave at the moment.
>>
>> Julien,
>>
>> why is it that you are making refcounting on p2m insertion / removal
>> a requirement for this series? We all know it's not there right now
>> (and similarly also not for the IOMMU, which might affect ARM as well
>> unless you always use shared page tables), and it's - as Paul validly
>> says - unrelated to his series.
>
> Well, we do at least have refcounting for foreign mapping on Arm. So if
> a foreign domain remove the mapping, the page will stay allocated until
> the last mapping is removed. For IOMMU, page tables are for the moment
> always shared.
>
> If you don't want to fix x86 now, then it is fine. But I would
> appreciate if you don't spread that on Arm.
>
> To give you a bit more context, I was ready to implement the Arm version
> of set_foreign_p2m_entry (it is quite trivial) to append at the end of
> this series. But given that refcounting is not done, I am more reluctant
> to do that.
I don't understand: The refcounting is to be done by ARM-specific
code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
not its caller. At least that's what I would have expected.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 15:47 ` Jan Beulich
@ 2017-10-19 16:06 ` Julien Grall
2017-10-19 16:21 ` Julien Grall
2017-10-20 6:17 ` Jan Beulich
0 siblings, 2 replies; 45+ messages in thread
From: Julien Grall @ 2017-10-19 16:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
Hi,
On 19/10/17 16:47, Jan Beulich wrote:
>>>> On 19.10.17 at 17:37, <julien.grall@linaro.org> wrote:
>> Hi,
>>
>> On 19/10/17 16:11, Jan Beulich wrote:
>>>>>> On 19.10.17 at 16:49, <Paul.Durrant@citrix.com> wrote:
>>>>>> I'd prefer to make the whole thing x86-only since that's the only platform
>>>>> on which I can test it, and indeed the code used to be x86-only. Jan
>> objected
>>>>> to this so all I'm trying to achieve is that it builds for ARM. Please can
>> you and
>>>>> Jan reach agreement on where the code should live and how, if at all, it
>>>>> should be #ifdef-ed?
>>>>>
>>>>> I am quite surprised of "it is tools-only" so it is fine to not protect
>>>>> it even if it is x86 only. That's probably going to bite us in the future.
>>>>>
>>>>
>>>> So, this appears to have reached an impasse. I don't know how to proceed
>>>> without having to also fix priv mapping for x86, which is a yak far too
>> large
>>>> for me to shave at the moment.
>>>
>>> Julien,
>>>
>>> why is it that you are making refcounting on p2m insertion / removal
>>> a requirement for this series? We all know it's not there right now
>>> (and similarly also not for the IOMMU, which might affect ARM as well
>>> unless you always use shared page tables), and it's - as Paul validly
>>> says - unrelated to his series.
>>
>> Well, we do at least have refcounting for foreign mapping on Arm. So if
>> a foreign domain remove the mapping, the page will stay allocated until
>> the last mapping is removed. For IOMMU, page tables are for the moment
>> always shared.
>>
>> If you don't want to fix x86 now, then it is fine. But I would
>> appreciate if you don't spread that on Arm.
>>
>> To give you a bit more context, I was ready to implement the Arm version
>> of set_foreign_p2m_entry (it is quite trivial) to append at the end of
>> this series. But given that refcounting is not done, I am more reluctant
>> to do that.
>
> I don't understand: The refcounting is to be done by ARM-specific
> code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
> not its caller. At least that's what I would have expected.
I thought I said it before, but it looks like not. Assuming the MFN is
always baked by a domain, the prototype would likely need to be extended
and take the foreign domain.
If it is not the case, we would need to find another way to do refcounting.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 16:06 ` Julien Grall
@ 2017-10-19 16:21 ` Julien Grall
2017-10-20 6:24 ` Jan Beulich
2017-10-20 6:17 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 16:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
Hi,
On 19/10/17 17:06, Julien Grall wrote:
> On 19/10/17 16:47, Jan Beulich wrote:
>>>>> On 19.10.17 at 17:37, <julien.grall@linaro.org> wrote:
>>> Hi,
>>>
>>> On 19/10/17 16:11, Jan Beulich wrote:
>>>>>>> On 19.10.17 at 16:49, <Paul.Durrant@citrix.com> wrote:
>>>>>>> I'd prefer to make the whole thing x86-only since that's the only
>>>>>>> platform
>>>>>> on which I can test it, and indeed the code used to be x86-only. Jan
>>> objected
>>>>>> to this so all I'm trying to achieve is that it builds for ARM.
>>>>>> Please can
>>> you and
>>>>>> Jan reach agreement on where the code should live and how, if at
>>>>>> all, it
>>>>>> should be #ifdef-ed?
>>>>>>
>>>>>> I am quite surprised of "it is tools-only" so it is fine to not
>>>>>> protect
>>>>>> it even if it is x86 only. That's probably going to bite us in the
>>>>>> future.
>>>>>>
>>>>>
>>>>> So, this appears to have reached an impasse. I don't know how to
>>>>> proceed
>>>>> without having to also fix priv mapping for x86, which is a yak far
>>>>> too
>>> large
>>>>> for me to shave at the moment.
>>>>
>>>> Julien,
>>>>
>>>> why is it that you are making refcounting on p2m insertion / removal
>>>> a requirement for this series? We all know it's not there right now
>>>> (and similarly also not for the IOMMU, which might affect ARM as well
>>>> unless you always use shared page tables), and it's - as Paul validly
>>>> says - unrelated to his series.
>>>
>>> Well, we do at least have refcounting for foreign mapping on Arm. So if
>>> a foreign domain remove the mapping, the page will stay allocated until
>>> the last mapping is removed. For IOMMU, page tables are for the moment
>>> always shared.
>>>
>>> If you don't want to fix x86 now, then it is fine. But I would
>>> appreciate if you don't spread that on Arm.
>>>
>>> To give you a bit more context, I was ready to implement the Arm version
>>> of set_foreign_p2m_entry (it is quite trivial) to append at the end of
>>> this series. But given that refcounting is not done, I am more reluctant
>>> to do that.
>>
>> I don't understand: The refcounting is to be done by ARM-specific
>> code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
>> not its caller. At least that's what I would have expected.
>
> I thought I said it before, but it looks like not. Assuming the MFN is
> always baked by a domain, the prototype would likely need to be extended
> and take the foreign domain.
>
> If it is not the case, we would need to find another way to do refcounting.
Looking a bit more at the resource you can acquire from this hypercall.
Some of them are allocated using alloc_xenheap_page() so not assigned to
a domain.
So I am not sure how you can expect a function set_foreign_p2m_entry to
take reference in that case.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 16:21 ` Julien Grall
@ 2017-10-20 6:24 ` Jan Beulich
2017-10-20 8:26 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2017-10-20 6:24 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
>>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
> Looking a bit more at the resource you can acquire from this hypercall.
> Some of them are allocated using alloc_xenheap_page() so not assigned to
> a domain.
>
> So I am not sure how you can expect a function set_foreign_p2m_entry to
> take reference in that case.
Hmm, with the domain parameter added, DOMID_XEN there (for
Xen heap pages) could identify no references to be taken, if that
was really the intended behavior in that case. However, even for
Xen heap pages life time tracking ought to be done - it is for a
reason that share_xen_page_with_guest() assigns the target
domain as the owner of such pages, as that allows get_page() to
succeed for them.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-20 6:24 ` Jan Beulich
@ 2017-10-20 8:26 ` Paul Durrant
2017-10-20 10:00 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-20 8:26 UTC (permalink / raw)
To: 'Jan Beulich', Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 October 2017 07:25
> To: Julien Grall <julien.grall@linaro.org>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad Rzeszutek
> Wilk <konrad.wilk@oracle.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> >>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
> > Looking a bit more at the resource you can acquire from this hypercall.
> > Some of them are allocated using alloc_xenheap_page() so not assigned to
> > a domain.
> >
> > So I am not sure how you can expect a function set_foreign_p2m_entry to
> > take reference in that case.
>
> Hmm, with the domain parameter added, DOMID_XEN there (for
> Xen heap pages) could identify no references to be taken, if that
> was really the intended behavior in that case. However, even for
> Xen heap pages life time tracking ought to be done - it is for a
> reason that share_xen_page_with_guest() assigns the target
> domain as the owner of such pages, as that allows get_page() to
> succeed for them.
>
So, nothing I'm doing here is making anything worse, right? Grant tables are assigned to the guest, and IOREQ server pages are allocated with alloc_domheap_page() so nothing is anonymous.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-20 8:26 ` Paul Durrant
@ 2017-10-20 10:00 ` Julien Grall
2017-10-20 10:10 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-20 10:00 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
Hi Paul,
On 20/10/17 09:26, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 October 2017 07:25
>> To: Julien Grall <julien.grall@linaro.org>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; George Dunlap
>> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
>> Durrant <Paul.Durrant@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad Rzeszutek
>> Wilk <konrad.wilk@oracle.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
>> Tim (Xen.org) <tim@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>>>>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
>>> Looking a bit more at the resource you can acquire from this hypercall.
>>> Some of them are allocated using alloc_xenheap_page() so not assigned to
>>> a domain.
>>>
>>> So I am not sure how you can expect a function set_foreign_p2m_entry to
>>> take reference in that case.
>>
>> Hmm, with the domain parameter added, DOMID_XEN there (for
>> Xen heap pages) could identify no references to be taken, if that
>> was really the intended behavior in that case. However, even for
>> Xen heap pages life time tracking ought to be done - it is for a
>> reason that share_xen_page_with_guest() assigns the target
>> domain as the owner of such pages, as that allows get_page() to
>> succeed for them.
>>
>
> So, nothing I'm doing here is making anything worse, right? Grant tables are assigned to the guest, and IOREQ server pages are allocated with alloc_domheap_page() so nothing is anonymous.
I don't think grant tables is assigned to the guest today. They are
allocated using xenheap_pages() and I can't find
share_xen_page_with_guest().
Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
going to be left unimplemented on Arm until someone as time to implement
correctly the function.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-20 10:00 ` Julien Grall
@ 2017-10-20 10:10 ` Paul Durrant
2017-10-23 18:04 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-20 10:10 UTC (permalink / raw)
To: 'Julien Grall', 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 20 October 2017 11:00
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> Hi Paul,
>
> On 20/10/17 09:26, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 20 October 2017 07:25
> >> To: Julien Grall <julien.grall@linaro.org>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; George Dunlap
> >> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> >> Durrant <Paul.Durrant@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
> Rzeszutek
> >> Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>;
> >> Tim (Xen.org) <tim@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >>>>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
> >>> Looking a bit more at the resource you can acquire from this hypercall.
> >>> Some of them are allocated using alloc_xenheap_page() so not assigned
> to
> >>> a domain.
> >>>
> >>> So I am not sure how you can expect a function set_foreign_p2m_entry
> to
> >>> take reference in that case.
> >>
> >> Hmm, with the domain parameter added, DOMID_XEN there (for
> >> Xen heap pages) could identify no references to be taken, if that
> >> was really the intended behavior in that case. However, even for
> >> Xen heap pages life time tracking ought to be done - it is for a
> >> reason that share_xen_page_with_guest() assigns the target
> >> domain as the owner of such pages, as that allows get_page() to
> >> succeed for them.
> >>
> >
Hi Julien,
> > So, nothing I'm doing here is making anything worse, right? Grant tables are
> assigned to the guest, and IOREQ server pages are allocated with
> alloc_domheap_page() so nothing is anonymous.
>
> I don't think grant tables is assigned to the guest today. They are
> allocated using xenheap_pages() and I can't find
> share_xen_page_with_guest().
The guest would not be able to map them if they were not assigned in some way!
See the code block at http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_table.c;hb=HEAD#l1716
It calls gnttab_create_shared_page() which is what calls through to share_xen_page_with_guest().
>
> Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
> going to be left unimplemented on Arm until someone as time to implement
> correctly the function.
>
That makes sense. Do you still have any issues with this patch apart from the cosmetic ones you spotted in the header?
Cheers,
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-20 10:10 ` Paul Durrant
@ 2017-10-23 18:04 ` Julien Grall
2017-10-25 8:40 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-23 18:04 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
On 20/10/17 11:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 20 October 2017 11:00
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
>> <JBeulich@suse.com>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; George Dunlap
>> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
>> Pau Monne <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
>> Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
>> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi Paul,
>>
>> On 20/10/17 09:26, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 20 October 2017 07:25
>>>> To: Julien Grall <julien.grall@linaro.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>>>> <Andrew.Cooper3@citrix.com>; George Dunlap
>>>> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
>>>> Durrant <Paul.Durrant@citrix.com>; Roger Pau Monne
>>>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
>> Rzeszutek
>>>> Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
>> <dgdegra@tycho.nsa.gov>;
>>>> Tim (Xen.org) <tim@xen.org>
>>>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>>>> HYPERVISOR_memory_op to acquire guest resources
>>>>
>>>>>>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
>>>>> Looking a bit more at the resource you can acquire from this hypercall.
>>>>> Some of them are allocated using alloc_xenheap_page() so not assigned
>> to
>>>>> a domain.
>>>>>
>>>>> So I am not sure how you can expect a function set_foreign_p2m_entry
>> to
>>>>> take reference in that case.
>>>>
>>>> Hmm, with the domain parameter added, DOMID_XEN there (for
>>>> Xen heap pages) could identify no references to be taken, if that
>>>> was really the intended behavior in that case. However, even for
>>>> Xen heap pages life time tracking ought to be done - it is for a
>>>> reason that share_xen_page_with_guest() assigns the target
>>>> domain as the owner of such pages, as that allows get_page() to
>>>> succeed for them.
>>>>
>>>
>
> Hi Julien,
>
>>> So, nothing I'm doing here is making anything worse, right? Grant tables are
>> assigned to the guest, and IOREQ server pages are allocated with
>> alloc_domheap_page() so nothing is anonymous.
>>
>> I don't think grant tables is assigned to the guest today. They are
>> allocated using xenheap_pages() and I can't find
>> share_xen_page_with_guest().
>
> The guest would not be able to map them if they were not assigned in some way!
Do you mean for PV? For HVM/PVH, we don't check whether the page is
assigned (see gnttab_map_frame).
> See the code block at http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_table.c;hb=HEAD#l1716
> It calls gnttab_create_shared_page() which is what calls through to share_xen_page_with_guest().
Thank you for the link, I will have a look.
>
>>
>> Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
>> going to be left unimplemented on Arm until someone as time to implement
>> correctly the function.
>>
>
> That makes sense. Do you still have any issues with this patch apart from the cosmetic ones you spotted in the header?
No. Although, may I request to add a comment in the ARM helpers about
the reference counting?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-23 18:04 ` Julien Grall
@ 2017-10-25 8:40 ` Paul Durrant
0 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-25 8:40 UTC (permalink / raw)
To: 'Julien Grall', 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf, Roger Pau Monne
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 23 October 2017 20:04
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
>
>
> On 20/10/17 11:10, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@linaro.org]
> >> Sent: 20 October 2017 11:00
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> >> <JBeulich@suse.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; George Dunlap
> >> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Roger
> >> Pau Monne <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
> Konrad
> >> Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> >> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi Paul,
> >>
> >> On 20/10/17 09:26, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>> Sent: 20 October 2017 07:25
> >>>> To: Julien Grall <julien.grall@linaro.org>
> >>>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >>>> <Andrew.Cooper3@citrix.com>; George Dunlap
> >>>> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Paul
> >>>> Durrant <Paul.Durrant@citrix.com>; Roger Pau Monne
> >>>> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano
> Stabellini
> >>>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad
> >> Rzeszutek
> >>>> Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> >> <dgdegra@tycho.nsa.gov>;
> >>>> Tim (Xen.org) <tim@xen.org>
> >>>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >>>> HYPERVISOR_memory_op to acquire guest resources
> >>>>
> >>>>>>> On 19.10.17 at 18:21, <julien.grall@linaro.org> wrote:
> >>>>> Looking a bit more at the resource you can acquire from this hypercall.
> >>>>> Some of them are allocated using alloc_xenheap_page() so not
> assigned
> >> to
> >>>>> a domain.
> >>>>>
> >>>>> So I am not sure how you can expect a function
> set_foreign_p2m_entry
> >> to
> >>>>> take reference in that case.
> >>>>
> >>>> Hmm, with the domain parameter added, DOMID_XEN there (for
> >>>> Xen heap pages) could identify no references to be taken, if that
> >>>> was really the intended behavior in that case. However, even for
> >>>> Xen heap pages life time tracking ought to be done - it is for a
> >>>> reason that share_xen_page_with_guest() assigns the target
> >>>> domain as the owner of such pages, as that allows get_page() to
> >>>> succeed for them.
> >>>>
> >>>
> >
> > Hi Julien,
> >
> >>> So, nothing I'm doing here is making anything worse, right? Grant tables
> are
> >> assigned to the guest, and IOREQ server pages are allocated with
> >> alloc_domheap_page() so nothing is anonymous.
> >>
> >> I don't think grant tables is assigned to the guest today. They are
> >> allocated using xenheap_pages() and I can't find
> >> share_xen_page_with_guest().
> >
> > The guest would not be able to map them if they were not assigned in
> some way!
>
> Do you mean for PV? For HVM/PVH, we don't check whether the page is
> assigned (see gnttab_map_frame).
Not there, but I though there were checks in guest_physmap_add_page() where the mfn passed back from gnttab_map_frame() is actually consumed. It's all quite convoluted though so I'm not sure.
>
> > See the code block at
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_tab
> le.c;hb=HEAD#l1716
> > It calls gnttab_create_shared_page() which is what calls through to
> share_xen_page_with_guest().
>
> Thank you for the link, I will have a look.
>
> >
> >>
> >> Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
> >> going to be left unimplemented on Arm until someone as time to
> implement
> >> correctly the function.
> >>
> >
> > That makes sense. Do you still have any issues with this patch apart from
> the cosmetic ones you spotted in the header?
>
> No. Although, may I request to add a comment in the ARM helpers about
> the reference counting?
>
Sure. Thanks,
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-19 16:06 ` Julien Grall
2017-10-19 16:21 ` Julien Grall
@ 2017-10-20 6:17 ` Jan Beulich
1 sibling, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2017-10-20 6:17 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim(Xen.org),
George Dunlap, Julien Grall, Paul Durrant, xen-devel,
Ian Jackson, Daniel De Graaf, Roger PauMonne
>>> On 19.10.17 at 18:06, <julien.grall@linaro.org> wrote:
> On 19/10/17 16:47, Jan Beulich wrote:
>> I don't understand: The refcounting is to be done by ARM-specific
>> code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
>> not its caller. At least that's what I would have expected.
>
> I thought I said it before, but it looks like not. Assuming the MFN is
> always baked by a domain, the prototype would likely need to be extended
> and take the foreign domain.
>
> If it is not the case, we would need to find another way to do refcounting.
Well, adding another parameter can't be that bad of a problem to solve.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-17 13:24 ` [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources Paul Durrant
2017-10-17 14:45 ` Daniel De Graaf
2017-10-19 12:22 ` Julien Grall
@ 2017-10-26 15:26 ` Jan Beulich
2017-10-26 15:32 ` Julien Grall
2017-10-30 12:05 ` Paul Durrant
2 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2017-10-26 15:26 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, IanJackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf
>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> @@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
> rc = -EFAULT;
> break;
>
> + case XENMEM_acquire_resource:
> + {
> + const xen_ulong_t *xen_frame_list =
> + (xen_ulong_t *)(nat.mar + 1);
> + compat_ulong_t *compat_frame_list =
> + (compat_ulong_t *)(nat.mar + 1);
> +
> + if ( cmp.mar.nr_frames == 0 )
Doesn't this need to be compat_handle_is_null(cmp.mar.frame_list), or
a combination of both?
> + {
> + DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> +
> + if ( __copy_field_to_guest(
> + guest_handle_cast(compat,
> + compat_mem_acquire_resource_t),
> + &cmp.mar, nr_frames) )
> + return -EFAULT;
> + }
> + else
> + {
> + /*
> + * NOTE: the smaller compat array overwrites the native
> + * array.
> + */
I think I had already asked for a respective BUILD_BUG_ON().
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> }
>
> +static int acquire_resource(
> + XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +{
> + struct domain *d, *currd = current->domain;
> + xen_mem_acquire_resource_t xmar;
> + unsigned long mfn_list[2];
> + int rc;
> +
> + if ( copy_from_guest(&xmar, arg, 1) )
> + return -EFAULT;
> +
> + if ( xmar.pad != 0 )
> + return -EINVAL;
> +
> + if ( guest_handle_is_null(xmar.frame_list) )
> + {
> + /* Special case for querying implementation limit */
> + if ( xmar.nr_frames == 0 )
Perhaps invert the condition to reduce ...
> + {
> + xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +
> + if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> + return -EFAULT;
> +
> + return 0;
> + }
... overall indentation?
> + return -EINVAL;
> + }
> +
> + if ( xmar.nr_frames == 0 )
> + return -EINVAL;
Why? (Almost?) everywhere else zero counts are simply no-ops, which
result in success returns.
> + if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> + return -E2BIG;
> +
> + d = rcu_lock_domain_by_any_id(xmar.domid);
This being a tools only interface, why "by_any_id" instead of
"remote_domain_by_id"? In particular ...
> + if ( d == NULL )
> + return -ESRCH;
> +
> + rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
... an unprivileged dm domain should probably not be permitted to
invoke this on itself.
> + if ( rc )
> + goto out;
> +
> + switch ( xmar.type )
> + {
> + default:
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + if ( rc )
> + goto out;
> +
> + if ( !paging_mode_translate(currd) )
> + {
> + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> + rc = -EFAULT;
> + }
> + else
> + {
> + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> + unsigned int i;
> +
> + rc = -EFAULT;
> + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> + goto out;
> +
> + for ( i = 0; i < xmar.nr_frames; i++ )
> + {
> + rc = set_foreign_p2m_entry(currd, gfn_list[i],
> + _mfn(mfn_list[i]));
> + if ( rc )
> + {
> + /*
> + * Make sure rc is -EIO for any interation other than
> + * the first.
"iteration", but why is this important in the first place?
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
> typedef struct xen_reserved_device_memory_map
> xen_reserved_device_memory_map_t;
> DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>
> +/*
> + * Get the pages for a particular guest resource, so that they can be
> + * mapped directly by a tools domain.
> + */
> +#define XENMEM_acquire_resource 28
> +struct xen_mem_acquire_resource {
> + /* IN - the domain whose resource is to be mapped */
> + domid_t domid;
> + /* IN - the type of resource */
> + uint16_t type;
> + /*
> + * IN - a type-specific resource identifier, which must be zero
> + * unless stated otherwise.
> + */
> + uint32_t id;
> + /* IN/OUT - As an IN parameter number of frames of the resource
> + * to be mapped. However, if the specified value is 0 and
> + * frame_list is NULL then this field will be set to the
> + * maximum value supported by the implementation on return.
> + */
> + uint32_t nr_frames;
> + uint32_t pad;
> + /* IN - the index of the initial frame to be mapped. This parameter
> + * is ignored if nr_frames is 0.
> + */
> + uint64_aligned_t frame;
> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
> + * will be populated with the MFNs of the resource.
> + * If the tools domain is HVM then it is expected that, on
> + * entry, frame_list will be populated with a list of GFNs
> + * that will be mapped to the MFNs of the resource.
> + * If -EIO is returned then the frame_list has only been
> + * partially mapped and it is up to the caller to unmap all
> + * the GFNs.
> + * This parameter may be NULL if nr_frames is 0.
> + */
> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
This is still xen_ulong_t, which I can live with, but then you shouldn't
copy into / out of arrays of other types in acquire_resource() (the
more that this is common code, and iirc xen_ulong_t and
unsigned long aren't the same thing on ARM32).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-26 15:26 ` Jan Beulich
@ 2017-10-26 15:32 ` Julien Grall
2017-10-26 15:39 ` Jan Beulich
2017-10-30 12:05 ` Paul Durrant
1 sibling, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-26 15:32 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, IanJackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf
On 26/10/17 16:26, Jan Beulich wrote:
>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
>> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>> + * will be populated with the MFNs of the resource.
>> + * If the tools domain is HVM then it is expected that, on
>> + * entry, frame_list will be populated with a list of GFNs
>> + * that will be mapped to the MFNs of the resource.
>> + * If -EIO is returned then the frame_list has only been
>> + * partially mapped and it is up to the caller to unmap all
>> + * the GFNs.
>> + * This parameter may be NULL if nr_frames is 0.
>> + */
>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>
> This is still xen_ulong_t, which I can live with, but then you shouldn't
> copy into / out of arrays of other types in acquire_resource() (the
> more that this is common code, and iirc xen_ulong_t and
> unsigned long aren't the same thing on ARM32).
xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
we use xen_pfn_t here?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-26 15:32 ` Julien Grall
@ 2017-10-26 15:39 ` Jan Beulich
2017-10-27 10:46 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2017-10-26 15:39 UTC (permalink / raw)
To: Paul Durrant, Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, IanJackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf
>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
> On 26/10/17 16:26, Jan Beulich wrote:
>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
>>> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>>> + * will be populated with the MFNs of the resource.
>>> + * If the tools domain is HVM then it is expected that, on
>>> + * entry, frame_list will be populated with a list of GFNs
>>> + * that will be mapped to the MFNs of the resource.
>>> + * If -EIO is returned then the frame_list has only been
>>> + * partially mapped and it is up to the caller to unmap all
>>> + * the GFNs.
>>> + * This parameter may be NULL if nr_frames is 0.
>>> + */
>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>
>> This is still xen_ulong_t, which I can live with, but then you shouldn't
>> copy into / out of arrays of other types in acquire_resource() (the
>> more that this is common code, and iirc xen_ulong_t and
>> unsigned long aren't the same thing on ARM32).
>
> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
> we use xen_pfn_t here?
I had put this question up earlier, but iirc Paul didn't like it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-26 15:39 ` Jan Beulich
@ 2017-10-27 10:46 ` Julien Grall
2017-10-27 15:19 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-27 10:46 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, IanJackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf
Hi,
On 26/10/17 16:39, Jan Beulich wrote:
>>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
>> On 26/10/17 16:26, Jan Beulich wrote:
>>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
>>>> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>>>> + * will be populated with the MFNs of the resource.
>>>> + * If the tools domain is HVM then it is expected that, on
>>>> + * entry, frame_list will be populated with a list of GFNs
>>>> + * that will be mapped to the MFNs of the resource.
>>>> + * If -EIO is returned then the frame_list has only been
>>>> + * partially mapped and it is up to the caller to unmap all
>>>> + * the GFNs.
>>>> + * This parameter may be NULL if nr_frames is 0.
>>>> + */
>>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>>
>>> This is still xen_ulong_t, which I can live with, but then you shouldn't
>>> copy into / out of arrays of other types in acquire_resource() (the
>>> more that this is common code, and iirc xen_ulong_t and
>>> unsigned long aren't the same thing on ARM32).
>>
>> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
>> we use xen_pfn_t here?
>
> I had put this question up earlier, but iirc Paul didn't like it.
I'd like to understand why Paul doesn't like it. We should never assume
that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
that purpose.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-27 10:46 ` Julien Grall
@ 2017-10-27 15:19 ` Paul Durrant
2017-10-30 12:08 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-27 15:19 UTC (permalink / raw)
To: 'Julien Grall', Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 27 October 2017 12:46
> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; 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@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> Hi,
>
> On 26/10/17 16:39, Jan Beulich wrote:
> >>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
> >> On 26/10/17 16:26, Jan Beulich wrote:
> >>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> >>>> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
> >>>> + * will be populated with the MFNs of the resource.
> >>>> + * If the tools domain is HVM then it is expected that, on
> >>>> + * entry, frame_list will be populated with a list of GFNs
> >>>> + * that will be mapped to the MFNs of the resource.
> >>>> + * If -EIO is returned then the frame_list has only been
> >>>> + * partially mapped and it is up to the caller to unmap all
> >>>> + * the GFNs.
> >>>> + * This parameter may be NULL if nr_frames is 0.
> >>>> + */
> >>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>>
> >>> This is still xen_ulong_t, which I can live with, but then you shouldn't
> >>> copy into / out of arrays of other types in acquire_resource() (the
> >>> more that this is common code, and iirc xen_ulong_t and
> >>> unsigned long aren't the same thing on ARM32).
> >>
> >> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
> >> we use xen_pfn_t here?
> >
> > I had put this question up earlier, but iirc Paul didn't like it.
>
> I'd like to understand why Paul doesn't like it. We should never assume
> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
> that purpose.
My reservation is whether xen_pfn_t is intended to hold either gfns or mfns, since this hypercall uses the same array for both. If it suitable then I am happy to change it, but Andrew led me to believe otherwise.
Paul
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-27 15:19 ` Paul Durrant
@ 2017-10-30 12:08 ` Julien Grall
2017-10-30 13:10 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-30 12:08 UTC (permalink / raw)
To: Paul Durrant, Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf
Hi Paul,
On 27/10/17 16:19, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 27 October 2017 12:46
>> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>; 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@lists.xenproject.org;
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
>> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi,
>>
>> On 26/10/17 16:39, Jan Beulich wrote:
>>>>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
>>>> On 26/10/17 16:26, Jan Beulich wrote:
>>>>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
>>>>>> + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>>>>>> + * will be populated with the MFNs of the resource.
>>>>>> + * If the tools domain is HVM then it is expected that, on
>>>>>> + * entry, frame_list will be populated with a list of GFNs
>>>>>> + * that will be mapped to the MFNs of the resource.
>>>>>> + * If -EIO is returned then the frame_list has only been
>>>>>> + * partially mapped and it is up to the caller to unmap all
>>>>>> + * the GFNs.
>>>>>> + * This parameter may be NULL if nr_frames is 0.
>>>>>> + */
>>>>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>>>>
>>>>> This is still xen_ulong_t, which I can live with, but then you shouldn't
>>>>> copy into / out of arrays of other types in acquire_resource() (the
>>>>> more that this is common code, and iirc xen_ulong_t and
>>>>> unsigned long aren't the same thing on ARM32).
>>>>
>>>> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
>>>> we use xen_pfn_t here?
>>>
>>> I had put this question up earlier, but iirc Paul didn't like it.
>>
>> I'd like to understand why Paul doesn't like it. We should never assume
>> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
>> that purpose.
>
> My reservation is whether xen_pfn_t is intended to hold either gfns or mfns, since this hypercall uses the same array for both. If it suitable then I am happy to change it, but Andrew led me to believe otherwise.
Looking at the public hearders, xen_pfn_t is been used for both MFN (see
xenpf_add_memtype) and GFN (see gnttab_setup_table).
So I think it would be fine to do the same here.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-30 12:08 ` Julien Grall
@ 2017-10-30 13:10 ` Paul Durrant
0 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-30 13:10 UTC (permalink / raw)
To: 'Julien Grall', Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 30 October 2017 12:09
> To: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <JBeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>; 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@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
>
> Hi Paul,
>
> On 27/10/17 16:19, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@linaro.org]
> >> Sent: 27 October 2017 12:46
> >> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> >> <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; 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@lists.xenproject.org;
> >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> >> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi,
> >>
> >> On 26/10/17 16:39, Jan Beulich wrote:
> >>>>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
> >>>> On 26/10/17 16:26, Jan Beulich wrote:
> >>>>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> >>>>>> + /* IN/OUT - If the tools domain is PV then, upon return,
> frame_list
> >>>>>> + * will be populated with the MFNs of the resource.
> >>>>>> + * If the tools domain is HVM then it is expected that, on
> >>>>>> + * entry, frame_list will be populated with a list of GFNs
> >>>>>> + * that will be mapped to the MFNs of the resource.
> >>>>>> + * If -EIO is returned then the frame_list has only been
> >>>>>> + * partially mapped and it is up to the caller to unmap all
> >>>>>> + * the GFNs.
> >>>>>> + * This parameter may be NULL if nr_frames is 0.
> >>>>>> + */
> >>>>>> + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>>>>
> >>>>> This is still xen_ulong_t, which I can live with, but then you shouldn't
> >>>>> copy into / out of arrays of other types in acquire_resource() (the
> >>>>> more that this is common code, and iirc xen_ulong_t and
> >>>>> unsigned long aren't the same thing on ARM32).
> >>>>
> >>>> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
> >>>> we use xen_pfn_t here?
> >>>
> >>> I had put this question up earlier, but iirc Paul didn't like it.
> >>
> >> I'd like to understand why Paul doesn't like it. We should never assume
> >> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
> >> that purpose.
> >
> > My reservation is whether xen_pfn_t is intended to hold either gfns or
> mfns, since this hypercall uses the same array for both. If it suitable then I am
> happy to change it, but Andrew led me to believe otherwise.
>
> Looking at the public hearders, xen_pfn_t is been used for both MFN (see
> xenpf_add_memtype) and GFN (see gnttab_setup_table).
>
> So I think it would be fine to do the same here.
Yes, I'm going to change it in the next version.
Cheers,
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
2017-10-26 15:26 ` Jan Beulich
2017-10-26 15:32 ` Julien Grall
@ 2017-10-30 12:05 ` Paul Durrant
1 sibling, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-30 12:05 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, xen-devel, Ian Jackson,
Daniel De Graaf
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 26 October 2017 16:27
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; 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@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
>
> >>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> > @@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> > rc = -EFAULT;
> > break;
> >
> > + case XENMEM_acquire_resource:
> > + {
> > + const xen_ulong_t *xen_frame_list =
> > + (xen_ulong_t *)(nat.mar + 1);
> > + compat_ulong_t *compat_frame_list =
> > + (compat_ulong_t *)(nat.mar + 1);
> > +
> > + if ( cmp.mar.nr_frames == 0 )
>
> Doesn't this need to be compat_handle_is_null(cmp.mar.frame_list), or
> a combination of both?
Sorry, yes this was a hang-over from the old scheme.
>
> > + {
> > +
> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> > +
> > + if ( __copy_field_to_guest(
> > + guest_handle_cast(compat,
> > + compat_mem_acquire_resource_t),
> > + &cmp.mar, nr_frames) )
> > + return -EFAULT;
> > + }
> > + else
> > + {
> > + /*
> > + * NOTE: the smaller compat array overwrites the native
> > + * array.
> > + */
>
> I think I had already asked for a respective BUILD_BUG_ON().
You asked for the comment. I can't find where you asked for a BUILD_BUG_ON() but I can certainly add one.
>
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> > return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> > }
> >
> > +static int acquire_resource(
> > + XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> > +{
> > + struct domain *d, *currd = current->domain;
> > + xen_mem_acquire_resource_t xmar;
> > + unsigned long mfn_list[2];
> > + int rc;
> > +
> > + if ( copy_from_guest(&xmar, arg, 1) )
> > + return -EFAULT;
> > +
> > + if ( xmar.pad != 0 )
> > + return -EINVAL;
> > +
> > + if ( guest_handle_is_null(xmar.frame_list) )
> > + {
> > + /* Special case for querying implementation limit */
> > + if ( xmar.nr_frames == 0 )
>
> Perhaps invert the condition to reduce ...
>
> > + {
> > + xmar.nr_frames = ARRAY_SIZE(mfn_list);
> > +
> > + if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> > + return -EFAULT;
> > +
> > + return 0;
> > + }
>
> ... overall indentation?
>
> > + return -EINVAL;
> > + }
> > +
> > + if ( xmar.nr_frames == 0 )
> > + return -EINVAL;
>
> Why? (Almost?) everywhere else zero counts are simply no-ops, which
> result in success returns.
Ok, I'll drop the check.
>
> > + if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> > + return -E2BIG;
> > +
> > + d = rcu_lock_domain_by_any_id(xmar.domid);
>
> This being a tools only interface, why "by_any_id" instead of
> "remote_domain_by_id"? In particular ...
>
> > + if ( d == NULL )
> > + return -ESRCH;
> > +
> > + rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
>
> ... an unprivileged dm domain should probably not be permitted to
> invoke this on itself.
True.
>
> > + if ( rc )
> > + goto out;
> > +
> > + switch ( xmar.type )
> > + {
> > + default:
> > + rc = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + if ( rc )
> > + goto out;
> > +
> > + if ( !paging_mode_translate(currd) )
> > + {
> > + if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> > + rc = -EFAULT;
> > + }
> > + else
> > + {
> > + xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > + unsigned int i;
> > +
> > + rc = -EFAULT;
> > + if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> > + goto out;
> > +
> > + for ( i = 0; i < xmar.nr_frames; i++ )
> > + {
> > + rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > + _mfn(mfn_list[i]));
> > + if ( rc )
> > + {
> > + /*
> > + * Make sure rc is -EIO for any interation other than
> > + * the first.
>
> "iteration", but why is this important in the first place?
The header explains:
"If -EIO is returned then the frame_list has only been partially mapped and it is up to the caller to unmap all the GFNs."
Particularly, on ARM, set_foreign_p2m_entry() will always return -EOPNOTSUPP so I want to make sure that is returned.
>
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
> > typedef struct xen_reserved_device_memory_map
> > xen_reserved_device_memory_map_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
> >
> > +/*
> > + * Get the pages for a particular guest resource, so that they can be
> > + * mapped directly by a tools domain.
> > + */
> > +#define XENMEM_acquire_resource 28
> > +struct xen_mem_acquire_resource {
> > + /* IN - the domain whose resource is to be mapped */
> > + domid_t domid;
> > + /* IN - the type of resource */
> > + uint16_t type;
> > + /*
> > + * IN - a type-specific resource identifier, which must be zero
> > + * unless stated otherwise.
> > + */
> > + uint32_t id;
> > + /* IN/OUT - As an IN parameter number of frames of the resource
> > + * to be mapped. However, if the specified value is 0 and
> > + * frame_list is NULL then this field will be set to the
> > + * maximum value supported by the implementation on return.
> > + */
> > + uint32_t nr_frames;
> > + uint32_t pad;
> > + /* IN - the index of the initial frame to be mapped. This parameter
> > + * is ignored if nr_frames is 0.
> > + */
> > + uint64_aligned_t frame;
> > + /* IN/OUT - If the tools domain is PV then, upon return, frame_list
> > + * will be populated with the MFNs of the resource.
> > + * If the tools domain is HVM then it is expected that, on
> > + * entry, frame_list will be populated with a list of GFNs
> > + * that will be mapped to the MFNs of the resource.
> > + * If -EIO is returned then the frame_list has only been
> > + * partially mapped and it is up to the caller to unmap all
> > + * the GFNs.
> > + * This parameter may be NULL if nr_frames is 0.
> > + */
> > + XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>
> This is still xen_ulong_t, which I can live with, but then you shouldn't
> copy into / out of arrays of other types in acquire_resource() (the
> more that this is common code, and iirc xen_ulong_t and
> unsigned long aren't the same thing on ARM32).
Given the weight of opinion, I'll change this to xen_pfn_t.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (4 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-19 12:31 ` Julien Grall
2017-10-26 15:36 ` Jan Beulich
2017-10-17 13:24 ` [PATCH v12 07/11] x86/mm: add an extra command to HYPERVISOR_mmu_update Paul Durrant
` (4 subsequent siblings)
10 siblings, 2 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Paul Durrant, Jan Beulich
... XENMEM_resource_ioreq_server
This patch adds support for a new resource type that can be mapped using
the XENMEM_acquire_resource memory op.
If an emulator makes use of this resource type then, instead of mapping
gfns, the IOREQ server will allocate pages from the heap. These pages
will never be present in the P2M of the guest at any point and so are
not vulnerable to any direct attack by the guest. They are only ever
accessible by Xen and any domain that has mapping privilege over the
guest (which may or may not be limited to the domain running the emulator).
NOTE: Use of the new resource type is not compatible with use of
XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns flag is
set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@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>
v12:
- Addressed more comments from Jan.
- Dropped George's A-b and Wei's R-b because of material change.
v11:
- Addressed more comments from Jan.
v10:
- Addressed comments from Jan.
v8:
- Re-base on new boilerplate.
- Adjust function signature of hvm_get_ioreq_server_frame(), and test
whether the bufioreq page is present.
v5:
- Use get_ioreq_server() function rather than indexing array directly.
- Add more explanation into comments to state than mapping guest frames
and allocation of pages for ioreq servers are not simultaneously
permitted.
- Add a comment into asm/ioreq.h stating the meaning of the index
value passed to hvm_get_ioreq_server_frame().
---
xen/arch/x86/hvm/ioreq.c | 156 ++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/mm.c | 22 ++++++
xen/common/memory.c | 5 ++
xen/include/asm-x86/hvm/ioreq.h | 2 +
xen/include/asm-x86/mm.h | 5 ++
xen/include/public/hvm/dm_op.h | 4 ++
xen/include/public/memory.h | 9 +++
7 files changed, 203 insertions(+)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index f654e7796c..2c611fbffa 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -259,6 +259,19 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
int rc;
+ if ( iorp->page )
+ {
+ /*
+ * If a page has already been allocated (which will happen on
+ * demand if hvm_get_ioreq_server_frame() is called), then
+ * mapping a guest frame is not permitted.
+ */
+ if ( gfn_eq(iorp->gfn, INVALID_GFN) )
+ return -EPERM;
+
+ return 0;
+ }
+
if ( d->is_dying )
return -EINVAL;
@@ -281,6 +294,70 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
return rc;
}
+static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
+{
+ struct domain *currd = current->domain;
+ struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+
+ if ( iorp->page )
+ {
+ /*
+ * If a guest frame has already been mapped (which may happen
+ * on demand if hvm_get_ioreq_server_info() is called), then
+ * allocating a page is not permitted.
+ */
+ if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
+ return -EPERM;
+
+ return 0;
+ }
+
+ /*
+ * Allocated IOREQ server pages are assigned to the emulating
+ * domain, not the target domain. This is because the emulator is
+ * likely to be destroyed after the target domain has been torn
+ * down, and we must use MEMF_no_refcount otherwise page allocation
+ * could fail if the emulating domain has already reached its
+ * maximum allocation.
+ */
+ iorp->page = alloc_domheap_page(currd, MEMF_no_refcount);
+ if ( !iorp->page )
+ return -ENOMEM;
+
+ if ( !get_page_type(iorp->page, PGT_writable_page) )
+ {
+ ASSERT_UNREACHABLE();
+ put_page(iorp->page);
+ iorp->page = NULL;
+ return -ENOMEM;
+ }
+
+ iorp->va = __map_domain_page_global(iorp->page);
+ if ( !iorp->va )
+ {
+ put_page_and_type(iorp->page);
+ iorp->page = NULL;
+ return -ENOMEM;
+ }
+
+ clear_page(iorp->va);
+ return 0;
+}
+
+static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
+{
+ struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+
+ if ( !iorp->page )
+ return;
+
+ unmap_domain_page_global(iorp->va);
+ iorp->va = NULL;
+
+ put_page_and_type(iorp->page);
+ iorp->page = NULL;
+}
+
bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
{
const struct hvm_ioreq_server *s;
@@ -484,6 +561,27 @@ static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
hvm_unmap_ioreq_gfn(s, false);
}
+static int hvm_ioreq_server_alloc_pages(struct hvm_ioreq_server *s)
+{
+ int rc;
+
+ rc = hvm_alloc_ioreq_mfn(s, false);
+
+ if ( !rc && (s->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF) )
+ rc = hvm_alloc_ioreq_mfn(s, true);
+
+ if ( rc )
+ hvm_free_ioreq_mfn(s, false);
+
+ return rc;
+}
+
+static void hvm_ioreq_server_free_pages(struct hvm_ioreq_server *s)
+{
+ hvm_free_ioreq_mfn(s, true);
+ hvm_free_ioreq_mfn(s, false);
+}
+
static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
{
unsigned int i;
@@ -612,7 +710,18 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
fail_add:
hvm_ioreq_server_remove_all_vcpus(s);
+
+ /*
+ * NOTE: It is safe to call both hvm_ioreq_server_unmap_pages() and
+ * hvm_ioreq_server_free_pages() in that order.
+ * This is because the former will do nothing if the pages
+ * are not mapped, leaving the page to be freed by the latter.
+ * However if the pages are mapped then the former will set
+ * the page_info pointer to NULL, meaning the latter will do
+ * nothing.
+ */
hvm_ioreq_server_unmap_pages(s);
+ hvm_ioreq_server_free_pages(s);
return rc;
}
@@ -622,6 +731,7 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
ASSERT(!s->enabled);
hvm_ioreq_server_remove_all_vcpus(s);
hvm_ioreq_server_unmap_pages(s);
+ hvm_ioreq_server_free_pages(s);
hvm_ioreq_server_free_rangesets(s);
}
@@ -777,6 +887,52 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
return rc;
}
+int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
+ unsigned long idx, mfn_t *mfn)
+{
+ struct hvm_ioreq_server *s;
+ int rc;
+
+ spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
+
+ if ( id == DEFAULT_IOSERVID )
+ return -EOPNOTSUPP;
+
+ s = get_ioreq_server(d, id);
+
+ ASSERT(!IS_DEFAULT(s));
+
+ rc = hvm_ioreq_server_alloc_pages(s);
+ if ( rc )
+ goto out;
+
+ switch ( idx )
+ {
+ case XENMEM_resource_ioreq_server_frame_bufioreq:
+ rc = -ENOENT;
+ if ( !HANDLE_BUFIOREQ(s) )
+ goto out;
+
+ *mfn = _mfn(page_to_mfn(s->bufioreq.page));
+ rc = 0;
+ break;
+
+ case XENMEM_resource_ioreq_server_frame_ioreq(0):
+ *mfn = _mfn(page_to_mfn(s->ioreq.page));
+ rc = 0;
+ break;
+
+ default:
+ rc = -EINVAL;
+ break;
+ }
+
+ out:
+ spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
+
+ return rc;
+}
+
int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
uint32_t type, uint64_t start,
uint64_t end)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d9df5ca69f..1d15ae2a15 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -122,6 +122,7 @@
#include <asm/fixmap.h>
#include <asm/io_apic.h>
#include <asm/pci.h>
+#include <asm/hvm/ioreq.h>
#include <asm/hvm/grant_table.h>
#include <asm/pv/grant_table.h>
@@ -3866,6 +3867,27 @@ int xenmem_add_to_physmap_one(
return rc;
}
+int xenmem_acquire_ioreq_server(struct domain *d, unsigned int id,
+ unsigned long frame,
+ unsigned int nr_frames,
+ unsigned long mfn_list[])
+{
+ unsigned int i;
+
+ for ( i = 0; i < nr_frames; i++ )
+ {
+ mfn_t mfn;
+ int rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
+
+ if ( rc )
+ return rc;
+
+ mfn_list[i] = mfn_x(mfn);
+ }
+
+ return 0;
+}
+
long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
int rc;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index cdd2e030cf..b27a71c4f1 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1011,6 +1011,11 @@ static int acquire_resource(
switch ( xmar.type )
{
+ case XENMEM_resource_ioreq_server:
+ rc = xenmem_acquire_ioreq_server(d, xmar.id, xmar.frame,
+ xmar.nr_frames, mfn_list);
+ break;
+
default:
rc = -EOPNOTSUPP;
break;
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index 1829fcf43e..9e37c97a37 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -31,6 +31,8 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
unsigned long *ioreq_gfn,
unsigned long *bufioreq_gfn,
evtchn_port_t *bufioreq_port);
+int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
+ unsigned long idx, mfn_t *mfn);
int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
uint32_t type, uint64_t start,
uint64_t end);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index f2e0f498c4..44aac9d225 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -615,4 +615,9 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
return mfn <= (virt_to_mfn(eva - 1) + 1);
}
+int xenmem_acquire_ioreq_server(struct domain *d, unsigned int id,
+ unsigned long frame,
+ unsigned int nr_frames,
+ unsigned long mfn_list[]);
+
#endif /* __ASM_X86_MM_H__ */
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 9677bd74e7..59b6006910 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -90,6 +90,10 @@ struct xen_dm_op_create_ioreq_server {
* the frame numbers passed back in gfns <ioreq_gfn> and <bufioreq_gfn>
* respectively. (If the IOREQ Server is not handling buffered emulation
* only <ioreq_gfn> will be valid).
+ *
+ * NOTE: To access the synchronous ioreq structures and buffered ioreq
+ * ring, it is preferable to use the XENMEM_acquire_resource memory
+ * op specifying resource type XENMEM_resource_ioreq_server.
*/
#define XEN_DMOP_get_ioreq_server_info 2
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 18118ea5c6..9596ebf2c7 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -609,9 +609,14 @@ struct xen_mem_acquire_resource {
domid_t domid;
/* IN - the type of resource */
uint16_t type;
+
+#define XENMEM_resource_ioreq_server 0
+
/*
* IN - a type-specific resource identifier, which must be zero
* unless stated otherwise.
+ *
+ * type == XENMEM_resource_ioreq_server -> id == ioreq server id
*/
uint32_t id;
/* IN/OUT - As an IN parameter number of frames of the resource
@@ -625,6 +630,10 @@ struct xen_mem_acquire_resource {
* is ignored if nr_frames is 0.
*/
uint64_aligned_t frame;
+
+#define XENMEM_resource_ioreq_server_frame_bufioreq 0
+#define XENMEM_resource_ioreq_server_frame_ioreq(n) (1 + (n))
+
/* IN/OUT - If the tools domain is PV then, upon return, frame_list
* will be populated with the MFNs of the resource.
* If the tools domain is HVM then it is expected that, on
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-17 13:24 ` [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type Paul Durrant
@ 2017-10-19 12:31 ` Julien Grall
2017-10-19 12:58 ` Paul Durrant
2017-10-26 15:36 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 12:31 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Jan Beulich
Hi,
On 17/10/17 14:24, Paul Durrant wrote:
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index cdd2e030cf..b27a71c4f1 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1011,6 +1011,11 @@ static int acquire_resource(
>
> switch ( xmar.type )
> {
> + case XENMEM_resource_ioreq_server:
> + rc = xenmem_acquire_ioreq_server(d, xmar.id, xmar.frame,
> + xmar.nr_frames, mfn_list);
> + break;
I fully appreciate you are not able to test on x86. However, I would
have expected you to at least build test it.
For instance, here you introduced this function on x86, call in common
code but does not introduce it on Arm.
Although, I don't think we should introduce it for Arm. Instead we
should provide arch helpers as we do for other memory operations.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-19 12:31 ` Julien Grall
@ 2017-10-19 12:58 ` Paul Durrant
2017-10-19 13:08 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-19 12:58 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Jan Beulich, Ian Jackson
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 19 October 2017 13:31
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v12 06/11] x86/hvm/ioreq: add a new
> mappable resource type...
>
> Hi,
>
> On 17/10/17 14:24, Paul Durrant wrote:
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index cdd2e030cf..b27a71c4f1 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1011,6 +1011,11 @@ static int acquire_resource(
> >
> > switch ( xmar.type )
> > {
> > + case XENMEM_resource_ioreq_server:
> > + rc = xenmem_acquire_ioreq_server(d, xmar.id, xmar.frame,
> > + xmar.nr_frames, mfn_list);
> > + break;
>
> I fully appreciate you are not able to test on x86. However, I would
> have expected you to at least build test it.
>
I don't actually know how to set up cross-compilation, which is why I'd originally #ifdef-ed this.
> For instance, here you introduced this function on x86, call in common
> code but does not introduce it on Arm.
>
> Although, I don't think we should introduce it for Arm. Instead we
> should provide arch helpers as we do for other memory operations.
>
That would be cleaner in this case.
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-19 12:58 ` Paul Durrant
@ 2017-10-19 13:08 ` Julien Grall
2017-10-19 13:08 ` Paul Durrant
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2017-10-19 13:08 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Jan Beulich, Ian Jackson
Hi Paul,
On 10/19/2017 01:58 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 19 October 2017 13:31
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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>; Jan Beulich <jbeulich@suse.com>
>> Subject: Re: [Xen-devel] [PATCH v12 06/11] x86/hvm/ioreq: add a new
>> mappable resource type...
>>
>> Hi,
>>
>> On 17/10/17 14:24, Paul Durrant wrote:
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index cdd2e030cf..b27a71c4f1 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1011,6 +1011,11 @@ static int acquire_resource(
>>>
>>> switch ( xmar.type )
>>> {
>>> + case XENMEM_resource_ioreq_server:
>>> + rc = xenmem_acquire_ioreq_server(d, xmar.id, xmar.frame,
>>> + xmar.nr_frames, mfn_list);
>>> + break;
>>
>> I fully appreciate you are not able to test on x86. However, I would
>> have expected you to at least build test it.
>>
>
> I don't actually know how to set up cross-compilation, which is why I'd originally #ifdef-ed this.
It is quite trivial. See
https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Cross_Compiling
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-19 13:08 ` Julien Grall
@ 2017-10-19 13:08 ` Paul Durrant
0 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-19 13:08 UTC (permalink / raw)
To: 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Jan Beulich, Ian Jackson
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 19 October 2017 14:08
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.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>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v12 06/11] x86/hvm/ioreq: add a new
> mappable resource type...
>
> Hi Paul,
>
> On 10/19/2017 01:58 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@linaro.org]
> >> Sent: 19 October 2017 13:31
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.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>; Jan Beulich <jbeulich@suse.com>
> >> Subject: Re: [Xen-devel] [PATCH v12 06/11] x86/hvm/ioreq: add a new
> >> mappable resource type...
> >>
> >> Hi,
> >>
> >> On 17/10/17 14:24, Paul Durrant wrote:
> >>> diff --git a/xen/common/memory.c b/xen/common/memory.c
> >>> index cdd2e030cf..b27a71c4f1 100644
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -1011,6 +1011,11 @@ static int acquire_resource(
> >>>
> >>> switch ( xmar.type )
> >>> {
> >>> + case XENMEM_resource_ioreq_server:
> >>> + rc = xenmem_acquire_ioreq_server(d, xmar.id, xmar.frame,
> >>> + xmar.nr_frames, mfn_list);
> >>> + break;
> >>
> >> I fully appreciate you are not able to test on x86. However, I would
> >> have expected you to at least build test it.
> >>
> >
> > I don't actually know how to set up cross-compilation, which is why I'd
> originally #ifdef-ed this.
>
> It is quite trivial. See
> https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#
> Cross_Compiling
>
Oh, that's way more trivial than I'd imagined. Thanks!
Paul
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type...
2017-10-17 13:24 ` [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type Paul Durrant
2017-10-19 12:31 ` Julien Grall
@ 2017-10-26 15:36 ` Jan Beulich
1 sibling, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2017-10-26 15:36 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> @@ -777,6 +887,52 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
> return rc;
> }
>
> +int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
There's another silent truncation issue here: Iirc ioservid_t is 16 bits.
> @@ -3866,6 +3867,27 @@ int xenmem_add_to_physmap_one(
> return rc;
> }
>
> +int xenmem_acquire_ioreq_server(struct domain *d, unsigned int id,
> + unsigned long frame,
> + unsigned int nr_frames,
> + unsigned long mfn_list[])
> +{
> + unsigned int i;
> +
> + for ( i = 0; i < nr_frames; i++ )
> + {
> + mfn_t mfn;
> + int rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
The caller here holds a 32-bit quantity in its hands, though. With
the upper half suitably checked in either place
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v12 07/11] x86/mm: add an extra command to HYPERVISOR_mmu_update...
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (5 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 06/11] x86/hvm/ioreq: add a new mappable resource type Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 08/11] tools/libxenforeignmemory: add support for resource mapping Paul Durrant
` (3 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Paul Durrant
...to allow the calling domain to prevent translation of specified l1e
value.
Despite what the comment in public/xen.h might imply, specifying a
command value of MMU_NORMAL_PT_UPDATE will not simply update an l1e with
the specified value. Instead, mod_l1_entry() tests whether foreign_dom
has PG_translate set in its paging mode and, if it does, assumes that the
the pfn value in the l1e is a gfn rather than an mfn.
To allow PV tools domain to map mfn values from a previously issued
HYPERVISOR_memory_op:XENMEM_acquire_resource, there needs to be a way
to tell HYPERVISOR_mmu_update that the specific l1e value does not
require translation regardless of the paging mode of foreign_dom. This
patch therefore defines a new command value, MMU_PT_UPDATE_NO_TRANSLATE,
which has the same semantics as MMU_NORMAL_PT_UPDATE except that the
paging mode of foreign_dom is ignored and the l1e value is used verbatim.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: 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>
v8:
- New in this version, replacing "allow a privileged PV domain to map
guest mfns".
---
xen/arch/x86/mm.c | 17 ++++++++++-------
xen/include/public/xen.h | 12 +++++++++---
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1d15ae2a15..63539d5d0b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1619,9 +1619,10 @@ void page_unlock(struct page_info *page)
/* Update the L1 entry at pl1e to new value nl1e. */
static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
- unsigned long gl1mfn, int preserve_ad,
+ unsigned long gl1mfn, unsigned int cmd,
struct vcpu *pt_vcpu, struct domain *pg_dom)
{
+ bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD);
l1_pgentry_t ol1e;
struct domain *pt_dom = pt_vcpu->domain;
int rc = 0;
@@ -1643,7 +1644,8 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
return -EINVAL;
}
- if ( paging_mode_translate(pg_dom) )
+ if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
+ paging_mode_translate(pg_dom) )
{
page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), NULL, P2M_ALLOC);
if ( !page )
@@ -3258,6 +3260,7 @@ long do_mmu_update(
*/
case MMU_NORMAL_PT_UPDATE:
case MMU_PT_UPDATE_PRESERVE_AD:
+ case MMU_PT_UPDATE_NO_TRANSLATE:
{
p2m_type_t p2mt;
@@ -3323,7 +3326,8 @@ long do_mmu_update(
p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
P2M_UNSHARE : P2M_ALLOC;
- if ( paging_mode_translate(pg_owner) )
+ if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
+ paging_mode_translate(pg_owner) )
target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
&l1e_p2mt, q);
@@ -3350,9 +3354,7 @@ long do_mmu_update(
break;
}
- rc = mod_l1_entry(va, l1e, mfn,
- cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
- pg_owner);
+ rc = mod_l1_entry(va, l1e, mfn, cmd, v, pg_owner);
if ( target )
put_page(target);
}
@@ -3630,7 +3632,8 @@ static int __do_update_va_mapping(
goto out;
}
- rc = mod_l1_entry(pl1e, val, mfn_x(gl1mfn), 0, v, pg_owner);
+ rc = mod_l1_entry(pl1e, val, mfn_x(gl1mfn), MMU_NORMAL_PT_UPDATE, v,
+ pg_owner);
page_unlock(gl1pg);
put_page(gl1pg);
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e24d..d2014a39eb 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -268,6 +268,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
* As MMU_NORMAL_PT_UPDATE above, but A/D bits currently in the PTE are ORed
* with those in @val.
*
+ * ptr[1:0] == MMU_PT_UPDATE_NO_TRANSLATE:
+ * As MMU_NORMAL_PT_UPDATE above, but @val is not translated though FD
+ * page tables.
+ *
* @val is usually the machine frame number along with some attributes.
* The attributes by default follow the architecture defined bits. Meaning that
* if this is a X86_64 machine and four page table layout is used, the layout
@@ -334,9 +338,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
*
* PAT (bit 7 on) --> PWT (bit 3 on) and clear bit 7.
*/
-#define MMU_NORMAL_PT_UPDATE 0 /* checked '*ptr = val'. ptr is MA. */
-#define MMU_MACHPHYS_UPDATE 1 /* ptr = MA of frame to modify entry for */
-#define MMU_PT_UPDATE_PRESERVE_AD 2 /* atomically: *ptr = val | (*ptr&(A|D)) */
+#define MMU_NORMAL_PT_UPDATE 0 /* checked '*ptr = val'. ptr is MA. */
+#define MMU_MACHPHYS_UPDATE 1 /* ptr = MA of frame to modify entry for */
+#define MMU_PT_UPDATE_PRESERVE_AD 2 /* atomically: *ptr = val | (*ptr&(A|D)) */
+#define MMU_PT_UPDATE_NO_TRANSLATE 3 /* checked '*ptr = val'. prt is MA. */
+ /* val never translated. */
/*
* MMU EXTENDED OPERATIONS
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 08/11] tools/libxenforeignmemory: add support for resource mapping
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (6 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 07/11] x86/mm: add an extra command to HYPERVISOR_mmu_update Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 09/11] tools/libxenforeignmemory: reduce xenforeignmemory_restrict code footprint Paul Durrant
` (2 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson
A previous patch introduced a new HYPERVISOR_memory_op to acquire guest
resources for direct priv-mapping.
This patch adds new functionality into libxenforeignmemory to make use
of a new privcmd ioctl [1] that uses the new memory op to make such
resources available via mmap(2).
[1] http://xenbits.xen.org/gitweb/?p=people/pauldu/linux.git;a=commit;h=ce59a05e6712
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
v4:
- Fixed errno and removed single-use label
- The unmap call now returns a status
- Use C99 initialization for ioctl struct
v2:
- Bump minor version up to 3.
---
tools/include/xen-sys/Linux/privcmd.h | 11 +++++
tools/libs/foreignmemory/Makefile | 2 +-
tools/libs/foreignmemory/core.c | 53 ++++++++++++++++++++++
.../libs/foreignmemory/include/xenforeignmemory.h | 41 +++++++++++++++++
tools/libs/foreignmemory/libxenforeignmemory.map | 5 ++
tools/libs/foreignmemory/linux.c | 45 ++++++++++++++++++
tools/libs/foreignmemory/private.h | 31 +++++++++++++
7 files changed, 187 insertions(+), 1 deletion(-)
diff --git a/tools/include/xen-sys/Linux/privcmd.h b/tools/include/xen-sys/Linux/privcmd.h
index 732ff7c15a..9531b728f9 100644
--- a/tools/include/xen-sys/Linux/privcmd.h
+++ b/tools/include/xen-sys/Linux/privcmd.h
@@ -86,6 +86,15 @@ typedef struct privcmd_dm_op {
const privcmd_dm_op_buf_t __user *ubufs;
} privcmd_dm_op_t;
+typedef struct privcmd_mmap_resource {
+ domid_t dom;
+ __u32 type;
+ __u32 id;
+ __u32 idx;
+ __u64 num;
+ __u64 addr;
+} privcmd_mmap_resource_t;
+
/*
* @cmd: IOCTL_PRIVCMD_HYPERCALL
* @arg: &privcmd_hypercall_t
@@ -103,5 +112,7 @@ typedef struct privcmd_dm_op {
_IOC(_IOC_NONE, 'P', 5, sizeof(privcmd_dm_op_t))
#define IOCTL_PRIVCMD_RESTRICT \
_IOC(_IOC_NONE, 'P', 6, sizeof(domid_t))
+#define IOCTL_PRIVCMD_MMAP_RESOURCE \
+ _IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
#endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index ab7f873f26..5c7f78f61d 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
include $(XEN_ROOT)/tools/Rules.mk
MAJOR = 1
-MINOR = 2
+MINOR = 3
SHLIB_LDFLAGS += -Wl,--version-script=libxenforeignmemory.map
CFLAGS += -Werror -Wmissing-prototypes
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index a6897dc561..8d3f9f178f 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -17,6 +17,8 @@
#include <assert.h>
#include <errno.h>
+#include <sys/mman.h>
+
#include "private.h"
xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
@@ -120,6 +122,57 @@ int xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
return osdep_xenforeignmemory_restrict(fmem, domid);
}
+xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
+ xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+ unsigned int id, unsigned long frame, unsigned long nr_frames,
+ void **paddr, int prot, int flags)
+{
+ xenforeignmemory_resource_handle *fres;
+ int rc;
+
+ /* Check flags only contains POSIX defined values */
+ if ( flags & ~(MAP_SHARED | MAP_PRIVATE) )
+ {
+ errno = EINVAL;
+ return NULL;
+ }
+
+ fres = calloc(1, sizeof(*fres));
+ if ( !fres )
+ {
+ errno = ENOMEM;
+ return NULL;
+ }
+
+ fres->domid = domid;
+ fres->type = type;
+ fres->id = id;
+ fres->frame = frame;
+ fres->nr_frames = nr_frames;
+ fres->addr = *paddr;
+ fres->prot = prot;
+ fres->flags = flags;
+
+ rc = osdep_xenforeignmemory_map_resource(fmem, fres);
+ if ( rc )
+ {
+ free(fres);
+ fres = NULL;
+ } else
+ *paddr = fres->addr;
+
+ return fres;
+}
+
+int xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ int rc = osdep_xenforeignmemory_unmap_resource(fmem, fres);
+
+ free(fres);
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libs/foreignmemory/include/xenforeignmemory.h b/tools/libs/foreignmemory/include/xenforeignmemory.h
index f4814c390f..d594be8df0 100644
--- a/tools/libs/foreignmemory/include/xenforeignmemory.h
+++ b/tools/libs/foreignmemory/include/xenforeignmemory.h
@@ -138,6 +138,47 @@ int xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
int xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
domid_t domid);
+typedef struct xenforeignmemory_resource_handle xenforeignmemory_resource_handle;
+
+/**
+ * This function maps a guest resource.
+ *
+ * @parm fmem handle to the open foreignmemory interface
+ * @parm domid the domain id
+ * @parm type the resource type
+ * @parm id the type-specific resource identifier
+ * @parm frame base frame index within the resource
+ * @parm nr_frames number of frames to map
+ * @parm paddr pointer to an address passed through to mmap(2)
+ * @parm prot passed through to mmap(2)
+ * @parm POSIX-only flags passed through to mmap(2)
+ * @return pointer to foreignmemory resource handle on success, NULL on
+ * failure
+ *
+ * *paddr is used, on entry, as a hint address for foreign map placement
+ * (see mmap(2)) so should be set to NULL if no specific placement is
+ * required. On return *paddr contains the address where the resource is
+ * mapped.
+ * As for xenforeignmemory_map2() flags is a set of additional flags
+ * for mmap(2). Not all of the flag combinations are possible due to
+ * implementation details on different platforms.
+ */
+xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
+ xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+ unsigned int id, unsigned long frame, unsigned long nr_frames,
+ void **paddr, int prot, int flags);
+
+/**
+ * This function releases a previously acquired resource.
+ *
+ * @parm fmem handle to the open foreignmemory interface
+ * @parm fres handle to the acquired resource
+ *
+ * Returns 0 on success on failure sets errno and returns -1.
+ */
+int xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
+
#endif
/*
diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map
index 716ecaf15c..d5323c87d9 100644
--- a/tools/libs/foreignmemory/libxenforeignmemory.map
+++ b/tools/libs/foreignmemory/libxenforeignmemory.map
@@ -14,3 +14,8 @@ VERS_1.2 {
global:
xenforeignmemory_map2;
} VERS_1.1;
+VERS_1.3 {
+ global:
+ xenforeignmemory_map_resource;
+ xenforeignmemory_unmap_resource;
+} VERS_1.2;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 374e45aed5..a6b41b0b7f 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -277,6 +277,51 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
return ioctl(fmem->fd, IOCTL_PRIVCMD_RESTRICT, &domid);
}
+int osdep_xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ return munmap(fres->addr, fres->nr_frames << PAGE_SHIFT);
+}
+
+int osdep_xenforeignmemory_map_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ privcmd_mmap_resource_t mr = {
+ .dom = fres->domid,
+ .type = fres->type,
+ .id = fres->id,
+ .idx = fres->frame,
+ .num = fres->nr_frames,
+ };
+ int rc;
+
+ fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+ fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
+ if ( fres->addr == MAP_FAILED )
+ return -1;
+
+ mr.addr = (uintptr_t)fres->addr;
+
+ rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
+ if ( rc )
+ {
+ int saved_errno;
+
+ if ( errno != ENOTTY )
+ PERROR("ioctl failed");
+ else
+ errno = EOPNOTSUPP;
+
+ saved_errno = errno;
+ (void)osdep_xenforeignmemory_unmap_resource(fmem, fres);
+ errno = saved_errno;
+
+ return -1;
+ }
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index c5c07cc4c4..80b22bdbfc 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -42,6 +42,37 @@ void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom,
xen_pfn_t *arr, int num);
#endif
+struct xenforeignmemory_resource_handle {
+ domid_t domid;
+ unsigned int type;
+ unsigned int id;
+ unsigned long frame;
+ unsigned long nr_frames;
+ void *addr;
+ int prot;
+ int flags;
+};
+
+#ifndef __linux__
+static inline int osdep_xenforeignmemory_map_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ errno = EOPNOTSUPP;
+ return -1;
+}
+
+static inline int osdep_xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+ return 0;
+}
+#else
+int osdep_xenforeignmemory_map_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
+int osdep_xenforeignmemory_unmap_resource(
+ xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
+#endif
+
#define PERROR(_f...) \
xtl_log(fmem->logger, XTL_ERROR, errno, "xenforeignmemory", _f)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 09/11] tools/libxenforeignmemory: reduce xenforeignmemory_restrict code footprint
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (7 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 08/11] tools/libxenforeignmemory: add support for resource mapping Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-17 13:24 ` [PATCH v12 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
2017-10-17 13:24 ` [PATCH v12 11/11] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson
By using a static inline stub in private.h for OS where this functionality
is not implemented, the various duplicate stubs in the OS-specific source
modules can be avoided.
Signed-off-by: Paul Durrant <paul.durrant@citrix.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>
v4:
- Removed extraneous freebsd code.
v3:
- Patch added in response to review comments.
---
tools/libs/foreignmemory/freebsd.c | 7 -------
tools/libs/foreignmemory/minios.c | 7 -------
tools/libs/foreignmemory/netbsd.c | 7 -------
tools/libs/foreignmemory/private.h | 12 +++++++++---
tools/libs/foreignmemory/solaris.c | 7 -------
5 files changed, 9 insertions(+), 31 deletions(-)
diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c
index dec447485a..6e6bc4b11f 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -95,13 +95,6 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
return munmap(addr, num << PAGE_SHIFT);
}
-int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
- domid_t domid)
-{
- errno = -EOPNOTSUPP;
- return -1;
-}
-
/*
* Local variables:
* mode: C
diff --git a/tools/libs/foreignmemory/minios.c b/tools/libs/foreignmemory/minios.c
index 75f340122e..43341ca301 100644
--- a/tools/libs/foreignmemory/minios.c
+++ b/tools/libs/foreignmemory/minios.c
@@ -58,13 +58,6 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
return munmap(addr, num << PAGE_SHIFT);
}
-int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
- domid_t domid)
-{
- errno = -EOPNOTSUPP;
- return -1;
-}
-
/*
* Local variables:
* mode: C
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index 9bf95ef4f0..54a418ebd6 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -100,13 +100,6 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
return munmap(addr, num*XC_PAGE_SIZE);
}
-int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
- domid_t domid)
-{
- errno = -EOPNOTSUPP;
- return -1;
-}
-
/*
* Local variables:
* mode: C
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 80b22bdbfc..b5d5f0a354 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -32,9 +32,6 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
void *addr, size_t num);
-int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
- domid_t domid);
-
#if defined(__NetBSD__) || defined(__sun__)
/* Strictly compat for those two only only */
void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom,
@@ -54,6 +51,13 @@ struct xenforeignmemory_resource_handle {
};
#ifndef __linux__
+static inline int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
+ domid_t domid)
+{
+ errno = EOPNOTSUPP;
+ return -1;
+}
+
static inline int osdep_xenforeignmemory_map_resource(
xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
{
@@ -67,6 +71,8 @@ static inline int osdep_xenforeignmemory_unmap_resource(
return 0;
}
#else
+int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
+ domid_t domid);
int osdep_xenforeignmemory_map_resource(
xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
int osdep_xenforeignmemory_unmap_resource(
diff --git a/tools/libs/foreignmemory/solaris.c b/tools/libs/foreignmemory/solaris.c
index a33decb4ae..ee8aae4fbd 100644
--- a/tools/libs/foreignmemory/solaris.c
+++ b/tools/libs/foreignmemory/solaris.c
@@ -97,13 +97,6 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
return munmap(addr, num*XC_PAGE_SIZE);
}
-int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
- domid_t domid)
-{
- errno = -EOPNOTSUPP;
- return -1;
-}
-
/*
* Local variables:
* mode: C
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v12 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (8 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 09/11] tools/libxenforeignmemory: reduce xenforeignmemory_restrict code footprint Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
2017-10-26 15:46 ` Jan Beulich
2017-10-17 13:24 ` [PATCH v12 11/11] tools/libxenctrl: use new xenforeignmemory API to seed grant table Paul Durrant
10 siblings, 1 reply; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
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.
Signed-off-by: Paul Durrant <paul.durrant@citrix.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: Jan Beulich <jbeulich@suse.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>
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 | 49 ++++++++++++++++++++++++++++++++++++++-----
xen/common/memory.c | 45 ++++++++++++++++++++++++++++++++++++++-
xen/include/public/memory.h | 6 ++++++
xen/include/xen/grant_table.h | 4 ++++
4 files changed, 98 insertions(+), 6 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6d20b17739..886579a7b0 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3756,13 +3756,12 @@ 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)
+/* Caller must hold write lock as version may change and table may grow */
+static int gnttab_get_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn)
{
- int rc = 0;
struct grant_table *gt = d->grant_table;
-
- grant_write_lock(gt);
+ int rc = 0;
if ( gt->gt_version == 0 )
gt->gt_version = 1;
@@ -3787,6 +3786,18 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
rc = -EINVAL;
}
+ return rc;
+}
+
+int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
+ mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+ int rc;
+
+ grant_write_lock(gt);
+
+ rc = gnttab_get_frame(d, idx, mfn);
if ( !rc )
gnttab_set_frame_gfn(gt, idx, gfn);
@@ -3795,6 +3806,34 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
return rc;
}
+int gnttab_get_grant_frame(struct domain *d, unsigned long idx,
+ mfn_t *mfn)
+{
+ struct grant_table *gt = d->grant_table;
+ int rc;
+
+ /* write lock required as version may change and/or table may grow */
+ grant_write_lock(gt);
+ rc = gnttab_get_frame(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;
+
+ /* write lock required as version may change and/or table may grow */
+ grant_write_lock(gt);
+ rc = gnttab_get_frame(d, idx | XENMAPIDX_grant_table_status, mfn);
+ 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 b27a71c4f1..ae46d95885 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>
@@ -965,12 +966,49 @@ 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,
+ unsigned long 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_grant:
+ rc = gnttab_get_grant_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)
{
struct domain *d, *currd = current->domain;
xen_mem_acquire_resource_t xmar;
- unsigned long mfn_list[2];
+ unsigned long mfn_list[32];
int rc;
if ( copy_from_guest(&xmar, arg, 1) )
@@ -1016,6 +1054,11 @@ static int acquire_resource(
xmar.nr_frames, mfn_list);
break;
+ case XENMEM_resource_grant_table:
+ rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
+ mfn_list);
+ break;
+
default:
rc = -EOPNOTSUPP;
break;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 9596ebf2c7..553304009a 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_grant 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
* frame_list is NULL then this field will be set to the
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index b3a95fda58..e9125e43e7 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -55,6 +55,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_grant_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.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v12 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
2017-10-17 13:24 ` [PATCH v12 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
@ 2017-10-26 15:46 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2017-10-26 15:46 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, KonradRzeszutek Wilk, George Dunlap,
Andrew Cooper, IanJackson, Tim Deegan, xen-devel
>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
> v12:
> - Dropped limit checks as requested by Jan.
Thanks, but ...
> +int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> + mfn_t *mfn)
> +{
> + struct grant_table *gt = d->grant_table;
> + int rc;
> +
> + /* write lock required as version may change and/or table may grow */
> + grant_write_lock(gt);
> + rc = gnttab_get_frame(d, idx | XENMAPIDX_grant_table_status, mfn);
... this ORing won't work then. You need to pass the function a
separate bool, which would likely call for a prereq patch.
> @@ -965,12 +966,49 @@ 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,
> + unsigned long 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_grant:
> + rc = gnttab_get_grant_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)
> {
> struct domain *d, *currd = current->domain;
> xen_mem_acquire_resource_t xmar;
> - unsigned long mfn_list[2];
> + unsigned long mfn_list[32];
Together with the GFN array further down this makes up for 512
bytes of stack space, if my calculation wasn't wrong. That's quite
a lot and certainly something that shouldn't be further increased.
I can accept it remaining this way for now (provided ARM has a
large enough stack as well), but please add a comment indicating
that if further growth is needed, converting to e.g. per-CPU
arrays is going to be necessary.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v12 11/11] tools/libxenctrl: use new xenforeignmemory API to seed grant table
2017-10-17 13:24 [PATCH v12 00/11] x86: guest resource mapping Paul Durrant
` (9 preceding siblings ...)
2017-10-17 13:24 ` [PATCH v12 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table Paul Durrant
@ 2017-10-17 13:24 ` Paul Durrant
10 siblings, 0 replies; 45+ messages in thread
From: Paul Durrant @ 2017-10-17 13:24 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>
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 6e06ef1dec..4216d63462 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -325,12 +325,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, domid_t domid,
- xen_pfn_t console_gmfn,
- xen_pfn_t xenstore_gmfn,
- domid_t console_domid,
- domid_t xenstore_domid);
-int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
+int xc_dom_gnttab_seed(xc_interface *xch, domid_t guest_domid,
+ bool is_hvm,
xen_pfn_t console_gmfn,
xen_pfn_t xenstore_gmfn,
domid_t console_domid,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 8a376d097c..0fe94aa255 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -282,11 +282,29 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, domid_t domid)
return gmfn;
}
-int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
- xen_pfn_t console_gmfn,
- xen_pfn_t xenstore_gmfn,
- domid_t console_domid,
- domid_t xenstore_domid)
+static void xc_dom_set_gnttab_entry(xc_interface *xch,
+ grant_entry_v1_t *gnttab,
+ unsigned int idx,
+ domid_t guest_domid,
+ domid_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, domid_t domid,
+ xen_pfn_t console_gmfn,
+ xen_pfn_t xenstore_gmfn,
+ domid_t console_domid,
+ domid_t xenstore_domid)
{
xen_pfn_t gnttab_gmfn;
@@ -310,18 +328,10 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_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 )
{
@@ -339,11 +349,11 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
return 0;
}
-int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
- xen_pfn_t console_gpfn,
- xen_pfn_t xenstore_gpfn,
- domid_t console_domid,
- domid_t xenstore_domid)
+static int compat_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
+ xen_pfn_t console_gpfn,
+ xen_pfn_t xenstore_gpfn,
+ domid_t console_domid,
+ domid_t xenstore_domid)
{
int rc;
xen_pfn_t scratch_gpfn;
@@ -382,7 +392,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_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)
@@ -407,18 +417,56 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
return 0;
}
-int xc_dom_gnttab_init(struct xc_dom_image *dom)
+int xc_dom_gnttab_seed(xc_interface *xch, domid_t guest_domid,
+ bool is_hvm, xen_pfn_t console_gmfn,
+ xen_pfn_t xenstore_gmfn, domid_t console_domid,
+ domid_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_grant, 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 1dca85354a..a5c661da8f 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -207,11 +207,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 50e25c162c..10635d436b 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -1104,7 +1104,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 ef834e652d..86f562549e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -903,7 +903,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 f501764100..51dc2afb1e 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.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread