Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 13.10.17 at 15:00, wrote: > On Vi, 2017-10-13 at 03:51 -0600, Jan Beulich wrote: >> > >> > >> > +BUILD_BUG_ON(sizeof(struct >> > xen_hvm_altp2m_set_mem_access_multi) < >> > + sizeof(struct >> > compat_hvm_altp2m_set_mem_access_multi)); >> What good does this do? >> Sorry, I don't understand how these size checks should look (are we >> testing that the left hand side is at least as wide as the right hand >> side?). Could you please give an example? Thanks! > >> > + */ >> > +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; >> There are _still_ no size checks here. > I'm not sure I understand what kind of size checks should be used here. > Are you expecting something similar with the CHECK_FIELD_ macro? Yes, as I've been stating repeatedly. You want to demonstrate that field sizes match, or that at least no truncation can occur. >> > >> > +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0 >> > ) >> Please also check rc here to avoid needlessly copying back. In >> fact _only_ checking rc ought to be fine. > Actually, in this case rc is overwritten in do_altp2m_op (-EFAULT if > the copy didn't work or the result of hypercall_create_continuation > otherwise). And it's the result of hypercall_create_continuation() which you want to check against (provided there's no other way for rc to obtain a positive value). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Vi, 2017-10-13 at 03:51 -0600, Jan Beulich wrote: > > > > > > +BUILD_BUG_ON(sizeof(struct > > xen_hvm_altp2m_set_mem_access_multi) < > > + sizeof(struct > > compat_hvm_altp2m_set_mem_access_multi)); > What good does this do? > Sorry, I don't understand how these size checks should look (are we > testing that the left hand side is at least as wide as the right hand > side?). Could you please give an example? Thanks! > > + */ > > +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; > There are _still_ no size checks here. I'm not sure I understand what kind of size checks should be used here. Are you expecting something similar with the CHECK_FIELD_ macro? > > > > +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0 > > ) > Please also check rc here to avoid needlessly copying back. In > fact _only_ checking rc ought to be fine. Actually, in this case rc is overwritten in do_altp2m_op (-EFAULT if the copy didn't work or the result of hypercall_create_continuation otherwise). I have used the check for nat.altp2m_op->u.set_mem_access_multi.opaque as it usually gets set to a positive value only if the hypercall gets preempted, Many thanks for your support, Petre > > > > > Jan > > > > This email was scanned by Bitdefender ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 11.10.17 at 18:26, wrote:
> @@ -4568,6 +4571,32 @@ 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 ||
> + a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +{
> +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,
> + a.u.set_mem_access_multi.opaque,
> + 0x3F,
Please add /* pretty arbitrary */ or something similar here.
> @@ -4586,6 +4615,80 @@ static int do_altp2m_op(
> return rc;
> }
>
> +DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> +
> +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(&a, 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:
Indentation.
> +BUILD_BUG_ON(sizeof(struct xen_hvm_altp2m_set_mem_access_multi) <
> + sizeof(struct
> compat_hvm_altp2m_set_mem_access_multi));
What good does this do?
> +#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(&nat.altp2m_op->u.set_mem_access_multi,
> +&a.u.set_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.
> + */
> +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;
There are _still_ no size checks here.
> +rc = do_altp2m_op(nat.hnd);
> +
> +switch ( a.cmd )
> +{
> +case HVMOP_altp2m_set_mem_access_multi:
Indentation.
> +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0 )
Please also check rc here to avoid needlessly copying back. In
fact _only_ checking rc ought to be fine.
> +{
> +a.u.set_mem_access_multi.opaque =
> +nat.altp2m_op->u.set_mem_access_multi.opaque;
You also need a size check here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
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 0x3Fa.
* 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.
---
tools/libxc/include/xenctrl.h | 3 ++
tools/libxc/xc_altp2m.c | 41
xen/arch/x86/hvm/hvm.c | 105 +++-
xen/include/Makefile| 1 +
xen/include/public/hvm/hvm_op.h | 36 --
xen/include/xlat.lst| 1 +
6 files changed, 181 insertions(+), 6 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bcab3c..4e2ce64 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1971,6 +1971,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 -
