* [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
@ 2017-03-08 9:01 Razvan Cojocaru
2017-03-08 18:30 ` Tamas K Lengyel
0 siblings, 1 reply; 11+ messages in thread
From: Razvan Cojocaru @ 2017-03-08 9:01 UTC (permalink / raw)
To: xen-devel
Cc: tamas, wei.liu2, Razvan Cojocaru, andrew.cooper3, ian.jackson, jbeulich
For the default EPT view we have xc_set_mem_access_multi(), which
is able to set an array of pages to an array of access rights with
a single hypercall. However, this functionality was lacking for the
altp2m subsystem, which could only set page restrictions for one
page at a time. This patch addresses the gap.
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
tools/libxc/include/xenctrl.h | 3 +++
tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
4 files changed, 94 insertions(+), 8 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a48981a..645b5bd 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t gfn,
xenmem_access_t access);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
+ uint16_t view_id, uint8_t *access,
+ uint64_t *pages, uint32_t nr);
int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t old_gfn,
xen_pfn_t new_gfn);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632..f202ca1 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
return rc;
}
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
+ uint16_t view_id, uint8_t *access,
+ uint64_t *pages, uint32_t nr)
+{
+ int rc;
+
+ DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+ DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+ DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
+ if ( arg == NULL )
+ return -1;
+
+ arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+ arg->cmd = HVMOP_altp2m_set_mem_access_multi;
+ arg->domain = domid;
+ arg->u.set_mem_access_multi.view = view_id;
+ arg->u.set_mem_access_multi.nr = nr;
+
+ if ( xc_hypercall_bounce_pre(xch, pages) ||
+ xc_hypercall_bounce_pre(xch, access) )
+ {
+ PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
+ return -1;
+ }
+
+ set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
+ set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
+
+ rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+ HYPERCALL_BUFFER_AS_ARG(arg));
+
+ xc_hypercall_buffer_free(xch, arg);
+ xc_hypercall_bounce_post(xch, access);
+ xc_hypercall_bounce_post(xch, pages);
+
+ return rc;
+}
+
int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
uint16_t view_id, xen_pfn_t old_gfn,
xen_pfn_t new_gfn)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ccfae4f..cc9b207 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4394,11 +4394,13 @@ static int hvmop_get_param(
}
static int do_altp2m_op(
+ unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
{
struct xen_hvm_altp2m_op a;
struct domain *d = NULL;
- int rc = 0;
+ long rc = 0;
+ unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
if ( !hvm_altp2m_supported() )
return -EOPNOTSUPP;
@@ -4419,6 +4421,7 @@ static int do_altp2m_op(
case HVMOP_altp2m_destroy_p2m:
case HVMOP_altp2m_switch_p2m:
case HVMOP_altp2m_set_mem_access:
+ case HVMOP_altp2m_set_mem_access_multi:
case HVMOP_altp2m_change_gfn:
break;
default:
@@ -4535,6 +4538,25 @@ static int do_altp2m_op(
a.u.set_mem_access.view);
break;
+ case HVMOP_altp2m_set_mem_access_multi:
+ if ( a.u.set_mem_access_multi.pad )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
+ a.u.set_mem_access_multi.access_list,
+ a.u.set_mem_access_multi.nr, start_iter,
+ MEMOP_CMD_MASK,
+ a.u.set_mem_access_multi.view);
+ if ( rc > 0 )
+ {
+ ASSERT(!(rc & MEMOP_CMD_MASK));
+ rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+ HVMOP_altp2m | rc, arg);
+ }
+ break;
+
case HVMOP_altp2m_change_gfn:
if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
rc = -EINVAL;
@@ -4608,10 +4630,12 @@ static int hvmop_get_mem_type(
return rc;
}
-long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
+long do_hvm_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
long rc = 0;
+ unsigned long op = cmd & MEMOP_CMD_MASK;
+
switch ( op )
{
case HVMOP_set_evtchn_upcall_vector:
@@ -4693,7 +4717,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
case HVMOP_altp2m:
- rc = do_altp2m_op(arg);
+ rc = do_altp2m_op(cmd, arg);
break;
default:
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bc00ef0..e226758 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -231,6 +231,21 @@ struct xen_hvm_altp2m_set_mem_access {
typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
+struct xen_hvm_altp2m_set_mem_access_multi {
+ /* view */
+ uint16_t view;
+ uint16_t pad;
+ /* Number of pages */
+ uint32_t nr;
+ /* List of pfns to set access for */
+ XEN_GUEST_HANDLE(const_uint64) pfn_list;
+ /* Corresponding list of access settings for pfn_list */
+ XEN_GUEST_HANDLE(const_uint8) access_list;
+};
+typedef struct xen_hvm_altp2m_set_mem_access_multi
+ xen_hvm_altp2m_set_mem_access_multi_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
+
struct xen_hvm_altp2m_change_gfn {
/* view */
uint16_t view;
@@ -262,15 +277,18 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_set_mem_access 7
/* Change a p2m entry to have a different gfn->mfn mapping */
#define HVMOP_altp2m_change_gfn 8
+/* Set access for an array of pages */
+#define HVMOP_altp2m_set_mem_access_multi 9
domid_t domain;
uint16_t pad1;
uint32_t pad2;
union {
- struct xen_hvm_altp2m_domain_state domain_state;
- struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
- struct xen_hvm_altp2m_view view;
- struct xen_hvm_altp2m_set_mem_access set_mem_access;
- struct xen_hvm_altp2m_change_gfn change_gfn;
+ struct xen_hvm_altp2m_domain_state domain_state;
+ struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
+ struct xen_hvm_altp2m_view view;
+ struct xen_hvm_altp2m_set_mem_access set_mem_access;
+ struct xen_hvm_altp2m_change_gfn change_gfn;
+ struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
uint8_t pad[64];
} u;
};
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 9:01 [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi() Razvan Cojocaru
@ 2017-03-08 18:30 ` Tamas K Lengyel
2017-03-08 19:19 ` Razvan Cojocaru
2017-03-09 10:16 ` Jan Beulich
0 siblings, 2 replies; 11+ messages in thread
From: Tamas K Lengyel @ 2017-03-08 18:30 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> For the default EPT view we have xc_set_mem_access_multi(), which
> is able to set an array of pages to an array of access rights with
> a single hypercall. However, this functionality was lacking for the
> altp2m subsystem, which could only set page restrictions for one
> page at a time. This patch addresses the gap.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
> tools/libxc/include/xenctrl.h | 3 +++
> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
> 4 files changed, 94 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index a48981a..645b5bd 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> uint16_t view_id, xen_pfn_t gfn,
> xenmem_access_t access);
> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
> + uint16_t view_id, uint8_t *access,
> + uint64_t *pages, uint32_t nr);
IMHO we might as well take an array of view_ids as well so you can set
multiple pages in multiple views at the same time.
> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
> uint16_t view_id, xen_pfn_t old_gfn,
> xen_pfn_t new_gfn);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 0639632..f202ca1 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> return rc;
> }
>
> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
> + uint16_t view_id, uint8_t *access,
> + uint64_t *pages, uint32_t nr)
> +{
> + int rc;
> +
> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +
> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
> + if ( arg == NULL )
> + return -1;
> +
> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
> + arg->domain = domid;
> + arg->u.set_mem_access_multi.view = view_id;
> + arg->u.set_mem_access_multi.nr = nr;
> +
> + if ( xc_hypercall_bounce_pre(xch, pages) ||
> + xc_hypercall_bounce_pre(xch, access) )
> + {
> + PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
> + return -1;
> + }
> +
> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
> +
> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> + xc_hypercall_buffer_free(xch, arg);
> + xc_hypercall_bounce_post(xch, access);
> + xc_hypercall_bounce_post(xch, pages);
> +
> + return rc;
> +}
> +
> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
> uint16_t view_id, xen_pfn_t old_gfn,
> xen_pfn_t new_gfn)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ccfae4f..cc9b207 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
> }
>
> static int do_altp2m_op(
> + unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> struct xen_hvm_altp2m_op a;
> struct domain *d = NULL;
> - int rc = 0;
> + long rc = 0;
> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
I believe we are trying to transition away from stashing the
continuation values into the cmd itself. In another patch of mine the
new way to do this has been by introducing an opaque variable into the
structure passed in by the user to be used for storing the
continuation value. Take a look at
https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439fcca47c493d902bbbb5ec17e
for an example.
>
> if ( !hvm_altp2m_supported() )
> return -EOPNOTSUPP;
> @@ -4419,6 +4421,7 @@ static int do_altp2m_op(
> case HVMOP_altp2m_destroy_p2m:
> case HVMOP_altp2m_switch_p2m:
> case HVMOP_altp2m_set_mem_access:
> + case HVMOP_altp2m_set_mem_access_multi:
> case HVMOP_altp2m_change_gfn:
> break;
> default:
> @@ -4535,6 +4538,25 @@ static int do_altp2m_op(
> a.u.set_mem_access.view);
> break;
>
> + case HVMOP_altp2m_set_mem_access_multi:
> + if ( a.u.set_mem_access_multi.pad )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> + a.u.set_mem_access_multi.access_list,
> + a.u.set_mem_access_multi.nr, start_iter,
> + MEMOP_CMD_MASK,
> + a.u.set_mem_access_multi.view);
> + if ( rc > 0 )
> + {
> + ASSERT(!(rc & MEMOP_CMD_MASK));
> + rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
> + HVMOP_altp2m | rc, arg);
> + }
> + break;
> +
> case HVMOP_altp2m_change_gfn:
> if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
> rc = -EINVAL;
> @@ -4608,10 +4630,12 @@ static int hvmop_get_mem_type(
> return rc;
> }
>
> -long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> +long do_hvm_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> long rc = 0;
>
> + unsigned long op = cmd & MEMOP_CMD_MASK;
> +
> switch ( op )
> {
> case HVMOP_set_evtchn_upcall_vector:
> @@ -4693,7 +4717,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> break;
>
> case HVMOP_altp2m:
> - rc = do_altp2m_op(arg);
> + rc = do_altp2m_op(cmd, arg);
> break;
>
> default:
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bc00ef0..e226758 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -231,6 +231,21 @@ struct xen_hvm_altp2m_set_mem_access {
> typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
> DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>
> +struct xen_hvm_altp2m_set_mem_access_multi {
> + /* view */
> + uint16_t view;
> + uint16_t pad;
> + /* Number of pages */
> + uint32_t nr;
> + /* List of pfns to set access for */
> + XEN_GUEST_HANDLE(const_uint64) pfn_list;
> + /* Corresponding list of access settings for pfn_list */
> + XEN_GUEST_HANDLE(const_uint8) access_list;
> +};
> +typedef struct xen_hvm_altp2m_set_mem_access_multi
> + xen_hvm_altp2m_set_mem_access_multi_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
> +
> struct xen_hvm_altp2m_change_gfn {
> /* view */
> uint16_t view;
> @@ -262,15 +277,18 @@ struct xen_hvm_altp2m_op {
> #define HVMOP_altp2m_set_mem_access 7
> /* Change a p2m entry to have a different gfn->mfn mapping */
> #define HVMOP_altp2m_change_gfn 8
> +/* Set access for an array of pages */
> +#define HVMOP_altp2m_set_mem_access_multi 9
> domid_t domain;
> uint16_t pad1;
> uint32_t pad2;
> union {
> - struct xen_hvm_altp2m_domain_state domain_state;
> - struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
> - struct xen_hvm_altp2m_view view;
> - struct xen_hvm_altp2m_set_mem_access set_mem_access;
> - struct xen_hvm_altp2m_change_gfn change_gfn;
> + struct xen_hvm_altp2m_domain_state domain_state;
> + struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
> + struct xen_hvm_altp2m_view view;
> + struct xen_hvm_altp2m_set_mem_access set_mem_access;
> + struct xen_hvm_altp2m_change_gfn change_gfn;
> + struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
> uint8_t pad[64];
> } u;
> };
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 18:30 ` Tamas K Lengyel
@ 2017-03-08 19:19 ` Razvan Cojocaru
2017-03-08 19:53 ` Tamas K Lengyel
2017-03-09 10:16 ` Jan Beulich
1 sibling, 1 reply; 11+ messages in thread
From: Razvan Cojocaru @ 2017-03-08 19:19 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> For the default EPT view we have xc_set_mem_access_multi(), which
>> is able to set an array of pages to an array of access rights with
>> a single hypercall. However, this functionality was lacking for the
>> altp2m subsystem, which could only set page restrictions for one
>> page at a time. This patch addresses the gap.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>> tools/libxc/include/xenctrl.h | 3 +++
>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index a48981a..645b5bd 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t gfn,
>> xenmem_access_t access);
>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>> + uint16_t view_id, uint8_t *access,
>> + uint64_t *pages, uint32_t nr);
>
> IMHO we might as well take an array of view_ids as well so you can set
> multiple pages in multiple views at the same time.
I'm not sure there's a real use case for that. The _multi() function has
been added to help with cases where we need to set thousands of pages -
in which case condensing it to a single hypercall vs. thousands of them
noticeably improves performance.
But with views, I'd bet that in almost all cases people will want to
only use one extra view, and even if they'd like to use more, it'll
still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
That's actually not even a valid use case, since if we're setting
restrictions on _all_ the views we might as well be simply using the
default view alone.
In other words, I would argue that the small gain does not justify the
extra development effort.
>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t old_gfn,
>> xen_pfn_t new_gfn);
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 0639632..f202ca1 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>> return rc;
>> }
>>
>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>> + uint16_t view_id, uint8_t *access,
>> + uint64_t *pages, uint32_t nr)
>> +{
>> + int rc;
>> +
>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +
>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>> + if ( arg == NULL )
>> + return -1;
>> +
>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>> + arg->domain = domid;
>> + arg->u.set_mem_access_multi.view = view_id;
>> + arg->u.set_mem_access_multi.nr = nr;
>> +
>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>> + xc_hypercall_bounce_pre(xch, access) )
>> + {
>> + PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
>> + return -1;
>> + }
>> +
>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>> +
>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>> + HYPERCALL_BUFFER_AS_ARG(arg));
>> +
>> + xc_hypercall_buffer_free(xch, arg);
>> + xc_hypercall_bounce_post(xch, access);
>> + xc_hypercall_bounce_post(xch, pages);
>> +
>> + return rc;
>> +}
>> +
>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t old_gfn,
>> xen_pfn_t new_gfn)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index ccfae4f..cc9b207 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>> }
>>
>> static int do_altp2m_op(
>> + unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> struct xen_hvm_altp2m_op a;
>> struct domain *d = NULL;
>> - int rc = 0;
>> + long rc = 0;
>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>
> I believe we are trying to transition away from stashing the
> continuation values into the cmd itself. In another patch of mine the
> new way to do this has been by introducing an opaque variable into the
> structure passed in by the user to be used for storing the
> continuation value. Take a look at
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439fcca47c493d902bbbb5ec17e
> for an example.
Are we? I'm also not a big fan of all the mask / bit-fiddling for
continuation purposes, but that's how p2m_set_mem_access_multi() works
at the moment (which I've used for both
XENMEM_access_op_set_access_multi and, in this patch,
HVMOP_altp2m_set_mem_access_multi). It's also used by
p2m_set_mem_access() / XENMEM_access_op_set_access.
Changing the way continuation works in this patch would mean reworking
all that code, which would effectively transform this relatively small
patch into a series. If that's required we can go in that direction, but
I'm not sure we want that. Waiting for further opinions.
>> if ( !hvm_altp2m_supported() )
>> return -EOPNOTSUPP;
>> @@ -4419,6 +4421,7 @@ static int do_altp2m_op(
>> case HVMOP_altp2m_destroy_p2m:
>> case HVMOP_altp2m_switch_p2m:
>> case HVMOP_altp2m_set_mem_access:
>> + case HVMOP_altp2m_set_mem_access_multi:
>> case HVMOP_altp2m_change_gfn:
>> break;
>> default:
>> @@ -4535,6 +4538,25 @@ static int do_altp2m_op(
>> a.u.set_mem_access.view);
>> break;
>>
>> + case HVMOP_altp2m_set_mem_access_multi:
>> + if ( a.u.set_mem_access_multi.pad )
>> + {
>> + rc = -EINVAL;
>> + break;
>> + }
>> + rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> + a.u.set_mem_access_multi.access_list,
>> + a.u.set_mem_access_multi.nr, start_iter,
>> + MEMOP_CMD_MASK,
>> + a.u.set_mem_access_multi.view);
>> + if ( rc > 0 )
>> + {
>> + ASSERT(!(rc & MEMOP_CMD_MASK));
>> + rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>> + HVMOP_altp2m | rc, arg);
>> + }
>> + break;
>> +
>> case HVMOP_altp2m_change_gfn:
>> if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>> rc = -EINVAL;
>> @@ -4608,10 +4630,12 @@ static int hvmop_get_mem_type(
>> return rc;
>> }
>>
>> -long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +long do_hvm_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> long rc = 0;
>>
>> + unsigned long op = cmd & MEMOP_CMD_MASK;
I'll need to remove this extra blank line, just noticed it. :)
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 19:19 ` Razvan Cojocaru
@ 2017-03-08 19:53 ` Tamas K Lengyel
2017-03-08 20:17 ` Razvan Cojocaru
0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-03-08 19:53 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>> is able to set an array of pages to an array of access rights with
>>> a single hypercall. However, this functionality was lacking for the
>>> altp2m subsystem, which could only set page restrictions for one
>>> page at a time. This patch addresses the gap.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> ---
>>> tools/libxc/include/xenctrl.h | 3 +++
>>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index a48981a..645b5bd 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t gfn,
>>> xenmem_access_t access);
>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>> + uint16_t view_id, uint8_t *access,
>>> + uint64_t *pages, uint32_t nr);
>>
>> IMHO we might as well take an array of view_ids as well so you can set
>> multiple pages in multiple views at the same time.
>
> I'm not sure there's a real use case for that. The _multi() function has
> been added to help with cases where we need to set thousands of pages -
> in which case condensing it to a single hypercall vs. thousands of them
> noticeably improves performance.
>
> But with views, I'd bet that in almost all cases people will want to
> only use one extra view, and even if they'd like to use more, it'll
> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
> That's actually not even a valid use case, since if we're setting
> restrictions on _all_ the views we might as well be simply using the
> default view alone.
FYI I already use more then one view..
> In other words, I would argue that the small gain does not justify the
> extra development effort.
I don't think it would be much extra development effort considering
both the access and page fields are passed in as an array already. But
anyway, it was just a suggestion, I won't hold the patch up over it.
>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t old_gfn,
>>> xen_pfn_t new_gfn);
>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>> index 0639632..f202ca1 100644
>>> --- a/tools/libxc/xc_altp2m.c
>>> +++ b/tools/libxc/xc_altp2m.c
>>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>> return rc;
>>> }
>>>
>>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>>> + uint16_t view_id, uint8_t *access,
>>> + uint64_t *pages, uint32_t nr)
>>> +{
>>> + int rc;
>>> +
>>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>> +
>>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>>> + if ( arg == NULL )
>>> + return -1;
>>> +
>>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>>> + arg->domain = domid;
>>> + arg->u.set_mem_access_multi.view = view_id;
>>> + arg->u.set_mem_access_multi.nr = nr;
>>> +
>>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>>> + xc_hypercall_bounce_pre(xch, access) )
>>> + {
>>> + PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
>>> + return -1;
>>> + }
>>> +
>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>>> +
>>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>> + HYPERCALL_BUFFER_AS_ARG(arg));
>>> +
>>> + xc_hypercall_buffer_free(xch, arg);
>>> + xc_hypercall_bounce_post(xch, access);
>>> + xc_hypercall_bounce_post(xch, pages);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t old_gfn,
>>> xen_pfn_t new_gfn)
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index ccfae4f..cc9b207 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>>> }
>>>
>>> static int do_altp2m_op(
>>> + unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> {
>>> struct xen_hvm_altp2m_op a;
>>> struct domain *d = NULL;
>>> - int rc = 0;
>>> + long rc = 0;
>>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>>
>> I believe we are trying to transition away from stashing the
>> continuation values into the cmd itself. In another patch of mine the
>> new way to do this has been by introducing an opaque variable into the
>> structure passed in by the user to be used for storing the
>> continuation value. Take a look at
>> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439fcca47c493d902bbbb5ec17e
>> for an example.
>
> Are we? I'm also not a big fan of all the mask / bit-fiddling for
> continuation purposes, but that's how p2m_set_mem_access_multi() works
> at the moment (which I've used for both
> XENMEM_access_op_set_access_multi and, in this patch,
> HVMOP_altp2m_set_mem_access_multi). It's also used by
> p2m_set_mem_access() / XENMEM_access_op_set_access.
>
> Changing the way continuation works in this patch would mean reworking
> all that code, which would effectively transform this relatively small
> patch into a series. If that's required we can go in that direction, but
> I'm not sure we want that. Waiting for further opinions.
I'm not saying you need to rework all pre-existing code to do that,
but at least for new ops being introduced it should be the way we
continue. If you can't reuse existing functions for it, introducing a
new one is desirable. We can figure out how to migrate pre-existing
hypercalls to the new method later.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 19:53 ` Tamas K Lengyel
@ 2017-03-08 20:17 ` Razvan Cojocaru
2017-03-08 20:53 ` Tamas K Lengyel
0 siblings, 1 reply; 11+ messages in thread
From: Razvan Cojocaru @ 2017-03-08 20:17 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
> On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>>> <rcojocaru@bitdefender.com> wrote:
>>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>>> is able to set an array of pages to an array of access rights with
>>>> a single hypercall. However, this functionality was lacking for the
>>>> altp2m subsystem, which could only set page restrictions for one
>>>> page at a time. This patch addresses the gap.
>>>>
>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>> ---
>>>> tools/libxc/include/xenctrl.h | 3 +++
>>>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>> index a48981a..645b5bd 100644
>>>> --- a/tools/libxc/include/xenctrl.h
>>>> +++ b/tools/libxc/include/xenctrl.h
>>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>> uint16_t view_id, xen_pfn_t gfn,
>>>> xenmem_access_t access);
>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>>> + uint16_t view_id, uint8_t *access,
>>>> + uint64_t *pages, uint32_t nr);
>>>
>>> IMHO we might as well take an array of view_ids as well so you can set
>>> multiple pages in multiple views at the same time.
>>
>> I'm not sure there's a real use case for that. The _multi() function has
>> been added to help with cases where we need to set thousands of pages -
>> in which case condensing it to a single hypercall vs. thousands of them
>> noticeably improves performance.
>>
>> But with views, I'd bet that in almost all cases people will want to
>> only use one extra view, and even if they'd like to use more, it'll
>> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
>> That's actually not even a valid use case, since if we're setting
>> restrictions on _all_ the views we might as well be simply using the
>> default view alone.
>
> FYI I already use more then one view..
Fair enough, but the question is, when you do, is it likely that you'll
want to set the same restrictions for the same multiple pages in several
views at one time?
>> In other words, I would argue that the small gain does not justify the
>> extra development effort.
>
> I don't think it would be much extra development effort considering
> both the access and page fields are passed in as an array already. But
> anyway, it was just a suggestion, I won't hold the patch up over it.
Passing the array / size to the hypervisor is obviously trivial, my
concern is that p2m_set_mem_access_multi() would need to be reworked to
use the array, which might also require special error handling (which
view had a problem?) and continuation logic (do we now require two start
indices, one for the gfn list and one for the view list and reset the
one for the gfn list at the end of processing a view?).
On an unrelated note, running scripts/get-maintainers.pl on this patch
did not list you - is that something that should be fixed? I value your
opinion and expertise with altp2m so I've CCd you anyway but this may be
something you may want to address in the MAINTAINERS file.
>>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>>> uint16_t view_id, xen_pfn_t old_gfn,
>>>> xen_pfn_t new_gfn);
>>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>>> index 0639632..f202ca1 100644
>>>> --- a/tools/libxc/xc_altp2m.c
>>>> +++ b/tools/libxc/xc_altp2m.c
>>>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>> return rc;
>>>> }
>>>>
>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>>>> + uint16_t view_id, uint8_t *access,
>>>> + uint64_t *pages, uint32_t nr)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>>>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>>> +
>>>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>>>> + if ( arg == NULL )
>>>> + return -1;
>>>> +
>>>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>>>> + arg->domain = domid;
>>>> + arg->u.set_mem_access_multi.view = view_id;
>>>> + arg->u.set_mem_access_multi.nr = nr;
>>>> +
>>>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>>>> + xc_hypercall_bounce_pre(xch, access) )
>>>> + {
>>>> + PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
>>>> + return -1;
>>>> + }
>>>> +
>>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>>>> +
>>>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>>> + HYPERCALL_BUFFER_AS_ARG(arg));
>>>> +
>>>> + xc_hypercall_buffer_free(xch, arg);
>>>> + xc_hypercall_bounce_post(xch, access);
>>>> + xc_hypercall_bounce_post(xch, pages);
>>>> +
>>>> + return rc;
>>>> +}
>>>> +
>>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>>> uint16_t view_id, xen_pfn_t old_gfn,
>>>> xen_pfn_t new_gfn)
>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>> index ccfae4f..cc9b207 100644
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>>>> }
>>>>
>>>> static int do_altp2m_op(
>>>> + unsigned long cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> {
>>>> struct xen_hvm_altp2m_op a;
>>>> struct domain *d = NULL;
>>>> - int rc = 0;
>>>> + long rc = 0;
>>>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>>>
>>> I believe we are trying to transition away from stashing the
>>> continuation values into the cmd itself. In another patch of mine the
>>> new way to do this has been by introducing an opaque variable into the
>>> structure passed in by the user to be used for storing the
>>> continuation value. Take a look at
>>> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439fcca47c493d902bbbb5ec17e
>>> for an example.
>>
>> Are we? I'm also not a big fan of all the mask / bit-fiddling for
>> continuation purposes, but that's how p2m_set_mem_access_multi() works
>> at the moment (which I've used for both
>> XENMEM_access_op_set_access_multi and, in this patch,
>> HVMOP_altp2m_set_mem_access_multi). It's also used by
>> p2m_set_mem_access() / XENMEM_access_op_set_access.
>>
>> Changing the way continuation works in this patch would mean reworking
>> all that code, which would effectively transform this relatively small
>> patch into a series. If that's required we can go in that direction, but
>> I'm not sure we want that. Waiting for further opinions.
>
> I'm not saying you need to rework all pre-existing code to do that,
> but at least for new ops being introduced it should be the way we
> continue. If you can't reuse existing functions for it, introducing a
> new one is desirable. We can figure out how to migrate pre-existing
> hypercalls to the new method later.
I'll look into it, and come back when I get a better feel of the
obstacles. In the meantime, of course, other opinions are welcome.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 20:17 ` Razvan Cojocaru
@ 2017-03-08 20:53 ` Tamas K Lengyel
2017-03-08 21:36 ` Razvan Cojocaru
0 siblings, 1 reply; 11+ messages in thread
From: Tamas K Lengyel @ 2017-03-08 20:53 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On Wed, Mar 8, 2017 at 1:17 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
>> On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>>>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>>>> <rcojocaru@bitdefender.com> wrote:
>>>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>>>> is able to set an array of pages to an array of access rights with
>>>>> a single hypercall. However, this functionality was lacking for the
>>>>> altp2m subsystem, which could only set page restrictions for one
>>>>> page at a time. This patch addresses the gap.
>>>>>
>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>> ---
>>>>> tools/libxc/include/xenctrl.h | 3 +++
>>>>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>>> index a48981a..645b5bd 100644
>>>>> --- a/tools/libxc/include/xenctrl.h
>>>>> +++ b/tools/libxc/include/xenctrl.h
>>>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>>> uint16_t view_id, xen_pfn_t gfn,
>>>>> xenmem_access_t access);
>>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>>>> + uint16_t view_id, uint8_t *access,
>>>>> + uint64_t *pages, uint32_t nr);
>>>>
>>>> IMHO we might as well take an array of view_ids as well so you can set
>>>> multiple pages in multiple views at the same time.
>>>
>>> I'm not sure there's a real use case for that. The _multi() function has
>>> been added to help with cases where we need to set thousands of pages -
>>> in which case condensing it to a single hypercall vs. thousands of them
>>> noticeably improves performance.
>>>
>>> But with views, I'd bet that in almost all cases people will want to
>>> only use one extra view, and even if they'd like to use more, it'll
>>> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
>>> That's actually not even a valid use case, since if we're setting
>>> restrictions on _all_ the views we might as well be simply using the
>>> default view alone.
>>
>> FYI I already use more then one view..
>
> Fair enough, but the question is, when you do, is it likely that you'll
> want to set the same restrictions for the same multiple pages in several
> views at one time?
Well, if you have the view id as an extra array, you could set
different permissions in different views using a single hypercall. For
example, you could do something along the lines:
view_ids[0,1,2]=1, access[0,1,2]=rw pages[0,1,2] = {10,11,12}
view_ids[3,4,5]=2, access[3,4,5]=x, pages[3,4,5] = {10,11,12}.
view_ids[6]=0, access[6]=rwx, pages[6] = 13
>
>>> In other words, I would argue that the small gain does not justify the
>>> extra development effort.
>>
>> I don't think it would be much extra development effort considering
>> both the access and page fields are passed in as an array already. But
>> anyway, it was just a suggestion, I won't hold the patch up over it.
>
> Passing the array / size to the hypervisor is obviously trivial, my
> concern is that p2m_set_mem_access_multi() would need to be reworked to
> use the array, which might also require special error handling (which
> view had a problem?) and continuation logic (do we now require two start
> indices, one for the gfn list and one for the view list and reset the
> one for the gfn list at the end of processing a view?).
I'm not sure I follow. I would imagine the size of the view_ids array
would be the same as the other arrays, so there is only one loop that
goes through the whole thing.
>
> On an unrelated note, running scripts/get-maintainers.pl on this patch
> did not list you - is that something that should be fixed? I value your
> opinion and expertise with altp2m so I've CCd you anyway but this may be
> something you may want to address in the MAINTAINERS file.
>
Yea, I'm not the maintainer of altp2m or any of the files you touched
in this patch so not being listed as a maintainer is correct.
>>>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>>>> uint16_t view_id, xen_pfn_t old_gfn,
>>>>> xen_pfn_t new_gfn);
>>>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>>>> index 0639632..f202ca1 100644
>>>>> --- a/tools/libxc/xc_altp2m.c
>>>>> +++ b/tools/libxc/xc_altp2m.c
>>>>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>>> return rc;
>>>>> }
>>>>>
>>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>>>>> + uint16_t view_id, uint8_t *access,
>>>>> + uint64_t *pages, uint32_t nr)
>>>>> +{
>>>>> + int rc;
>>>>> +
>>>>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>>>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>>>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>>>>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>>>> +
>>>>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>>>>> + if ( arg == NULL )
>>>>> + return -1;
>>>>> +
>>>>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>>>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>>>>> + arg->domain = domid;
>>>>> + arg->u.set_mem_access_multi.view = view_id;
>>>>> + arg->u.set_mem_access_multi.nr = nr;
>>>>> +
>>>>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>>>>> + xc_hypercall_bounce_pre(xch, access) )
>>>>> + {
>>>>> + PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>>>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>>>>> +
>>>>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>>>> + HYPERCALL_BUFFER_AS_ARG(arg));
>>>>> +
>>>>> + xc_hypercall_buffer_free(xch, arg);
>>>>> + xc_hypercall_bounce_post(xch, access);
>>>>> + xc_hypercall_bounce_post(xch, pages);
>>>>> +
>>>>> + return rc;
>>>>> +}
>>>>> +
>>>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>>>> uint16_t view_id, xen_pfn_t old_gfn,
>>>>> xen_pfn_t new_gfn)
>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>> index ccfae4f..cc9b207 100644
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>>>>> }
>>>>>
>>>>> static int do_altp2m_op(
>>>>> + unsigned long cmd,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>> {
>>>>> struct xen_hvm_altp2m_op a;
>>>>> struct domain *d = NULL;
>>>>> - int rc = 0;
>>>>> + long rc = 0;
>>>>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>>>>
>>>> I believe we are trying to transition away from stashing the
>>>> continuation values into the cmd itself. In another patch of mine the
>>>> new way to do this has been by introducing an opaque variable into the
>>>> structure passed in by the user to be used for storing the
>>>> continuation value. Take a look at
>>>> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439fcca47c493d902bbbb5ec17e
>>>> for an example.
>>>
>>> Are we? I'm also not a big fan of all the mask / bit-fiddling for
>>> continuation purposes, but that's how p2m_set_mem_access_multi() works
>>> at the moment (which I've used for both
>>> XENMEM_access_op_set_access_multi and, in this patch,
>>> HVMOP_altp2m_set_mem_access_multi). It's also used by
>>> p2m_set_mem_access() / XENMEM_access_op_set_access.
>>>
>>> Changing the way continuation works in this patch would mean reworking
>>> all that code, which would effectively transform this relatively small
>>> patch into a series. If that's required we can go in that direction, but
>>> I'm not sure we want that. Waiting for further opinions.
>>
>> I'm not saying you need to rework all pre-existing code to do that,
>> but at least for new ops being introduced it should be the way we
>> continue. If you can't reuse existing functions for it, introducing a
>> new one is desirable. We can figure out how to migrate pre-existing
>> hypercalls to the new method later.
>
> I'll look into it, and come back when I get a better feel of the
> obstacles. In the meantime, of course, other opinions are welcome.
Thanks!
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 20:53 ` Tamas K Lengyel
@ 2017-03-08 21:36 ` Razvan Cojocaru
2017-03-08 22:03 ` Tamas K Lengyel
0 siblings, 1 reply; 11+ messages in thread
From: Razvan Cojocaru @ 2017-03-08 21:36 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On 03/08/2017 10:53 PM, Tamas K Lengyel wrote:
> On Wed, Mar 8, 2017 at 1:17 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
>>> On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
>>> <rcojocaru@bitdefender.com> wrote:
>>>> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>>>>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>>>>> is able to set an array of pages to an array of access rights with
>>>>>> a single hypercall. However, this functionality was lacking for the
>>>>>> altp2m subsystem, which could only set page restrictions for one
>>>>>> page at a time. This patch addresses the gap.
>>>>>>
>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>> ---
>>>>>> tools/libxc/include/xenctrl.h | 3 +++
>>>>>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>>>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>>>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>>>> index a48981a..645b5bd 100644
>>>>>> --- a/tools/libxc/include/xenctrl.h
>>>>>> +++ b/tools/libxc/include/xenctrl.h
>>>>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>>>> uint16_t view_id, xen_pfn_t gfn,
>>>>>> xenmem_access_t access);
>>>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>>>>> + uint16_t view_id, uint8_t *access,
>>>>>> + uint64_t *pages, uint32_t nr);
>>>>>
>>>>> IMHO we might as well take an array of view_ids as well so you can set
>>>>> multiple pages in multiple views at the same time.
>>>>
>>>> I'm not sure there's a real use case for that. The _multi() function has
>>>> been added to help with cases where we need to set thousands of pages -
>>>> in which case condensing it to a single hypercall vs. thousands of them
>>>> noticeably improves performance.
>>>>
>>>> But with views, I'd bet that in almost all cases people will want to
>>>> only use one extra view, and even if they'd like to use more, it'll
>>>> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
>>>> That's actually not even a valid use case, since if we're setting
>>>> restrictions on _all_ the views we might as well be simply using the
>>>> default view alone.
>>>
>>> FYI I already use more then one view..
>>
>> Fair enough, but the question is, when you do, is it likely that you'll
>> want to set the same restrictions for the same multiple pages in several
>> views at one time?
>
> Well, if you have the view id as an extra array, you could set
> different permissions in different views using a single hypercall. For
> example, you could do something along the lines:
>
> view_ids[0,1,2]=1, access[0,1,2]=rw pages[0,1,2] = {10,11,12}
> view_ids[3,4,5]=2, access[3,4,5]=x, pages[3,4,5] = {10,11,12}.
> view_ids[6]=0, access[6]=rwx, pages[6] = 13
I see what you mean now. I thought you meant:
view_ids={1,2,3,4,5}, access={rw,rx,rwx} pages={0xff,0xfa,0xfb}
In which case, obviously the view_ids array would need its own size
sent, and for each view we'd go through the pages/access arrays and set
page restrictions.
>>>> In other words, I would argue that the small gain does not justify the
>>>> extra development effort.
>>>
>>> I don't think it would be much extra development effort considering
>>> both the access and page fields are passed in as an array already. But
>>> anyway, it was just a suggestion, I won't hold the patch up over it.
>>
>> Passing the array / size to the hypervisor is obviously trivial, my
>> concern is that p2m_set_mem_access_multi() would need to be reworked to
>> use the array, which might also require special error handling (which
>> view had a problem?) and continuation logic (do we now require two start
>> indices, one for the gfn list and one for the view list and reset the
>> one for the gfn list at the end of processing a view?).
>
> I'm not sure I follow. I would imagine the size of the view_ids array
> would be the same as the other arrays, so there is only one loop that
> goes through the whole thing.
Right, we clearly had different pictures of what you meant by "an array
of view_ids" (as seen above).
That's somewhat more approachable, although it still poses the question
of what happens when we set page X 'r' in view 1, for example, and then
again set page X, but 'rwx' in view 0 (the default EPT, which then also
alters all the other views). This can have subtle consequences.
It would also require p2m switching in the loop, so a different or
significantly altered p2m_set_mem_access_multi(). Also with potential
inefficient TLB flushing consequences (though this needs checking).
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 21:36 ` Razvan Cojocaru
@ 2017-03-08 22:03 ` Tamas K Lengyel
0 siblings, 0 replies; 11+ messages in thread
From: Tamas K Lengyel @ 2017-03-08 22:03 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Jan Beulich, Xen-devel
On Wed, Mar 8, 2017 at 2:36 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 03/08/2017 10:53 PM, Tamas K Lengyel wrote:
>> On Wed, Mar 8, 2017 at 1:17 PM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>> On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
>>>> On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
>>>> <rcojocaru@bitdefender.com> wrote:
>>>>> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>>>>>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>>>>>> is able to set an array of pages to an array of access rights with
>>>>>>> a single hypercall. However, this functionality was lacking for the
>>>>>>> altp2m subsystem, which could only set page restrictions for one
>>>>>>> page at a time. This patch addresses the gap.
>>>>>>>
>>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>>> ---
>>>>>>> tools/libxc/include/xenctrl.h | 3 +++
>>>>>>> tools/libxc/xc_altp2m.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>>>>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>>>>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>>>>> index a48981a..645b5bd 100644
>>>>>>> --- a/tools/libxc/include/xenctrl.h
>>>>>>> +++ b/tools/libxc/include/xenctrl.h
>>>>>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>>>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>>>>> uint16_t view_id, xen_pfn_t gfn,
>>>>>>> xenmem_access_t access);
>>>>>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>>>>>> + uint16_t view_id, uint8_t *access,
>>>>>>> + uint64_t *pages, uint32_t nr);
>>>>>>
>>>>>> IMHO we might as well take an array of view_ids as well so you can set
>>>>>> multiple pages in multiple views at the same time.
>>>>>
>>>>> I'm not sure there's a real use case for that. The _multi() function has
>>>>> been added to help with cases where we need to set thousands of pages -
>>>>> in which case condensing it to a single hypercall vs. thousands of them
>>>>> noticeably improves performance.
>>>>>
>>>>> But with views, I'd bet that in almost all cases people will want to
>>>>> only use one extra view, and even if they'd like to use more, it'll
>>>>> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
>>>>> That's actually not even a valid use case, since if we're setting
>>>>> restrictions on _all_ the views we might as well be simply using the
>>>>> default view alone.
>>>>
>>>> FYI I already use more then one view..
>>>
>>> Fair enough, but the question is, when you do, is it likely that you'll
>>> want to set the same restrictions for the same multiple pages in several
>>> views at one time?
>>
>> Well, if you have the view id as an extra array, you could set
>> different permissions in different views using a single hypercall. For
>> example, you could do something along the lines:
>>
>> view_ids[0,1,2]=1, access[0,1,2]=rw pages[0,1,2] = {10,11,12}
>> view_ids[3,4,5]=2, access[3,4,5]=x, pages[3,4,5] = {10,11,12}.
>> view_ids[6]=0, access[6]=rwx, pages[6] = 13
>
> I see what you mean now. I thought you meant:
>
> view_ids={1,2,3,4,5}, access={rw,rx,rwx} pages={0xff,0xfa,0xfb}
>
> In which case, obviously the view_ids array would need its own size
> sent, and for each view we'd go through the pages/access arrays and set
> page restrictions.
>
>>>>> In other words, I would argue that the small gain does not justify the
>>>>> extra development effort.
>>>>
>>>> I don't think it would be much extra development effort considering
>>>> both the access and page fields are passed in as an array already. But
>>>> anyway, it was just a suggestion, I won't hold the patch up over it.
>>>
>>> Passing the array / size to the hypervisor is obviously trivial, my
>>> concern is that p2m_set_mem_access_multi() would need to be reworked to
>>> use the array, which might also require special error handling (which
>>> view had a problem?) and continuation logic (do we now require two start
>>> indices, one for the gfn list and one for the view list and reset the
>>> one for the gfn list at the end of processing a view?).
>>
>> I'm not sure I follow. I would imagine the size of the view_ids array
>> would be the same as the other arrays, so there is only one loop that
>> goes through the whole thing.
>
> Right, we clearly had different pictures of what you meant by "an array
> of view_ids" (as seen above).
>
> That's somewhat more approachable, although it still poses the question
> of what happens when we set page X 'r' in view 1, for example, and then
> again set page X, but 'rwx' in view 0 (the default EPT, which then also
> alters all the other views). This can have subtle consequences.
That's true. In light of that it indeed might be saner to keep your
current design.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-08 18:30 ` Tamas K Lengyel
2017-03-08 19:19 ` Razvan Cojocaru
@ 2017-03-09 10:16 ` Jan Beulich
2017-03-09 10:19 ` Razvan Cojocaru
1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2017-03-09 10:16 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Xen-devel
>>> On 08.03.17 at 19:30, <tamas@tklengyel.com> wrote:
> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> For the default EPT view we have xc_set_mem_access_multi(), which
>> is able to set an array of pages to an array of access rights with
>> a single hypercall. However, this functionality was lacking for the
>> altp2m subsystem, which could only set page restrictions for one
>> page at a time. This patch addresses the gap.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>> tools/libxc/include/xenctrl.h | 3 +++
>> tools/libxc/xc_altp2m.c | 41
> +++++++++++++++++++++++++++++++++++++++++
>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index a48981a..645b5bd 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle,
> domid_t domid,
>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t gfn,
>> xenmem_access_t access);
>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>> + uint16_t view_id, uint8_t *access,
>> + uint64_t *pages, uint32_t nr);
>
> IMHO we might as well take an array of view_ids as well so you can set
> multiple pages in multiple views at the same time.
>
>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t old_gfn,
>> xen_pfn_t new_gfn);
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 0639632..f202ca1 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle,
> domid_t domid,
>> return rc;
>> }
>>
>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>> + uint16_t view_id, uint8_t *access,
>> + uint64_t *pages, uint32_t nr)
>> +{
>> + int rc;
>> +
>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +
>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>> + if ( arg == NULL )
>> + return -1;
>> +
>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>> + arg->domain = domid;
>> + arg->u.set_mem_access_multi.view = view_id;
>> + arg->u.set_mem_access_multi.nr = nr;
>> +
>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>> + xc_hypercall_bounce_pre(xch, access) )
>> + {
>> + PERROR("Could not bounce memory for
> HVMOP_altp2m_set_mem_access_multi");
>> + return -1;
>> + }
>> +
>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>> +
>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>> + HYPERCALL_BUFFER_AS_ARG(arg));
>> +
>> + xc_hypercall_buffer_free(xch, arg);
>> + xc_hypercall_bounce_post(xch, access);
>> + xc_hypercall_bounce_post(xch, pages);
>> +
>> + return rc;
>> +}
>> +
>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>> uint16_t view_id, xen_pfn_t old_gfn,
>> xen_pfn_t new_gfn)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index ccfae4f..cc9b207 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>> }
>>
>> static int do_altp2m_op(
>> + unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> struct xen_hvm_altp2m_op a;
>> struct domain *d = NULL;
>> - int rc = 0;
>> + long rc = 0;
>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>
> I believe we are trying to transition away from stashing the
> continuation values into the cmd itself. In another patch of mine the
> new way to do this has been by introducing an opaque variable into the
> structure passed in by the user to be used for storing the
> continuation value. Take a look at
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439
> fcca47c493d902bbbb5ec17e
> for an example.
I think it was a mistake to allow this in - imo memop-s should be
consistent in how they handle continuations. For new hypercalls
(like dmop) the story is different of course.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-09 10:16 ` Jan Beulich
@ 2017-03-09 10:19 ` Razvan Cojocaru
2017-03-09 10:24 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Razvan Cojocaru @ 2017-03-09 10:19 UTC (permalink / raw)
To: Jan Beulich, Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Xen-devel
On 03/09/2017 12:16 PM, Jan Beulich wrote:
>>>> On 08.03.17 at 19:30, <tamas@tklengyel.com> wrote:
>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>> is able to set an array of pages to an array of access rights with
>>> a single hypercall. However, this functionality was lacking for the
>>> altp2m subsystem, which could only set page restrictions for one
>>> page at a time. This patch addresses the gap.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> ---
>>> tools/libxc/include/xenctrl.h | 3 +++
>>> tools/libxc/xc_altp2m.c | 41
>> +++++++++++++++++++++++++++++++++++++++++
>>> xen/arch/x86/hvm/hvm.c | 30 +++++++++++++++++++++++++++---
>>> xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>> 4 files changed, 94 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index a48981a..645b5bd 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle,
>> domid_t domid,
>>> int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t gfn,
>>> xenmem_access_t access);
>>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>>> + uint16_t view_id, uint8_t *access,
>>> + uint64_t *pages, uint32_t nr);
>>
>> IMHO we might as well take an array of view_ids as well so you can set
>> multiple pages in multiple views at the same time.
>>
>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t old_gfn,
>>> xen_pfn_t new_gfn);
>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>> index 0639632..f202ca1 100644
>>> --- a/tools/libxc/xc_altp2m.c
>>> +++ b/tools/libxc/xc_altp2m.c
>>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle,
>> domid_t domid,
>>> return rc;
>>> }
>>>
>>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>>> + uint16_t view_id, uint8_t *access,
>>> + uint64_t *pages, uint32_t nr)
>>> +{
>>> + int rc;
>>> +
>>> + DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>> + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>> + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>>> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
>>> +
>>> + arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>>> + if ( arg == NULL )
>>> + return -1;
>>> +
>>> + arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>> + arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>>> + arg->domain = domid;
>>> + arg->u.set_mem_access_multi.view = view_id;
>>> + arg->u.set_mem_access_multi.nr = nr;
>>> +
>>> + if ( xc_hypercall_bounce_pre(xch, pages) ||
>>> + xc_hypercall_bounce_pre(xch, access) )
>>> + {
>>> + PERROR("Could not bounce memory for
>> HVMOP_altp2m_set_mem_access_multi");
>>> + return -1;
>>> + }
>>> +
>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>>> + set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>>> +
>>> + rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>> + HYPERCALL_BUFFER_AS_ARG(arg));
>>> +
>>> + xc_hypercall_buffer_free(xch, arg);
>>> + xc_hypercall_bounce_post(xch, access);
>>> + xc_hypercall_bounce_post(xch, pages);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>> uint16_t view_id, xen_pfn_t old_gfn,
>>> xen_pfn_t new_gfn)
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index ccfae4f..cc9b207 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>>> }
>>>
>>> static int do_altp2m_op(
>>> + unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> {
>>> struct xen_hvm_altp2m_op a;
>>> struct domain *d = NULL;
>>> - int rc = 0;
>>> + long rc = 0;
>>> + unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>>
>> I believe we are trying to transition away from stashing the
>> continuation values into the cmd itself. In another patch of mine the
>> new way to do this has been by introducing an opaque variable into the
>> structure passed in by the user to be used for storing the
>> continuation value. Take a look at
>> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439
>> fcca47c493d902bbbb5ec17e
>> for an example.
>
> I think it was a mistake to allow this in - imo memop-s should be
> consistent in how they handle continuations. For new hypercalls
> (like dmop) the story is different of course.
So should I revert to a V3 that's basically V1 then? I've been
trigger-happy and submitted V2 roughly an hour ago based on Tamas'
suggestion.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
2017-03-09 10:19 ` Razvan Cojocaru
@ 2017-03-09 10:24 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2017-03-09 10:24 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel
Cc: Andrew Cooper, wei.liu2, Ian Jackson, Xen-devel
>>> On 09.03.17 at 11:19, <rcojocaru@bitdefender.com> wrote:
> So should I revert to a V3 that's basically V1 then? I've been
> trigger-happy and submitted V2 roughly an hour ago based on Tamas'
> suggestion.
Well, I'd suggest waiting for further opinions. Others may agree with
Tamas, or may have a 3rd opinion.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-09 10:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-08 9:01 [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi() Razvan Cojocaru
2017-03-08 18:30 ` Tamas K Lengyel
2017-03-08 19:19 ` Razvan Cojocaru
2017-03-08 19:53 ` Tamas K Lengyel
2017-03-08 20:17 ` Razvan Cojocaru
2017-03-08 20:53 ` Tamas K Lengyel
2017-03-08 21:36 ` Razvan Cojocaru
2017-03-08 22:03 ` Tamas K Lengyel
2017-03-09 10:16 ` Jan Beulich
2017-03-09 10:19 ` Razvan Cojocaru
2017-03-09 10:24 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.