Mailing List Archive

[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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@arm.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
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

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 931404c..b5fc066 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,11 +26,19 @@
#include "xg_private.h"
#include "xc_dom.h"

-#define NR_MAGIC_PAGES 4
+
#define CONSOLE_PFN_OFFSET 0
#define XENSTORE_PFN_OFFSET 1
#define MEMACCESS_PFN_OFFSET 2
#define VUART_PFN_OFFSET 3
+#define IOREQ_SERVER_PFN_OFFSET 4
+
+#define NR_IOREQ_SERVER_PAGES 8
+#define NR_MAGIC_PAGES (4 + NR_IOREQ_SERVER_PAGES)
+
+#define GUEST_MAGIC_BASE_PFN (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT)
+
+#define special_pfn(x) (GUEST_MAGIC_BASE_PFN + (x))

#define LPAE_SHIFT 9

@@ -51,7 +59,7 @@ const char *xc_domain_get_native_protocol(xc_interface *xch,
static int alloc_magic_pages(struct xc_dom_image *dom)
{
int rc, i;
- const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+ const xen_pfn_t base = special_pfn(0);
xen_pfn_t p2m[NR_MAGIC_PAGES];

BUILD_BUG_ON(NR_MAGIC_PAGES > GUEST_MAGIC_SIZE >> XC_PAGE_SHIFT);
@@ -71,10 +79,9 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
dom->vuart_gfn = base + VUART_PFN_OFFSET;

- xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
- xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
- xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
- xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
+ /* XXX: Check return */
+ xc_clear_domain_pages(dom->xch, dom->guest_domid, special_pfn(0),
+ NR_MAGIC_PAGES);

xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
dom->console_pfn);
@@ -88,6 +95,12 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
dom->xenstore_evtchn);

+ /* Tell the domain where the pages are and how many there are. */
+ xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_IOREQ_SERVER_PFN,
+ special_pfn(IOREQ_SERVER_PFN_OFFSET));
+ xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_NR_IOREQ_SERVER_PAGES,
+ NR_IOREQ_SERVER_PAGES);
+
return 0;
}

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2777388..6b8a969 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -13,6 +13,7 @@ config ARM_64
def_bool y
depends on 64BIT
select HAS_FAST_MULTIPLY
+ select IOREQ_SERVER

config ARM
def_bool y
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7e82b21..617fa3e 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..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>
+
+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:
+ */
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 3116932..658eec0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -12,6 +12,7 @@
#include <xen/bitops.h>
#include <xen/errno.h>
#include <xen/grant_table.h>
+#include <xen/hvm/ioreq.h>
#include <xen/hypercall.h>
#include <xen/init.h>
#include <xen/lib.h>
@@ -681,6 +682,10 @@ int arch_domain_create(struct domain *d,

ASSERT(config != NULL);

+#ifdef CONFIG_IOREQ_SERVER
+ hvm_ioreq_init(d);
+#endif
+
/* p2m_init relies on some value initialized by the IOMMU subsystem */
if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
@@ -999,6 +1004,10 @@ int domain_relinquish_resources(struct domain *d)
if (ret )
return ret;

+#ifdef CONFIG_IOREQ_SERVER
+ hvm_destroy_all_ioreq_servers(d);
+#endif
+
PROGRESS(xen):
ret = relinquish_memory(d, &d->xenpage_list);
if ( ret )
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 8951b34..0379493 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -51,6 +51,14 @@ static int hvm_allow_set_param(const struct domain *d, unsigned int param)
case HVM_PARAM_MONITOR_RING_PFN:
return d == current->domain ? -EPERM : 0;

+ /*
+ * XXX: Do we need to follow x86's logic here:
+ * "The following parameters should only be changed once"?
+ */
+ case HVM_PARAM_IOREQ_SERVER_PFN:
+ case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+ return 0;
+
/* Writeable only by Xen, hole, deprecated, or out-of-range. */
default:
return -EINVAL;
@@ -69,6 +77,11 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param)
case HVM_PARAM_CONSOLE_EVTCHN:
return 0;

+ /* XXX: Could these be read by someone? What policy to apply? */
+ case HVM_PARAM_IOREQ_SERVER_PFN:
+ case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+ return 0;
+
/*
* The following parameters are intended for toolstack usage only.
* They may not be read by the domain.
@@ -82,6 +95,37 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param)
}
}

+static int hvmop_set_param(struct domain *d, const struct xen_hvm_param *a)
+{
+ int rc = 0;
+
+ switch ( a->index )
+ {
+ case HVM_PARAM_IOREQ_SERVER_PFN:
+ d->arch.hvm.ioreq_gfn.base = a->value;
+ break;
+ case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+ {
+ unsigned int i;
+
+ if ( a->value == 0 ||
+ a->value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 )
+ {
+ rc = -EINVAL;
+ break;
+ }
+ for ( i = 0; i < a->value; i++ )
+ set_bit(i, &d->arch.hvm.ioreq_gfn.mask);
+
+ break;
+ }
+ }
+
+ d->arch.hvm.params[a->index] = a->value;
+
+ return rc;
+}
+
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
+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);
+ 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");
+ 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 */
+ 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)
+{
+ 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;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8f40d0e..4cdf098 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -18,6 +18,7 @@

#include <xen/domain_page.h>
#include <xen/errno.h>
+#include <xen/hvm/ioreq.h>
#include <xen/hypercall.h>
#include <xen/init.h>
#include <xen/iocap.h>
@@ -1384,6 +1385,9 @@ static arm_hypercall_t arm_hypercall_table[] = {
#ifdef CONFIG_HYPFS
HYPERCALL(hypfs_op, 5),
#endif
+#ifdef CONFIG_IOREQ_SERVER
+ HYPERCALL(dm_op, 3),
+#endif
};

#ifndef NDEBUG
@@ -1958,6 +1962,9 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
case IO_UNHANDLED:
/* IO unhandled, try another way to handle it. */
break;
+ default:
+ /* XXX: Handle IO_RETRY */
+ ASSERT_UNREACHABLE();
}
}

@@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
*/
void leave_hypervisor_to_guest(void)
{
+#ifdef CONFIG_IOREQ_SERVER
+ /*
+ * XXX: Check the return. Shall we call that in
+ * continue_running and context_switch instead?
+ * The benefits would be to avoid calling
+ * handle_hvm_io_completion on every return.
+ */
+ local_irq_enable();
+ handle_hvm_io_completion(current);
+#endif
local_irq_disable();

check_for_vcpu_work();
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
-
#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;
+};
+
+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)
+
+bool handle_mmio(void);
+
+static inline bool handle_pio(uint16_t port, unsigned int size, int dir)
+{
+ /* XXX */
+ BUG();
+ return true;
+}
+
+static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
+{
+ return p->addr;
+}
+
+static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p)
+{
+ unsigned long size = p->size;
+
+ return p->addr + size - 1;
+}
+
+struct hvm_ioreq_server;
+
+static inline int p2m_set_ioreq_server(struct domain *d,
+ unsigned int flags,
+ struct hvm_ioreq_server *s)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void msix_write_completion(struct vcpu *v)
+{
+}
+
+static inline void handle_realmode_completion(void)
+{
+ ASSERT_UNREACHABLE();
+}
+
+static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn)
+{
+}
+
+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__ */
+
+/*
+ * 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);
}

/*
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 2368ace..317455a 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -713,14 +713,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 a80bcf3..2a9b39d 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -177,8 +177,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
@@ -688,13 +688,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 d4cce68..e3afd06 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -148,8 +148,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 a314bf8..645192a 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1662,14 +1662,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);
@@ -1872,8 +1871,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
--
2.7.4
RE: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
> Sent: 03 August 2020 19:21
> To: xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>;
> Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
> <george.dunlap@citrix.com>; Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Julien Grall
> <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features
>
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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@arm.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 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
>
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 931404c..b5fc066 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -26,11 +26,19 @@
> #include "xg_private.h"
> #include "xc_dom.h"
>
> -#define NR_MAGIC_PAGES 4
> +
> #define CONSOLE_PFN_OFFSET 0
> #define XENSTORE_PFN_OFFSET 1
> #define MEMACCESS_PFN_OFFSET 2
> #define VUART_PFN_OFFSET 3
> +#define IOREQ_SERVER_PFN_OFFSET 4
> +
> +#define NR_IOREQ_SERVER_PAGES 8
> +#define NR_MAGIC_PAGES (4 + NR_IOREQ_SERVER_PAGES)
> +
> +#define GUEST_MAGIC_BASE_PFN (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT)
> +
> +#define special_pfn(x) (GUEST_MAGIC_BASE_PFN + (x))

Why introduce 'magic pages' for Arm? It's quite a horrible hack that we have begun to do away with by adding resource mapping.

Paul
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
Hi Paul,

On 04/08/2020 08:49, Paul Durrant wrote:
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index 931404c..b5fc066 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -26,11 +26,19 @@
>> #include "xg_private.h"
>> #include "xc_dom.h"
>>
>> -#define NR_MAGIC_PAGES 4
>> +
>> #define CONSOLE_PFN_OFFSET 0
>> #define XENSTORE_PFN_OFFSET 1
>> #define MEMACCESS_PFN_OFFSET 2
>> #define VUART_PFN_OFFSET 3
>> +#define IOREQ_SERVER_PFN_OFFSET 4
>> +
>> +#define NR_IOREQ_SERVER_PAGES 8
>> +#define NR_MAGIC_PAGES (4 + NR_IOREQ_SERVER_PAGES)
>> +
>> +#define GUEST_MAGIC_BASE_PFN (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT)
>> +
>> +#define special_pfn(x) (GUEST_MAGIC_BASE_PFN + (x))
>
> Why introduce 'magic pages' for Arm? It's quite a horrible hack that we have begun to do away with by adding resource mapping.

This would require us to mandate at least Linux 4.17 in a domain that
will run an IOREQ server. If we don't mandate this, the minimum version
would be 4.10 where DM OP was introduced.

Because of XSA-300, we could technically not safely run an IOREQ server
with existing Linux. So it is probably OK to enforce the use of the
acquire interface.

Note that I haven't yet looked at the rest of the series. So I am not
sure if there is more work necessary to enable it.

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On Tue, 4 Aug 2020, Julien Grall wrote:
> On 04/08/2020 08:49, Paul Durrant wrote:
> > > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > > index 931404c..b5fc066 100644
> > > --- a/tools/libxc/xc_dom_arm.c
> > > +++ b/tools/libxc/xc_dom_arm.c
> > > @@ -26,11 +26,19 @@
> > > #include "xg_private.h"
> > > #include "xc_dom.h"
> > >
> > > -#define NR_MAGIC_PAGES 4
> > > +
> > > #define CONSOLE_PFN_OFFSET 0
> > > #define XENSTORE_PFN_OFFSET 1
> > > #define MEMACCESS_PFN_OFFSET 2
> > > #define VUART_PFN_OFFSET 3
> > > +#define IOREQ_SERVER_PFN_OFFSET 4
> > > +
> > > +#define NR_IOREQ_SERVER_PAGES 8
> > > +#define NR_MAGIC_PAGES (4 + NR_IOREQ_SERVER_PAGES)
> > > +
> > > +#define GUEST_MAGIC_BASE_PFN (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT)
> > > +
> > > +#define special_pfn(x) (GUEST_MAGIC_BASE_PFN + (x))
> >
> > Why introduce 'magic pages' for Arm? It's quite a horrible hack that we have
> > begun to do away with by adding resource mapping.
>
> This would require us to mandate at least Linux 4.17 in a domain that will run
> an IOREQ server. If we don't mandate this, the minimum version would be 4.10
> where DM OP was introduced.
>
> Because of XSA-300, we could technically not safely run an IOREQ server with
> existing Linux. So it is probably OK to enforce the use of the acquire
> interface.

+1
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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

Speaking of buffered_ioreq, if I recall correctly, they were only used
for VGA-related things on x86. It looks like it is still true.

If so, do we need it on ARM? Note that I don't think we can get rid of
it from the interface as it is baked into ioreq, but it might be
possible to have a dummy implementation on ARM. Or maybe not: looking at
xen/common/hvm/ioreq.c it looks like it would be difficult to
disentangle bufioreq stuff from the rest of the code.


> 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@arm.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

[...]


> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
> */
> void leave_hypervisor_to_guest(void)
> {
> +#ifdef CONFIG_IOREQ_SERVER
> + /*
> + * XXX: Check the return. Shall we call that in
> + * continue_running and context_switch instead?
> + * The benefits would be to avoid calling
> + * handle_hvm_io_completion on every return.
> + */

Yeah, that could be a simple and good optimization


> + local_irq_enable();
> + handle_hvm_io_completion(current);
> +#endif
> local_irq_disable();
>
> check_for_vcpu_work();
> 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;
> +};
> +
> +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

