| Message ID | 1610488352-18494-15-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: > From: Julien Grall <julien.grall@arm.com> > > This patch adds basic IOREQ/DM support on Arm. The subsequent > patches will improve functionality and add remaining bits. > > The IOREQ/DM features are supposed to be built with IOREQ_SERVER > option enabled, which is disabled by default on Arm for now. > > Please note, the "PIO handling" TODO is expected to left unaddressed > for the current series. It is not an big issue for now while Xen > doesn't have support for vPCI on Arm. On Arm64 they are only used > for PCI IO Bar and we would probably want to expose them to emulator > as PIO access to make a DM completely arch-agnostic. So "PIO handling" > should be implemented when we add support for vPCI. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > [On Arm only] > Tested-by: Wei Chen <Wei.Chen@arm.com> > > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Changes RFC -> V1: > - was split into: > - arm/ioreq: Introduce arch specific bits for IOREQ/DM features > - xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm > - update patch description > - update asm-arm/hvm/ioreq.h according to the newly introduced arch functions: > - arch_hvm_destroy_ioreq_server() > - arch_handle_hvm_io_completion() > - update arch files to include xen/ioreq.h > - remove HVMOP plumbing > - rewrite a logic to handle properly case when hvm_send_ioreq() returns IO_RETRY > - add a logic to handle properly handle_hvm_io_completion() return value > - rename handle_mmio() to ioreq_handle_complete_mmio() > - move paging_mark_pfn_dirty() to asm-arm/paging.h > - remove forward declaration for hvm_ioreq_server in asm-arm/paging.h > - move try_fwd_ioserv() to ioreq.c, provide stubs if !CONFIG_IOREQ_SERVER > - do not remove #ifdef CONFIG_IOREQ_SERVER in memory.c for guarding xen/ioreq.h > - use gdprintk in try_fwd_ioserv(), remove unneeded prints > - update list of #include-s > - move has_vpci() to asm-arm/domain.h > - add a comment (TODO) to unimplemented yet handle_pio() > - remove hvm_mmio_first(last)_byte() and hvm_ioreq_(page/vcpu/server) structs > from the arch files, they were already moved to the common code > - remove set_foreign_p2m_entry() changes, they will be properly implemented > in the follow-up patch > - select IOREQ_SERVER for Arm instead of Arm64 in Kconfig > - remove x86's realmode and other unneeded stubs from xen/ioreq.h > - clafify ioreq_t p.df usage in try_fwd_ioserv() > - set ioreq_t p.count to 1 in try_fwd_ioserv() > > Changes V1 -> V2: > - was split into: > - arm/ioreq: Introduce arch specific bits for IOREQ/DM features > - xen/arm: Stick around in leave_hypervisor_to_guest until I/O has completed > - update the author of a patch > - update patch description > - move a loop in leave_hypervisor_to_guest() to a separate patch > - set IOREQ_SERVER disabled by default > - remove already clarified /* XXX */ > - replace BUG() by ASSERT_UNREACHABLE() in handle_pio() > - remove default case for handling the return value of try_handle_mmio() > - remove struct hvm_domain, enum hvm_io_completion, struct hvm_vcpu_io, > struct hvm_vcpu from asm-arm/domain.h, these are common materials now > - update everything according to the recent changes (IOREQ related function > names don't contain "hvm" prefixes/infixes anymore, IOREQ related fields > are part of common struct vcpu/domain now, etc) > > Changes V2 -> V3: > - update patch according the "legacy interface" is x86 specific > - add dummy arch hooks > - remove dummy paging_mark_pfn_dirty() > - don’t include <xen/domain_page.h> in common ioreq.c > - don’t include <public/hvm/ioreq.h> in arch ioreq.h > - remove #define ioreq_params(d, i) > > Changes V3 -> V4: > - rebase > - update patch according to the renaming IO_ -> VIO_ (io_ -> vio_) > and misc changes to arch hooks > - update patch according to the IOREQ related dm-op handling changes > - don't include <xen/ioreq.h> from arch header > - make all arch hooks out-of-line > - add a comment above IOREQ_STATUS_* #define-s > --- > xen/arch/arm/Makefile | 2 + > xen/arch/arm/dm.c | 122 +++++++++++++++++++++++ > xen/arch/arm/domain.c | 9 ++ > xen/arch/arm/io.c | 12 ++- > xen/arch/arm/ioreq.c | 213 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/traps.c | 13 +++ > xen/include/asm-arm/domain.h | 3 + > xen/include/asm-arm/hvm/ioreq.h | 72 ++++++++++++++ > xen/include/asm-arm/mmio.h | 1 + > 9 files changed, 446 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/arm/dm.c > create mode 100644 xen/arch/arm/ioreq.c > create mode 100644 xen/include/asm-arm/hvm/ioreq.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 512ffdd..16e6523 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -13,6 +13,7 @@ obj-y += cpuerrata.o > obj-y += cpufeature.o > obj-y += decode.o > obj-y += device.o > +obj-$(CONFIG_IOREQ_SERVER) += dm.o > obj-y += domain.o > obj-y += domain_build.init.o > obj-y += domctl.o > @@ -27,6 +28,7 @@ obj-y += guest_atomics.o > obj-y += guest_walk.o > obj-y += hvm.o > obj-y += io.o > +obj-$(CONFIG_IOREQ_SERVER) += ioreq.o > obj-y += irq.o > obj-y += kernel.init.o > obj-$(CONFIG_LIVEPATCH) += livepatch.o > diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c > new file mode 100644 > index 0000000..e6dedf4 > --- /dev/null > +++ b/xen/arch/arm/dm.c > @@ -0,0 +1,122 @@ > +/* > + * Copyright (c) 2019 Arm ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <xen/dm.h> > +#include <xen/guest_access.h> > +#include <xen/hypercall.h> > +#include <xen/ioreq.h> > +#include <xen/nospec.h> > + > +static int dm_op(const struct dmop_args *op_args) > +{ > + struct domain *d; > + struct xen_dm_op op; > + bool const_op = true; > + long rc; > + size_t offset; > + > + static const uint8_t op_size[] = { > + [XEN_DMOP_create_ioreq_server] = sizeof(struct xen_dm_op_create_ioreq_server), > + [XEN_DMOP_get_ioreq_server_info] = sizeof(struct xen_dm_op_get_ioreq_server_info), > + [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), > + [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), > + [XEN_DMOP_set_ioreq_server_state] = sizeof(struct xen_dm_op_set_ioreq_server_state), > + [XEN_DMOP_destroy_ioreq_server] = sizeof(struct xen_dm_op_destroy_ioreq_server), > + }; > + > + rc = rcu_lock_remote_domain_by_id(op_args->domid, &d); > + if ( rc ) > + return rc; > + > + rc = xsm_dm_op(XSM_DM_PRIV, d); > + if ( rc ) > + goto out; > + > + offset = offsetof(struct xen_dm_op, u); > + > + rc = -EFAULT; > + if ( op_args->buf[0].size < offset ) > + goto out; > + > + if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, offset) ) > + goto out; > + > + if ( op.op >= ARRAY_SIZE(op_size) ) > + { > + rc = -EOPNOTSUPP; > + goto out; > + } > + > + op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size)); > + > + if ( op_args->buf[0].size < offset + op_size[op.op] ) > + goto out; > + > + if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset, > + op_size[op.op]) ) > + goto out; > + > + rc = -EINVAL; > + if ( op.pad ) > + goto out; > + > + rc = ioreq_server_dm_op(&op, d, &const_op); > + > + if ( (!rc || rc == -ERESTART) && > + !const_op && copy_to_guest_offset(op_args->buf[0].h, offset, > + (void *)&op.u, op_size[op.op]) ) > + rc = -EFAULT; > + > + out: > + rcu_unlock_domain(d); > + > + return rc; > +} > + > +long do_dm_op(domid_t domid, > + unsigned int nr_bufs, > + XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs) > +{ > + struct dmop_args args; > + int rc; > + > + if ( nr_bufs > ARRAY_SIZE(args.buf) ) > + return -E2BIG; > + > + args.domid = domid; > + args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1); > + > + if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) ) > + return -EFAULT; > + > + rc = dm_op(&args); > + > + if ( rc == -ERESTART ) > + rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih", > + domid, nr_bufs, bufs); > + > + return rc; > +} I might have missed something in the discussions but this function is identical to xen/arch/x86/hvm/dm.c:do_dm_op, why not make it common? Also the previous function dm_op is very similar to xen/arch/x86/hvm/dm.c:dm_op I would prefer to make them common if possible. Was this already discussed? > +/* > + * 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 18cafcd..8f55aba 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -15,6 +15,7 @@ > #include <xen/guest_access.h> > #include <xen/hypercall.h> > #include <xen/init.h> > +#include <xen/ioreq.h> > #include <xen/lib.h> > #include <xen/livepatch.h> > #include <xen/sched.h> > @@ -696,6 +697,10 @@ int arch_domain_create(struct domain *d, > > ASSERT(config != NULL); > > +#ifdef CONFIG_IOREQ_SERVER > + ioreq_domain_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; > @@ -1014,6 +1019,10 @@ int domain_relinquish_resources(struct domain *d) > if (ret ) > return ret; > > +#ifdef CONFIG_IOREQ_SERVER > + ioreq_server_destroy_all(d); > +#endif > + > PROGRESS(xen): > ret = relinquish_memory(d, &d->xenpage_list); > if ( ret ) > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index ae7ef96..9814481 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/ioreq.h> > #include <xen/lib.h> > #include <xen/spinlock.h> > #include <xen/sched.h> > @@ -23,6 +24,7 @@ > #include <asm/cpuerrata.h> > #include <asm/current.h> > #include <asm/mmio.h> > +#include <asm/hvm/ioreq.h> > > #include "decode.h" > > @@ -123,7 +125,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; > + > + rc = try_fwd_ioserv(regs, v, &info); > + if ( rc == IO_HANDLED ) > + return handle_ioserv(regs, v); > + > + 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..3c4a24d > --- /dev/null > +++ b/xen/arch/arm/ioreq.c > @@ -0,0 +1,213 @@ > +/* > + * 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/domain.h> > +#include <xen/ioreq.h> > + > +#include <asm/traps.h> > + > +#include <public/hvm/ioreq.h> > + > +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > +{ > + 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->io.req.data; > + > + /* We are done with the IO */ > + v->io.req.state = STATE_IOREQ_NONE; > + > + if ( dabt.write ) > + return IO_HANDLED; > + > + /* > + * 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 IO_HANDLED; > +} > + > +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info) > +{ > + struct vcpu_io *vio = &v->io; > + ioreq_t p = { > + .type = IOREQ_TYPE_COPY, > + .addr = info->gpa, > + .size = 1 << info->dabt.size, > + .count = 1, > + .dir = !info->dabt.write, > + /* > + * On x86, df 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. > + */ > + .df = 0, > + .data = get_user_reg(regs, info->dabt.reg), > + .state = STATE_IOREQ_READY, > + }; > + struct ioreq_server *s = NULL; > + enum io_state rc; > + > + switch ( vio->req.state ) > + { > + case STATE_IOREQ_NONE: > + break; > + > + case STATE_IORESP_READY: > + return IO_HANDLED; > + > + default: > + gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); > + return IO_ABORT; > + } > + > + s = ioreq_server_select(v->domain, &p); > + if ( !s ) > + return IO_UNHANDLED; > + > + if ( !info->dabt.valid ) > + return IO_ABORT; > + > + vio->req = p; > + > + rc = ioreq_send(s, &p, 0); > + if ( rc != IO_RETRY || v->domain->is_shutting_down ) > + vio->req.state = STATE_IOREQ_NONE; > + else if ( !ioreq_needs_completion(&vio->req) ) > + rc = IO_HANDLED; > + else > + vio->completion = VIO_mmio_completion; > + > + return rc; > +} > + > +bool arch_ioreq_complete_mmio(void) > +{ > + struct vcpu *v = current; > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + const union hsr hsr = { .bits = regs->hsr }; > + paddr_t addr = v->io.req.addr; > + > + if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > + { > + advance_pc(regs, hsr); > + return true; > + } > + > + return false; > +} > + > +bool arch_vcpu_ioreq_completion(enum vio_completion completion) > +{ > + ASSERT_UNREACHABLE(); > + return true; > +} > + > +/* > + * The "legacy" mechanism of mapping magic pages for the IOREQ servers > + * is x86 specific, so the following hooks don't need to be implemented on Arm: > + * - arch_ioreq_server_map_pages > + * - arch_ioreq_server_unmap_pages > + * - arch_ioreq_server_enable > + * - arch_ioreq_server_disable > + */ > +int arch_ioreq_server_map_pages(struct ioreq_server *s) > +{ > + return -EOPNOTSUPP; > +} > + > +void arch_ioreq_server_unmap_pages(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_enable(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_disable(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_destroy(struct ioreq_server *s) > +{ > +} > + > +int arch_ioreq_server_map_mem_type(struct domain *d, > + struct ioreq_server *s, > + uint32_t flags) > +{ > + return -EOPNOTSUPP; > +} > + > +void arch_ioreq_server_map_mem_type_completed(struct domain *d, > + struct ioreq_server *s, > + uint32_t flags) > +{ > +} > + > +bool arch_ioreq_server_destroy_all(struct domain *d) > +{ > + return true; > +} > + > +bool arch_ioreq_server_get_type_addr(const struct domain *d, > + const ioreq_t *p, > + uint8_t *type, > + uint64_t *addr) > +{ > + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) > + return false; > + > + *type = (p->type == IOREQ_TYPE_PIO) ? > + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; > + *addr = p->addr; > + > + return true; > +} > + > +void arch_ioreq_domain_init(struct domain *d) > +{ > +} > + > +/* > + * 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 22bd1bd..036b13f 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -21,6 +21,7 @@ > #include <xen/hypercall.h> > #include <xen/init.h> > #include <xen/iocap.h> > +#include <xen/ioreq.h> > #include <xen/irq.h> > #include <xen/lib.h> > #include <xen/mem_access.h> > @@ -1385,6 +1386,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 > @@ -1956,6 +1960,9 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, > case IO_HANDLED: > advance_pc(regs, hsr); > return; > + case IO_RETRY: > + /* finish later */ > + return; > case IO_UNHANDLED: > /* IO unhandled, try another way to handle it. */ > break; > @@ -2254,6 +2261,12 @@ static void check_for_vcpu_work(void) > { > struct vcpu *v = current; > > +#ifdef CONFIG_IOREQ_SERVER > + local_irq_enable(); > + vcpu_ioreq_handle_completion(v); > + local_irq_disable(); > +#endif > + > if ( likely(!v->arch.need_flush_to_ram) ) > return; > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 6819a3b..c235e5b 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -10,6 +10,7 @@ > #include <asm/gic.h> > #include <asm/vgic.h> > #include <asm/vpl011.h> > +#include <public/hvm/dm_op.h> > #include <public/hvm/params.h> > > struct hvm_domain > @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > +#define has_vpci(d) ({ (void)(d); false; }) > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/asm-arm/hvm/ioreq.h b/xen/include/asm-arm/hvm/ioreq.h > new file mode 100644 > index 0000000..19e1247 > --- /dev/null > +++ b/xen/include/asm-arm/hvm/ioreq.h > @@ -0,0 +1,72 @@ > +/* > + * 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__ > + > +#ifdef CONFIG_IOREQ_SERVER > +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v); > +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info); > +#else > +static inline enum io_state handle_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v) > +{ > + return IO_UNHANDLED; > +} > + > +static inline enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info) > +{ > + return IO_UNHANDLED; > +} > +#endif > + > +bool ioreq_complete_mmio(void); > + > +static inline bool handle_pio(uint16_t port, unsigned int size, int dir) > +{ > + /* > + * TODO: For Arm64, the main user will be PCI. So this should be > + * implemented when we add support for vPCI. > + */ > + ASSERT_UNREACHABLE(); > + return true; > +} > + > +static inline void msix_write_completion(struct vcpu *v) > +{ > +} > + > +/* This correlation must not be altered */ > +#define IOREQ_STATUS_HANDLED IO_HANDLED > +#define IOREQ_STATUS_UNHANDLED IO_UNHANDLED > +#define IOREQ_STATUS_RETRY IO_RETRY > + > +#endif /* __ASM_ARM_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, > -- > 2.7.4 >
On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 18cafcd..8f55aba 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -15,6 +15,7 @@ > #include <xen/guest_access.h> > #include <xen/hypercall.h> > #include <xen/init.h> > +#include <xen/ioreq.h> > #include <xen/lib.h> > #include <xen/livepatch.h> > #include <xen/sched.h> > @@ -696,6 +697,10 @@ int arch_domain_create(struct domain *d, > > ASSERT(config != NULL); > > +#ifdef CONFIG_IOREQ_SERVER > + ioreq_domain_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; > @@ -1014,6 +1019,10 @@ int domain_relinquish_resources(struct domain *d) > if (ret ) > return ret; > > +#ifdef CONFIG_IOREQ_SERVER > + ioreq_server_destroy_all(d); > +#endif The placement of this call feels quite odd. Shouldn't this moved in case 0? > + > PROGRESS(xen): > ret = relinquish_memory(d, &d->xenpage_list); > if ( ret ) > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index ae7ef96..9814481 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/ioreq.h> > #include <xen/lib.h> > #include <xen/spinlock.h> > #include <xen/sched.h> > @@ -23,6 +24,7 @@ > #include <asm/cpuerrata.h> > #include <asm/current.h> > #include <asm/mmio.h> > +#include <asm/hvm/ioreq.h> Shouldn't this have been included by "xen/ioreq.h"? > > #include "decode.h" > > @@ -123,7 +125,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; > + > + rc = try_fwd_ioserv(regs, v, &info); > + if ( rc == IO_HANDLED ) > + return handle_ioserv(regs, v); > + > + 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..3c4a24d > --- /dev/null > +++ b/xen/arch/arm/ioreq.c > @@ -0,0 +1,213 @@ > +/* > + * 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/domain.h> > +#include <xen/ioreq.h> > + > +#include <asm/traps.h> > + > +#include <public/hvm/ioreq.h> > + > +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > +{ > + 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->io.req.data; > + > + /* We are done with the IO */ > + v->io.req.state = STATE_IOREQ_NONE; > + > + if ( dabt.write ) > + return IO_HANDLED; > + > + /* > + * 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; > + } Looking at the rest of the series, this code is going to be refactored in patch #19 and then hardened. It would have been better to do the refactoring first and then use it. This helps a lot for the review and to reduce what I would call churn in the series. I am OK to keep it like that for this series. > + > + set_user_reg(regs, dabt.reg, r); > + > + return IO_HANDLED; > +} > + > +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info) > +{ > + struct vcpu_io *vio = &v->io; > + ioreq_t p = { > + .type = IOREQ_TYPE_COPY, > + .addr = info->gpa, > + .size = 1 << info->dabt.size, > + .count = 1, > + .dir = !info->dabt.write, > + /* > + * On x86, df 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. > + */ > + .df = 0, > + .data = get_user_reg(regs, info->dabt.reg), > + .state = STATE_IOREQ_READY, > + }; > + struct ioreq_server *s = NULL; > + enum io_state rc; > + > + switch ( vio->req.state ) > + { > + case STATE_IOREQ_NONE: > + break; > + > + case STATE_IORESP_READY: > + return IO_HANDLED; With the Arm code in mind, I am a bit confused with this check. If vio->req.state == STATE_IORESP_READY, then it would imply that the previous I/O emulation was somehow not completed (from Xen PoV). If you return IO_HANDLED here, then it means the we will take care of previous I/O but the current one is going to be ignored. So shouldn't we use the default path here as well? > + > + default: > + gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); > + return IO_ABORT; > + } > + > + s = ioreq_server_select(v->domain, &p); > + if ( !s ) > + return IO_UNHANDLED; > + > + if ( !info->dabt.valid ) > + return IO_ABORT; > + > + vio->req = p; > + > + rc = ioreq_send(s, &p, 0); > + if ( rc != IO_RETRY || v->domain->is_shutting_down ) > + vio->req.state = STATE_IOREQ_NONE; > + else if ( !ioreq_needs_completion(&vio->req) ) > + rc = IO_HANDLED; > + else > + vio->completion = VIO_mmio_completion; > + > + return rc; > +} > + > +bool arch_ioreq_complete_mmio(void) > +{ > + struct vcpu *v = current; > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + const union hsr hsr = { .bits = regs->hsr }; > + paddr_t addr = v->io.req.addr; > + > + if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > + { > + advance_pc(regs, hsr); > + return true; > + } > + > + return false; > +} > + > +bool arch_vcpu_ioreq_completion(enum vio_completion completion) > +{ > + ASSERT_UNREACHABLE(); > + return true; > +} > + > +/* > + * The "legacy" mechanism of mapping magic pages for the IOREQ servers > + * is x86 specific, so the following hooks don't need to be implemented on Arm: > + * - arch_ioreq_server_map_pages > + * - arch_ioreq_server_unmap_pages > + * - arch_ioreq_server_enable > + * - arch_ioreq_server_disable > + */ > +int arch_ioreq_server_map_pages(struct ioreq_server *s) > +{ > + return -EOPNOTSUPP; > +} > + > +void arch_ioreq_server_unmap_pages(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_enable(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_disable(struct ioreq_server *s) > +{ > +} > + > +void arch_ioreq_server_destroy(struct ioreq_server *s) > +{ > +} > + > +int arch_ioreq_server_map_mem_type(struct domain *d, > + struct ioreq_server *s, > + uint32_t flags) > +{ > + return -EOPNOTSUPP; > +} > + > +void arch_ioreq_server_map_mem_type_completed(struct domain *d, > + struct ioreq_server *s, > + uint32_t flags) > +{ > +} > + > +bool arch_ioreq_server_destroy_all(struct domain *d) > +{ > + return true; > +} > + > +bool arch_ioreq_server_get_type_addr(const struct domain *d, > + const ioreq_t *p, > + uint8_t *type, > + uint64_t *addr) > +{ > + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) > + return false; > + > + *type = (p->type == IOREQ_TYPE_PIO) ? > + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; > + *addr = p->addr; > + > + return true; > +} > + > +void arch_ioreq_domain_init(struct domain *d) > +{ > +} > + > +/* > + * 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 22bd1bd..036b13f 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -21,6 +21,7 @@ > #include <xen/hypercall.h> > #include <xen/init.h> > #include <xen/iocap.h> > +#include <xen/ioreq.h> > #include <xen/irq.h> > #include <xen/lib.h> > #include <xen/mem_access.h> > @@ -1385,6 +1386,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 > @@ -1956,6 +1960,9 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, > case IO_HANDLED: > advance_pc(regs, hsr); > return; > + case IO_RETRY: > + /* finish later */ > + return; > case IO_UNHANDLED: > /* IO unhandled, try another way to handle it. */ > break; > @@ -2254,6 +2261,12 @@ static void check_for_vcpu_work(void) > { > struct vcpu *v = current; > > +#ifdef CONFIG_IOREQ_SERVER > + local_irq_enable(); > + vcpu_ioreq_handle_completion(v); > + local_irq_disable(); > +#endif > + > if ( likely(!v->arch.need_flush_to_ram) ) > return; > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 6819a3b..c235e5b 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -10,6 +10,7 @@ > #include <asm/gic.h> > #include <asm/vgic.h> > #include <asm/vpl011.h> > +#include <public/hvm/dm_op.h> May I ask, why do you need to include dm_op.h here? > #include <public/hvm/params.h> > > struct hvm_domain > @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > +#define has_vpci(d) ({ (void)(d); false; }) > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/asm-arm/hvm/ioreq.h b/xen/include/asm-arm/hvm/ioreq.h > new file mode 100644 > index 0000000..19e1247 > --- /dev/null > +++ b/xen/include/asm-arm/hvm/ioreq.h Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as the IOREQ is now meant to be agnostic? > @@ -0,0 +1,72 @@ > +/* > + * 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__ > + > +#ifdef CONFIG_IOREQ_SERVER > +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v); > +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info); > +#else > +static inline enum io_state handle_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v) > +{ > + return IO_UNHANDLED; > +} > + > +static inline enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > + struct vcpu *v, mmio_info_t *info) > +{ > + return IO_UNHANDLED; > +} > +#endif > + > +bool ioreq_complete_mmio(void); > + > +static inline bool handle_pio(uint16_t port, unsigned int size, int dir) > +{ > + /* > + * TODO: For Arm64, the main user will be PCI. So this should be > + * implemented when we add support for vPCI. > + */ > + ASSERT_UNREACHABLE(); > + return true; > +} > + > +static inline void msix_write_completion(struct vcpu *v) > +{ > +} > + > +/* This correlation must not be altered */ > +#define IOREQ_STATUS_HANDLED IO_HANDLED > +#define IOREQ_STATUS_UNHANDLED IO_UNHANDLED > +#define IOREQ_STATUS_RETRY IO_RETRY > + > +#endif /* __ASM_ARM_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, >
On 15.01.21 02:55, Stefano Stabellini wrote: Hi Stefano > On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: >> From: Julien Grall <julien.grall@arm.com> >> >> This patch adds basic IOREQ/DM support on Arm. The subsequent >> patches will improve functionality and add remaining bits. >> >> The IOREQ/DM features are supposed to be built with IOREQ_SERVER >> option enabled, which is disabled by default on Arm for now. >> >> Please note, the "PIO handling" TODO is expected to left unaddressed >> for the current series. It is not an big issue for now while Xen >> doesn't have support for vPCI on Arm. On Arm64 they are only used >> for PCI IO Bar and we would probably want to expose them to emulator >> as PIO access to make a DM completely arch-agnostic. So "PIO handling" >> should be implemented when we add support for vPCI. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> [On Arm only] >> Tested-by: Wei Chen <Wei.Chen@arm.com> >> >> --- >> Please note, this is a split/cleanup/hardening of Julien's PoC: >> "Add support for Guest IO forwarding to a device emulator" >> >> Changes RFC -> V1: >> - was split into: >> - arm/ioreq: Introduce arch specific bits for IOREQ/DM features >> - xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm >> - update patch description >> - update asm-arm/hvm/ioreq.h according to the newly introduced arch functions: >> - arch_hvm_destroy_ioreq_server() >> - arch_handle_hvm_io_completion() >> - update arch files to include xen/ioreq.h >> - remove HVMOP plumbing >> - rewrite a logic to handle properly case when hvm_send_ioreq() returns IO_RETRY >> - add a logic to handle properly handle_hvm_io_completion() return value >> - rename handle_mmio() to ioreq_handle_complete_mmio() >> - move paging_mark_pfn_dirty() to asm-arm/paging.h >> - remove forward declaration for hvm_ioreq_server in asm-arm/paging.h >> - move try_fwd_ioserv() to ioreq.c, provide stubs if !CONFIG_IOREQ_SERVER >> - do not remove #ifdef CONFIG_IOREQ_SERVER in memory.c for guarding xen/ioreq.h >> - use gdprintk in try_fwd_ioserv(), remove unneeded prints >> - update list of #include-s >> - move has_vpci() to asm-arm/domain.h >> - add a comment (TODO) to unimplemented yet handle_pio() >> - remove hvm_mmio_first(last)_byte() and hvm_ioreq_(page/vcpu/server) structs >> from the arch files, they were already moved to the common code >> - remove set_foreign_p2m_entry() changes, they will be properly implemented >> in the follow-up patch >> - select IOREQ_SERVER for Arm instead of Arm64 in Kconfig >> - remove x86's realmode and other unneeded stubs from xen/ioreq.h >> - clafify ioreq_t p.df usage in try_fwd_ioserv() >> - set ioreq_t p.count to 1 in try_fwd_ioserv() >> >> Changes V1 -> V2: >> - was split into: >> - arm/ioreq: Introduce arch specific bits for IOREQ/DM features >> - xen/arm: Stick around in leave_hypervisor_to_guest until I/O has completed >> - update the author of a patch >> - update patch description >> - move a loop in leave_hypervisor_to_guest() to a separate patch >> - set IOREQ_SERVER disabled by default >> - remove already clarified /* XXX */ >> - replace BUG() by ASSERT_UNREACHABLE() in handle_pio() >> - remove default case for handling the return value of try_handle_mmio() >> - remove struct hvm_domain, enum hvm_io_completion, struct hvm_vcpu_io, >> struct hvm_vcpu from asm-arm/domain.h, these are common materials now >> - update everything according to the recent changes (IOREQ related function >> names don't contain "hvm" prefixes/infixes anymore, IOREQ related fields >> are part of common struct vcpu/domain now, etc) >> >> Changes V2 -> V3: >> - update patch according the "legacy interface" is x86 specific >> - add dummy arch hooks >> - remove dummy paging_mark_pfn_dirty() >> - don’t include <xen/domain_page.h> in common ioreq.c >> - don’t include <public/hvm/ioreq.h> in arch ioreq.h >> - remove #define ioreq_params(d, i) >> >> Changes V3 -> V4: >> - rebase >> - update patch according to the renaming IO_ -> VIO_ (io_ -> vio_) >> and misc changes to arch hooks >> - update patch according to the IOREQ related dm-op handling changes >> - don't include <xen/ioreq.h> from arch header >> - make all arch hooks out-of-line >> - add a comment above IOREQ_STATUS_* #define-s >> --- >> xen/arch/arm/Makefile | 2 + >> xen/arch/arm/dm.c | 122 +++++++++++++++++++++++ >> xen/arch/arm/domain.c | 9 ++ >> xen/arch/arm/io.c | 12 ++- >> xen/arch/arm/ioreq.c | 213 ++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/traps.c | 13 +++ >> xen/include/asm-arm/domain.h | 3 + >> xen/include/asm-arm/hvm/ioreq.h | 72 ++++++++++++++ >> xen/include/asm-arm/mmio.h | 1 + >> 9 files changed, 446 insertions(+), 1 deletion(-) >> create mode 100644 xen/arch/arm/dm.c >> create mode 100644 xen/arch/arm/ioreq.c >> create mode 100644 xen/include/asm-arm/hvm/ioreq.h >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 512ffdd..16e6523 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -13,6 +13,7 @@ obj-y += cpuerrata.o >> obj-y += cpufeature.o >> obj-y += decode.o >> obj-y += device.o >> +obj-$(CONFIG_IOREQ_SERVER) += dm.o >> obj-y += domain.o >> obj-y += domain_build.init.o >> obj-y += domctl.o >> @@ -27,6 +28,7 @@ obj-y += guest_atomics.o >> obj-y += guest_walk.o >> obj-y += hvm.o >> obj-y += io.o >> +obj-$(CONFIG_IOREQ_SERVER) += ioreq.o >> obj-y += irq.o >> obj-y += kernel.init.o >> obj-$(CONFIG_LIVEPATCH) += livepatch.o >> diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c >> new file mode 100644 >> index 0000000..e6dedf4 >> --- /dev/null >> +++ b/xen/arch/arm/dm.c >> @@ -0,0 +1,122 @@ >> +/* >> + * Copyright (c) 2019 Arm ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <xen/dm.h> >> +#include <xen/guest_access.h> >> +#include <xen/hypercall.h> >> +#include <xen/ioreq.h> >> +#include <xen/nospec.h> >> + >> +static int dm_op(const struct dmop_args *op_args) >> +{ >> + struct domain *d; >> + struct xen_dm_op op; >> + bool const_op = true; >> + long rc; >> + size_t offset; >> + >> + static const uint8_t op_size[] = { >> + [XEN_DMOP_create_ioreq_server] = sizeof(struct xen_dm_op_create_ioreq_server), >> + [XEN_DMOP_get_ioreq_server_info] = sizeof(struct xen_dm_op_get_ioreq_server_info), >> + [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), >> + [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), >> + [XEN_DMOP_set_ioreq_server_state] = sizeof(struct xen_dm_op_set_ioreq_server_state), >> + [XEN_DMOP_destroy_ioreq_server] = sizeof(struct xen_dm_op_destroy_ioreq_server), >> + }; >> + >> + rc = rcu_lock_remote_domain_by_id(op_args->domid, &d); >> + if ( rc ) >> + return rc; >> + >> + rc = xsm_dm_op(XSM_DM_PRIV, d); >> + if ( rc ) >> + goto out; >> + >> + offset = offsetof(struct xen_dm_op, u); >> + >> + rc = -EFAULT; >> + if ( op_args->buf[0].size < offset ) >> + goto out; >> + >> + if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, offset) ) >> + goto out; >> + >> + if ( op.op >= ARRAY_SIZE(op_size) ) >> + { >> + rc = -EOPNOTSUPP; >> + goto out; >> + } >> + >> + op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size)); >> + >> + if ( op_args->buf[0].size < offset + op_size[op.op] ) >> + goto out; >> + >> + if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset, >> + op_size[op.op]) ) >> + goto out; >> + >> + rc = -EINVAL; >> + if ( op.pad ) >> + goto out; >> + >> + rc = ioreq_server_dm_op(&op, d, &const_op); >> + >> + if ( (!rc || rc == -ERESTART) && >> + !const_op && copy_to_guest_offset(op_args->buf[0].h, offset, >> + (void *)&op.u, op_size[op.op]) ) >> + rc = -EFAULT; >> + >> + out: >> + rcu_unlock_domain(d); >> + >> + return rc; >> +} >> + >> +long do_dm_op(domid_t domid, >> + unsigned int nr_bufs, >> + XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs) >> +{ >> + struct dmop_args args; >> + int rc; >> + >> + if ( nr_bufs > ARRAY_SIZE(args.buf) ) >> + return -E2BIG; >> + >> + args.domid = domid; >> + args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1); >> + >> + if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) ) >> + return -EFAULT; >> + >> + rc = dm_op(&args); >> + >> + if ( rc == -ERESTART ) >> + rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih", >> + domid, nr_bufs, bufs); >> + >> + return rc; >> +} > I might have missed something in the discussions but this function is > identical to xen/arch/x86/hvm/dm.c:do_dm_op, why not make it common? > > Also the previous function dm_op is very similar to > xen/arch/x86/hvm/dm.c:dm_op I would prefer to make them common if > possible. Was this already discussed? Well, let me explain. Both dm_op() and do_dm_op() were indeed common (top level dm-op handling common) for previous versions, so Arm's dm.c didn't contain this stuff. The idea to make it other way around (top level dm-op handling arch-specific and call into ioreq_server_dm_op() for otherwise unhandled ops) was discussed at [1] which besides it's Pros leads to code duplication, so Arm's dm.c has to duplicate some stuff, etc. I was thinking about moving do_dm_op() which is _same_ for both arches to common code, but I am not sure whether it is conceptually correct which that new "alternative" approach of handling dm-op. Please see [2]. [1] https://lore.kernel.org/xen-devel/1606732298-22107-10-git-send-email-olekstysh@gmail.com/ [2] https://lore.kernel.org/xen-devel/1610488352-18494-10-git-send-email-olekstysh@gmail.com/
On 15.01.21 22:26, Julien Grall wrote: Hi Julien > > > On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 18cafcd..8f55aba 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -15,6 +15,7 @@ >> #include <xen/guest_access.h> >> #include <xen/hypercall.h> >> #include <xen/init.h> >> +#include <xen/ioreq.h> >> #include <xen/lib.h> >> #include <xen/livepatch.h> >> #include <xen/sched.h> >> @@ -696,6 +697,10 @@ int arch_domain_create(struct domain *d, >> ASSERT(config != NULL); >> +#ifdef CONFIG_IOREQ_SERVER >> + ioreq_domain_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; >> @@ -1014,6 +1019,10 @@ int domain_relinquish_resources(struct domain *d) >> if (ret ) >> return ret; >> +#ifdef CONFIG_IOREQ_SERVER >> + ioreq_server_destroy_all(d); >> +#endif > > The placement of this call feels quite odd. Shouldn't this moved in > case 0? Indeed it is odd to call it here, will move. > >> + >> PROGRESS(xen): >> ret = relinquish_memory(d, &d->xenpage_list); >> if ( ret ) >> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >> index ae7ef96..9814481 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/ioreq.h> >> #include <xen/lib.h> >> #include <xen/spinlock.h> >> #include <xen/sched.h> >> @@ -23,6 +24,7 @@ >> #include <asm/cpuerrata.h> >> #include <asm/current.h> >> #include <asm/mmio.h> >> +#include <asm/hvm/ioreq.h> > > Shouldn't this have been included by "xen/ioreq.h"? Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned out that there was nothing inside common header required arch one to be included and I was asked to include arch header where it was indeed needed (several *.c files). > > >> #include "decode.h" >> @@ -123,7 +125,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; >> + >> + rc = try_fwd_ioserv(regs, v, &info); >> + if ( rc == IO_HANDLED ) >> + return handle_ioserv(regs, v); >> + >> + 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..3c4a24d >> --- /dev/null >> +++ b/xen/arch/arm/ioreq.c >> @@ -0,0 +1,213 @@ >> +/* >> + * 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/domain.h> >> +#include <xen/ioreq.h> >> + >> +#include <asm/traps.h> >> + >> +#include <public/hvm/ioreq.h> >> + >> +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) >> +{ >> + 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->io.req.data; >> + >> + /* We are done with the IO */ >> + v->io.req.state = STATE_IOREQ_NONE; >> + >> + if ( dabt.write ) >> + return IO_HANDLED; >> + >> + /* >> + * 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; >> + } > > Looking at the rest of the series, this code is going to be refactored > in patch #19 and then hardened. It would have been better to do the > refactoring first and then use it. > > This helps a lot for the review and to reduce what I would call churn > in the series. Agree, it would be better. > > > I am OK to keep it like that for this series. Thank you, this saves me some time. > > >> + >> + set_user_reg(regs, dabt.reg, r); >> + >> + return IO_HANDLED; >> +} >> + >> +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >> + struct vcpu *v, mmio_info_t *info) >> +{ >> + struct vcpu_io *vio = &v->io; >> + ioreq_t p = { >> + .type = IOREQ_TYPE_COPY, >> + .addr = info->gpa, >> + .size = 1 << info->dabt.size, >> + .count = 1, >> + .dir = !info->dabt.write, >> + /* >> + * On x86, df 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. >> + */ >> + .df = 0, >> + .data = get_user_reg(regs, info->dabt.reg), >> + .state = STATE_IOREQ_READY, >> + }; >> + struct ioreq_server *s = NULL; >> + enum io_state rc; >> + >> + switch ( vio->req.state ) >> + { >> + case STATE_IOREQ_NONE: >> + break; >> + >> + case STATE_IORESP_READY: >> + return IO_HANDLED; > > With the Arm code in mind, I am a bit confused with this check. If > vio->req.state == STATE_IORESP_READY, then it would imply that the > previous I/O emulation was somehow not completed (from Xen PoV). Agree > > If you return IO_HANDLED here, then it means the we will take care of > previous I/O but the current one is going to be ignored. Which current one? As I understand, if try_fwd_ioserv() gets called with vio->req.state == STATE_IORESP_READY then this is a second round after emulator completes the emulation (the first round was when we returned IO_RETRY down the function and claimed that we would need a completion), so we are still dealing with previous I/O. vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() And after we return IO_HANDLED here, handle_ioserv() will be called to complete the handling of this previous I/O emulation. Or I really missed something? > So shouldn't we use the default path here as well? I am afraid, I don't entirely get the suggestion. > > >> + >> + default: >> + gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); >> + return IO_ABORT; >> + } >> + >> + s = ioreq_server_select(v->domain, &p); >> + if ( !s ) >> + return IO_UNHANDLED; >> + >> + if ( !info->dabt.valid ) >> + return IO_ABORT; >> + >> + vio->req = p; >> + >> + rc = ioreq_send(s, &p, 0); >> + if ( rc != IO_RETRY || v->domain->is_shutting_down ) >> + vio->req.state = STATE_IOREQ_NONE; >> + else if ( !ioreq_needs_completion(&vio->req) ) >> + rc = IO_HANDLED; >> + else >> + vio->completion = VIO_mmio_completion; >> + >> + return rc; >> +} >> + >> +bool arch_ioreq_complete_mmio(void) >> +{ >> + struct vcpu *v = current; >> + struct cpu_user_regs *regs = guest_cpu_user_regs(); >> + const union hsr hsr = { .bits = regs->hsr }; >> + paddr_t addr = v->io.req.addr; >> + >> + if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) >> + { >> + advance_pc(regs, hsr); >> + return true; >> + } >> + >> + return false; >> +} >> + >> +bool arch_vcpu_ioreq_completion(enum vio_completion completion) >> +{ >> + ASSERT_UNREACHABLE(); >> + return true; >> +} >> + >> +/* >> + * The "legacy" mechanism of mapping magic pages for the IOREQ servers >> + * is x86 specific, so the following hooks don't need to be >> implemented on Arm: >> + * - arch_ioreq_server_map_pages >> + * - arch_ioreq_server_unmap_pages >> + * - arch_ioreq_server_enable >> + * - arch_ioreq_server_disable >> + */ >> +int arch_ioreq_server_map_pages(struct ioreq_server *s) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +void arch_ioreq_server_unmap_pages(struct ioreq_server *s) >> +{ >> +} >> + >> +void arch_ioreq_server_enable(struct ioreq_server *s) >> +{ >> +} >> + >> +void arch_ioreq_server_disable(struct ioreq_server *s) >> +{ >> +} >> + >> +void arch_ioreq_server_destroy(struct ioreq_server *s) >> +{ >> +} >> + >> +int arch_ioreq_server_map_mem_type(struct domain *d, >> + struct ioreq_server *s, >> + uint32_t flags) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +void arch_ioreq_server_map_mem_type_completed(struct domain *d, >> + struct ioreq_server *s, >> + uint32_t flags) >> +{ >> +} >> + >> +bool arch_ioreq_server_destroy_all(struct domain *d) >> +{ >> + return true; >> +} >> + >> +bool arch_ioreq_server_get_type_addr(const struct domain *d, >> + const ioreq_t *p, >> + uint8_t *type, >> + uint64_t *addr) >> +{ >> + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) >> + return false; >> + >> + *type = (p->type == IOREQ_TYPE_PIO) ? >> + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; >> + *addr = p->addr; >> + >> + return true; >> +} >> + >> +void arch_ioreq_domain_init(struct domain *d) >> +{ >> +} >> + >> +/* >> + * 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 22bd1bd..036b13f 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -21,6 +21,7 @@ >> #include <xen/hypercall.h> >> #include <xen/init.h> >> #include <xen/iocap.h> >> +#include <xen/ioreq.h> >> #include <xen/irq.h> >> #include <xen/lib.h> >> #include <xen/mem_access.h> >> @@ -1385,6 +1386,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 >> @@ -1956,6 +1960,9 @@ static void do_trap_stage2_abort_guest(struct >> cpu_user_regs *regs, >> case IO_HANDLED: >> advance_pc(regs, hsr); >> return; >> + case IO_RETRY: >> + /* finish later */ >> + return; >> case IO_UNHANDLED: >> /* IO unhandled, try another way to handle it. */ >> break; >> @@ -2254,6 +2261,12 @@ static void check_for_vcpu_work(void) >> { >> struct vcpu *v = current; >> +#ifdef CONFIG_IOREQ_SERVER >> + local_irq_enable(); >> + vcpu_ioreq_handle_completion(v); >> + local_irq_disable(); >> +#endif >> + >> if ( likely(!v->arch.need_flush_to_ram) ) >> return; >> diff --git a/xen/include/asm-arm/domain.h >> b/xen/include/asm-arm/domain.h >> index 6819a3b..c235e5b 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -10,6 +10,7 @@ >> #include <asm/gic.h> >> #include <asm/vgic.h> >> #include <asm/vpl011.h> >> +#include <public/hvm/dm_op.h> > > May I ask, why do you need to include dm_op.h here? I needed to include that header to make some bits visible (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a really good question. I don't remember exactly, probably I followed x86's domain.h which also included it. So, trying to remove the inclusion here, I get several build failures on Arm which could be fixed if I include that header from dm.h and ioreq.h: Shall I do this way? diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 6bd2d85..9de7c4e 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -10,7 +10,6 @@ #include <asm/gic.h> #include <asm/vgic.h> #include <asm/vpl011.h> -#include <public/hvm/dm_op.h> #include <public/hvm/params.h> struct hvm_domain diff --git a/xen/include/xen/dm.h b/xen/include/xen/dm.h index 2c9952d..4ce6655 100644 --- a/xen/include/xen/dm.h +++ b/xen/include/xen/dm.h @@ -19,6 +19,8 @@ #include <xen/sched.h> +#include <public/hvm/dm_op.h> + struct dmop_args { domid_t domid; unsigned int nr_bufs; diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h index dc47ec7..7b74983 100644 --- a/xen/include/xen/ioreq.h +++ b/xen/include/xen/ioreq.h @@ -21,6 +21,8 @@ #include <xen/sched.h> +#include <public/hvm/dm_op.h> + struct ioreq_page { gfn_t gfn; struct page_info *page; (END) > > >> #include <public/hvm/params.h> >> struct hvm_domain >> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >> *v) {} >> #define arch_vm_assist_valid_mask(d) (1UL << >> VMASST_TYPE_runstate_update_flag) >> +#define has_vpci(d) ({ (void)(d); false; }) >> + >> #endif /* __ASM_DOMAIN_H__ */ >> /* >> diff --git a/xen/include/asm-arm/hvm/ioreq.h >> b/xen/include/asm-arm/hvm/ioreq.h >> new file mode 100644 >> index 0000000..19e1247 >> --- /dev/null >> +++ b/xen/include/asm-arm/hvm/ioreq.h > > Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as > the IOREQ is now meant to be agnostic? Good question... The _common_ IOREQ code is indeed arch-agnostic. But, can the _arch_ IOREQ code be treated as really subarch-agnostic? I think, on Arm it can and it is most likely ok to keep it in "asm-arm/", but how it would be correlated with x86's IOREQ code which is HVM specific and located in "hvm" subdir? > >> @@ -0,0 +1,72 @@ >> +/* >> + * 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__ >> + >> +#ifdef CONFIG_IOREQ_SERVER >> +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu >> *v); >> +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >> + struct vcpu *v, mmio_info_t *info); >> +#else >> +static inline enum io_state handle_ioserv(struct cpu_user_regs *regs, >> + struct vcpu *v) >> +{ >> + return IO_UNHANDLED; >> +} >> + >> +static inline enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >> + struct vcpu *v, >> mmio_info_t *info) >> +{ >> + return IO_UNHANDLED; >> +} >> +#endif >> + >> +bool ioreq_complete_mmio(void); >> + >> +static inline bool handle_pio(uint16_t port, unsigned int size, int >> dir) >> +{ >> + /* >> + * TODO: For Arm64, the main user will be PCI. So this should be >> + * implemented when we add support for vPCI. >> + */ >> + ASSERT_UNREACHABLE(); >> + return true; >> +} >> + >> +static inline void msix_write_completion(struct vcpu *v) >> +{ >> +} >> + >> +/* This correlation must not be altered */ >> +#define IOREQ_STATUS_HANDLED IO_HANDLED >> +#define IOREQ_STATUS_UNHANDLED IO_UNHANDLED >> +#define IOREQ_STATUS_RETRY IO_RETRY >> + >> +#endif /* __ASM_ARM_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, >> >
On 17/01/2021 17:11, Oleksandr wrote: > > On 15.01.21 22:26, Julien Grall wrote: > > Hi Julien Hi Oleksandr, >> >>> + >>> PROGRESS(xen): >>> ret = relinquish_memory(d, &d->xenpage_list); >>> if ( ret ) >>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >>> index ae7ef96..9814481 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/ioreq.h> >>> #include <xen/lib.h> >>> #include <xen/spinlock.h> >>> #include <xen/sched.h> >>> @@ -23,6 +24,7 @@ >>> #include <asm/cpuerrata.h> >>> #include <asm/current.h> >>> #include <asm/mmio.h> >>> +#include <asm/hvm/ioreq.h> >> >> Shouldn't this have been included by "xen/ioreq.h"? > Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned > out that there was nothing inside common header required arch one to be > included and > I was asked to include arch header where it was indeed needed (several > *.c files). Fair enough. [...] >> >> If you return IO_HANDLED here, then it means the we will take care of >> previous I/O but the current one is going to be ignored. > Which current one? As I understand, if try_fwd_ioserv() gets called with > vio->req.state == STATE_IORESP_READY then this is a second round after > emulator completes the emulation (the first round was when > we returned IO_RETRY down the function and claimed that we would need a > completion), so we are still dealing with previous I/O. > vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> > try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() > And after we return IO_HANDLED here, handle_ioserv() will be called to > complete the handling of this previous I/O emulation. > Or I really missed something? Hmmm... I somehow thought try_fw_ioserv() would only be called the first time. Do you have a branch with your code applied? This would help to follow the different paths. >>> diff --git a/xen/include/asm-arm/domain.h >>> b/xen/include/asm-arm/domain.h >>> index 6819a3b..c235e5b 100644 >>> --- a/xen/include/asm-arm/domain.h >>> +++ b/xen/include/asm-arm/domain.h >>> @@ -10,6 +10,7 @@ >>> #include <asm/gic.h> >>> #include <asm/vgic.h> >>> #include <asm/vpl011.h> >>> +#include <public/hvm/dm_op.h> >> >> May I ask, why do you need to include dm_op.h here? > I needed to include that header to make some bits visible > (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a > really good question. > I don't remember exactly, probably I followed x86's domain.h which also > included it. > So, trying to remove the inclusion here, I get several build failures on > Arm which could be fixed if I include that header from dm.h and ioreq.h: > > Shall I do this way? If the failure are indeded because ioreq.h and dm.h use definition from public/hvm/dm_op.h, then yes. Can you post the errors? [...] >>> #include <public/hvm/params.h> >>> struct hvm_domain >>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>> *v) {} >>> #define arch_vm_assist_valid_mask(d) (1UL << >>> VMASST_TYPE_runstate_update_flag) >>> +#define has_vpci(d) ({ (void)(d); false; }) >>> + >>> #endif /* __ASM_DOMAIN_H__ */ >>> /* >>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>> b/xen/include/asm-arm/hvm/ioreq.h >>> new file mode 100644 >>> index 0000000..19e1247 >>> --- /dev/null >>> +++ b/xen/include/asm-arm/hvm/ioreq.h >> >> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as >> the IOREQ is now meant to be agnostic? > Good question... The _common_ IOREQ code is indeed arch-agnostic. But, > can the _arch_ IOREQ code be treated as really subarch-agnostic? > I think, on Arm it can and it is most likely ok to keep it in > "asm-arm/", but how it would be correlated with x86's IOREQ code which > is HVM specific and located > in "hvm" subdir? Sorry, I don't understand your answer/questions. So let me ask the question differently, is asm-arm/hvm/ioreq.h going to be included from common code? If the answer is no, then I see no reason to follow the x86 here. If the answer is yes, then I am quite confused why half of the series tried to remove "hvm" from the function name but we still include "asm/hvm/ioreq.h". Cheers,
On 17.01.21 20:07, Julien Grall wrote: > > > On 17/01/2021 17:11, Oleksandr wrote: >> >> On 15.01.21 22:26, Julien Grall wrote: >> >> Hi Julien > > Hi Oleksandr, Hi Julien > >>> >>>> + >>>> PROGRESS(xen): >>>> ret = relinquish_memory(d, &d->xenpage_list); >>>> if ( ret ) >>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >>>> index ae7ef96..9814481 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/ioreq.h> >>>> #include <xen/lib.h> >>>> #include <xen/spinlock.h> >>>> #include <xen/sched.h> >>>> @@ -23,6 +24,7 @@ >>>> #include <asm/cpuerrata.h> >>>> #include <asm/current.h> >>>> #include <asm/mmio.h> >>>> +#include <asm/hvm/ioreq.h> >>> >>> Shouldn't this have been included by "xen/ioreq.h"? >> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it >> turned out that there was nothing inside common header required arch >> one to be included and >> I was asked to include arch header where it was indeed needed >> (several *.c files). > > Fair enough. > > [...] > >>> >>> If you return IO_HANDLED here, then it means the we will take care >>> of previous I/O but the current one is going to be ignored. >> Which current one? As I understand, if try_fwd_ioserv() gets called >> with vio->req.state == STATE_IORESP_READY then this is a second round >> after emulator completes the emulation (the first round was when >> we returned IO_RETRY down the function and claimed that we would need >> a completion), so we are still dealing with previous I/O. >> vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> >> try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() >> And after we return IO_HANDLED here, handle_ioserv() will be called >> to complete the handling of this previous I/O emulation. >> Or I really missed something? > > Hmmm... I somehow thought try_fw_ioserv() would only be called the > first time. Do you have a branch with your code applied? This would > help to follow the different paths. Yes, I mentioned about it in cover letter. Please see https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5 why 5 - because I started counting from the RFC) > >>>> diff --git a/xen/include/asm-arm/domain.h >>>> b/xen/include/asm-arm/domain.h >>>> index 6819a3b..c235e5b 100644 >>>> --- a/xen/include/asm-arm/domain.h >>>> +++ b/xen/include/asm-arm/domain.h >>>> @@ -10,6 +10,7 @@ >>>> #include <asm/gic.h> >>>> #include <asm/vgic.h> >>>> #include <asm/vpl011.h> >>>> +#include <public/hvm/dm_op.h> >>> >>> May I ask, why do you need to include dm_op.h here? >> I needed to include that header to make some bits visible >> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >> really good question. >> I don't remember exactly, probably I followed x86's domain.h which >> also included it. >> So, trying to remove the inclusion here, I get several build failures >> on Arm which could be fixed if I include that header from dm.h and >> ioreq.h: >> >> Shall I do this way? > > If the failure are indeded because ioreq.h and dm.h use definition > from public/hvm/dm_op.h, then yes. Can you post the errors? Please see attached, although I built for Arm32 (and the whole series), I think errors are valid for Arm64 also. error1.txt - when remove #include <public/hvm/dm_op.h> from asm-arm/domain.h error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h error3.txt - when add #include <public/hvm/dm_op.h> to xen/dm.h > > > [...] > >>>> #include <public/hvm/params.h> >>>> struct hvm_domain >>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>>> *v) {} >>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>> VMASST_TYPE_runstate_update_flag) >>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>> + >>>> #endif /* __ASM_DOMAIN_H__ */ >>>> /* >>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>> b/xen/include/asm-arm/hvm/ioreq.h >>>> new file mode 100644 >>>> index 0000000..19e1247 >>>> --- /dev/null >>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>> >>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ >>> as the IOREQ is now meant to be agnostic? >> Good question... The _common_ IOREQ code is indeed arch-agnostic. >> But, can the _arch_ IOREQ code be treated as really subarch-agnostic? >> I think, on Arm it can and it is most likely ok to keep it in >> "asm-arm/", but how it would be correlated with x86's IOREQ code >> which is HVM specific and located >> in "hvm" subdir? > > Sorry, I don't understand your answer/questions. So let me ask the > question differently, is asm-arm/hvm/ioreq.h going to be included from > common code? Sorry if I was unclear. > > If the answer is no, then I see no reason to follow the x86 here. > If the answer is yes, then I am quite confused why half of the series > tried to remove "hvm" from the function name but we still include > "asm/hvm/ioreq.h". Answer is yes. Even if we could to avoid including that header from the common code somehow, we would still have #include <public/hvm/*>, is_hvm_domain(). > > > Cheers, >
On 17.01.2021 18:11, Oleksandr wrote: > On 15.01.21 22:26, Julien Grall wrote: >> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: >>> --- 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/ioreq.h> >>> #include <xen/lib.h> >>> #include <xen/spinlock.h> >>> #include <xen/sched.h> >>> @@ -23,6 +24,7 @@ >>> #include <asm/cpuerrata.h> >>> #include <asm/current.h> >>> #include <asm/mmio.h> >>> +#include <asm/hvm/ioreq.h> >> >> Shouldn't this have been included by "xen/ioreq.h"? > Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned > out that there was nothing inside common header required arch one to be > included and > I was asked to include arch header where it was indeed needed (several > *.c files). I guess the general usage model of the two headers needs to be established first: If the per-arch header declares only stuff needed by the soon common/ioreq.c, then indeed it should be only that file and the producer(s) of the arch_*() functions which include that header; it should then in particular not be included by xen/ioreq.h. However, with the change request on patch 1 I think that usage model goes away at least for now, at which point the question is what exactly the per-arch header still declares, and based on that it would need to be decided whether xen/ioreq.h should include it. >>> --- a/xen/include/asm-arm/domain.h >>> +++ b/xen/include/asm-arm/domain.h >>> @@ -10,6 +10,7 @@ >>> #include <asm/gic.h> >>> #include <asm/vgic.h> >>> #include <asm/vpl011.h> >>> +#include <public/hvm/dm_op.h> >> >> May I ask, why do you need to include dm_op.h here? > I needed to include that header to make some bits visible > (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a > really good question. > I don't remember exactly, probably I followed x86's domain.h which also > included it. > So, trying to remove the inclusion here, I get several build failures on > Arm which could be fixed if I include that header from dm.h and ioreq.h: > > Shall I do this way? The general rule ought to be that header include what they need, but not more. Header dependencies are quite problematic already, so every dependency we can avoid (or eliminate) will help. This goes as far as only forward declaring structure where possible. >>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>> *v) {} >>> #define arch_vm_assist_valid_mask(d) (1UL << >>> VMASST_TYPE_runstate_update_flag) >>> +#define has_vpci(d) ({ (void)(d); false; }) >>> + >>> #endif /* __ASM_DOMAIN_H__ */ >>> /* >>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>> b/xen/include/asm-arm/hvm/ioreq.h >>> new file mode 100644 >>> index 0000000..19e1247 >>> --- /dev/null >>> +++ b/xen/include/asm-arm/hvm/ioreq.h >> >> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as >> the IOREQ is now meant to be agnostic? > Good question... The _common_ IOREQ code is indeed arch-agnostic. But, > can the _arch_ IOREQ code be treated as really subarch-agnostic? > I think, on Arm it can and it is most likely ok to keep it in > "asm-arm/", but how it would be correlated with x86's IOREQ code which > is HVM specific and located > in "hvm" subdir? I think for Arm's sake this should be used as asm/ioreq.h, where x86 would gain a new header consisting of just #include <asm/hvm/ioreq.h> as there the functionality is needed for HVM only. Jan
On 18.01.21 12:44, Jan Beulich wrote: Hi Jan > On 17.01.2021 18:11, Oleksandr wrote: >> On 15.01.21 22:26, Julien Grall wrote: >>> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: >>>> --- 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/ioreq.h> >>>> #include <xen/lib.h> >>>> #include <xen/spinlock.h> >>>> #include <xen/sched.h> >>>> @@ -23,6 +24,7 @@ >>>> #include <asm/cpuerrata.h> >>>> #include <asm/current.h> >>>> #include <asm/mmio.h> >>>> +#include <asm/hvm/ioreq.h> Note to self: Remove obsolete bool ioreq_complete_mmio(void) from asm-arm/hvm/ioreq.h >>> Shouldn't this have been included by "xen/ioreq.h"? >> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned >> out that there was nothing inside common header required arch one to be >> included and >> I was asked to include arch header where it was indeed needed (several >> *.c files). > I guess the general usage model of the two headers needs to be > established first: If the per-arch header declares only stuff > needed by the soon common/ioreq.c, then indeed it should be > only that file and the producer(s) of the arch_*() functions > which include that header; it should then in particular not be > included by xen/ioreq.h. > > However, with the change request on patch 1 I think that usage > model goes away at least for now, at which point the question > is what exactly the per-arch header still declares, and based > on that it would need to be decided whether xen/ioreq.h > should include it. ok, well. x86's arch header now contains few IOREQ_STATUS_* #define-s, but Arm's contains more stuff besides that: - stuff which is needed by common/ioreq.c, mostly stubs which are not implemented yet (handle_pio, etc) - stuff which is not needed by common/ioreq.c, internal Arm bits (handle_ioserv, try_fwd_ioserv) Could we please decide based on the information above? > >>>> --- a/xen/include/asm-arm/domain.h >>>> +++ b/xen/include/asm-arm/domain.h >>>> @@ -10,6 +10,7 @@ >>>> #include <asm/gic.h> >>>> #include <asm/vgic.h> >>>> #include <asm/vpl011.h> >>>> +#include <public/hvm/dm_op.h> >>> May I ask, why do you need to include dm_op.h here? >> I needed to include that header to make some bits visible >> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >> really good question. >> I don't remember exactly, probably I followed x86's domain.h which also >> included it. >> So, trying to remove the inclusion here, I get several build failures on >> Arm which could be fixed if I include that header from dm.h and ioreq.h: >> >> Shall I do this way? > The general rule ought to be that header include what they need, > but not more. Header dependencies are quite problematic already, > so every dependency we can avoid (or eliminate) will help. This > goes as far as only forward declaring structure where possible. I got it. > >>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>>> *v) {} >>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>> VMASST_TYPE_runstate_update_flag) >>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>> + >>>> #endif /* __ASM_DOMAIN_H__ */ >>>> /* >>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>> b/xen/include/asm-arm/hvm/ioreq.h >>>> new file mode 100644 >>>> index 0000000..19e1247 >>>> --- /dev/null >>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as >>> the IOREQ is now meant to be agnostic? >> Good question... The _common_ IOREQ code is indeed arch-agnostic. But, >> can the _arch_ IOREQ code be treated as really subarch-agnostic? >> I think, on Arm it can and it is most likely ok to keep it in >> "asm-arm/", but how it would be correlated with x86's IOREQ code which >> is HVM specific and located >> in "hvm" subdir? > I think for Arm's sake this should be used as asm/ioreq.h, where > x86 would gain a new header consisting of just > > #include <asm/hvm/ioreq.h> > > as there the functionality is needed for HVM only. For me this sounds perfectly fine. I think, this would also address Julien's question. May I introduce that new header together with moving IOREQ to the common code (patch #4)?
On 18.01.2021 16:52, Oleksandr wrote: > On 18.01.21 12:44, Jan Beulich wrote: >> On 17.01.2021 18:11, Oleksandr wrote: >>> On 15.01.21 22:26, Julien Grall wrote: >>>> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: >>>>> --- 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/ioreq.h> >>>>> #include <xen/lib.h> >>>>> #include <xen/spinlock.h> >>>>> #include <xen/sched.h> >>>>> @@ -23,6 +24,7 @@ >>>>> #include <asm/cpuerrata.h> >>>>> #include <asm/current.h> >>>>> #include <asm/mmio.h> >>>>> +#include <asm/hvm/ioreq.h> > > > Note to self: > > Remove obsolete bool ioreq_complete_mmio(void) from asm-arm/hvm/ioreq.h > > > >>>> Shouldn't this have been included by "xen/ioreq.h"? >>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned >>> out that there was nothing inside common header required arch one to be >>> included and >>> I was asked to include arch header where it was indeed needed (several >>> *.c files). >> I guess the general usage model of the two headers needs to be >> established first: If the per-arch header declares only stuff >> needed by the soon common/ioreq.c, then indeed it should be >> only that file and the producer(s) of the arch_*() functions >> which include that header; it should then in particular not be >> included by xen/ioreq.h. >> >> However, with the change request on patch 1 I think that usage >> model goes away at least for now, at which point the question >> is what exactly the per-arch header still declares, and based >> on that it would need to be decided whether xen/ioreq.h >> should include it. > > ok, well. > > x86's arch header now contains few IOREQ_STATUS_* #define-s, but Arm's > contains more stuff > besides that: > - stuff which is needed by common/ioreq.c, mostly stubs which are not > implemented yet (handle_pio, etc) > - stuff which is not needed by common/ioreq.c, internal Arm bits > (handle_ioserv, try_fwd_ioserv) > > Could we please decide based on the information above? You're in the best position to tell. The IOREQ_STATUS_* you mention may require including from xen/ioreq.h, but as said, ... >>>>> --- a/xen/include/asm-arm/domain.h >>>>> +++ b/xen/include/asm-arm/domain.h >>>>> @@ -10,6 +10,7 @@ >>>>> #include <asm/gic.h> >>>>> #include <asm/vgic.h> >>>>> #include <asm/vpl011.h> >>>>> +#include <public/hvm/dm_op.h> >>>> May I ask, why do you need to include dm_op.h here? >>> I needed to include that header to make some bits visible >>> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >>> really good question. >>> I don't remember exactly, probably I followed x86's domain.h which also >>> included it. >>> So, trying to remove the inclusion here, I get several build failures on >>> Arm which could be fixed if I include that header from dm.h and ioreq.h: >>> >>> Shall I do this way? >> The general rule ought to be that header include what they need, >> but not more. Header dependencies are quite problematic already, >> so every dependency we can avoid (or eliminate) will help. This >> goes as far as only forward declaring structure where possible. > > I got it. ... it depends. If xen/ioreq.h needs nothing from asm/ioreq.h, the I wouldn't see why it should include it. >>>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>>>> *v) {} >>>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>>> VMASST_TYPE_runstate_update_flag) >>>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>>> + >>>>> #endif /* __ASM_DOMAIN_H__ */ >>>>> /* >>>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>>> b/xen/include/asm-arm/hvm/ioreq.h >>>>> new file mode 100644 >>>>> index 0000000..19e1247 >>>>> --- /dev/null >>>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as >>>> the IOREQ is now meant to be agnostic? >>> Good question... The _common_ IOREQ code is indeed arch-agnostic. But, >>> can the _arch_ IOREQ code be treated as really subarch-agnostic? >>> I think, on Arm it can and it is most likely ok to keep it in >>> "asm-arm/", but how it would be correlated with x86's IOREQ code which >>> is HVM specific and located >>> in "hvm" subdir? >> I think for Arm's sake this should be used as asm/ioreq.h, where >> x86 would gain a new header consisting of just >> >> #include <asm/hvm/ioreq.h> >> >> as there the functionality is needed for HVM only. > For me this sounds perfectly fine. I think, this would also address > Julien's question. > May I introduce that new header together with moving IOREQ to the common > code (patch #4)? As with about everything, introduce new things the first time you need them, unless this results in overly big patches (in which case suitably splitting up is desirable, but of course no always possible). IOW if you introduce xen/ioreq.h and it needs to include asm/ioreq.h, then of course at this point you also need to introduce the asm-x86/ioreq.h wrapper. Jan
On 18.01.21 18:00, Jan Beulich wrote: Hi Jan > On 18.01.2021 16:52, Oleksandr wrote: >> On 18.01.21 12:44, Jan Beulich wrote: >>> On 17.01.2021 18:11, Oleksandr wrote: >>>> On 15.01.21 22:26, Julien Grall wrote: >>>>> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: >>>>>> --- 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/ioreq.h> >>>>>> #include <xen/lib.h> >>>>>> #include <xen/spinlock.h> >>>>>> #include <xen/sched.h> >>>>>> @@ -23,6 +24,7 @@ >>>>>> #include <asm/cpuerrata.h> >>>>>> #include <asm/current.h> >>>>>> #include <asm/mmio.h> >>>>>> +#include <asm/hvm/ioreq.h> >> >> Note to self: >> >> Remove obsolete bool ioreq_complete_mmio(void) from asm-arm/hvm/ioreq.h >> >> >> >>>>> Shouldn't this have been included by "xen/ioreq.h"? >>>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned >>>> out that there was nothing inside common header required arch one to be >>>> included and >>>> I was asked to include arch header where it was indeed needed (several >>>> *.c files). >>> I guess the general usage model of the two headers needs to be >>> established first: If the per-arch header declares only stuff >>> needed by the soon common/ioreq.c, then indeed it should be >>> only that file and the producer(s) of the arch_*() functions >>> which include that header; it should then in particular not be >>> included by xen/ioreq.h. >>> >>> However, with the change request on patch 1 I think that usage >>> model goes away at least for now, at which point the question >>> is what exactly the per-arch header still declares, and based >>> on that it would need to be decided whether xen/ioreq.h >>> should include it. >> ok, well. >> >> x86's arch header now contains few IOREQ_STATUS_* #define-s, but Arm's >> contains more stuff >> besides that: >> - stuff which is needed by common/ioreq.c, mostly stubs which are not >> implemented yet (handle_pio, etc) >> - stuff which is not needed by common/ioreq.c, internal Arm bits >> (handle_ioserv, try_fwd_ioserv) >> >> Could we please decide based on the information above? > You're in the best position to tell. The IOREQ_STATUS_* you > mention may require including from xen/ioreq.h, but as said, > ... > >>>>>> --- a/xen/include/asm-arm/domain.h >>>>>> +++ b/xen/include/asm-arm/domain.h >>>>>> @@ -10,6 +10,7 @@ >>>>>> #include <asm/gic.h> >>>>>> #include <asm/vgic.h> >>>>>> #include <asm/vpl011.h> >>>>>> +#include <public/hvm/dm_op.h> >>>>> May I ask, why do you need to include dm_op.h here? >>>> I needed to include that header to make some bits visible >>>> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >>>> really good question. >>>> I don't remember exactly, probably I followed x86's domain.h which also >>>> included it. >>>> So, trying to remove the inclusion here, I get several build failures on >>>> Arm which could be fixed if I include that header from dm.h and ioreq.h: >>>> >>>> Shall I do this way? >>> The general rule ought to be that header include what they need, >>> but not more. Header dependencies are quite problematic already, >>> so every dependency we can avoid (or eliminate) will help. This >>> goes as far as only forward declaring structure where possible. >> I got it. > ... it depends. If xen/ioreq.h needs nothing from asm/ioreq.h, > the I wouldn't see why it should include it. > >>>>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>>>>> *v) {} >>>>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>>>> VMASST_TYPE_runstate_update_flag) >>>>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>>>> + >>>>>> #endif /* __ASM_DOMAIN_H__ */ >>>>>> /* >>>>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>>>> b/xen/include/asm-arm/hvm/ioreq.h >>>>>> new file mode 100644 >>>>>> index 0000000..19e1247 >>>>>> --- /dev/null >>>>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>>>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as >>>>> the IOREQ is now meant to be agnostic? >>>> Good question... The _common_ IOREQ code is indeed arch-agnostic. But, >>>> can the _arch_ IOREQ code be treated as really subarch-agnostic? >>>> I think, on Arm it can and it is most likely ok to keep it in >>>> "asm-arm/", but how it would be correlated with x86's IOREQ code which >>>> is HVM specific and located >>>> in "hvm" subdir? >>> I think for Arm's sake this should be used as asm/ioreq.h, where >>> x86 would gain a new header consisting of just >>> >>> #include <asm/hvm/ioreq.h> >>> >>> as there the functionality is needed for HVM only. >> For me this sounds perfectly fine. I think, this would also address >> Julien's question. >> May I introduce that new header together with moving IOREQ to the common >> code (patch #4)? > As with about everything, introduce new things the first time you > need them, unless this results in overly big patches (in which > case suitably splitting up is desirable, but of course no always > possible). IOW if you introduce xen/ioreq.h and it needs to > include asm/ioreq.h, then of course at this point you also need > to introduce the asm-x86/ioreq.h wrapper. Thank you for the clarification.
Hi Oleksandr, On 17/01/2021 18:52, Oleksandr wrote: > > On 17.01.21 20:07, Julien Grall wrote: >> >> >> On 17/01/2021 17:11, Oleksandr wrote: >>> >>> On 15.01.21 22:26, Julien Grall wrote: >>> >>> Hi Julien >> >> Hi Oleksandr, > > > Hi Julien > > > >> >>>> >>>>> + >>>>> PROGRESS(xen): >>>>> ret = relinquish_memory(d, &d->xenpage_list); >>>>> if ( ret ) >>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >>>>> index ae7ef96..9814481 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/ioreq.h> >>>>> #include <xen/lib.h> >>>>> #include <xen/spinlock.h> >>>>> #include <xen/sched.h> >>>>> @@ -23,6 +24,7 @@ >>>>> #include <asm/cpuerrata.h> >>>>> #include <asm/current.h> >>>>> #include <asm/mmio.h> >>>>> +#include <asm/hvm/ioreq.h> >>>> >>>> Shouldn't this have been included by "xen/ioreq.h"? >>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it >>> turned out that there was nothing inside common header required arch >>> one to be included and >>> I was asked to include arch header where it was indeed needed >>> (several *.c files). >> >> Fair enough. >> >> [...] >> >>>> >>>> If you return IO_HANDLED here, then it means the we will take care >>>> of previous I/O but the current one is going to be ignored. >>> Which current one? As I understand, if try_fwd_ioserv() gets called >>> with vio->req.state == STATE_IORESP_READY then this is a second round >>> after emulator completes the emulation (the first round was when >>> we returned IO_RETRY down the function and claimed that we would need >>> a completion), so we are still dealing with previous I/O. >>> vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> >>> try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() >>> And after we return IO_HANDLED here, handle_ioserv() will be called >>> to complete the handling of this previous I/O emulation. >>> Or I really missed something? >> >> Hmmm... I somehow thought try_fw_ioserv() would only be called the >> first time. Do you have a branch with your code applied? This would >> help to follow the different paths. > Yes, I mentioned about it in cover letter. > > Please see > https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5 > why 5 - because I started counting from the RFC) Oh, I looked at the cover letter and didn't find it. Hence why I asked. I should have looked more carefully. Thanks! I have looked closer at the question and I am not sure to understand why arch_ioreq_complete_mmio() is going to call try_handle_mmio(). This looks pretty innefficient to me because we already now the IO was handled by the IOREQ server. I realize that x86 is calling handle_mmio() again. However, I don't think we need the same on Arm because the instruction for accessing device memory are a lot simpler (you can only read or store at most a 64-bit value). So I would like to keep our emulation simple and not rely on try_ioserv_fw() to always return true when call from completion (AFAICT it is not possible to return false then). I will answer to the rest separately. Cheers,
Hi Julien >> >> >>> >>>>> >>>>>> + >>>>>> PROGRESS(xen): >>>>>> ret = relinquish_memory(d, &d->xenpage_list); >>>>>> if ( ret ) >>>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >>>>>> index ae7ef96..9814481 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/ioreq.h> >>>>>> #include <xen/lib.h> >>>>>> #include <xen/spinlock.h> >>>>>> #include <xen/sched.h> >>>>>> @@ -23,6 +24,7 @@ >>>>>> #include <asm/cpuerrata.h> >>>>>> #include <asm/current.h> >>>>>> #include <asm/mmio.h> >>>>>> +#include <asm/hvm/ioreq.h> >>>>> >>>>> Shouldn't this have been included by "xen/ioreq.h"? >>>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it >>>> turned out that there was nothing inside common header required >>>> arch one to be included and >>>> I was asked to include arch header where it was indeed needed >>>> (several *.c files). >>> >>> Fair enough. >>> >>> [...] >>> >>>>> >>>>> If you return IO_HANDLED here, then it means the we will take care >>>>> of previous I/O but the current one is going to be ignored. >>>> Which current one? As I understand, if try_fwd_ioserv() gets called >>>> with vio->req.state == STATE_IORESP_READY then this is a second >>>> round after emulator completes the emulation (the first round was when >>>> we returned IO_RETRY down the function and claimed that we would >>>> need a completion), so we are still dealing with previous I/O. >>>> vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> >>>> try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() >>>> And after we return IO_HANDLED here, handle_ioserv() will be called >>>> to complete the handling of this previous I/O emulation. >>>> Or I really missed something? >>> >>> Hmmm... I somehow thought try_fw_ioserv() would only be called the >>> first time. Do you have a branch with your code applied? This would >>> help to follow the different paths. >> Yes, I mentioned about it in cover letter. >> >> Please see >> https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5 >> why 5 - because I started counting from the RFC) > > Oh, I looked at the cover letter and didn't find it. Hence why I > asked. I should have looked more carefully. Thanks! > > I have looked closer at the question and I am not sure to understand > why arch_ioreq_complete_mmio() is going to call try_handle_mmio(). > > This looks pretty innefficient to me because we already now the IO was > handled by the IOREQ server. > > I realize that x86 is calling handle_mmio() again. However, I don't > think we need the same on Arm because the instruction for accessing > device memory are a lot simpler (you can only read or store at most a > 64-bit value). I think, I agree. > > So I would like to keep our emulation simple and not rely on > try_ioserv_fw() to always return true when call from completion > (AFAICT it is not possible to return false then). So what you are proposing is just a replacement try_ioserv_fw() by handle_ioserv() technically? diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 40b9e59..0508bd8 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, bool arch_ioreq_complete_mmio(void) { - struct vcpu *v = current; struct cpu_user_regs *regs = guest_cpu_user_regs(); const union hsr hsr = { .bits = regs->hsr }; - paddr_t addr = v->io.req.addr; - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) + if ( handle_ioserv(regs, current) == IO_HANDLED ) { advance_pc(regs, hsr); return true; > > I will answer to the rest separately. Thank you. > > Cheers, >
On Sun, 17 Jan 2021, Oleksandr wrote: > On 15.01.21 02:55, Stefano Stabellini wrote: > > On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: > > > From: Julien Grall <julien.grall@arm.com> > > > > > > This patch adds basic IOREQ/DM support on Arm. The subsequent > > > patches will improve functionality and add remaining bits. > > > > > > The IOREQ/DM features are supposed to be built with IOREQ_SERVER > > > option enabled, which is disabled by default on Arm for now. > > > > > > Please note, the "PIO handling" TODO is expected to left unaddressed > > > for the current series. It is not an big issue for now while Xen > > > doesn't have support for vPCI on Arm. On Arm64 they are only used > > > for PCI IO Bar and we would probably want to expose them to emulator > > > as PIO access to make a DM completely arch-agnostic. So "PIO handling" > > > should be implemented when we add support for vPCI. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > > [On Arm only] > > > Tested-by: Wei Chen <Wei.Chen@arm.com> > > > > > > --- > > > Please note, this is a split/cleanup/hardening of Julien's PoC: > > > "Add support for Guest IO forwarding to a device emulator" > > > > > > Changes RFC -> V1: > > > - was split into: > > > - arm/ioreq: Introduce arch specific bits for IOREQ/DM features > > > - xen/mm: Handle properly reference in set_foreign_p2m_entry() on > > > Arm > > > - update patch description > > > - update asm-arm/hvm/ioreq.h according to the newly introduced arch > > > functions: > > > - arch_hvm_destroy_ioreq_server() > > > - arch_handle_hvm_io_completion() > > > - update arch files to include xen/ioreq.h > > > - remove HVMOP plumbing > > > - rewrite a logic to handle properly case when hvm_send_ioreq() > > > returns IO_RETRY > > > - add a logic to handle properly handle_hvm_io_completion() return > > > value > > > - rename handle_mmio() to ioreq_handle_complete_mmio() > > > - move paging_mark_pfn_dirty() to asm-arm/paging.h > > > - remove forward declaration for hvm_ioreq_server in asm-arm/paging.h > > > - move try_fwd_ioserv() to ioreq.c, provide stubs if > > > !CONFIG_IOREQ_SERVER > > > - do not remove #ifdef CONFIG_IOREQ_SERVER in memory.c for guarding > > > xen/ioreq.h > > > - use gdprintk in try_fwd_ioserv(), remove unneeded prints > > > - update list of #include-s > > > - move has_vpci() to asm-arm/domain.h > > > - add a comment (TODO) to unimplemented yet handle_pio() > > > - remove hvm_mmio_first(last)_byte() and hvm_ioreq_(page/vcpu/server) > > > structs > > > from the arch files, they were already moved to the common code > > > - remove set_foreign_p2m_entry() changes, they will be properly > > > implemented > > > in the follow-up patch > > > - select IOREQ_SERVER for Arm instead of Arm64 in Kconfig > > > - remove x86's realmode and other unneeded stubs from xen/ioreq.h > > > - clafify ioreq_t p.df usage in try_fwd_ioserv() > > > - set ioreq_t p.count to 1 in try_fwd_ioserv() > > > > > > Changes V1 -> V2: > > > - was split into: > > > - arm/ioreq: Introduce arch specific bits for IOREQ/DM features > > > - xen/arm: Stick around in leave_hypervisor_to_guest until I/O has > > > completed > > > - update the author of a patch > > > - update patch description > > > - move a loop in leave_hypervisor_to_guest() to a separate patch > > > - set IOREQ_SERVER disabled by default > > > - remove already clarified /* XXX */ > > > - replace BUG() by ASSERT_UNREACHABLE() in handle_pio() > > > - remove default case for handling the return value of > > > try_handle_mmio() > > > - remove struct hvm_domain, enum hvm_io_completion, struct > > > hvm_vcpu_io, > > > struct hvm_vcpu from asm-arm/domain.h, these are common materials > > > now > > > - update everything according to the recent changes (IOREQ related > > > function > > > names don't contain "hvm" prefixes/infixes anymore, IOREQ related > > > fields > > > are part of common struct vcpu/domain now, etc) > > > > > > Changes V2 -> V3: > > > - update patch according the "legacy interface" is x86 specific > > > - add dummy arch hooks > > > - remove dummy paging_mark_pfn_dirty() > > > - don’t include <xen/domain_page.h> in common ioreq.c > > > - don’t include <public/hvm/ioreq.h> in arch ioreq.h > > > - remove #define ioreq_params(d, i) > > > > > > Changes V3 -> V4: > > > - rebase > > > - update patch according to the renaming IO_ -> VIO_ (io_ -> vio_) > > > and misc changes to arch hooks > > > - update patch according to the IOREQ related dm-op handling changes > > > - don't include <xen/ioreq.h> from arch header > > > - make all arch hooks out-of-line > > > - add a comment above IOREQ_STATUS_* #define-s > > > --- > > > xen/arch/arm/Makefile | 2 + > > > xen/arch/arm/dm.c | 122 +++++++++++++++++++++++ > > > xen/arch/arm/domain.c | 9 ++ > > > xen/arch/arm/io.c | 12 ++- > > > xen/arch/arm/ioreq.c | 213 > > > ++++++++++++++++++++++++++++++++++++++++ > > > xen/arch/arm/traps.c | 13 +++ > > > xen/include/asm-arm/domain.h | 3 + > > > xen/include/asm-arm/hvm/ioreq.h | 72 ++++++++++++++ > > > xen/include/asm-arm/mmio.h | 1 + > > > 9 files changed, 446 insertions(+), 1 deletion(-) > > > create mode 100644 xen/arch/arm/dm.c > > > create mode 100644 xen/arch/arm/ioreq.c > > > create mode 100644 xen/include/asm-arm/hvm/ioreq.h > > > > > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > > > index 512ffdd..16e6523 100644 > > > --- a/xen/arch/arm/Makefile > > > +++ b/xen/arch/arm/Makefile > > > @@ -13,6 +13,7 @@ obj-y += cpuerrata.o > > > obj-y += cpufeature.o > > > obj-y += decode.o > > > obj-y += device.o > > > +obj-$(CONFIG_IOREQ_SERVER) += dm.o > > > obj-y += domain.o > > > obj-y += domain_build.init.o > > > obj-y += domctl.o > > > @@ -27,6 +28,7 @@ obj-y += guest_atomics.o > > > obj-y += guest_walk.o > > > obj-y += hvm.o > > > obj-y += io.o > > > +obj-$(CONFIG_IOREQ_SERVER) += ioreq.o > > > obj-y += irq.o > > > obj-y += kernel.init.o > > > obj-$(CONFIG_LIVEPATCH) += livepatch.o > > > diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c > > > new file mode 100644 > > > index 0000000..e6dedf4 > > > --- /dev/null > > > +++ b/xen/arch/arm/dm.c > > > @@ -0,0 +1,122 @@ > > > +/* > > > + * Copyright (c) 2019 Arm ltd. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > it > > > + * under the terms and conditions of the GNU General Public License, > > > + * version 2, as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope it will be useful, but WITHOUT > > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > > for > > > + * more details. > > > + * > > > + * You should have received a copy of the GNU General Public License > > > along with > > > + * this program; If not, see <http://www.gnu.org/licenses/>. > > > + */ > > > + > > > +#include <xen/dm.h> > > > +#include <xen/guest_access.h> > > > +#include <xen/hypercall.h> > > > +#include <xen/ioreq.h> > > > +#include <xen/nospec.h> > > > + > > > +static int dm_op(const struct dmop_args *op_args) > > > +{ > > > + struct domain *d; > > > + struct xen_dm_op op; > > > + bool const_op = true; > > > + long rc; > > > + size_t offset; > > > + > > > + static const uint8_t op_size[] = { > > > + [XEN_DMOP_create_ioreq_server] = sizeof(struct > > > xen_dm_op_create_ioreq_server), > > > + [XEN_DMOP_get_ioreq_server_info] = sizeof(struct > > > xen_dm_op_get_ioreq_server_info), > > > + [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct > > > xen_dm_op_ioreq_server_range), > > > + [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct > > > xen_dm_op_ioreq_server_range), > > > + [XEN_DMOP_set_ioreq_server_state] = sizeof(struct > > > xen_dm_op_set_ioreq_server_state), > > > + [XEN_DMOP_destroy_ioreq_server] = sizeof(struct > > > xen_dm_op_destroy_ioreq_server), > > > + }; > > > + > > > + rc = rcu_lock_remote_domain_by_id(op_args->domid, &d); > > > + if ( rc ) > > > + return rc; > > > + > > > + rc = xsm_dm_op(XSM_DM_PRIV, d); > > > + if ( rc ) > > > + goto out; > > > + > > > + offset = offsetof(struct xen_dm_op, u); > > > + > > > + rc = -EFAULT; > > > + if ( op_args->buf[0].size < offset ) > > > + goto out; > > > + > > > + if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, > > > offset) ) > > > + goto out; > > > + > > > + if ( op.op >= ARRAY_SIZE(op_size) ) > > > + { > > > + rc = -EOPNOTSUPP; > > > + goto out; > > > + } > > > + > > > + op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size)); > > > + > > > + if ( op_args->buf[0].size < offset + op_size[op.op] ) > > > + goto out; > > > + > > > + if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset, > > > + op_size[op.op]) ) > > > + goto out; > > > + > > > + rc = -EINVAL; > > > + if ( op.pad ) > > > + goto out; > > > + > > > + rc = ioreq_server_dm_op(&op, d, &const_op); > > > + > > > + if ( (!rc || rc == -ERESTART) && > > > + !const_op && copy_to_guest_offset(op_args->buf[0].h, offset, > > > + (void *)&op.u, op_size[op.op]) > > > ) > > > + rc = -EFAULT; > > > + > > > + out: > > > + rcu_unlock_domain(d); > > > + > > > + return rc; > > > +} > > > + > > > +long do_dm_op(domid_t domid, > > > + unsigned int nr_bufs, > > > + XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs) > > > +{ > > > + struct dmop_args args; > > > + int rc; > > > + > > > + if ( nr_bufs > ARRAY_SIZE(args.buf) ) > > > + return -E2BIG; > > > + > > > + args.domid = domid; > > > + args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1); > > > + > > > + if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) ) > > > + return -EFAULT; > > > + > > > + rc = dm_op(&args); > > > + > > > + if ( rc == -ERESTART ) > > > + rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih", > > > + domid, nr_bufs, bufs); > > > + > > > + return rc; > > > +} > > I might have missed something in the discussions but this function is > > identical to xen/arch/x86/hvm/dm.c:do_dm_op, why not make it common? > > > > Also the previous function dm_op is very similar to > > xen/arch/x86/hvm/dm.c:dm_op I would prefer to make them common if > > possible. Was this already discussed? > Well, let me explain. Both dm_op() and do_dm_op() were indeed common (top > level dm-op handling common) for previous versions, so Arm's dm.c didn't > contain this stuff. > The idea to make it other way around (top level dm-op handling arch-specific > and call into ioreq_server_dm_op() for otherwise unhandled ops) was discussed > at [1] which besides > it's Pros leads to code duplication, so Arm's dm.c has to duplicate some > stuff, etc. > I was thinking about moving do_dm_op() which is _same_ for both arches to > common code, but I am not sure whether it is conceptually correct which that > new "alternative" approach of handling dm-op. Yes, I think it makes sense to make do_dm_op common because it is identical. That should be easy. I realize that the common part of dm_op is the initial boilerplate which is similar for every hypercall, so I think it is also OK if we don't share it and leave it as it is in this version of the series.
On Tue, 19 Jan 2021, Oleksandr wrote: > > > > > > > PROGRESS(xen): > > > > > > > ret = relinquish_memory(d, &d->xenpage_list); > > > > > > > if ( ret ) > > > > > > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > > > > > > > index ae7ef96..9814481 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/ioreq.h> > > > > > > > #include <xen/lib.h> > > > > > > > #include <xen/spinlock.h> > > > > > > > #include <xen/sched.h> > > > > > > > @@ -23,6 +24,7 @@ > > > > > > > #include <asm/cpuerrata.h> > > > > > > > #include <asm/current.h> > > > > > > > #include <asm/mmio.h> > > > > > > > +#include <asm/hvm/ioreq.h> > > > > > > > > > > > > Shouldn't this have been included by "xen/ioreq.h"? > > > > > Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it > > > > > turned out that there was nothing inside common header required arch > > > > > one to be included and > > > > > I was asked to include arch header where it was indeed needed (several > > > > > *.c files). > > > > > > > > Fair enough. > > > > > > > > [...] > > > > > > > > > > > > > > > > If you return IO_HANDLED here, then it means the we will take care > > > > > > of previous I/O but the current one is going to be ignored. > > > > > Which current one? As I understand, if try_fwd_ioserv() gets called > > > > > with vio->req.state == STATE_IORESP_READY then this is a second round > > > > > after emulator completes the emulation (the first round was when > > > > > we returned IO_RETRY down the function and claimed that we would need > > > > > a completion), so we are still dealing with previous I/O. > > > > > vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> > > > > > try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv() > > > > > And after we return IO_HANDLED here, handle_ioserv() will be called to > > > > > complete the handling of this previous I/O emulation. > > > > > Or I really missed something? > > > > > > > > Hmmm... I somehow thought try_fw_ioserv() would only be called the first > > > > time. Do you have a branch with your code applied? This would help to > > > > follow the different paths. > > > Yes, I mentioned about it in cover letter. > > > > > > Please see > > > https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5 > > > why 5 - because I started counting from the RFC) > > > > Oh, I looked at the cover letter and didn't find it. Hence why I asked. I > > should have looked more carefully. Thanks! > > > > I have looked closer at the question and I am not sure to understand why > > arch_ioreq_complete_mmio() is going to call try_handle_mmio(). > > > > This looks pretty innefficient to me because we already now the IO was > > handled by the IOREQ server. > > > > I realize that x86 is calling handle_mmio() again. However, I don't think we > > need the same on Arm because the instruction for accessing device memory are > > a lot simpler (you can only read or store at most a 64-bit value). > > I think, I agree. Yes I agree too > > So I would like to keep our emulation simple and not rely on try_ioserv_fw() > > to always return true when call from completion (AFAICT it is not possible > > to return false then). > > > So what you are proposing is just a replacement try_ioserv_fw() by > handle_ioserv() technically? > > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > index 40b9e59..0508bd8 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > > bool arch_ioreq_complete_mmio(void) > { > - struct vcpu *v = current; > struct cpu_user_regs *regs = guest_cpu_user_regs(); > const union hsr hsr = { .bits = regs->hsr }; > - paddr_t addr = v->io.req.addr; > > - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > + if ( handle_ioserv(regs, current) == IO_HANDLED ) > { > advance_pc(regs, hsr); > return true; Yes, but I think we want to keep the check vio->req.state == STATE_IORESP_READY So maybe (uncompiled, untested): if ( v->io.req.state != STATE_IORESP_READY ) return false; if ( handle_ioserv(regs, current) == IO_HANDLED ) { advance_pc(regs, hsr); return true; }
Hi Oleksandr, On 17/01/2021 18:52, Oleksandr wrote: >>>>> diff --git a/xen/include/asm-arm/domain.h >>>>> b/xen/include/asm-arm/domain.h >>>>> index 6819a3b..c235e5b 100644 >>>>> --- a/xen/include/asm-arm/domain.h >>>>> +++ b/xen/include/asm-arm/domain.h >>>>> @@ -10,6 +10,7 @@ >>>>> #include <asm/gic.h> >>>>> #include <asm/vgic.h> >>>>> #include <asm/vpl011.h> >>>>> +#include <public/hvm/dm_op.h> >>>> >>>> May I ask, why do you need to include dm_op.h here? >>> I needed to include that header to make some bits visible >>> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >>> really good question. >>> I don't remember exactly, probably I followed x86's domain.h which >>> also included it. >>> So, trying to remove the inclusion here, I get several build failures >>> on Arm which could be fixed if I include that header from dm.h and >>> ioreq.h: >>> >>> Shall I do this way? >> >> If the failure are indeded because ioreq.h and dm.h use definition >> from public/hvm/dm_op.h, then yes. Can you post the errors? > Please see attached, although I built for Arm32 (and the whole series), > I think errors are valid for Arm64 also. Thanks! > error1.txt - when remove #include <public/hvm/dm_op.h> from > asm-arm/domain.h For this one, I agree that including <public/hvm/dm_op.h> in xen.h looks the best solution. > error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h It looks like the error is happening in dm.c rather than xen/dm.h. Any reason to not include <public/hvm/dm_op.h> in dm.c directly? > error3.txt - when add #include <public/hvm/dm_op.h> to xen/dm.h I am a bit confused with this one. Isn't it the same as error1.txt? > > >> >> >> [...] >> >>>>> #include <public/hvm/params.h> >>>>> struct hvm_domain >>>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu >>>>> *v) {} >>>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>>> VMASST_TYPE_runstate_update_flag) >>>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>>> + >>>>> #endif /* __ASM_DOMAIN_H__ */ >>>>> /* >>>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>>> b/xen/include/asm-arm/hvm/ioreq.h >>>>> new file mode 100644 >>>>> index 0000000..19e1247 >>>>> --- /dev/null >>>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>>> >>>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ >>>> as the IOREQ is now meant to be agnostic? >>> Good question... The _common_ IOREQ code is indeed arch-agnostic. >>> But, can the _arch_ IOREQ code be treated as really subarch-agnostic? >>> I think, on Arm it can and it is most likely ok to keep it in >>> "asm-arm/", but how it would be correlated with x86's IOREQ code >>> which is HVM specific and located >>> in "hvm" subdir? >> >> Sorry, I don't understand your answer/questions. So let me ask the >> question differently, is asm-arm/hvm/ioreq.h going to be included from >> common code? > > Sorry if I was unclear. > > >> >> If the answer is no, then I see no reason to follow the x86 here. >> If the answer is yes, then I am quite confused why half of the series >> tried to remove "hvm" from the function name but we still include >> "asm/hvm/ioreq.h". > > Answer is yes. Even if we could to avoid including that header from the > common code somehow, we would still have #include <public/hvm/*>, > is_hvm_domain(). I saw Jan answered about this one. Let me know if you need more input. Cheers,
Hi Stefano, On 20/01/2021 00:50, Stefano Stabellini wrote: > On Tue, 19 Jan 2021, Oleksandr wrote: >> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c >> index 40b9e59..0508bd8 100644 >> --- a/xen/arch/arm/ioreq.c >> +++ b/xen/arch/arm/ioreq.c >> @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >> >> bool arch_ioreq_complete_mmio(void) >> { >> - struct vcpu *v = current; >> struct cpu_user_regs *regs = guest_cpu_user_regs(); >> const union hsr hsr = { .bits = regs->hsr }; >> - paddr_t addr = v->io.req.addr; >> >> - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) >> + if ( handle_ioserv(regs, current) == IO_HANDLED ) >> { >> advance_pc(regs, hsr); >> return true; > > Yes, but I think we want to keep the check > > vio->req.state == STATE_IORESP_READY > > So maybe (uncompiled, untested): > > if ( v->io.req.state != STATE_IORESP_READY ) > return false; Is it possible to reach this function with v->io.req.state != STATE_IORESP_READY? If not, then I would suggest to add an ASSERT_UNREACHABLE() before the return. Cheers,
On Wed, 20 Jan 2021, Julien Grall wrote: > Hi Stefano, > > On 20/01/2021 00:50, Stefano Stabellini wrote: > > On Tue, 19 Jan 2021, Oleksandr wrote: > > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > > > index 40b9e59..0508bd8 100644 > > > --- a/xen/arch/arm/ioreq.c > > > +++ b/xen/arch/arm/ioreq.c > > > @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs > > > *regs, > > > > > > bool arch_ioreq_complete_mmio(void) > > > { > > > - struct vcpu *v = current; > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > const union hsr hsr = { .bits = regs->hsr }; > > > - paddr_t addr = v->io.req.addr; > > > > > > - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > > > + if ( handle_ioserv(regs, current) == IO_HANDLED ) > > > { > > > advance_pc(regs, hsr); > > > return true; > > > > Yes, but I think we want to keep the check > > > > vio->req.state == STATE_IORESP_READY > > > > So maybe (uncompiled, untested): > > > > if ( v->io.req.state != STATE_IORESP_READY ) > > return false; > > Is it possible to reach this function with v->io.req.state != > STATE_IORESP_READY? If not, then I would suggest to add an > ASSERT_UNREACHABLE() before the return. If I am reading the state machine right it should *not* be possible to get here with v->io.req.state != STATE_IORESP_READY, so yes, ASSERT_UNREACHABLE() would work.
On 20.01.21 17:50, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > On 17/01/2021 18:52, Oleksandr wrote: >>>>>> diff --git a/xen/include/asm-arm/domain.h >>>>>> b/xen/include/asm-arm/domain.h >>>>>> index 6819a3b..c235e5b 100644 >>>>>> --- a/xen/include/asm-arm/domain.h >>>>>> +++ b/xen/include/asm-arm/domain.h >>>>>> @@ -10,6 +10,7 @@ >>>>>> #include <asm/gic.h> >>>>>> #include <asm/vgic.h> >>>>>> #include <asm/vpl011.h> >>>>>> +#include <public/hvm/dm_op.h> >>>>> >>>>> May I ask, why do you need to include dm_op.h here? >>>> I needed to include that header to make some bits visible >>>> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a >>>> really good question. >>>> I don't remember exactly, probably I followed x86's domain.h which >>>> also included it. >>>> So, trying to remove the inclusion here, I get several build >>>> failures on Arm which could be fixed if I include that header from >>>> dm.h and ioreq.h: >>>> >>>> Shall I do this way? >>> >>> If the failure are indeded because ioreq.h and dm.h use definition >>> from public/hvm/dm_op.h, then yes. Can you post the errors? >> Please see attached, although I built for Arm32 (and the whole >> series), I think errors are valid for Arm64 also. > > Thanks! > >> error1.txt - when remove #include <public/hvm/dm_op.h> from >> asm-arm/domain.h > > For this one, I agree that including <public/hvm/dm_op.h> in xen.h > looks the best solution. Yes, I assume you meant in "ioreq.h" > > >> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h > > It looks like the error is happening in dm.c rather than xen/dm.h. Any > reason to not include <public/hvm/dm_op.h> in dm.c directly? Including it directly doesn't solve build issue. If I am not mistaken in order to follow requirements how to include headers (alphabetic order, public* should be included after xen* and asm* ones, etc) the dm.h gets included the first in dm.c, and dm_op.h gets included the last. We can avoid build issue if we change inclusion order a bit, what I mean is to include dm.h after hypercall.h at least (because hypercall.h already includes dm_op.h). But this breaks the requirements and is not way to go. Now I am in doubt how to overcome this. > > > >> error3.txt - when add #include <public/hvm/dm_op.h> to xen/dm.h > > I am a bit confused with this one. Isn't it the same as error1.txt? The same, please ignore them, sorry for the confusion. > > >> >> >>> >>> >>> [...] >>> >>>>>> #include <public/hvm/params.h> >>>>>> struct hvm_domain >>>>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct >>>>>> vcpu *v) {} >>>>>> #define arch_vm_assist_valid_mask(d) (1UL << >>>>>> VMASST_TYPE_runstate_update_flag) >>>>>> +#define has_vpci(d) ({ (void)(d); false; }) >>>>>> + >>>>>> #endif /* __ASM_DOMAIN_H__ */ >>>>>> /* >>>>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h >>>>>> b/xen/include/asm-arm/hvm/ioreq.h >>>>>> new file mode 100644 >>>>>> index 0000000..19e1247 >>>>>> --- /dev/null >>>>>> +++ b/xen/include/asm-arm/hvm/ioreq.h >>>>> >>>>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ >>>>> as the IOREQ is now meant to be agnostic? >>>> Good question... The _common_ IOREQ code is indeed arch-agnostic. >>>> But, can the _arch_ IOREQ code be treated as really subarch-agnostic? >>>> I think, on Arm it can and it is most likely ok to keep it in >>>> "asm-arm/", but how it would be correlated with x86's IOREQ code >>>> which is HVM specific and located >>>> in "hvm" subdir? >>> >>> Sorry, I don't understand your answer/questions. So let me ask the >>> question differently, is asm-arm/hvm/ioreq.h going to be included >>> from common code? >> >> Sorry if I was unclear. >> >> >>> >>> If the answer is no, then I see no reason to follow the x86 here. >>> If the answer is yes, then I am quite confused why half of the >>> series tried to remove "hvm" from the function name but we still >>> include "asm/hvm/ioreq.h". >> >> Answer is yes. Even if we could to avoid including that header from >> the common code somehow, we would still have #include <public/hvm/*>, >> is_hvm_domain(). > > I saw Jan answered about this one. Let me know if you need more input. Thank you, I think, no. Everything is clear at the moment. > > Cheers, >
On 20.01.21 21:47, Stefano Stabellini wrote: Hi Julien, Stefano > On Wed, 20 Jan 2021, Julien Grall wrote: >> Hi Stefano, >> >> On 20/01/2021 00:50, Stefano Stabellini wrote: >>> On Tue, 19 Jan 2021, Oleksandr wrote: >>>> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c >>>> index 40b9e59..0508bd8 100644 >>>> --- a/xen/arch/arm/ioreq.c >>>> +++ b/xen/arch/arm/ioreq.c >>>> @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs >>>> *regs, >>>> >>>> bool arch_ioreq_complete_mmio(void) >>>> { >>>> - struct vcpu *v = current; >>>> struct cpu_user_regs *regs = guest_cpu_user_regs(); >>>> const union hsr hsr = { .bits = regs->hsr }; >>>> - paddr_t addr = v->io.req.addr; >>>> >>>> - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) >>>> + if ( handle_ioserv(regs, current) == IO_HANDLED ) >>>> { >>>> advance_pc(regs, hsr); >>>> return true; >>> Yes, but I think we want to keep the check >>> >>> vio->req.state == STATE_IORESP_READY >>> >>> So maybe (uncompiled, untested): >>> >>> if ( v->io.req.state != STATE_IORESP_READY ) >>> return false; >> Is it possible to reach this function with v->io.req.state != >> STATE_IORESP_READY? If not, then I would suggest to add an >> ASSERT_UNREACHABLE() before the return. > If I am reading the state machine right it should *not* be possible to > get here with v->io.req.state != STATE_IORESP_READY, so yes, > ASSERT_UNREACHABLE() would work. Agree here. If the assumption is not correct (unlikely), I think I will catch this during testing. In addition, we can probably drop case STATE_IORESP_READY in try_fwd_ioserv(). [not tested] diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 40b9e59..c7ee1a7 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -71,9 +71,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, case STATE_IOREQ_NONE: break; - case STATE_IORESP_READY: - return IO_HANDLED; - default: gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); return IO_ABORT; @@ -104,9 +101,14 @@ bool arch_ioreq_complete_mmio(void) struct vcpu *v = current; struct cpu_user_regs *regs = guest_cpu_user_regs(); const union hsr hsr = { .bits = regs->hsr }; - paddr_t addr = v->io.req.addr; - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) + if ( v->io.req.state != STATE_IORESP_READY ) + { + ASSERT_UNREACHABLE(); + return false; + } + + if ( handle_ioserv(regs, v) == IO_HANDLED ) { advance_pc(regs, hsr); return true;
On 20.01.21 02:23, Stefano Stabellini wrote: Hi Stefano > On Sun, 17 Jan 2021, Oleksandr wrote: >> On 15.01.21 02:55, Stefano Stabellini wrote: >>> On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: >>>> From: Julien Grall <julien.grall@arm.com> >>>> >>>> This patch adds basic IOREQ/DM support on Arm. The subsequent >>>> patches will improve functionality and add remaining bits. >>>> >>>> The IOREQ/DM features are supposed to be built with IOREQ_SERVER >>>> option enabled, which is disabled by default on Arm for now. >>>> >>>> Please note, the "PIO handling" TODO is expected to left unaddressed >>>> for the current series. It is not an big issue for now while Xen >>>> doesn't have support for vPCI on Arm. On Arm64 they are only used >>>> for PCI IO Bar and we would probably want to expose them to emulator >>>> as PIO access to make a DM completely arch-agnostic. So "PIO handling" >>>> should be implemented when we add support for vPCI. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> [On Arm only] >>>> Tested-by: Wei Chen <Wei.Chen@arm.com> >>>> >>>> --- >>>> Please note, this is a split/cleanup/hardening of Julien's PoC: >>>> "Add support for Guest IO forwarding to a device emulator" >>>> >>>> Changes RFC -> V1: >>>> - was split into: >>>> - arm/ioreq: Introduce arch specific bits for IOREQ/DM features >>>> - xen/mm: Handle properly reference in set_foreign_p2m_entry() on >>>> Arm >>>> - update patch description >>>> - update asm-arm/hvm/ioreq.h according to the newly introduced arch >>>> functions: >>>> - arch_hvm_destroy_ioreq_server() >>>> - arch_handle_hvm_io_completion() >>>> - update arch files to include xen/ioreq.h >>>> - remove HVMOP plumbing >>>> - rewrite a logic to handle properly case when hvm_send_ioreq() >>>> returns IO_RETRY >>>> - add a logic to handle properly handle_hvm_io_completion() return >>>> value >>>> - rename handle_mmio() to ioreq_handle_complete_mmio() >>>> - move paging_mark_pfn_dirty() to asm-arm/paging.h >>>> - remove forward declaration for hvm_ioreq_server in asm-arm/paging.h >>>> - move try_fwd_ioserv() to ioreq.c, provide stubs if >>>> !CONFIG_IOREQ_SERVER >>>> - do not remove #ifdef CONFIG_IOREQ_SERVER in memory.c for guarding >>>> xen/ioreq.h >>>> - use gdprintk in try_fwd_ioserv(), remove unneeded prints >>>> - update list of #include-s >>>> - move has_vpci() to asm-arm/domain.h >>>> - add a comment (TODO) to unimplemented yet handle_pio() >>>> - remove hvm_mmio_first(last)_byte() and hvm_ioreq_(page/vcpu/server) >>>> structs >>>> from the arch files, they were already moved to the common code >>>> - remove set_foreign_p2m_entry() changes, they will be properly >>>> implemented >>>> in the follow-up patch >>>> - select IOREQ_SERVER for Arm instead of Arm64 in Kconfig >>>> - remove x86's realmode and other unneeded stubs from xen/ioreq.h >>>> - clafify ioreq_t p.df usage in try_fwd_ioserv() >>>> - set ioreq_t p.count to 1 in try_fwd_ioserv() >>>> >>>> Changes V1 -> V2: >>>> - was split into: >>>> - arm/ioreq: Introduce arch specific bits for IOREQ/DM features >>>> - xen/arm: Stick around in leave_hypervisor_to_guest until I/O has >>>> completed >>>> - update the author of a patch >>>> - update patch description >>>> - move a loop in leave_hypervisor_to_guest() to a separate patch >>>> - set IOREQ_SERVER disabled by default >>>> - remove already clarified /* XXX */ >>>> - replace BUG() by ASSERT_UNREACHABLE() in handle_pio() >>>> - remove default case for handling the return value of >>>> try_handle_mmio() >>>> - remove struct hvm_domain, enum hvm_io_completion, struct >>>> hvm_vcpu_io, >>>> struct hvm_vcpu from asm-arm/domain.h, these are common materials >>>> now >>>> - update everything according to the recent changes (IOREQ related >>>> function >>>> names don't contain "hvm" prefixes/infixes anymore, IOREQ related >>>> fields >>>> are part of common struct vcpu/domain now, etc) >>>> >>>> Changes V2 -> V3: >>>> - update patch according the "legacy interface" is x86 specific >>>> - add dummy arch hooks >>>> - remove dummy paging_mark_pfn_dirty() >>>> - don’t include <xen/domain_page.h> in common ioreq.c >>>> - don’t include <public/hvm/ioreq.h> in arch ioreq.h >>>> - remove #define ioreq_params(d, i) >>>> >>>> Changes V3 -> V4: >>>> - rebase >>>> - update patch according to the renaming IO_ -> VIO_ (io_ -> vio_) >>>> and misc changes to arch hooks >>>> - update patch according to the IOREQ related dm-op handling changes >>>> - don't include <xen/ioreq.h> from arch header >>>> - make all arch hooks out-of-line >>>> - add a comment above IOREQ_STATUS_* #define-s >>>> --- >>>> xen/arch/arm/Makefile | 2 + >>>> xen/arch/arm/dm.c | 122 +++++++++++++++++++++++ >>>> xen/arch/arm/domain.c | 9 ++ >>>> xen/arch/arm/io.c | 12 ++- >>>> xen/arch/arm/ioreq.c | 213 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> xen/arch/arm/traps.c | 13 +++ >>>> xen/include/asm-arm/domain.h | 3 + >>>> xen/include/asm-arm/hvm/ioreq.h | 72 ++++++++++++++ >>>> xen/include/asm-arm/mmio.h | 1 + >>>> 9 files changed, 446 insertions(+), 1 deletion(-) >>>> create mode 100644 xen/arch/arm/dm.c >>>> create mode 100644 xen/arch/arm/ioreq.c >>>> create mode 100644 xen/include/asm-arm/hvm/ioreq.h >>>> >>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>>> index 512ffdd..16e6523 100644 >>>> --- a/xen/arch/arm/Makefile >>>> +++ b/xen/arch/arm/Makefile >>>> @@ -13,6 +13,7 @@ obj-y += cpuerrata.o >>>> obj-y += cpufeature.o >>>> obj-y += decode.o >>>> obj-y += device.o >>>> +obj-$(CONFIG_IOREQ_SERVER) += dm.o >>>> obj-y += domain.o >>>> obj-y += domain_build.init.o >>>> obj-y += domctl.o >>>> @@ -27,6 +28,7 @@ obj-y += guest_atomics.o >>>> obj-y += guest_walk.o >>>> obj-y += hvm.o >>>> obj-y += io.o >>>> +obj-$(CONFIG_IOREQ_SERVER) += ioreq.o >>>> obj-y += irq.o >>>> obj-y += kernel.init.o >>>> obj-$(CONFIG_LIVEPATCH) += livepatch.o >>>> diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c >>>> new file mode 100644 >>>> index 0000000..e6dedf4 >>>> --- /dev/null >>>> +++ b/xen/arch/arm/dm.c >>>> @@ -0,0 +1,122 @@ >>>> +/* >>>> + * Copyright (c) 2019 Arm ltd. >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> it >>>> + * under the terms and conditions of the GNU General Public License, >>>> + * version 2, as published by the Free Software Foundation. >>>> + * >>>> + * This program is distributed in the hope it will be useful, but WITHOUT >>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >>>> for >>>> + * more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> along with >>>> + * this program; If not, see <http://www.gnu.org/licenses/>. >>>> + */ >>>> + >>>> +#include <xen/dm.h> >>>> +#include <xen/guest_access.h> >>>> +#include <xen/hypercall.h> >>>> +#include <xen/ioreq.h> >>>> +#include <xen/nospec.h> >>>> + >>>> +static int dm_op(const struct dmop_args *op_args) >>>> +{ >>>> + struct domain *d; >>>> + struct xen_dm_op op; >>>> + bool const_op = true; >>>> + long rc; >>>> + size_t offset; >>>> + >>>> + static const uint8_t op_size[] = { >>>> + [XEN_DMOP_create_ioreq_server] = sizeof(struct >>>> xen_dm_op_create_ioreq_server), >>>> + [XEN_DMOP_get_ioreq_server_info] = sizeof(struct >>>> xen_dm_op_get_ioreq_server_info), >>>> + [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct >>>> xen_dm_op_ioreq_server_range), >>>> + [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct >>>> xen_dm_op_ioreq_server_range), >>>> + [XEN_DMOP_set_ioreq_server_state] = sizeof(struct >>>> xen_dm_op_set_ioreq_server_state), >>>> + [XEN_DMOP_destroy_ioreq_server] = sizeof(struct >>>> xen_dm_op_destroy_ioreq_server), >>>> + }; >>>> + >>>> + rc = rcu_lock_remote_domain_by_id(op_args->domid, &d); >>>> + if ( rc ) >>>> + return rc; >>>> + >>>> + rc = xsm_dm_op(XSM_DM_PRIV, d); >>>> + if ( rc ) >>>> + goto out; >>>> + >>>> + offset = offsetof(struct xen_dm_op, u); >>>> + >>>> + rc = -EFAULT; >>>> + if ( op_args->buf[0].size < offset ) >>>> + goto out; >>>> + >>>> + if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, >>>> offset) ) >>>> + goto out; >>>> + >>>> + if ( op.op >= ARRAY_SIZE(op_size) ) >>>> + { >>>> + rc = -EOPNOTSUPP; >>>> + goto out; >>>> + } >>>> + >>>> + op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size)); >>>> + >>>> + if ( op_args->buf[0].size < offset + op_size[op.op] ) >>>> + goto out; >>>> + >>>> + if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset, >>>> + op_size[op.op]) ) >>>> + goto out; >>>> + >>>> + rc = -EINVAL; >>>> + if ( op.pad ) >>>> + goto out; >>>> + >>>> + rc = ioreq_server_dm_op(&op, d, &const_op); >>>> + >>>> + if ( (!rc || rc == -ERESTART) && >>>> + !const_op && copy_to_guest_offset(op_args->buf[0].h, offset, >>>> + (void *)&op.u, op_size[op.op]) >>>> ) >>>> + rc = -EFAULT; >>>> + >>>> + out: >>>> + rcu_unlock_domain(d); >>>> + >>>> + return rc; >>>> +} >>>> + >>>> +long do_dm_op(domid_t domid, >>>> + unsigned int nr_bufs, >>>> + XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs) >>>> +{ >>>> + struct dmop_args args; >>>> + int rc; >>>> + >>>> + if ( nr_bufs > ARRAY_SIZE(args.buf) ) >>>> + return -E2BIG; >>>> + >>>> + args.domid = domid; >>>> + args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1); >>>> + >>>> + if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) ) >>>> + return -EFAULT; >>>> + >>>> + rc = dm_op(&args); >>>> + >>>> + if ( rc == -ERESTART ) >>>> + rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih", >>>> + domid, nr_bufs, bufs); >>>> + >>>> + return rc; >>>> +} >>> I might have missed something in the discussions but this function is >>> identical to xen/arch/x86/hvm/dm.c:do_dm_op, why not make it common? >>> >>> Also the previous function dm_op is very similar to >>> xen/arch/x86/hvm/dm.c:dm_op I would prefer to make them common if >>> possible. Was this already discussed? >> Well, let me explain. Both dm_op() and do_dm_op() were indeed common (top >> level dm-op handling common) for previous versions, so Arm's dm.c didn't >> contain this stuff. >> The idea to make it other way around (top level dm-op handling arch-specific >> and call into ioreq_server_dm_op() for otherwise unhandled ops) was discussed >> at [1] which besides >> it's Pros leads to code duplication, so Arm's dm.c has to duplicate some >> stuff, etc. >> I was thinking about moving do_dm_op() which is _same_ for both arches to >> common code, but I am not sure whether it is conceptually correct which that >> new "alternative" approach of handling dm-op. > Yes, I think it makes sense to make do_dm_op common because it is > identical. That should be easy. Absolutely identical) Agree, technically it is not hard. Well, let's continue discussion in [1] (which actually leads to the duplication) to see if all parties are happy with that. > > I realize that the common part of dm_op is the initial boilerplate which > is similar for every hypercall, so I think it is also OK if we don't > share it and leave it as it is in this version of the series. ok [1] https://lore.kernel.org/xen-devel/1610488352-18494-10-git-send-email-olekstysh@gmail.com/
On Thu, 21 Jan 2021, Oleksandr wrote: > On 20.01.21 21:47, Stefano Stabellini wrote: > > On Wed, 20 Jan 2021, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 20/01/2021 00:50, Stefano Stabellini wrote: > > > > On Tue, 19 Jan 2021, Oleksandr wrote: > > > > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > > > > > index 40b9e59..0508bd8 100644 > > > > > --- a/xen/arch/arm/ioreq.c > > > > > +++ b/xen/arch/arm/ioreq.c > > > > > @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct > > > > > cpu_user_regs > > > > > *regs, > > > > > > > > > > bool arch_ioreq_complete_mmio(void) > > > > > { > > > > > - struct vcpu *v = current; > > > > > struct cpu_user_regs *regs = guest_cpu_user_regs(); > > > > > const union hsr hsr = { .bits = regs->hsr }; > > > > > - paddr_t addr = v->io.req.addr; > > > > > > > > > > - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > > > > > + if ( handle_ioserv(regs, current) == IO_HANDLED ) > > > > > { > > > > > advance_pc(regs, hsr); > > > > > return true; > > > > Yes, but I think we want to keep the check > > > > > > > > vio->req.state == STATE_IORESP_READY > > > > > > > > So maybe (uncompiled, untested): > > > > > > > > if ( v->io.req.state != STATE_IORESP_READY ) > > > > return false; > > > Is it possible to reach this function with v->io.req.state != > > > STATE_IORESP_READY? If not, then I would suggest to add an > > > ASSERT_UNREACHABLE() before the return. > > If I am reading the state machine right it should *not* be possible to > > get here with v->io.req.state != STATE_IORESP_READY, so yes, > > ASSERT_UNREACHABLE() would work. > Agree here. If the assumption is not correct (unlikely), I think I will catch > this during testing. > In addition, we can probably drop case STATE_IORESP_READY in try_fwd_ioserv(). > > > [not tested] Yes, looks OK > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > index 40b9e59..c7ee1a7 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -71,9 +71,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > case STATE_IOREQ_NONE: > break; > > - case STATE_IORESP_READY: > - return IO_HANDLED; > - > default: > gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); > return IO_ABORT; > @@ -104,9 +101,14 @@ bool arch_ioreq_complete_mmio(void) > struct vcpu *v = current; > struct cpu_user_regs *regs = guest_cpu_user_regs(); > const union hsr hsr = { .bits = regs->hsr }; > - paddr_t addr = v->io.req.addr; > > - if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) > + if ( v->io.req.state != STATE_IORESP_READY ) > + { > + ASSERT_UNREACHABLE(); > + return false; > + } > + > + if ( handle_ioserv(regs, v) == IO_HANDLED ) > { > advance_pc(regs, hsr); > return true;
On 21.01.2021 09:50, Oleksandr wrote: > On 20.01.21 17:50, Julien Grall wrote: >> On 17/01/2021 18:52, Oleksandr wrote: >>> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h >> >> It looks like the error is happening in dm.c rather than xen/dm.h. Any >> reason to not include <public/hvm/dm_op.h> in dm.c directly? > Including it directly doesn't solve build issue. > If I am not mistaken in order to follow requirements how to include > headers (alphabetic order, public* should be included after xen* and > asm* ones, etc) > the dm.h gets included the first in dm.c, and dm_op.h gets included the > last. We can avoid build issue if we change inclusion order a bit, > what I mean is to include dm.h after hypercall.h at least (because > hypercall.h already includes dm_op.h). But this breaks the requirements > and is not way to go. > Now I am in doubt how to overcome this. First, violating the alphabetic ordering rule is perhaps less of a problem than putting seemingly stray #include-s anywhere. However, as soon as ordering starts mattering, this is indicative of problems with the headers: Either the (seemingly) too early included one lacks some #include-s, or you've run into a circular dependency. In the former case the needed #include-s should be added, and all ought to be fine. In the latter case, however, disentangling may be a significant effort, and hence it may be sensible and acceptable to instead use unusual ordering of #include-s in the one place where it matters (suitably justified in the description). Ideally such would come with a promise to try to sort this later on, when time is less constrained. Jan
On 27.01.21 12:24, Jan Beulich wrote: Hi Jan > On 21.01.2021 09:50, Oleksandr wrote: >> On 20.01.21 17:50, Julien Grall wrote: >>> On 17/01/2021 18:52, Oleksandr wrote: >>>> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h >>> It looks like the error is happening in dm.c rather than xen/dm.h. Any >>> reason to not include <public/hvm/dm_op.h> in dm.c directly? >> Including it directly doesn't solve build issue. >> If I am not mistaken in order to follow requirements how to include >> headers (alphabetic order, public* should be included after xen* and >> asm* ones, etc) >> the dm.h gets included the first in dm.c, and dm_op.h gets included the >> last. We can avoid build issue if we change inclusion order a bit, >> what I mean is to include dm.h after hypercall.h at least (because >> hypercall.h already includes dm_op.h). But this breaks the requirements >> and is not way to go. >> Now I am in doubt how to overcome this. > First, violating the alphabetic ordering rule is perhaps less > of a problem than putting seemingly stray #include-s anywhere. > However, as soon as ordering starts mattering, this is > indicative of problems with the headers: Either the (seemingly) > too early included one lacks some #include-s, or you've run > into a circular dependency. In the former case the needed > #include-s should be added, and all ought to be fine. In the > latter case, however, disentangling may be a significant > effort, and hence it may be sensible and acceptable to instead > use unusual ordering of #include-s in the one place where it > matters (suitably justified in the description). Ideally such > would come with a promise to try to sort this later on, when > time is less constrained. Thank you for the explanation. I think, I am facing the former case (too early included one lacks some #include-s), actually both common/dm.c and arch/arm/dm.c suffer from that. It works for me if I add the following to both files (violating the alphabetic ordering rule): +#include <xen/types.h> +#include <public/hvm/dm_op.h> + #include <xen/dm.h> So, if I got your point correctly, we could include these both headers by dm.h) Would it be appropriate (with suitable justification of course)? I think, we could avoid including xen/sched.h by dm.h (need to recheck), just these two headers above. > > Jan
On 27.01.2021 13:22, Oleksandr wrote: > On 27.01.21 12:24, Jan Beulich wrote: >> On 21.01.2021 09:50, Oleksandr wrote: >>> On 20.01.21 17:50, Julien Grall wrote: >>>> On 17/01/2021 18:52, Oleksandr wrote: >>>>> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h >>>> It looks like the error is happening in dm.c rather than xen/dm.h. Any >>>> reason to not include <public/hvm/dm_op.h> in dm.c directly? >>> Including it directly doesn't solve build issue. >>> If I am not mistaken in order to follow requirements how to include >>> headers (alphabetic order, public* should be included after xen* and >>> asm* ones, etc) >>> the dm.h gets included the first in dm.c, and dm_op.h gets included the >>> last. We can avoid build issue if we change inclusion order a bit, >>> what I mean is to include dm.h after hypercall.h at least (because >>> hypercall.h already includes dm_op.h). But this breaks the requirements >>> and is not way to go. >>> Now I am in doubt how to overcome this. >> First, violating the alphabetic ordering rule is perhaps less >> of a problem than putting seemingly stray #include-s anywhere. >> However, as soon as ordering starts mattering, this is >> indicative of problems with the headers: Either the (seemingly) >> too early included one lacks some #include-s, or you've run >> into a circular dependency. In the former case the needed >> #include-s should be added, and all ought to be fine. In the >> latter case, however, disentangling may be a significant >> effort, and hence it may be sensible and acceptable to instead >> use unusual ordering of #include-s in the one place where it >> matters (suitably justified in the description). Ideally such >> would come with a promise to try to sort this later on, when >> time is less constrained. > Thank you for the explanation. I think, I am facing the former case (too > early included one lacks some #include-s), > actually both common/dm.c and arch/arm/dm.c suffer from that. > It works for me if I add the following to both files (violating the > alphabetic ordering rule): > > +#include <xen/types.h> > +#include <public/hvm/dm_op.h> > + > #include <xen/dm.h> > > > So, if I got your point correctly, we could include these both headers > by dm.h) Would it be appropriate (with suitable justification of course)? Perhaps - this is a header you introduce aiui, so it's up to you to arrange for it to include all further headers it depends upon. In such a case (new header) you don't need to explicitly justify what you include, but of course you don't want to include excessive ones, or you risk getting back "Why?" from reviewers. Jan
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 512ffdd..16e6523 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -13,6 +13,7 @@ obj-y += cpuerrata.o obj-y += cpufeature.o obj-y += decode.o obj-y += device.o +obj-$(CONFIG_IOREQ_SERVER) += dm.o obj-y += domain.o obj-y += domain_build.init.o obj-y += domctl.o @@ -27,6 +28,7 @@ obj-y += guest_atomics.o obj-y += guest_walk.o obj-y += hvm.o obj-y += io.o +obj-$(CONFIG_IOREQ_SERVER) += ioreq.o obj-y += irq.o obj-y += kernel.init.o obj-$(CONFIG_LIVEPATCH) += livepatch.o diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c new file mode 100644 index 0000000..e6dedf4 --- /dev/null +++ b/xen/arch/arm/dm.c @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2019 Arm ltd. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#include <xen/dm.h> +#include <xen/guest_access.h> +#include <xen/hypercall.h> +#include <xen/ioreq.h> +#include <xen/nospec.h> + +static int dm_op(const struct dmop_args *op_args) +{ + struct domain *d; + struct xen_dm_op op; + bool const_op = true; + long rc; + size_t offset; + + static const uint8_t op_size[] = { + [XEN_DMOP_create_ioreq_server] = sizeof(struct xen_dm_op_create_ioreq_server), + [XEN_DMOP_get_ioreq_server_info] = sizeof(struct xen_dm_op_get_ioreq_server_info), + [XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), + [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range), + [XEN_DMOP_set_ioreq_server_state] = sizeof(struct xen_dm_op_set_ioreq_server_state), + [XEN_DMOP_destroy_ioreq_server] = sizeof(struct xen_dm_op_destroy_ioreq_server), + }; + + rc = rcu_lock_remote_domain_by_id(op_args->domid, &d); + if ( rc ) + return rc; + + rc = xsm_dm_op(XSM_DM_PRIV, d); + if ( rc ) + goto out; + + offset = offsetof(struct xen_dm_op, u); + + rc = -EFAULT; + if ( op_args->buf[0].size < offset ) + goto out; + + if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, offset) ) + goto out; + + if ( op.op >= ARRAY_SIZE(op_size) ) + { + rc = -EOPNOTSUPP; + goto out; + } + + op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size)); + + if ( op_args->buf[0].size < offset + op_size[op.op] ) + goto out; + + if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset, + op_size[op.op]) ) + goto out; + + rc = -EINVAL; + if ( op.pad ) + goto out; + + rc = ioreq_server_dm_op(&op, d, &const_op); + + if ( (!rc || rc == -ERESTART) && + !const_op && copy_to_guest_offset(op_args->buf[0].h, offset, + (void *)&op.u, op_size[op.op]) ) + rc = -EFAULT; + + out: + rcu_unlock_domain(d); + + return rc; +} + +long do_dm_op(domid_t domid, + unsigned int nr_bufs, + XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs) +{ + struct dmop_args args; + int rc; + + if ( nr_bufs > ARRAY_SIZE(args.buf) ) + return -E2BIG; + + args.domid = domid; + args.nr_bufs = array_index_nospec(nr_bufs, ARRAY_SIZE(args.buf) + 1); + + if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) ) + return -EFAULT; + + rc = dm_op(&args); + + if ( rc == -ERESTART ) + rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih", + domid, nr_bufs, bufs); + + return rc; +} + +/* + * 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 18cafcd..8f55aba 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -15,6 +15,7 @@ #include <xen/guest_access.h> #include <xen/hypercall.h> #include <xen/init.h> +#include <xen/ioreq.h> #include <xen/lib.h> #include <xen/livepatch.h> #include <xen/sched.h> @@ -696,6 +697,10 @@ int arch_domain_create(struct domain *d, ASSERT(config != NULL); +#ifdef CONFIG_IOREQ_SERVER + ioreq_domain_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; @@ -1014,6 +1019,10 @@ int domain_relinquish_resources(struct domain *d) if (ret ) return ret; +#ifdef CONFIG_IOREQ_SERVER + ioreq_server_destroy_all(d); +#endif + PROGRESS(xen): ret = relinquish_memory(d, &d->xenpage_list); if ( ret ) diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index ae7ef96..9814481 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/ioreq.h> #include <xen/lib.h> #include <xen/spinlock.h> #include <xen/sched.h> @@ -23,6 +24,7 @@ #include <asm/cpuerrata.h> #include <asm/current.h> #include <asm/mmio.h> +#include <asm/hvm/ioreq.h> #include "decode.h" @@ -123,7 +125,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; + + rc = try_fwd_ioserv(regs, v, &info); + if ( rc == IO_HANDLED ) + return handle_ioserv(regs, v); + + 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..3c4a24d --- /dev/null +++ b/xen/arch/arm/ioreq.c @@ -0,0 +1,213 @@ +/* + * 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/domain.h> +#include <xen/ioreq.h> + +#include <asm/traps.h> + +#include <public/hvm/ioreq.h> + +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) +{ + 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->io.req.data; + + /* We are done with the IO */ + v->io.req.state = STATE_IOREQ_NONE; + + if ( dabt.write ) + return IO_HANDLED; + + /* + * 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 IO_HANDLED; +} + +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, + struct vcpu *v, mmio_info_t *info) +{ + struct vcpu_io *vio = &v->io; + ioreq_t p = { + .type = IOREQ_TYPE_COPY, + .addr = info->gpa, + .size = 1 << info->dabt.size, + .count = 1, + .dir = !info->dabt.write, + /* + * On x86, df 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. + */ + .df = 0, + .data = get_user_reg(regs, info->dabt.reg), + .state = STATE_IOREQ_READY, + }; + struct ioreq_server *s = NULL; + enum io_state rc; + + switch ( vio->req.state ) + { + case STATE_IOREQ_NONE: + break; + + case STATE_IORESP_READY: + return IO_HANDLED; + + default: + gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); + return IO_ABORT; + } + + s = ioreq_server_select(v->domain, &p); + if ( !s ) + return IO_UNHANDLED; + + if ( !info->dabt.valid ) + return IO_ABORT; + + vio->req = p; + + rc = ioreq_send(s, &p, 0); + if ( rc != IO_RETRY || v->domain->is_shutting_down ) + vio->req.state = STATE_IOREQ_NONE; + else if ( !ioreq_needs_completion(&vio->req) ) + rc = IO_HANDLED; + else + vio->completion = VIO_mmio_completion; + + return rc; +} + +bool arch_ioreq_complete_mmio(void) +{ + struct vcpu *v = current; + struct cpu_user_regs *regs = guest_cpu_user_regs(); + const union hsr hsr = { .bits = regs->hsr }; + paddr_t addr = v->io.req.addr; + + if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED ) + { + advance_pc(regs, hsr); + return true; + } + + return false; +} + +bool arch_vcpu_ioreq_completion(enum vio_completion completion) +{ + ASSERT_UNREACHABLE(); + return true; +} + +/* + * The "legacy" mechanism of mapping magic pages for the IOREQ servers + * is x86 specific, so the following hooks don't need to be implemented on Arm: + * - arch_ioreq_server_map_pages + * - arch_ioreq_server_unmap_pages + * - arch_ioreq_server_enable + * - arch_ioreq_server_disable + */ +int arch_ioreq_server_map_pages(struct ioreq_server *s) +{ + return -EOPNOTSUPP; +} + +void arch_ioreq_server_unmap_pages(struct ioreq_server *s) +{ +} + +void arch_ioreq_server_enable(struct ioreq_server *s) +{ +} + +void arch_ioreq_server_disable(struct ioreq_server *s) +{ +} + +void arch_ioreq_server_destroy(struct ioreq_server *s) +{ +} + +int arch_ioreq_server_map_mem_type(struct domain *d, + struct ioreq_server *s, + uint32_t flags) +{ + return -EOPNOTSUPP; +} + +void arch_ioreq_server_map_mem_type_completed(struct domain *d, + struct ioreq_server *s, + uint32_t flags) +{ +} + +bool arch_ioreq_server_destroy_all(struct domain *d) +{ + return true; +} + +bool arch_ioreq_server_get_type_addr(const struct domain *d, + const ioreq_t *p, + uint8_t *type, + uint64_t *addr) +{ + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO ) + return false; + + *type = (p->type == IOREQ_TYPE_PIO) ? + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY; + *addr = p->addr; + + return true; +} + +void arch_ioreq_domain_init(struct domain *d) +{ +} + +/* + * 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 22bd1bd..036b13f 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -21,6 +21,7 @@ #include <xen/hypercall.h> #include <xen/init.h> #include <xen/iocap.h> +#include <xen/ioreq.h> #include <xen/irq.h> #include <xen/lib.h> #include <xen/mem_access.h> @@ -1385,6 +1386,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 @@ -1956,6 +1960,9 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, case IO_HANDLED: advance_pc(regs, hsr); return; + case IO_RETRY: + /* finish later */ + return; case IO_UNHANDLED: /* IO unhandled, try another way to handle it. */ break; @@ -2254,6 +2261,12 @@ static void check_for_vcpu_work(void) { struct vcpu *v = current; +#ifdef CONFIG_IOREQ_SERVER + local_irq_enable(); + vcpu_ioreq_handle_completion(v); + local_irq_disable(); +#endif + if ( likely(!v->arch.need_flush_to_ram) ) return; diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 6819a3b..c235e5b 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -10,6 +10,7 @@ #include <asm/gic.h> #include <asm/vgic.h> #include <asm/vpl011.h> +#include <public/hvm/dm_op.h> #include <public/hvm/params.h> struct hvm_domain @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) +#define has_vpci(d) ({ (void)(d); false; }) + #endif /* __ASM_DOMAIN_H__ */ /* diff --git a/xen/include/asm-arm/hvm/ioreq.h b/xen/include/asm-arm/hvm/ioreq.h new file mode 100644 index 0000000..19e1247 --- /dev/null +++ b/xen/include/asm-arm/hvm/ioreq.h @@ -0,0 +1,72 @@ +/* + * 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__ + +#ifdef CONFIG_IOREQ_SERVER +enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v); +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, + struct vcpu *v, mmio_info_t *info); +#else +static inline enum io_state handle_ioserv(struct cpu_user_regs *regs, + struct vcpu *v) +{ + return IO_UNHANDLED; +} + +static inline enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, + struct vcpu *v, mmio_info_t *info) +{ + return IO_UNHANDLED; +} +#endif + +bool ioreq_complete_mmio(void); + +static inline bool handle_pio(uint16_t port, unsigned int size, int dir) +{ + /* + * TODO: For Arm64, the main user will be PCI. So this should be + * implemented when we add support for vPCI. + */ + ASSERT_UNREACHABLE(); + return true; +} + +static inline void msix_write_completion(struct vcpu *v) +{ +} + +/* This correlation must not be altered */ +#define IOREQ_STATUS_HANDLED IO_HANDLED +#define IOREQ_STATUS_UNHANDLED IO_UNHANDLED +#define IOREQ_STATUS_RETRY IO_RETRY + +#endif /* __ASM_ARM_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,