[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
On 20.01.21 02:23, Stefano Stabellini wrote:
Hi Stefano
On Sun, 17 Jan 2021, Oleksandr wrote:
On 15.01.21 02:55, Stefano Stabellini wrote:
On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote:
From: Julien Grall <julien.grall@xxxxxxx>
This patch adds basic IOREQ/DM support on Arm. The subsequent
patches will improve functionality and add remaining bits.
The IOREQ/DM features are supposed to be built with IOREQ_SERVER
option enabled, which is disabled by default on Arm for now.
Please note, the "PIO handling" TODO is expected to left unaddressed
for the current series. It is not an big issue for now while Xen
doesn't have support for vPCI on Arm. On Arm64 they are only used
for PCI IO Bar and we would probably want to expose them to emulator
as PIO access to make a DM completely arch-agnostic. So "PIO handling"
should be implemented when we add support for vPCI.
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
[On Arm only]
Tested-by: Wei Chen <Wei.Chen@xxxxxxx>
---
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:
- was split into:
- arm/ioreq: Introduce arch specific bits for IOREQ/DM features
- xen/mm: Handle properly reference in set_foreign_p2m_entry() on
Arm
- update patch description
- update asm-arm/hvm/ioreq.h according to the newly introduced arch
functions:
- arch_hvm_destroy_ioreq_server()
- arch_handle_hvm_io_completion()
- update arch files to include xen/ioreq.h
- remove HVMOP plumbing
- rewrite a logic to handle properly case when hvm_send_ioreq()
returns IO_RETRY
- add a logic to handle properly handle_hvm_io_completion() return
value
- rename handle_mmio() to ioreq_handle_complete_mmio()
- move paging_mark_pfn_dirty() to asm-arm/paging.h
- remove forward declaration for hvm_ioreq_server in asm-arm/paging.h
- move try_fwd_ioserv() to ioreq.c, provide stubs if
!CONFIG_IOREQ_SERVER
- do not remove #ifdef CONFIG_IOREQ_SERVER in memory.c for guarding
xen/ioreq.h
- use gdprintk in try_fwd_ioserv(), remove unneeded prints
- update list of #include-s
- move has_vpci() to asm-arm/domain.h
- add a comment (TODO) to unimplemented yet handle_pio()
- remove hvm_mmio_first(last)_byte() and hvm_ioreq_(page/vcpu/server)
structs
from the arch files, they were already moved to the common code
- remove set_foreign_p2m_entry() changes, they will be properly
implemented
in the follow-up patch
- select IOREQ_SERVER for Arm instead of Arm64 in Kconfig
- remove x86's realmode and other unneeded stubs from xen/ioreq.h
- clafify ioreq_t p.df usage in try_fwd_ioserv()
- set ioreq_t p.count to 1 in try_fwd_ioserv()
Changes V1 -> V2:
- was split into:
- arm/ioreq: Introduce arch specific bits for IOREQ/DM features
- xen/arm: Stick around in leave_hypervisor_to_guest until I/O has
completed
- update the author of a patch
- update patch description
- move a loop in leave_hypervisor_to_guest() to a separate patch
- set IOREQ_SERVER disabled by default
- remove already clarified /* XXX */
- replace BUG() by ASSERT_UNREACHABLE() in handle_pio()
- remove default case for handling the return value of
try_handle_mmio()
- remove struct hvm_domain, enum hvm_io_completion, struct
hvm_vcpu_io,
struct hvm_vcpu from asm-arm/domain.h, these are common materials
now
- update everything according to the recent changes (IOREQ related
function
names don't contain "hvm" prefixes/infixes anymore, IOREQ related
fields
are part of common struct vcpu/domain now, etc)
Changes V2 -> V3:
- update patch according the "legacy interface" is x86 specific
- add dummy arch hooks
- remove dummy paging_mark_pfn_dirty()
- don’t include <xen/domain_page.h> in common ioreq.c
- don’t include <public/hvm/ioreq.h> in arch ioreq.h
- remove #define ioreq_params(d, i)
Changes V3 -> V4:
- rebase
- update patch according to the renaming IO_ -> VIO_ (io_ -> vio_)
and misc changes to arch hooks
- update patch according to the IOREQ related dm-op handling changes
- don't include <xen/ioreq.h> from arch header
- make all arch hooks out-of-line
- add a comment above IOREQ_STATUS_* #define-s
---
xen/arch/arm/Makefile | 2 +
xen/arch/arm/dm.c | 122 +++++++++++++++++++++++
xen/arch/arm/domain.c | 9 ++
xen/arch/arm/io.c | 12 ++-
xen/arch/arm/ioreq.c | 213
++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/traps.c | 13 +++
xen/include/asm-arm/domain.h | 3 +
xen/include/asm-arm/hvm/ioreq.h | 72 ++++++++++++++
xen/include/asm-arm/mmio.h | 1 +
9 files changed, 446 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/arm/dm.c
create mode 100644 xen/arch/arm/ioreq.c
create mode 100644 xen/include/asm-arm/hvm/ioreq.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 512ffdd..16e6523 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -13,6 +13,7 @@ obj-y += cpuerrata.o
obj-y += cpufeature.o
obj-y += decode.o
obj-y += device.o
+obj-$(CONFIG_IOREQ_SERVER) += dm.o
obj-y += domain.o
obj-y += domain_build.init.o
obj-y += domctl.o
@@ -27,6 +28,7 @@ obj-y += guest_atomics.o
obj-y += guest_walk.o
obj-y += hvm.o
obj-y += io.o
+obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
obj-y += irq.o
obj-y += kernel.init.o
obj-$(CONFIG_LIVEPATCH) += livepatch.o
diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
new file mode 100644
index 0000000..e6dedf4
--- /dev/null
+++ b/xen/arch/arm/dm.c
@@ -0,0 +1,122 @@
+/*
+ * 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/guest_access.h>
+#include <xen/hypercall.h>
+#include <xen/ioreq.h>
+#include <xen/nospec.h>
+
+static int dm_op(const struct dmop_args *op_args)
+{
+ struct domain *d;
+ struct xen_dm_op op;
+ bool const_op = true;
+ long rc;
+ size_t offset;
+
+ static const uint8_t op_size[] = {
+ [XEN_DMOP_create_ioreq_server] = sizeof(struct
xen_dm_op_create_ioreq_server),
+ [XEN_DMOP_get_ioreq_server_info] = sizeof(struct
xen_dm_op_get_ioreq_server_info),
+ [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct
xen_dm_op_ioreq_server_range),
+ [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct
xen_dm_op_ioreq_server_range),
+ [XEN_DMOP_set_ioreq_server_state] = sizeof(struct
xen_dm_op_set_ioreq_server_state),
+ [XEN_DMOP_destroy_ioreq_server] = sizeof(struct
xen_dm_op_destroy_ioreq_server),
+ };
+
+ rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
+ if ( rc )
+ return rc;
+
+ rc = xsm_dm_op(XSM_DM_PRIV, d);
+ if ( rc )
+ goto out;
+
+ offset = offsetof(struct xen_dm_op, u);
+
+ rc = -EFAULT;
+ if ( op_args->buf[0].size < offset )
+ goto out;
+
+ if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0,
offset) )
+ goto out;
+
+ if ( op.op >= ARRAY_SIZE(op_size) )
+ {
+ rc = -EOPNOTSUPP;
+ goto out;
+ }
+
+ op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size));
+
+ if ( op_args->buf[0].size < offset + op_size[op.op] )
+ goto out;
+
+ if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset,
+ op_size[op.op]) )
+ goto out;
+
+ rc = -EINVAL;
+ if ( op.pad )
+ goto out;
+
+ rc = ioreq_server_dm_op(&op, d, &const_op);
+
+ if ( (!rc || rc == -ERESTART) &&
+ !const_op && copy_to_guest_offset(op_args->buf[0].h, offset,
+ (void *)&op.u, op_size[op.op])
)
+ rc = -EFAULT;
+
+ out:
+ rcu_unlock_domain(d);
+
+ return rc;
+}
+
+long do_dm_op(domid_t domid,
+ unsigned int nr_bufs,
+ XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
+{
+ struct dmop_args args;
+ int rc;
+
+ if ( nr_bufs > ARRAY_SIZE(args.buf) )
+ return -E2BIG;
+
+ args.domid = domid;
+ args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1);
+
+ if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) )
+ return -EFAULT;
+
+ rc = dm_op(&args);
+
+ if ( rc == -ERESTART )
+ rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
+ domid, nr_bufs, bufs);
+
+ return rc;
+}
I might have missed something in the discussions but this function is
identical to xen/arch/x86/hvm/dm.c:do_dm_op, why not make it common?
Also the previous function dm_op is very similar to
xen/arch/x86/hvm/dm.c:dm_op I would prefer to make them common if
possible. Was this already discussed?
Well, let me explain. Both dm_op() and do_dm_op() were indeed common (top
level dm-op handling common) for previous versions, so Arm's dm.c didn't
contain this stuff.
The idea to make it other way around (top level dm-op handling arch-specific
and call into ioreq_server_dm_op() for otherwise unhandled ops) was discussed
at [1] which besides
it's Pros leads to code duplication, so Arm's dm.c has to duplicate some
stuff, etc.
I was thinking about moving do_dm_op() which is _same_ for both arches to
common code, but I am not sure whether it is conceptually correct which that
new "alternative" approach of handling dm-op.
Yes, I think it makes sense to make do_dm_op common because it is
identical. That should be easy.
Absolutely identical) Agree, technically it is not hard. Well, let's
continue discussion in [1]
(which actually leads to the duplication) to see if all parties are
happy with that.
I realize that the common part of dm_op is the initial boilerplate which
is similar for every hypercall, so I think it is also OK if we don't
share it and leave it as it is in this version of the series.
ok
[1]
https://lore.kernel.org/xen-devel/1610488352-18494-10-git-send-email-olekstysh@xxxxxxxxx/
--
Regards,
Oleksandr Tyshchenko
|