realmode is an x86-ism (as pio), I wonder if we could get rid of it on ARM


> +};
> +
> +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)
> +
> +bool handle_mmio(void);
> +
> +static inline bool handle_pio(uint16_t port, unsigned int size, int dir)
> +{
> + /* XXX */
> + BUG();
> + return true;
> +}
> +
> +static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
> +{
> + return p->addr;
> +}
> +
> +static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p)
> +{
> + unsigned long size = p->size;
> +
> + return p->addr + size - 1;
> +}
> +
> +struct hvm_ioreq_server;
> +
> +static inline int p2m_set_ioreq_server(struct domain *d,
> + unsigned int flags,
> + struct hvm_ioreq_server *s)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static inline void msix_write_completion(struct vcpu *v)
> +{
> +}
> +
> +static inline void handle_realmode_completion(void)
> +{
> + ASSERT_UNREACHABLE();
> +}
> +
> +static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn)
> +{
> +}
> +
> +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__ */
> +
> +/*
> + * 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/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.

Just as a reference, and without taking away anything from the comment,
I think that QEMU is doing its own internal reference counting for these
mappings.


> */
> - return -EOPNOTSUPP;
> +
> + return guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_ram_rw);
> }
>
> /*
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.2020 01:22, Stefano Stabellini wrote:
> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>> --- 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.
>
> Just as a reference, and without taking away anything from the comment,
> I think that QEMU is doing its own internal reference counting for these
> mappings.

Which of course in no way replaces the need to do proper ref counting
in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)

Jan
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
Hi Stefano,

On 05/08/2020 00:22, Stefano Stabellini wrote:
> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> 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
>
> Speaking of buffered_ioreq, if I recall correctly, they were only used
> for VGA-related things on x86. It looks like it is still true.
>
> If so, do we need it on ARM? Note that I don't think we can get rid of
> it from the interface as it is baked into ioreq, but it might be
> possible to have a dummy implementation on ARM. Or maybe not: looking at
> xen/common/hvm/ioreq.c it looks like it would be difficult to
> disentangle bufioreq stuff from the rest of the code.

We possibly don't need it right now. However, this could possibly be
used in the future (e.g. virtio notification doorbell).

>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>> */
>> void leave_hypervisor_to_guest(void)
>> {
>> +#ifdef CONFIG_IOREQ_SERVER
>> + /*
>> + * XXX: Check the return. Shall we call that in
>> + * continue_running and context_switch instead?
>> + * The benefits would be to avoid calling
>> + * handle_hvm_io_completion on every return.
>> + */
>
> Yeah, that could be a simple and good optimization

Well, it is not simple as it is sounds :). handle_hvm_io_completion() is
the function in charge to mark the vCPU as waiting for I/O. So we would
at least need to split the function.

I wrote this TODO because I wasn't sure about the complexity of
handle_hvm_io_completion(current). Looking at it again, the main
complexity is the looping over the IOREQ servers.

I think it would be better to optimize handle_hvm_io_completion() rather
than trying to hack the context_switch() or continue_running().

[...]

>> 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.
>
> Just as a reference, and without taking away anything from the comment,
> I think that QEMU is doing its own internal reference counting for these
> mappings.

I am not sure how this matters here? We can't really trust the DM to do
the right thing if it is not running in dom0.

But, IIRC, the problem is some of the pages doesn't belong to do a
domain, so it is not possible to treat them as foreign mapping (e.g. you
wouldn't be able to grab a reference). This investigation was done a
couple of years ago, so this may have changed in recent Xen.

As a side note, I am a bit surprised to see most of my original TODOs
present in the code. What is the plan to solve them?

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
Hi,

On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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@arm.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 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
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.2020 16:12, Julien Grall wrote:
> On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
>> --- /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.

On x86 it wants to live together with a bunch of other has_v*()
macros.

Jan
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.20 12:32, Julien Grall wrote:

Hi Julien.

> Hi Stefano,
>
> On 05/08/2020 00:22, Stefano Stabellini wrote:
>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> 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
>>
>> Speaking of buffered_ioreq, if I recall correctly, they were only used
>> for VGA-related things on x86. It looks like it is still true.
>>
>> If so, do we need it on ARM? Note that I don't think we can get rid of
>> it from the interface as it is baked into ioreq, but it might be
>> possible to have a dummy implementation on ARM. Or maybe not: looking at
>> xen/common/hvm/ioreq.c it looks like it would be difficult to
>> disentangle bufioreq stuff from the rest of the code.
>
> We possibly don't need it right now. However, this could possibly be
> used in the future (e.g. virtio notification doorbell).
>
>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>    */
>>>   void leave_hypervisor_to_guest(void)
>>>   {
>>> +#ifdef CONFIG_IOREQ_SERVER
>>> +    /*
>>> +     * XXX: Check the return. Shall we call that in
>>> +     * continue_running and context_switch instead?
>>> +     * The benefits would be to avoid calling
>>> +     * handle_hvm_io_completion on every return.
>>> +     */
>>
>> Yeah, that could be a simple and good optimization
>
> Well, it is not simple as it is sounds :). handle_hvm_io_completion()
> is the function in charge to mark the vCPU as waiting for I/O. So we
> would at least need to split the function.
>
> I wrote this TODO because I wasn't sure about the complexity of
> handle_hvm_io_completion(current). Looking at it again, the main
> complexity is the looping over the IOREQ servers.
>
> I think it would be better to optimize handle_hvm_io_completion()
> rather than trying to hack the context_switch() or continue_running().
>
> [...]
>
>>> 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.
>>
>> Just as a reference, and without taking away anything from the comment,
>> I think that QEMU is doing its own internal reference counting for these
>> mappings.
>
> I am not sure how this matters here? We can't really trust the DM to
> do the right thing if it is not running in dom0.
>
> But, IIRC, the problem is some of the pages doesn't belong to do a
> domain, so it is not possible to treat them as foreign mapping (e.g.
> you wouldn't be able to grab a reference). This investigation was done
> a couple of years ago, so this may have changed in recent Xen.
>
> As a side note, I am a bit surprised to see most of my original TODOs
> present in the code. What is the plan to solve them?
The plan is to solve most critical TODOs in current series, and rest in
follow-up series if no objections of course. Any pointers how to solve
them properly would be much appreciated. Unfortunately, now I have a
weak understanding how they should be fixed. I see at least 3 major TODO
here:

1. handle properly reference in set_foreign_p2m_entry()
2. optimize handle_hvm_io_completion()
3. hande properly IO_RETRY in try_fwd_ioserv()


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -713,14 +713,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 a80bcf3..2a9b39d 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -177,8 +177,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
> @@ -688,13 +688,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 d4cce68..e3afd06 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -148,8 +148,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 a314bf8..645192a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1662,14 +1662,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);
> @@ -1872,8 +1871,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

All of this looks to belong into patch 2?

Jan
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On Wed, 5 Aug 2020, Jan Beulich wrote:
> On 05.08.2020 01:22, Stefano Stabellini wrote:
> > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> >> --- 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.
> >
> > Just as a reference, and without taking away anything from the comment,
> > I think that QEMU is doing its own internal reference counting for these
> > mappings.
>
> Which of course in no way replaces the need to do proper ref counting
> in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)

