|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V3 09/23] xen/dm: Make x86's DM feature common
Hi Jan I think, I can say that I have considered the alternative (doing it the other way around), of course if I got your suggestion for V2 correctly. Agree, the alternative is more natural, also compat_dm_op() was left in x86 code. For me the downside is in code duplication. With the alternative both arches have to duplicate do_dm_op() and "common" part of dm_op() (only "switch ( op.op )" is unique). Probably, do_dm_op() could be moved to common dm.c as well as dm_op() could become global...On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:From: Julien Grall <julien.grall@xxxxxxx> As a lot of x86 code can be re-used on Arm later on, this patch splits devicemodel support into common and arch specific parts. The common DM feature is supposed to be built with IOREQ_SERVER option enabled (as well as the IOREQ feature), which is selected for x86's config HVM for now. Also update XSM code a bit to let DM op be used on Arm. This support is going to be used on Arm to be able run device emulator outside of Xen hypervisor. Signed-off-by: Julien Grall <julien.grall@xxxxxxx> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> --- Please note, this is a split/cleanup/hardening of Julien's PoC: "Add support for Guest IO forwarding to a device emulator" Changes RFC -> V1: - update XSM, related changes were pulled from:[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM featuresChanges V1 -> V2: - update the author of a patch - update patch description - introduce xen/dm.h and move definitions here Changes V2 -> V3: - no changesAnd my concern regarding the common vs arch nesting also hasn't changed.I am sorry, I might misread your comment, but I failed to see any obvious to me request(s) for changes. I have just re-read previous discussion... So the question about considering doing it the other way around (top level dm-op handling arch-specific and call into e.g. ioreq_server_dm_op() for otherwise unhandled ops) is exactly a concern which I should have addressed?Well, on v2 you replied you didn't consider the alternative. I would have expected that you would at least go through this consideration process, and see whether there are better reasons to stick with the apparently backwards arrangement than to change to the more conventional one. If there are such reasons, I would expect them to be called out in reply and perhaps also in the commit message; the latter because down the road more people may wonder why the more narrow / special set of cases gets handled at a higher layer than the wider set of remaining ones, and they would then be able to find an explanation without having to resort to searching through list archives.Ah, will investigate. Sorry for not paying enough attention to it.Yes, IOREQ (I mean "common") ops are 7 out of 18 right now. The subsequent patch is adding one more DM op - XEN_DMOP_set_irq_level. There are several PCI related ops which might want to be common in the future (but I am not sure). Now the question is which approach to take ("current" or "alternative") for me to prepare for V4. Personally, I would be OK with the both (with a little preference for "alternative"). Also, If we decide to go with the alternative, should the common files still be named dm.*?
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d3e2a9e..dc8e47d 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -16,6 +16,7 @@
#include <xen/event.h>
#include <xen/guest_access.h>
+#include <xen/dm.h>
#include <xen/hypercall.h>
#include <xen/ioreq.h>
#include <xen/nospec.h>
@@ -29,13 +30,6 @@
#include <public/hvm/hvm_op.h>
-struct dmop_args {
- domid_t domid;
- unsigned int nr_bufs;
- /* Reserve enough buf elements for all current hypercalls. */
- struct xen_dm_op_buf buf[2];
-};
-
static bool _raw_copy_from_guest_buf_offset(void *dst,
const struct dmop_args *args,
unsigned int buf_idx,
@@ -408,71 +402,6 @@ static int dm_op(const struct dmop_args *op_args)
switch ( op.op )
{
- case XEN_DMOP_create_ioreq_server:
- {
- struct xen_dm_op_create_ioreq_server *data =
- &op.u.create_ioreq_server;
-
- const_op = false;
-
- rc = -EINVAL;
- if ( data->pad[0] || data->pad[1] || data->pad[2] )
- break;
-
- rc = hvm_create_ioreq_server(d, data->handle_bufioreq,
- &data->id);
- break;
- }
-
- case XEN_DMOP_get_ioreq_server_info:
- {
- 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->flags & ~valid_flags )
- break;
-
- rc = hvm_get_ioreq_server_info(d, data->id,
- (data->flags & XEN_DMOP_no_gfns) ?
- NULL : &data->ioreq_gfn,
- (data->flags & XEN_DMOP_no_gfns) ?
- NULL : &data->bufioreq_gfn,
- &data->bufioreq_port);
- break;
- }
-
- case XEN_DMOP_map_io_range_to_ioreq_server:
- {
- const struct xen_dm_op_ioreq_server_range *data =
- &op.u.map_io_range_to_ioreq_server;
-
- rc = -EINVAL;
- if ( data->pad )
- break;
-
- rc = hvm_map_io_range_to_ioreq_server(d, data->id, data->type,
- data->start, data->end);
- break;
- }
-
- case XEN_DMOP_unmap_io_range_from_ioreq_server:
- {
- const struct xen_dm_op_ioreq_server_range *data =
- &op.u.unmap_io_range_from_ioreq_server;
-
- rc = -EINVAL;
- if ( data->pad )
- break;
-
- rc = hvm_unmap_io_range_from_ioreq_server(d, data->id, data->type,
- data->start, data->end);
- break;
- }
-
case XEN_DMOP_map_mem_type_to_ioreq_server:
{
struct xen_dm_op_map_mem_type_to_ioreq_server *data =
@@ -523,32 +452,6 @@ static int dm_op(const struct dmop_args *op_args)
break;
}
- case XEN_DMOP_set_ioreq_server_state:
- {
- const struct xen_dm_op_set_ioreq_server_state *data =
- &op.u.set_ioreq_server_state;
-
- rc = -EINVAL;
- if ( data->pad )
- break;
-
- rc = hvm_set_ioreq_server_state(d, data->id, !!data->enabled);
- break;
- }
-
- case XEN_DMOP_destroy_ioreq_server:
- {
- const struct xen_dm_op_destroy_ioreq_server *data =
- &op.u.destroy_ioreq_server;
-
- rc = -EINVAL;
- if ( data->pad )
- break;
-
- rc = hvm_destroy_ioreq_server(d, data->id);
- break;
- }
-
case XEN_DMOP_track_dirty_vram:
{
const struct xen_dm_op_track_dirty_vram *data =
@@ -703,7 +606,7 @@ static int dm_op(const struct dmop_args *op_args)
}
default:
- rc = -EOPNOTSUPP;
+ rc = ioreq_server_dm_op(&op, d, &const_op);
break;
}
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b161381..8110431 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_CORE_PARKING) += core_parking.o
obj-y += cpu.o
obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
+obj-$(CONFIG_IOREQ_SERVER) += dm.o
obj-y += domain.o
obj-y += event_2l.o
obj-y += event_channel.o
diff --git a/xen/common/dm.c b/xen/common/dm.c
new file mode 100644
index 0000000..5653bcd
--- /dev/null
+++ b/xen/common/dm.c
@@ -0,0 +1,135 @@
+/*
+ * Copyright (c) 2016 Citrix Systems Inc.
+ * Copyright (c) 2019 Arm ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
License for
+ * more details. + *+ * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#include <xen/dm.h> +#include <xen/hypercall.h> +#include <xen/ioreq.h> ++int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op)
+{
+ long rc;
+
+ switch ( op->op )
+ {
+ case XEN_DMOP_create_ioreq_server:
+ {
+ struct xen_dm_op_create_ioreq_server *data =
+ &op->u.create_ioreq_server;
+
+ *const_op = false;
+
+ rc = -EINVAL;
+ if ( data->pad[0] || data->pad[1] || data->pad[2] )
+ break;
+
+ rc = hvm_create_ioreq_server(d, data->handle_bufioreq,
+ &data->id);
+ break;
+ }
+
+ case XEN_DMOP_get_ioreq_server_info:
+ {
+ 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->flags & ~valid_flags )
+ break;
+
+ rc = hvm_get_ioreq_server_info(d, data->id,
+ (data->flags & XEN_DMOP_no_gfns) ?
+ NULL : (unsigned long
*)&data->ioreq_gfn,
+ (data->flags & XEN_DMOP_no_gfns) ?+ NULL : (unsigned long *)&data->bufioreq_gfn,
+ &data->bufioreq_port);
+ break;
+ }
+
+ case XEN_DMOP_map_io_range_to_ioreq_server:
+ {
+ const struct xen_dm_op_ioreq_server_range *data =
+ &op->u.map_io_range_to_ioreq_server;
+
+ rc = -EINVAL;
+ if ( data->pad )
+ break;
+
+ rc = hvm_map_io_range_to_ioreq_server(d, data->id, data->type,
+ data->start, data->end);
+ break;
+ }
+
+ case XEN_DMOP_unmap_io_range_from_ioreq_server:
+ {
+ const struct xen_dm_op_ioreq_server_range *data =
+ &op->u.unmap_io_range_from_ioreq_server;
+
+ rc = -EINVAL;
+ if ( data->pad )
+ break;
+
+ rc = hvm_unmap_io_range_from_ioreq_server(d, data->id, data->type,
+ data->start, data->end);
+ break;
+ }
+
+ case XEN_DMOP_set_ioreq_server_state:
+ {
+ const struct xen_dm_op_set_ioreq_server_state *data =
+ &op->u.set_ioreq_server_state;
+
+ rc = -EINVAL;
+ if ( data->pad )
+ break;
+
+ rc = hvm_set_ioreq_server_state(d, data->id, !!data->enabled);
+ break;
+ }
+
+ case XEN_DMOP_destroy_ioreq_server:
+ {
+ const struct xen_dm_op_destroy_ioreq_server *data =
+ &op->u.destroy_ioreq_server;
+
+ rc = -EINVAL;
+ if ( data->pad )
+ break;
+
+ rc = hvm_destroy_ioreq_server(d, data->id);
+ break;
+ }
+
+ default:
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
+ return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/dm.h b/xen/include/xen/dm.h
new file mode 100644
index 0000000..8451f3c
--- /dev/null
+++ b/xen/include/xen/dm.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2016 Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
License for
+ * more details. + *+ * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef __XEN_DM_H__ +#define __XEN_DM_H__ + +#include <xen/sched.h> + +struct dmop_args { + domid_t domid; + unsigned int nr_bufs; + /* Reserve enough buf elements for all current hypercalls. */ + struct xen_dm_op_buf buf[2]; +}; ++int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op); + +#endif /* __XEN_DM_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 7ae3c40..5c61d8e 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h@@ -707,14 +707,14 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
}
}
+#endif /* CONFIG_X86 */
+
static XSM_INLINE int xsm_dm_op(XSM_DEFAULT_ARG struct domain *d)
{
XSM_ASSERT_ACTION(XSM_DM_PRIV);
return xsm_default_action(action, current->domain, d);
}
-#endif /* CONFIG_X86 */
-
#ifdef CONFIG_ARGO
static XSM_INLINE int xsm_argo_enable(const struct domain *d)
{
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 7bd03d8..91ecff4 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -176,8 +176,8 @@ struct xsm_operations {
int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t
e, uint8_t allow);
int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e,
uint8_t allow);
int (*pmu_op) (struct domain *d, unsigned int op); - int (*dm_op) (struct domain *d); #endif + int (*dm_op) (struct domain *d); int (*xen_version) (uint32_t cmd); int (*domain_resource_map) (struct domain *d); #ifdef CONFIG_ARGO@@ -682,13 +682,13 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
return xsm_ops->pmu_op(d, op);
}
+#endif /* CONFIG_X86 */
+
static inline int xsm_dm_op(xsm_default_t def, struct domain *d)
{
return xsm_ops->dm_op(d);
}
-#endif /* CONFIG_X86 */
-
static inline int xsm_xen_version (xsm_default_t def, uint32_t op)
{
return xsm_ops->xen_version(op);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 9e09512..8bdffe7 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -147,8 +147,8 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, ioport_permission);
set_to_dummy_if_null(ops, ioport_mapping);
set_to_dummy_if_null(ops, pmu_op);
- set_to_dummy_if_null(ops, dm_op);
#endif
+ set_to_dummy_if_null(ops, dm_op);
set_to_dummy_if_null(ops, xen_version);
set_to_dummy_if_null(ops, domain_resource_map);
#ifdef CONFIG_ARGO
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 19b0d9e..11784d7 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1656,14 +1656,13 @@ static int flask_pmu_op (struct domain *d,
unsigned int op)
return -EPERM;
}
}
+#endif /* CONFIG_X86 */
static int flask_dm_op(struct domain *d)
{
return current_has_perm(d, SECCLASS_HVM, HVM__DM);
}
-#endif /* CONFIG_X86 */
-
static int flask_xen_version (uint32_t op)
{
u32 dsid = domain_sid(current->domain);
@@ -1865,8 +1864,8 @@ static struct xsm_operations flask_ops = {
.ioport_permission = flask_ioport_permission,
.ioport_mapping = flask_ioport_mapping,
.pmu_op = flask_pmu_op,
- .dm_op = flask_dm_op,
#endif
+ .dm_op = flask_dm_op,
.xen_version = flask_xen_version,
.domain_resource_map = flask_domain_resource_map,
#ifdef CONFIG_ARGO
--
Regards,
Oleksandr Tyshchenko
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |