Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

Yes, I think it makes sense to make do_dm_op common because it is
identical. That should be easy.

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.