Given the state of the series, which is a RFC, I only meant to say that
the lack of refcounting shouldn't prevent things from working when using
QEMU. In the sense that if somebody wants to give it a try for an early
demo, they should be able to see it running without crashes.

Of course, refcounting needs to be implemented.
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.20 17:12, Julien Grall wrote:
> Hi,

Hi Julien


>
> On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> 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@arm.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   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)
Sure, will split.
However, I don't quite understand where we should leave HVMOP plumbing.
If I understand correctly the suggestion was to switch to acquire
interface instead (which requires a Linux version to be v4.17 at least)?
I suspect, this is all about "xen/privcmd: add
IOCTL_PRIVCMD_MMAP_RESOURCE" support for Linux?
Sorry, if asking a lot of questions, my developing environment is based
on Vendor's BSP which uses v4.14 currently.


>
> [...]
>
>> 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?

Sure, I moved arch_dm_op internals to the next patch in this series, but
forgot to move corresponding headers as well.


>
>
>> +
>> +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?

Sure


>
>
>> +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.

ok


>
>> +        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.

ok


>
>> +        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.

Noted


>
>
>> +    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?
Agree, sounds confusing a bit. I assume it is supposed to complete Guest
MMIO access after finishing emulation.

Shall I rename it to something appropriate (maybe by adding ioreq prefix)?


>
>
>> +{
>> +    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?
The reason I guarded that header is to make "xen/mm: Make x86's
XENMEM_resource_ioreq_server handling common" (previous) patch buildable
on Arm
without arch IOREQ header added yet. I tried to make sure that the
result after each patch was buildable to retain bisectability.
As current patch adds Arm IOREQ specific bits (including header), that
guard could be removed as not needed anymore.


>
>>   #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.

Make sense. Probably worth moving. I assume this also applies to x86 ones.


>
>
>> +
>> +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.

ok, will move.


>
>
>> +
>> +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?
I didn't expect this to be called on Arm. Sorry, I am not sure l have an
idea how to handle this properly. I would keep it unimplemented until a
real reason.
Will expand TODO.


>
>
>> +    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.
Well, no objections at all.
Could you please clarify how could 'df' (Direction Flag?) be
handled/used on Arm? I see that try_fwd_ioserv() always sets it 0. Or I
need to just reuse x86's helpers as is,
which (together with count = df = 0) will result in what we actually
have here?


>
>> +
>> +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?

I don't remember exactly, probably this way I temporally solved a build
issue. Please let me recheck whether we could avoid using it.


>
>
>> +
>> +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?

ok, will move.


In this series I tried to make as much IOREQ code common as possible and
avoid complicating things, in order to achieve that a few stubs were
added here. Please note,
that I also considered splitting into arch parts. But some functions
couldn't be split easily.
This one is called from common hvm_destroy_ioreq_server() with flag
being 0 (which will result in unmapping ioreq server from p2m type on x86).
I could add a comment describing why this stub is present here.


>
>
>> +
>> +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.

I agree with you that realmode is x86 specific and looks not good in Arm
header. I was thinking how to split handle_hvm_io_completion()
gracefully but I failed find a good solution for that, so decided to add
two stubs (msix_write_completion and handle_realmode_completion) on Arm.
I could add a comment describing why they are here if appropriate. But
if you think they shouldn't be called from the common code in any way, I
will try to split it.


>
>> +
>> +static inline void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn)
>> +{
>> +}
>
> This will want to be stubbed in asm-arm/paging.h.

ok


>
>
>> +
>> +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/

ok


>
>> +
>> +/*
>> + * 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.

ok



--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.20 19:41, Stefano Stabellini wrote:
Hi Stefano

> On Wed, 5 Aug 2020, Jan Beulich wrote:
>> On 05.08.2020 01:22, Stefano Stabellini wrote:
>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>> --- 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.
>>> Just as a reference, and without taking away anything from the comment,
>>> I think that QEMU is doing its own internal reference counting for these
>>> mappings.
>> Which of course in no way replaces the need to do proper ref counting
>> in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)
> Given the state of the series, which is a RFC, I only meant to say that
> the lack of refcounting shouldn't prevent things from working when using
> QEMU. In the sense that if somebody wants to give it a try for an early
> demo, they should be able to see it running without crashes.

Yes, for the early demo it works fine, however I don't use Qemu.


>
> Of course, refcounting needs to be implemented.

+


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.20 19:13, Jan Beulich wrote:

Hi, Jan

> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -713,14 +713,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 a80bcf3..2a9b39d 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -177,8 +177,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
>> @@ -688,13 +688,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 d4cce68..e3afd06 100644
>> --- a/xen/xsm/dummy.c
>> +++ b/xen/xsm/dummy.c
>> @@ -148,8 +148,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 a314bf8..645192a 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1662,14 +1662,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);
>> @@ -1872,8 +1871,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
> All of this looks to belong into patch 2?


Good point. Will move.

--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
Hi,

On 05/08/2020 16:41, Oleksandr wrote:
>
> On 05.08.20 12:32, Julien Grall wrote:
>
> Hi Julien.
>
>> Hi Stefano,
>>
>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> 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
>>>
>>> Speaking of buffered_ioreq, if I recall correctly, they were only used
>>> for VGA-related things on x86. It looks like it is still true.
>>>
>>> If so, do we need it on ARM? Note that I don't think we can get rid of
>>> it from the interface as it is baked into ioreq, but it might be
>>> possible to have a dummy implementation on ARM. Or maybe not: looking at
>>> xen/common/hvm/ioreq.c it looks like it would be difficult to
>>> disentangle bufioreq stuff from the rest of the code.
>>
>> We possibly don't need it right now. However, this could possibly be
>> used in the future (e.g. virtio notification doorbell).
>>
>>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>>    */
>>>>   void leave_hypervisor_to_guest(void)
>>>>   {
>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>> +    /*
>>>> +     * XXX: Check the return. Shall we call that in
>>>> +     * continue_running and context_switch instead?
>>>> +     * The benefits would be to avoid calling
>>>> +     * handle_hvm_io_completion on every return.
>>>> +     */
>>>
>>> Yeah, that could be a simple and good optimization
>>
>> Well, it is not simple as it is sounds :). handle_hvm_io_completion()
>> is the function in charge to mark the vCPU as waiting for I/O. So we
>> would at least need to split the function.
>>
>> I wrote this TODO because I wasn't sure about the complexity of
>> handle_hvm_io_completion(current). Looking at it again, the main
>> complexity is the looping over the IOREQ servers.
>>
>> I think it would be better to optimize handle_hvm_io_completion()
>> rather than trying to hack the context_switch() or continue_running().
>>
>> [...]
>>
>>>> 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.
>>>
>>> Just as a reference, and without taking away anything from the comment,
>>> I think that QEMU is doing its own internal reference counting for these
>>> mappings.
>>
>> I am not sure how this matters here? We can't really trust the DM to
>> do the right thing if it is not running in dom0.
>>
>> But, IIRC, the problem is some of the pages doesn't belong to do a
>> domain, so it is not possible to treat them as foreign mapping (e.g.
>> you wouldn't be able to grab a reference). This investigation was done
>> a couple of years ago, so this may have changed in recent Xen.
>>
>> As a side note, I am a bit surprised to see most of my original TODOs
>> present in the code. What is the plan to solve them?
> The plan is to solve most critical TODOs in current series, and rest in
> follow-up series if no objections of course. Any pointers how to solve
> them properly would be much appreciated. Unfortunately, now I have a
> weak understanding how they should be fixed.

AFAICT, there is already some discussion about those 3 major TODOs
happening. I would suggest to go through the discussions. We can clarify
anything if needed.

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05/08/2020 20:30, Oleksandr wrote:
>
> On 05.08.20 17:12, Julien Grall wrote:
>> Hi,
>
> Hi Julien
>
>
>>
>> On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> 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@arm.com>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>   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)
> Sure, will split.
> However, I don't quite understand where we should leave HVMOP plumbing.

