[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features
- To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: Julien Grall <julien@xxxxxxx>
- Date: Wed, 5 Aug 2020 15:12:20 +0100
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
- Delivery-date: Wed, 05 Aug 2020 14:12:49 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi,
On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.
Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.
Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone
wants to enable CONFIG_IOREQ_SERVER due to the lack of
cmpxchg_64 support on Arm32.
Please note, this is a split/cleanup of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
tools/libxc/xc_dom_arm.c | 25 +++++++---
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/Makefile | 2 +
xen/arch/arm/dm.c | 34 +++++++++++++
xen/arch/arm/domain.c | 9 ++++
xen/arch/arm/hvm.c | 46 +++++++++++++++++-
xen/arch/arm/io.c | 67 +++++++++++++++++++++++++-
xen/arch/arm/ioreq.c | 86 +++++++++++++++++++++++++++++++++
xen/arch/arm/traps.c | 17 +++++++
xen/common/memory.c | 5 +-
xen/include/asm-arm/domain.h | 80 +++++++++++++++++++++++++++++++
xen/include/asm-arm/hvm/ioreq.h | 103 ++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/mmio.h | 1 +
xen/include/asm-arm/p2m.h | 7 +--
xen/include/xsm/dummy.h | 4 +-
xen/include/xsm/xsm.h | 6 +--
xen/xsm/dummy.c | 2 +-
xen/xsm/flask/hooks.c | 5 +-
18 files changed, 476 insertions(+), 24 deletions(-)
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
It feels to me the patch is doing quite a few things that are indirectly
related. Can this be split to make the review easier?
I would like at least the following split from the series:
- The tools changes
- The P2M changes
- The HVMOP plumbing (if we still require them)
[...]
diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
new file mode 100644
index 0000000..2437099
--- /dev/null
+++ b/xen/arch/arm/dm.c
@@ -0,0 +1,34 @@
+/*
+ * 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/hypercall.h>
+#include <asm/vgic.h>
The list of includes sounds strange. Can we make sure to include only
necessary headers and add the others when they are required?
+
+int arch_dm_op(struct xen_dm_op *op, struct domain *d,
+ const struct dmop_args *op_args, bool *const_op)
+{
+ return -EOPNOTSUPP;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
{
long rc = 0;
@@ -111,7 +155,7 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) arg)
if ( rc )
goto param_fail;
- d->arch.hvm.params[a.index] = a.value;
+ rc = hvmop_set_param(d, &a);
}
else
{
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ae7ef96..436f669 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -16,6 +16,7 @@
* GNU General Public License for more details.
*/
+#include <xen/hvm/ioreq.h>
#include <xen/lib.h>
#include <xen/spinlock.h>
#include <xen/sched.h>
@@ -107,6 +108,62 @@ static const struct mmio_handler *find_mmio_handler(struct
domain *d,
return handler;
}
+#ifdef CONFIG_IOREQ_SERVER
Can we just implement this function in ioreq.c and provide a stub when
!CONFIG_IOREQ_SERVER?
+static enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
+ struct vcpu *v, mmio_info_t *info)
+{
+ struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
+ ioreq_t p = {
+ .type = IOREQ_TYPE_COPY,
+ .addr = info->gpa,
+ .size = 1 << info->dabt.size,
+ .count = 0,
+ .dir = !info->dabt.write,
+ .df = 0, /* XXX: What's for? */
+ .data = get_user_reg(regs, info->dabt.reg),
+ .state = STATE_IOREQ_READY,
+ };
+ struct hvm_ioreq_server *s = NULL;
+ enum io_state rc;
+
+ switch ( vio->io_req.state )
+ {
+ case STATE_IOREQ_NONE:
+ break;
+ default:
+ printk("d%u wrong state %u\n", v->domain->domain_id,
+ vio->io_req.state);
This will likely want to be a gprintk() or gdprintk() to avoid a guest
spamming Xen.
+ return IO_ABORT;
+ }
+
+ s = hvm_select_ioreq_server(v->domain, &p);
+ if ( !s )
+ return IO_UNHANDLED;
+
+ if ( !info->dabt.valid )
+ {
+ printk("Valid bit not set\n");
Same here. However, I am not convinced this is a useful message to keep.
+ return IO_ABORT;
+ }
+
+ vio->io_req = p;
+
+ rc = hvm_send_ioreq(s, &p, 0);
+ if ( rc != IO_RETRY || v->domain->is_shutting_down )
+ vio->io_req.state = STATE_IOREQ_NONE;
+ else if ( !hvm_ioreq_needs_completion(&vio->io_req) )
+ rc = IO_HANDLED;
+ else
+ vio->io_completion = HVMIO_mmio_completion;
+
+ /* XXX: Decide what to do */
We want to understand how IO_RETRY can happen on x86 first. With that,
we should be able to understand whether this can happen on Arm as well.
+ if ( rc == IO_RETRY )
+ rc = IO_HANDLED;
+
+ return rc;
+}
+#endif
+
enum io_state try_handle_mmio(struct cpu_user_regs *regs,
const union hsr hsr,
paddr_t gpa)
@@ -123,7 +180,15 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
handler = find_mmio_handler(v->domain, info.gpa);
if ( !handler )
- return IO_UNHANDLED;
+ {
+ int rc = IO_UNHANDLED;
+
+#ifdef CONFIG_IOREQ_SERVER
+ rc = try_fwd_ioserv(regs, v, &info);
+#endif
+
+ return rc;
+ }
/* All the instructions used on emulated MMIO region should be valid */
if ( !dabt.valid )
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
new file mode 100644
index 0000000..a9cc839
--- /dev/null
+++ b/xen/arch/arm/ioreq.c
@@ -0,0 +1,86 @@
+/*
+ * arm/ioreq.c: hardware virtual machine I/O emulation
+ *
+ * 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/ctype.h>
+#include <xen/hvm/ioreq.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/trace.h>
+#include <xen/sched.h>
+#include <xen/irq.h>
+#include <xen/softirq.h>
+#include <xen/domain.h>
+#include <xen/domain_page.h>
+#include <xen/event.h>
+#include <xen/paging.h>
+#include <xen/vpci.h>
+
+#include <public/hvm/dm_op.h>
+#include <public/hvm/ioreq.h>
+
+bool handle_mmio(void)
The name of the function is pretty generic and can be confusing on Arm
(we already have a try_handle_mmio()).
What is this function supposed to do?
+{
+ struct vcpu *v = current;
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ const union hsr hsr = { .bits = regs->hsr };
+ const struct hsr_dabt dabt = hsr.dabt;
+ /* Code is similar to handle_read */
+ uint8_t size = (1 << dabt.size) * 8;
+ register_t r = v->arch.hvm.hvm_io.io_req.data;
+
+ /* We should only be here on Guest Data Abort */
+ ASSERT(dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+
+ /* We are done with the IO */
+ /* XXX: Is it the right place? */
+ v->arch.hvm.hvm_io.io_req.state = STATE_IOREQ_NONE;
+
+ /* XXX: Do we need to take care of write here ? */
+ if ( dabt.write )
+ return true;
+
+ /*
+ * Sign extend if required.
+ * Note that we expect the read handler to have zeroed the bits
+ * outside the requested access size.
+ */
+ if ( dabt.sign && (r & (1UL << (size - 1))) )
+ {
+ /*
+ * We are relying on register_t using the same as
+ * an unsigned long in order to keep the 32-bit assembly
+ * code smaller.
+ */
+ BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
+ r |= (~0UL) << size;
+ }
+
+ set_user_reg(regs, dabt.reg, r);
+
+ return true;
+}
[...]
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9283e5e..0000477 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -8,6 +8,7 @@
*/
#include <xen/domain_page.h>
+#include <xen/hvm/ioreq.h>
#include <xen/types.h>
#include <xen/lib.h>
#include <xen/mm.h>
@@ -30,10 +31,6 @@
#include <public/memory.h>
#include <xsm/xsm.h>
-#ifdef CONFIG_IOREQ_SERVER
-#include <xen/hvm/ioreq.h>
-#endif
-
Why do you remove something your just introduced?
#ifdef CONFIG_X86
#include <asm/guest.h>
#endif
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4e2f582..e060b0a 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -11,12 +11,64 @@
#include <asm/vgic.h>
#include <asm/vpl011.h>
#include <public/hvm/params.h>
+#include <public/hvm/dm_op.h>
+#include <public/hvm/ioreq.h>
#include <xen/serial.h>
#include <xen/rbtree.h>
+struct hvm_ioreq_page {
+ gfn_t gfn;
+ struct page_info *page;
+ void *va;
+};
AFAICT all the structures/define you introduced here are used by the
code common. So it feels to me they should be defined in a common header.
+
+struct hvm_ioreq_vcpu {
+ struct list_head list_entry;
+ struct vcpu *vcpu;
+ evtchn_port_t ioreq_evtchn;
+ bool pending;
+};
+
+#define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_PCI + 1)
+#define MAX_NR_IO_RANGES 256
+
+#define MAX_NR_IOREQ_SERVERS 8
+#define DEFAULT_IOSERVID 0
+
+struct hvm_ioreq_server {
+ struct domain *target, *emulator;
+
+ /* Lock to serialize toolstack modifications */
+ spinlock_t lock;
+
+ struct hvm_ioreq_page ioreq;
+ struct list_head ioreq_vcpu_list;
+ struct hvm_ioreq_page bufioreq;
+
+ /* Lock to serialize access to buffered ioreq ring */
+ spinlock_t bufioreq_lock;
+ evtchn_port_t bufioreq_evtchn;
+ struct rangeset *range[NR_IO_RANGE_TYPES];
+ bool enabled;
+ uint8_t bufioreq_handling;
+};
+
struct hvm_domain
{
uint64_t params[HVM_NR_PARAMS];
+
+ /* Guest page range used for non-default ioreq servers */
+ struct {
+ unsigned long base;
+ unsigned long mask;
+ unsigned long legacy_mask; /* indexed by HVM param number */
+ } ioreq_gfn;
+
+ /* Lock protects all other values in the sub-struct and the default */
+ struct {
+ spinlock_t lock;
+ struct hvm_ioreq_server *server[MAX_NR_IOREQ_SERVERS];
+ } ioreq_server;
};
#ifdef CONFIG_ARM_64
@@ -93,6 +145,29 @@ struct arch_domain
#endif
} __cacheline_aligned;
+enum hvm_io_completion {
+ HVMIO_no_completion,
+ HVMIO_mmio_completion,
+ HVMIO_pio_completion,
+ HVMIO_realmode_completion
+};
+
+struct hvm_vcpu_io {
+ /* I/O request in flight to device model. */
+ enum hvm_io_completion io_completion;
+ ioreq_t io_req;
+
+ /*
+ * HVM emulation:
+ * Linear address @mmio_gla maps to MMIO physical frame @mmio_gpfn.
+ * The latter is known to be an MMIO frame (not RAM).
+ * This translation is only valid for accesses as per @mmio_access.
+ */
+ struct npfec mmio_access;
+ unsigned long mmio_gla;
+ unsigned long mmio_gpfn;
+};
+
struct arch_vcpu
{
struct {
@@ -206,6 +281,11 @@ struct arch_vcpu
*/
bool need_flush_to_ram;
+ struct hvm_vcpu
+ {
+ struct hvm_vcpu_io hvm_io;
+ } hvm;
+
} __cacheline_aligned;
void vcpu_show_execution_state(struct vcpu *);
diff --git a/xen/include/asm-arm/hvm/ioreq.h b/xen/include/asm-arm/hvm/ioreq.h
new file mode 100644
index 0000000..83a560c
--- /dev/null
+++ b/xen/include/asm-arm/hvm/ioreq.h
@@ -0,0 +1,103 @@
+/*
+ * hvm.h: Hardware virtual machine assist interface definitions.
+ *
+ * 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/>.
+ */
+
+#ifndef __ASM_ARM_HVM_IOREQ_H__
+#define __ASM_ARM_HVM_IOREQ_H__
+
+#include <public/hvm/ioreq.h>
+#include <public/hvm/dm_op.h>
+
+#define has_vpci(d) (false)
It feels to me this wants to be defined in vcpi.h.
+
+bool handle_mmio(void);
+
+static inline bool handle_pio(uint16_t port, unsigned int size, int dir)
+{
+ /* XXX */
Can you expand this TODO? What do you expect to do?
+ BUG();
+ return true;
+}
+
+static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
+{
+ return p->addr;
+}
I understand that the x86 version is more complex as it check p->df.
However, aside reducing the complexity, I am not sure why we would want
to diverge it.
After all, IOREQ is now meant to be a common feature.
+
+static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p)
+{
+ unsigned long size = p->size;
+
+ return p->addr + size - 1;
+}
Same.
+
+struct hvm_ioreq_server;
Why do we need a forward declaration?
+
+static inline int p2m_set_ioreq_server(struct domain *d,
+ unsigned int flags,
+ struct hvm_ioreq_server *s)
+{
+ return -EOPNOTSUPP;
+}
This should be defined in p2m.h. But I am not even sure what it is meant
for. Can you expand it?
+
+static inline void msix_write_completion(struct vcpu *v)
+{
+}
+
+static inline void handle_realmode_completion(void)
+{
+ ASSERT_UNREACHABLE();
+}
realmode is very x86 specific. So I don't think this function should be
called from common code. It might be worth considering to split
handle_hvm_io_completion() is 2 parts: common and arch specific.
+
+static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn)
+{
+}
This will want to be stubbed in asm-arm/paging.h.
+
+static inline void hvm_get_ioreq_server_range_type(struct domain *d,
+ ioreq_t *p,
+ uint8_t *type,
+ uint64_t *addr)
+{
+ *type = (p->type == IOREQ_TYPE_PIO) ?
+ XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
+ *addr = p->addr;
+}
+
+static inline void arch_hvm_ioreq_init(struct domain *d)
+{
+}
+
+static inline void arch_hvm_ioreq_destroy(struct domain *d)
+{
+}
+
+#define IOREQ_IO_HANDLED IO_HANDLED
+#define IOREQ_IO_UNHANDLED IO_UNHANDLED
+#define IOREQ_IO_RETRY IO_RETRY
+
+#endif /* __ASM_X86_HVM_IOREQ_H__ */
s/X86/ARM/
+
+/*
+ * 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/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index 8dbfb27..7ab873c 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -37,6 +37,7 @@ enum io_state
IO_ABORT, /* The IO was handled by the helper and led to an abort.
*/
IO_HANDLED, /* The IO was successfully handled by the helper. */
IO_UNHANDLED, /* The IO was not handled by the helper. */
+ IO_RETRY, /* Retry the emulation for some reason */
};
typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5fdb6e8..5823f11 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct domain *d,
unsigned long gfn,
mfn_t mfn)
{
/*
- * NOTE: If this is implemented then proper reference counting of
- * foreign entries will need to be implemented.
+ * XXX: handle properly reference. It looks like the page may not always
+ * belong to d.
*/
- return -EOPNOTSUPP;
+
+ return guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_ram_rw);
Treating foreign as p2m_ram_rw is more an hack that a real fix. I have
answered to this separately (see my answer on Stefano's e-mail), so we
can continue the conversation there.
Cheers,
--
Julien Grall
|
