RE: [PATCH V4 09/24] xen/ioreq: Make x86's IOREQ related dm-op handling common

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
> Sent: 12 January 2021 21:52
> To: xen-devel@lists.xenproject.org
> Cc: Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Julien Grall <julien@xen.org>;
> Stefano Stabellini <sstabellini@kernel.org>; Paul Durrant <paul@xen.org>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Subject: [PATCH V4 09/24] xen/ioreq: Make x86's IOREQ related dm-op handling common
> 
> From: Julien Grall <julien.grall@arm.com>
> 
> As a lot of x86 code can be re-used on Arm later on, this patch
> moves the IOREQ related dm-op handling to the common code.
> 
> The idea is to have the top level dm-op handling arch-specific
> and call into ioreq_server_dm_op() for otherwise unhandled ops.
> Pros:
> - More natural than doing it other way around (top level dm-op
> handling common).
> - Leave compat_dm_op() in x86 code.
> Cons:
> - Code duplication. Both arches have to duplicate do_dm_op(), etc.
> 
> Also update XSM code a bit to let dm-op be used on Arm.
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.
> 
> 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"
> 
> ***
> I decided to leave common dm.h to keep struct dmop_args declaration
> (to be included by Arm's dm.c), alternatively we could avoid
> introducing new header by moving the declaration into the existing
> header, but failed to find a suitable one which context would fit.
> ***
> 
> Changes RFC -> V1:
>    - update XSM, related changes were pulled from:
>      [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features
> 
> Changes V1 -> V2:
>    - update the author of a patch
>    - update patch description
>    - introduce xen/dm.h and move definitions here
> 
> Changes V2 -> V3:
>    - no changes
> 
> Changes V3 -> V4:
>    - rework to have the top level dm-op handling arch-specific
>    - update patch subject/description, was "xen/dm: Make x86's DM feature common"
>    - make a few functions static in common ioreq.c
> ---
>  xen/arch/x86/hvm/dm.c   | 101 +-----------------------------------
>  xen/common/ioreq.c      | 135 ++++++++++++++++++++++++++++++++++++++++++------
>  xen/include/xen/dm.h    |  39 ++++++++++++++
>  xen/include/xen/ioreq.h |  17 +-----
>  xen/include/xsm/dummy.h |   4 +-
>  xen/include/xsm/xsm.h   |   6 +--
>  xen/xsm/dummy.c         |   2 +-
>  xen/xsm/flask/hooks.c   |   5 +-
>  8 files changed, 171 insertions(+), 138 deletions(-)
>  create mode 100644 xen/include/xen/dm.h
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d3e2a9e..dc8e47d 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -16,6 +16,7 @@
> 
>  #include <xen/event.h>
>  #include <xen/guest_access.h>
> +#include <xen/dm.h>
>  #include <xen/hypercall.h>
>  #include <xen/ioreq.h>
>  #include <xen/nospec.h>
> @@ -29,13 +30,6 @@
> 
>  #include <public/hvm/hvm_op.h>
> 
> -struct dmop_args {
> -    domid_t domid;
> -    unsigned int nr_bufs;
> -    /* Reserve enough buf elements for all current hypercalls. */
> -    struct xen_dm_op_buf buf[2];
> -};
> -
>  static bool _raw_copy_from_guest_buf_offset(void *dst,
>                                              const struct dmop_args *args,
>                                              unsigned int buf_idx,
> @@ -408,71 +402,6 @@ static int dm_op(const struct dmop_args *op_args)
> 
>      switch ( op.op )
>      {
> -    case XEN_DMOP_create_ioreq_server:
> -    {
> -        struct xen_dm_op_create_ioreq_server *data =
> -            &op.u.create_ioreq_server;
> -
> -        const_op = false;
> -
> -        rc = -EINVAL;
> -        if ( data->pad[0] || data->pad[1] || data->pad[2] )
> -            break;
> -
> -        rc = hvm_create_ioreq_server(d, data->handle_bufioreq,
> -                                     &data->id);
> -        break;
> -    }
> -
> -    case XEN_DMOP_get_ioreq_server_info:
> -    {
> -        struct xen_dm_op_get_ioreq_server_info *data =
> -            &op.u.get_ioreq_server_info;
> -        const uint16_t valid_flags = XEN_DMOP_no_gfns;
> -
> -        const_op = false;
> -
> -        rc = -EINVAL;
> -        if ( data->flags & ~valid_flags )
> -            break;
> -
> -        rc = hvm_get_ioreq_server_info(d, data->id,
> -                                       (data->flags & XEN_DMOP_no_gfns) ?
> -                                       NULL : &data->ioreq_gfn,
> -                                       (data->flags & XEN_DMOP_no_gfns) ?
> -                                       NULL : &data->bufioreq_gfn,
> -                                       &data->bufioreq_port);
> -        break;
> -    }
> -
> -    case XEN_DMOP_map_io_range_to_ioreq_server:
> -    {
> -        const struct xen_dm_op_ioreq_server_range *data =
> -            &op.u.map_io_range_to_ioreq_server;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = hvm_map_io_range_to_ioreq_server(d, data->id, data->type,
> -                                              data->start, data->end);
> -        break;
> -    }
> -
> -    case XEN_DMOP_unmap_io_range_from_ioreq_server:
> -    {
> -        const struct xen_dm_op_ioreq_server_range *data =
> -            &op.u.unmap_io_range_from_ioreq_server;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = hvm_unmap_io_range_from_ioreq_server(d, data->id, data->type,
> -                                                  data->start, data->end);
> -        break;
> -    }
> -
>      case XEN_DMOP_map_mem_type_to_ioreq_server:
>      {
>          struct xen_dm_op_map_mem_type_to_ioreq_server *data =
> @@ -523,32 +452,6 @@ static int dm_op(const struct dmop_args *op_args)
>          break;
>      }
> 
> -    case XEN_DMOP_set_ioreq_server_state:
> -    {
> -        const struct xen_dm_op_set_ioreq_server_state *data =
> -            &op.u.set_ioreq_server_state;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = hvm_set_ioreq_server_state(d, data->id, !!data->enabled);
> -        break;
> -    }
> -
> -    case XEN_DMOP_destroy_ioreq_server:
> -    {
> -        const struct xen_dm_op_destroy_ioreq_server *data =
> -            &op.u.destroy_ioreq_server;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = hvm_destroy_ioreq_server(d, data->id);
> -        break;
> -    }
> -
>      case XEN_DMOP_track_dirty_vram:
>      {
>          const struct xen_dm_op_track_dirty_vram *data =
> @@ -703,7 +606,7 @@ static int dm_op(const struct dmop_args *op_args)
>      }
> 
>      default:
> -        rc = -EOPNOTSUPP;
> +        rc = ioreq_server_dm_op(&op, d, &const_op);
>          break;
>      }
> 
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index a319c88..72b5da0 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -591,8 +591,8 @@ static void hvm_ioreq_server_deinit(struct ioreq_server *s)
>      put_domain(s->emulator);
>  }
> 
> -int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
> -                            ioservid_t *id)
> +static int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
> +                                   ioservid_t *id)

Would this not be a good opportunity to drop the 'hvm_' prefix (here and elsewhere)?

  Paul