I think they will need to be droppped if we decide to use the acquire
interface.

> If I understand correctly the suggestion was to switch to acquire
> interface instead (which requires a Linux version to be v4.17 at least)?

This was the suggestion.

> I suspect, this is all about "xen/privcmd: add
> IOCTL_PRIVCMD_MMAP_RESOURCE" support for Linux?

Correct.

>> What is this function supposed to do?
> Agree, sounds confusing a bit. I assume it is supposed to complete Guest
> MMIO access after finishing emulation.
>
> Shall I rename it to something appropriate (maybe by adding ioreq prefix)?

How about ioreq_handle_complete_mmio()?

>>> 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?
> The reason I guarded that header is to make "xen/mm: Make x86's
> XENMEM_resource_ioreq_server handling common" (previous) patch buildable
> on Arm
> without arch IOREQ header added yet. I tried to make sure that the
> result after each patch was buildable to retain bisectability.
> As current patch adds Arm IOREQ specific bits (including header), that
> guard could be removed as not needed anymore.
I agree we want to have the build bisectable. However, I am still
puzzled why it is necessary to remove the #ifdef and move it earlier in
the list.

Do you mind to provide more details?

[...]

>>> +
>>> +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?
> I didn't expect this to be called on Arm. Sorry, I am not sure l have an
> idea how to handle this properly. I would keep it unimplemented until a
> real reason.
> Will expand TODO.

Let see how the conversation on patch#1 goes about PIO vs MMIO.

>>
>>
>>> +    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.
> Well, no objections at all.
> Could you please clarify how could 'df' (Direction Flag?) be
> handled/used on Arm?

On x86, this is used by 'rep' instruction to tell the direction to
iterate (forward or backward).

On Arm, all the accesses to MMIO region will do a single memory access.
So for now, we can safely always set to 0.

> I see that try_fwd_ioserv() always sets it 0. Or I
> need to just reuse x86's helpers as is,
> which (together with count = df = 0) will result in what we actually
> have here?
AFAIU, both count and df should be 0 on Arm.

>>
>>
>>> +
>>> +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?
>
> ok, will move.
>
>
> In this series I tried to make as much IOREQ code common as possible and
> avoid complicating things, in order to achieve that a few stubs were
> added here. Please note,
> that I also considered splitting into arch parts. But some functions
> couldn't be split easily.
> This one is called from common hvm_destroy_ioreq_server() with flag
> being 0 (which will result in unmapping ioreq server from p2m type on x86).
> I could add a comment describing why this stub is present here.

Sorry if I wasn't clear. I wasn't asking why the stub is there but what
should be the expected implementation of the function.

In particular, you are returning -EOPNOTSUPP. The only reason you are
getting away from trouble is because the caller doesn't check the return.

Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?

>
>
>>
>>
>>> +
>>> +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.
>
> I agree with you that realmode is x86 specific and looks not good in Arm
> header.
It is not a problem of looking good or not. Instead, it is about
abstraction. A developper shouldn't need to understand all the other
architectures we support in order to follow the common code.

> I was thinking how to split handle_hvm_io_completion()
> gracefully but I failed find a good solution for that, so decided to add
> two stubs (msix_write_completion and handle_realmode_completion) on Arm.
> I could add a comment describing why they are here if appropriate. But
> if you think they shouldn't be called from the common code in any way, I
> will try to split it.

I am not entirely sure what msix_write_completion is meant to do on x86.
Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could help?

Regarding handle_realmode_completion, I would add a new stub:

arch_ioreq_handle_io_completion() that is called from the default case
of the switch.

On x86 it would be implemented as:

switch (io_completion)
{
case HVMIO_realmode_completion:
...
default:
ASSERT_UNREACHABLE();
}

On Arm, it would be implemented as:

ASSERT_UNREACHABLE();

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 06.08.2020 13:08, Julien Grall wrote:
> On 05/08/2020 20:30, Oleksandr wrote:
>> I was thinking how to split handle_hvm_io_completion()
>> gracefully but I failed find a good solution for that, so decided to add
>> two stubs (msix_write_completion and handle_realmode_completion) on Arm.
>> I could add a comment describing why they are here if appropriate. But
>> if you think they shouldn't be called from the common code in any way, I
>> will try to split it.
>
> I am not entirely sure what msix_write_completion is meant to do on x86.
> Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could help?

