Re: [Xen-devel] [PATCH v6] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 13.10.17 at 22:42,  wrote:
> @@ -4586,6 +4620,107 @@ static int do_altp2m_op(
>  return rc;
>  }
>  
> +DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> +
> +#ifndef CHECK_hvm_altp2m_op
> +#define CHECK_hvm_altp2m_op \
> +CHECK_SIZE_(struct, hvm_altp2m_op); \
> +CHECK_FIELD_(struct, hvm_altp2m_op, version); \
> +CHECK_FIELD_(struct, hvm_altp2m_op, cmd); \
> +CHECK_FIELD_(struct, hvm_altp2m_op, domain); \
> +CHECK_FIELD_(struct, hvm_altp2m_op, pad1); \
> +CHECK_FIELD_(struct, hvm_altp2m_op, pad2);
> +#endif /* CHECK_hvm_altp2m_op */
> +
> +#ifndef CHECK_hvm_altp2m_set_mem_access_multi
> +#define CHECK_hvm_altp2m_set_mem_access_multi \
> +CHECK_SIZE_(struct, hvm_altp2m_set_mem_access_multi); \
> +CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, view); \
> +CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, pad); \
> +CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, nr); \
> +CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, opaque);
> +#endif /* CHECK_hvm_altp2m_set_mem_access_multi */

Please omit the trailing semicolons in both cases, just like the
generated macros don't have them. They're ...

> +CHECK_hvm_altp2m_op;
> +CHECK_hvm_altp2m_set_mem_access_multi;

... redundant with the ones here.

> +static int compat_altp2m_op(
> +XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +int rc = 0;
> +struct compat_hvm_altp2m_op a;
> +union
> +{
> +XEN_GUEST_HANDLE_PARAM(void) hnd;
> +struct xen_hvm_altp2m_op *altp2m_op;
> +} nat;
> +
> +if ( !hvm_altp2m_supported() )
> +return -EOPNOTSUPP;
> +
> +if ( copy_from_guest(, arg, 1) )
> +return -EFAULT;
> +
> +if ( a.pad1 || a.pad2 ||
> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +return -EINVAL;
> +
> +set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +switch ( a.cmd )
> +{
> +case HVMOP_altp2m_set_mem_access_multi:
> +BUILD_BUG_ON(sizeof(struct xen_hvm_altp2m_set_mem_access_multi) <
> + sizeof(struct compat_hvm_altp2m_set_mem_access_multi));

With the checking macros above I would hope this isn't needed
anymore.

> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +
> XLAT_hvm_altp2m_set_mem_access_multi(_op->u.set_mem_access_multi,
> + _mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +break;
> +default:
> +return do_altp2m_op(arg);
> +}
> +
> +/*
> + * Manually fill the common part of the xen_hvm_altp2m_op structure 
> because
> + * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the
> + * translation of all fields from compat_hvm_altp2m_op to 
> xen_hvm_altp2m_op.
> + */

I think a variant of this comment would now better be placed ahead
of the CHECK_hvm_altp2m_* macro definitions above, with the one
here becoming brief by simply referring to the larger one.

> +nat.altp2m_op->version  = a.version;
> +nat.altp2m_op->cmd  = a.cmd;
> +nat.altp2m_op->domain   = a.domain;
> +nat.altp2m_op->pad1 = a.pad1;
> +nat.altp2m_op->pad2 = a.pad2;
> +
> +rc = do_altp2m_op(nat.hnd);
> +
> +switch ( a.cmd )
> +{
> +case HVMOP_altp2m_set_mem_access_multi:
> +/*
> + * The return code can be positive only if it is the return value
> + * of hypercall_create_continuation. In this case, the opaque value
> + * must be copied back to the guest.
> + */
> +if ( rc > 0 )
> +{

ASSERT(rc == ...);

> +a.u.set_mem_access_multi.opaque =
> +nat.altp2m_op->u.set_mem_access_multi.opaque;
> +if ( __copy_field_to_guest(guest_handle_cast(arg,
> + 
> compat_hvm_altp2m_op_t),
> +   , u.set_mem_access_multi.opaque) )
> +rc = -EFAULT;
> +}
> +break;

default:
ASSERT_UNREACHABLE();

Jan

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

[Xen-devel] [PATCH v6] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[petre . pircalabu]

From: Razvan Cojocaru 

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.

HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
hence with the original altp2m design, where domains are allowed - with the
proper altp2m access rights - to alter these settings), in the absence of an
official position on the issue from the original altp2m designers.

Signed-off-by: Razvan Cojocaru 
Signed-off-by: Petre Pircalabu 

---

Changed since v2:
* Added support for compat arguments translation

Changed since v3:
* Replaced  __copy_to_guest with __copy_field_to_guest
* Removed the un-needed parentheses.
* Fixed xlat.lst ordering
* Added comment to patch description explaining why the
functionality was added as an HVMOP.
* Guard using XEN_GENERATING_COMPAT_HEADERS the hvmmem_type_t definition.
This will prevent suplicate definitions to be generated for the
compat equivalent.
* Added comment describing the manual translation of
xen_hvm_altp2m_op_t generic fields from compat_hvm_altp2m_op_t.

Changed since v4:
* Changed the mask parameter to 0x3F.
* Split long lines.
* Added "improperly named HVMMEM_(*)" to the comment explaining the
XEN_GENERATING_COMPAT_HEADERS guard.
* Removed typedef and XEN_GUEST_HANDLE for 
xen_hvm_altp2m_set_mem_access_multi.
* Copied the "opaque" field to guest in compat_altp2m_op.
* Added build time test to check if the size of
xen_hvm_altp2m_set_mem_access_multi at least equal to the size of
compat_hvm_altp2m_set_mem_access_multi.

Changed since v5:
* Changed the domid parameter type to uint32_t to match 5b42c82f.
* Added comment to explain why the 0x3F value was chosen.
* Fixed switch indentation in compat_altp2m_op.
* Changed the condition used to check if the opaque field has to
be copied to the guest.
* Added CHECK_hvm_altp2m_op and CHECK_hvm_altp2m_set_mem_access_multi.
---
 tools/libxc/include/xenctrl.h   |   3 +
 tools/libxc/xc_altp2m.c |  40 
 xen/arch/x86/hvm/hvm.c  | 137 +++-
 xen/include/Makefile|   1 +
 xen/include/public/hvm/hvm_op.h |  36 +--
 xen/include/xlat.lst|   1 +
 6 files changed, 212 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 666db0b919..f17166852d 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
uint32_t domid,
 int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
  uint16_t view_id, xen_pfn_t old_gfn,
  xen_pfn_t new_gfn);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
+   uint16_t view_id, uint8_t *access,
+   uint64_t *pages, uint32_t nr);
 
 /** 
  * Mem paging operations.
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 07fcd18326..e170fe390d 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -213,3 +213,43 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t 
domid,
 return rc;
 }
 
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_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));
+
+