Due to the split brain model of handling PCI pass-through (between
Xen and qemu), a guest writing to an MSI-X entry needs this write
handed to qemu, and upon completion of the write there Xen also
needs to take some extra action.

Jan
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 06.08.20 14:08, Julien Grall wrote:

Hi Julien

>
>>> What is this function supposed to do?
>> Agree, sounds confusing a bit. I assume it is supposed to complete
>> Guest MMIO access after finishing emulation.
>>
>> Shall I rename it to something appropriate (maybe by adding ioreq
>> prefix)?
>
> How about ioreq_handle_complete_mmio()?

For me it sounds fine.



>
>>>> 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?
>> The reason I guarded that header is to make "xen/mm: Make x86's
>> XENMEM_resource_ioreq_server handling common" (previous) patch
>> buildable on Arm
>> without arch IOREQ header added yet. I tried to make sure that the
>> result after each patch was buildable to retain bisectability.
>> As current patch adds Arm IOREQ specific bits (including header),
>> that guard could be removed as not needed anymore.
> I agree we want to have the build bisectable. However, I am still
> puzzled why it is necessary to remove the #ifdef and move it earlier
> in the list.
>
> Do you mind to provide more details?
Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server handling
common" breaks build on Arm as it includes xen/hvm/ioreq.h which
requires arch header
to be present (asm/hvm/ioreq.h). But the missing arch header together
with other arch specific bits are introduced here in current patch.
Probably I should have rearranged
changes in a way to not introduce #ifdef and then remove it...


>
> [...]
>
>>>> +
>>>> +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?
>> I didn't expect this to be called on Arm. Sorry, I am not sure l have
>> an idea how to handle this properly. I would keep it unimplemented
>> until a real reason.
>> Will expand TODO.
>
> Let see how the conversation on patch#1 goes about PIO vs MMIO.

ok


>
>>>
>>>
>>>> +    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.
>> Well, no objections at all.
>> Could you please clarify how could 'df' (Direction Flag?) be
>> handled/used on Arm?
>
> On x86, this is used by 'rep' instruction to tell the direction to
> iterate (forward or backward).
>
> On Arm, all the accesses to MMIO region will do a single memory
> access. So for now, we can safely always set to 0.
>
>> I see that try_fwd_ioserv() always sets it 0. Or I need to just reuse
>> x86's helpers as is,
>> which (together with count = df = 0) will result in what we actually
>> have here?
> AFAIU, both count and df should be 0 on Arm.

Thanks for the explanation. The only one question remains where to put
common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common io.h?)?


>>>
>>>> +
>>>> +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?
>>
>> ok, will move.
>>
>>
>> In this series I tried to make as much IOREQ code common as possible
>> and avoid complicating things, in order to achieve that a few stubs
>> were added here. Please note,
>> that I also considered splitting into arch parts. But some functions
>> couldn't be split easily.
>> This one is called from common hvm_destroy_ioreq_server() with flag
>> being 0 (which will result in unmapping ioreq server from p2m type on
>> x86).
>> I could add a comment describing why this stub is present here.
>
> Sorry if I wasn't clear. I wasn't asking why the stub is there but
> what should be the expected implementation of the function.
>
> In particular, you are returning -EOPNOTSUPP. The only reason you are
> getting away from trouble is because the caller doesn't check the return.

True.


>
> Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?

With what has been said above, it make sense, will create.


>>>> +
>>>> +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.
>>
>> I agree with you that realmode is x86 specific and looks not good in
>> Arm header.
> It is not a problem of looking good or not. Instead, it is about
> abstraction. A developper shouldn't need to understand all the other
> architectures we support in order to follow the common code.
>
>> I was thinking how to split handle_hvm_io_completion() gracefully but
>> I failed find a good solution for that, so decided to add two stubs
>> (msix_write_completion and handle_realmode_completion) on Arm. I
>> could add a comment describing why they are here if appropriate. But
>> if you think they shouldn't be called from the common code in any
>> way, I will try to split it.
>
> I am not entirely sure what msix_write_completion is meant to do on
> x86. Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could
> help?
>
> Regarding handle_realmode_completion, I would add a new stub:
>
> arch_ioreq_handle_io_completion() that is called from the default case
> of the switch.
>
> On x86 it would be implemented as:
>
>  switch (io_completion)
>  {
>     case HVMIO_realmode_completion:
>       ...
>     default:
>       ASSERT_UNREACHABLE();
>  }
>
> On Arm, it would be implemented as:
>
>   ASSERT_UNREACHABLE();


Good point, will update.


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 05.08.20 12:32, Julien Grall wrote:

Hi Julien

>
>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>    */
>>>   void leave_hypervisor_to_guest(void)
>>>   {
>>> +#ifdef CONFIG_IOREQ_SERVER
>>> +    /*
>>> +     * XXX: Check the return. Shall we call that in
>>> +     * continue_running and context_switch instead?
>>> +     * The benefits would be to avoid calling
>>> +     * handle_hvm_io_completion on every return.
>>> +     */
>>
>> Yeah, that could be a simple and good optimization
>
> Well, it is not simple as it is sounds :). handle_hvm_io_completion()
> is the function in charge to mark the vCPU as waiting for I/O. So we
> would at least need to split the function.
>
> I wrote this TODO because I wasn't sure about the complexity of
> handle_hvm_io_completion(current). Looking at it again, the main
> complexity is the looping over the IOREQ servers.
>
> I think it would be better to optimize handle_hvm_io_completion()
> rather than trying to hack the context_switch() or continue_running().
Well, is the idea in proposed dirty test patch below close to what you
expect? Patch optimizes handle_hvm_io_completion() to avoid extra
actions if vcpu's domain doesn't have ioreq_server, alternatively
the check could be moved out of handle_hvm_io_completion() to avoid
calling that function at all. BTW, TODO also suggests checking the
return value of handle_hvm_io_completion(), but I am completely sure we
can simply
just return from leave_hypervisor_to_guest() at this point. Could you
please share your opinion?


---
 xen/common/hvm/ioreq.c       | 12 +++++++++++-
 xen/include/asm-arm/domain.h |  1 +
 xen/include/xen/hvm/ioreq.h  |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/common/hvm/ioreq.c b/xen/common/hvm/ioreq.c
index 7e1fa23..dc6647a 100644
--- a/xen/common/hvm/ioreq.c
+++ b/xen/common/hvm/ioreq.c
@@ -38,9 +38,15 @@ static void set_ioreq_server(struct domain *d,
unsigned int id,
                              struct hvm_ioreq_server *s)
 {
     ASSERT(id < MAX_NR_IOREQ_SERVERS);
-    ASSERT(!s || !d->arch.hvm.ioreq_server.server[id]);
+    ASSERT((!s && d->arch.hvm.ioreq_server.server[id]) ||
+           (s && !d->arch.hvm.ioreq_server.server[id]));

     d->arch.hvm.ioreq_server.server[id] = s;
+
+    if ( s )
+        d->arch.hvm.ioreq_server.nr_servers ++;
+    else
+        d->arch.hvm.ioreq_server.nr_servers --;
 }

 /*
@@ -169,6 +175,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
         return false;
     }

+    if ( !hvm_domain_has_ioreq_server(d) )
+        return true;
+
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
@@ -1415,6 +1424,7 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool
buffered)
 void hvm_ioreq_init(struct domain *d)
 {
     spin_lock_init(&d->arch.hvm.ioreq_server.lock);
+    d->arch.hvm.ioreq_server.nr_servers = 0;

     arch_hvm_ioreq_init(d);
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 6a01d69..484bd1a 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -68,6 +68,7 @@ struct hvm_domain
     struct {
         spinlock_t              lock;
         struct hvm_ioreq_server *server[MAX_NR_IOREQ_SERVERS];
+        unsigned int            nr_servers;
     } ioreq_server;

     bool_t qemu_mapcache_invalidate;
diff --git a/xen/include/xen/hvm/ioreq.h b/xen/include/xen/hvm/ioreq.h
index 40b7b5e..8f78852 100644
--- a/xen/include/xen/hvm/ioreq.h
+++ b/xen/include/xen/hvm/ioreq.h
@@ -23,6 +23,11 @@

 #include <asm/hvm/ioreq.h>

+static inline bool hvm_domain_has_ioreq_server(const struct domain *d)
+{
+    return (d->arch.hvm.ioreq_server.nr_servers > 0);
+}
+
 #define GET_IOREQ_SERVER(d, id) \
     (d)->arch.hvm.ioreq_server.server[id]

--
2.7.4





--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
Hi


>
>>
>>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>>    */
>>>>   void leave_hypervisor_to_guest(void)
>>>>   {
>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>> +    /*
>>>> +     * XXX: Check the return. Shall we call that in
>>>> +     * continue_running and context_switch instead?
>>>> +     * The benefits would be to avoid calling
>>>> +     * handle_hvm_io_completion on every return.
>>>> +     */
>>>
>>> Yeah, that could be a simple and good optimization
>>
>> Well, it is not simple as it is sounds :). handle_hvm_io_completion()
>> is the function in charge to mark the vCPU as waiting for I/O. So we
>> would at least need to split the function.
>>
>> I wrote this TODO because I wasn't sure about the complexity of
>> handle_hvm_io_completion(current). Looking at it again, the main
>> complexity is the looping over the IOREQ servers.
>>
>> I think it would be better to optimize handle_hvm_io_completion()
>> rather than trying to hack the context_switch() or continue_running().
> Well, is the idea in proposed dirty test patch below close to what you
> expect? Patch optimizes handle_hvm_io_completion() to avoid extra
> actions if vcpu's domain doesn't have ioreq_server, alternatively
> the check could be moved out of handle_hvm_io_completion() to avoid
> calling that function at all. BTW, TODO also suggests checking the
> return value of handle_hvm_io_completion(), but I am completely sure
> we can simply
> just return from leave_hypervisor_to_guest() at this point.

Sorry, made a mistake in last sentence).  s / I am completely sure / I
am not completely sure


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 06/08/2020 14:27, Oleksandr wrote:
>
> On 06.08.20 14:08, Julien Grall wrote:
>
> Hi Julien
>
>>
>>>> What is this function supposed to do?
>>> Agree, sounds confusing a bit. I assume it is supposed to complete
>>> Guest MMIO access after finishing emulation.
>>>
>>> Shall I rename it to something appropriate (maybe by adding ioreq
>>> prefix)?
>>
>> How about ioreq_handle_complete_mmio()?
>
> For me it sounds fine.
>
>
>
>>
>>>>> 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?
>>> The reason I guarded that header is to make "xen/mm: Make x86's
>>> XENMEM_resource_ioreq_server handling common" (previous) patch
>>> buildable on Arm
>>> without arch IOREQ header added yet. I tried to make sure that the
>>> result after each patch was buildable to retain bisectability.
>>> As current patch adds Arm IOREQ specific bits (including header),
>>> that guard could be removed as not needed anymore.
>> I agree we want to have the build bisectable. However, I am still
>> puzzled why it is necessary to remove the #ifdef and move it earlier
>> in the list.
>>
>> Do you mind to provide more details?
> Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server handling
> common" breaks build on Arm as it includes xen/hvm/ioreq.h which
> requires arch header
> to be present (asm/hvm/ioreq.h). But the missing arch header together
> with other arch specific bits are introduced here in current patch.

I understand that both Arm and x86 now implement the asm/hvm/ioreq.h.
However, please keep in mind that there might be other architectures in
the future.

With your change here, you would impose a new arch to implement
asm/hvm/ioreq.h even if the developper have no plan to use the feature.

> Probably I should have rearranged
> changes in a way to not introduce #ifdef and then remove it...

Ideally we want to avoid #ifdef in the common code. But if this can't be
done in an header, then the #ifdef here would be fine.

>
>
>>
>> [...]
>>
>>>>> +
>>>>> +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?
>>> I didn't expect this to be called on Arm. Sorry, I am not sure l have
>>> an idea how to handle this properly. I would keep it unimplemented
>>> until a real reason.
>>> Will expand TODO.
>>
>> Let see how the conversation on patch#1 goes about PIO vs MMIO.
>
> ok
>
>
>>
>>>>
>>>>
>>>>> +    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.
>>> Well, no objections at all.
>>> Could you please clarify how could 'df' (Direction Flag?) be
>>> handled/used on Arm?
>>
>> On x86, this is used by 'rep' instruction to tell the direction to
>> iterate (forward or backward).
>>
>> On Arm, all the accesses to MMIO region will do a single memory
>> access. So for now, we can safely always set to 0.
>>
>>> I see that try_fwd_ioserv() always sets it 0. Or I need to just reuse
>>> x86's helpers as is,
>>> which (together with count = df = 0) will result in what we actually
>>> have here?
>> AFAIU, both count and df should be 0 on Arm.
>
> Thanks for the explanation. The only one question remains where to put
> common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common io.h?)?

It feels to me it should be part of the common ioreq.h.

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 10/08/2020 19:09, Oleksandr wrote:
>
> On 05.08.20 12:32, Julien Grall wrote:
>
> Hi Julien
>
>>
>>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>>    */
>>>>   void leave_hypervisor_to_guest(void)
>>>>   {
>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>> +    /*
>>>> +     * XXX: Check the return. Shall we call that in
>>>> +     * continue_running and context_switch instead?
>>>> +     * The benefits would be to avoid calling
>>>> +     * handle_hvm_io_completion on every return.
>>>> +     */
>>>
>>> Yeah, that could be a simple and good optimization
>>
>> Well, it is not simple as it is sounds :). handle_hvm_io_completion()
>> is the function in charge to mark the vCPU as waiting for I/O. So we
>> would at least need to split the function.
>>
>> I wrote this TODO because I wasn't sure about the complexity of
>> handle_hvm_io_completion(current). Looking at it again, the main
>> complexity is the looping over the IOREQ servers.
>>
>> I think it would be better to optimize handle_hvm_io_completion()
>> rather than trying to hack the context_switch() or continue_running().
> Well, is the idea in proposed dirty test patch below close to what you
> expect? Patch optimizes handle_hvm_io_completion() to avoid extra
> actions if vcpu's domain doesn't have ioreq_server, alternatively
> the check could be moved out of handle_hvm_io_completion() to avoid
> calling that function at all.

This looks ok to me.

> BTW, TODO also suggests checking the
> return value of handle_hvm_io_completion(), but I am completely sure we
> can simply
> just return from leave_hypervisor_to_guest() at this point. Could you
> please share your opinion?

From my understanding, handle_hvm_io_completion() may return false if
there is pending I/O or a failure.

In the former case, I think we want to call handle_hvm_io_completion()
later on. Possibly after we call do_softirq().

I am wondering whether check_for_vcpu_work() could return whether there
are more work todo on the behalf of the vCPU.

So we could have:

do
{
check_for_pcpu_work();
} while (check_for_vcpu_work())

The implementation of check_for_vcpu_work() would be:

if ( !handle_hvm_io_completion() )
return true;

/* Rest of the existing code */

return false;

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 10.08.20 21:25, Julien Grall wrote:

Hi Julien

>
>>>
>>> Do you mind to provide more details?
>> Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server
>> handling common" breaks build on Arm as it includes xen/hvm/ioreq.h
>> which requires arch header
>> to be present (asm/hvm/ioreq.h). But the missing arch header together
>> with other arch specific bits are introduced here in current patch.
>
> I understand that both Arm and x86 now implement the asm/hvm/ioreq.h.
> However, please keep in mind that there might be other architectures
> in the future.
>
> With your change here, you would impose a new arch to implement
> asm/hvm/ioreq.h even if the developper have no plan to use the feature.
>
>> Probably I should have rearranged
>> changes in a way to not introduce #ifdef and then remove it...
>
> Ideally we want to avoid #ifdef in the common code. But if this can't
> be done in an header, then the #ifdef here would be fine.

Got it.


>
>>>>> 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.
>>>> Well, no objections at all.
>>>> Could you please clarify how could 'df' (Direction Flag?) be
>>>> handled/used on Arm?
>>>
>>> On x86, this is used by 'rep' instruction to tell the direction to
>>> iterate (forward or backward).
>>>
>>> On Arm, all the accesses to MMIO region will do a single memory
>>> access. So for now, we can safely always set to 0.
>>>
>>>> I see that try_fwd_ioserv() always sets it 0. Or I need to just
>>>> reuse x86's helpers as is,
>>>> which (together with count = df = 0) will result in what we
>>>> actually have here?
>>> AFAIU, both count and df should be 0 on Arm.
>>
>> Thanks for the explanation. The only one question remains where to
>> put common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common
>> io.h?)?
>
> It feels to me it should be part of the common ioreq.h.

ok, will move.


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features [ In reply to ]
On 10.08.20 22:00, Julien Grall wrote:

Hi Julien

>
>>>
>>>>> @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
>>>>>    */
>>>>>   void leave_hypervisor_to_guest(void)
>>>>>   {
>>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>>> +    /*
>>>>> +     * XXX: Check the return. Shall we call that in
>>>>> +     * continue_running and context_switch instead?
>>>>> +     * The benefits would be to avoid calling
>>>>> +     * handle_hvm_io_completion on every return.
>>>>> +     */
>>>>
>>>> Yeah, that could be a simple and good optimization
>>>
>>> Well, it is not simple as it is sounds :).
>>> handle_hvm_io_completion() is the function in charge to mark the
>>> vCPU as waiting for I/O. So we would at least need to split the
>>> function.
>>>
>>> I wrote this TODO because I wasn't sure about the complexity of
>>> handle_hvm_io_completion(current). Looking at it again, the main
>>> complexity is the looping over the IOREQ servers.
>>>
>>> I think it would be better to optimize handle_hvm_io_completion()
>>> rather than trying to hack the context_switch() or continue_running().
>> Well, is the idea in proposed dirty test patch below close to what
>> you expect? Patch optimizes handle_hvm_io_completion() to avoid extra
>> actions if vcpu's domain doesn't have ioreq_server, alternatively
>> the check could be moved out of handle_hvm_io_completion() to avoid
>> calling that function at all.
>
> This looks ok to me.
>
>> BTW, TODO also suggests checking the return value of
>> handle_hvm_io_completion(), but I am completely sure we can simply
>> just return from leave_hypervisor_to_guest() at this point. Could you
>> please share your opinion?
>
> From my understanding, handle_hvm_io_completion() may return false if
> there is pending I/O or a failure.

It seems, yes


>
> In the former case, I think we want to call handle_hvm_io_completion()
> later on. Possibly after we call do_softirq().
>
> I am wondering whether check_for_vcpu_work() could return whether
> there are more work todo on the behalf of the vCPU.
>
> So we could have:
>
> do
> {
>   check_for_pcpu_work();
> } while (check_for_vcpu_work())
>
> The implementation of check_for_vcpu_work() would be:
>
> if ( !handle_hvm_io_completion() )
>   return true;
>
> /* Rest of the existing code */
>
> return false;

Thank you, will give it a try.

Can we behave the same way for both "pending I/O" and "failure" or we
need to distinguish them?

Probably we need some sort of safe timeout/number attempts in order to
not spin forever?


--
Regards,

Oleksandr Tyshchenko

1 2  View All