RE: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Oleksandr Tyshchenko <olekstysh@gmail.com>
> Sent: 03 August 2020 19:21
> To: xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>;
> George Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall
> <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Paul Durrant <paul@xen.org>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Tim Deegan <tim@xen.org>; Julien
> Grall <julien.grall@arm.com>
> Subject: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> As a lot of x86 code can be re-used on Arm later on, this patch
> splits IOREQ support into common and arch specific parts.
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.
> 
> Please note, this is a split/cleanup of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/arch/x86/Kconfig            |    1 +
>  xen/arch/x86/hvm/dm.c           |    2 +-
>  xen/arch/x86/hvm/emulate.c      |    2 +-
>  xen/arch/x86/hvm/hvm.c          |    2 +-
>  xen/arch/x86/hvm/io.c           |    2 +-
>  xen/arch/x86/hvm/ioreq.c        | 1431 +--------------------------------------
>  xen/arch/x86/hvm/stdvga.c       |    2 +-
>  xen/arch/x86/hvm/vmx/realmode.c |    1 +
>  xen/arch/x86/hvm/vmx/vvmx.c     |    2 +-
>  xen/arch/x86/mm.c               |    2 +-
>  xen/arch/x86/mm/shadow/common.c |    2 +-
>  xen/common/Kconfig              |    3 +
>  xen/common/Makefile             |    1 +
>  xen/common/hvm/Makefile         |    1 +
>  xen/common/hvm/ioreq.c          | 1430 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/ioreq.h |   45 +-
>  xen/include/asm-x86/hvm/vcpu.h  |    7 -
>  xen/include/xen/hvm/ioreq.h     |   89 +++
>  18 files changed, 1575 insertions(+), 1450 deletions(-)
>  create mode 100644 xen/common/hvm/Makefile
>  create mode 100644 xen/common/hvm/ioreq.c
>  create mode 100644 xen/include/xen/hvm/ioreq.h

You need to adjust the MAINTAINERS file since there will now be common 'I/O EMULATION' code. Since I wrote most of ioreq.c, please retain me as a maintainer of the common code.

[snip]
> @@ -1528,13 +143,19 @@ static int hvm_access_cf8(
>      return X86EMUL_UNHANDLEABLE;
>  }
> 
> -void hvm_ioreq_init(struct domain *d)
> +void arch_hvm_ioreq_init(struct domain *d)
>  {
>      spin_lock_init(&d->arch.hvm.ioreq_server.lock);
> 
>      register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
>  }
> 
> +void arch_hvm_ioreq_destroy(struct domain *d)
> +{
> +    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> +        return;

There's not really a lot of point in this. I think an empty function here would be ok.

> +}
> +
>  /*
>   * Local variables:
>   * mode: C

[snip]
> +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> +                                                 ioreq_t *p)
> +{
> +    struct hvm_ioreq_server *s;
> +    uint8_t type;
> +    uint64_t addr;
> +    unsigned int id;
> +
> +    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> +        return NULL;
> +
> +    hvm_get_ioreq_server_range_type(d, p, &type, &addr);

Looking at this, I think it would make more sense to fold the check of p->type into hvm_get_ioreq_server_range_type() and have it return success/failure.

> +
> +    FOR_EACH_IOREQ_SERVER(d, id, s)
> +    {
> +        struct rangeset *r;
> +
> +        if ( !s->enabled )
> +            continue;
> +
> +        r = s->range[type];
> +
> +        switch ( type )
> +        {
> +            unsigned long start, end;
> +
> +        case XEN_DMOP_IO_RANGE_PORT:
> +            start = addr;
> +            end = start + p->size - 1;
> +            if ( rangeset_contains_range(r, start, end) )
> +                return s;
> +
> +            break;
> +
> +        case XEN_DMOP_IO_RANGE_MEMORY:
> +            start = hvm_mmio_first_byte(p);
> +            end = hvm_mmio_last_byte(p);
> +
> +            if ( rangeset_contains_range(r, start, end) )
> +                return s;
> +
> +            break;
> +
> +        case XEN_DMOP_IO_RANGE_PCI:
> +            if ( rangeset_contains_singleton(r, addr >> 32) )
> +            {
> +                p->type = IOREQ_TYPE_PCI_CONFIG;
> +                p->addr = addr;
> +                return s;
> +            }
> +
> +            break;
> +        }
> +    }
> +
> +    return NULL;
> +}

[snip]
> diff --git a/xen/include/xen/hvm/ioreq.h b/xen/include/xen/hvm/ioreq.h
> new file mode 100644
> index 0000000..40b7b5e
> --- /dev/null
> +++ b/xen/include/xen/hvm/ioreq.h
> @@ -0,0 +1,89 @@
> +/*
> + * hvm.h: Hardware virtual machine assist interface definitions.
> + *
> + * Copyright (c) 2016 Citrix Systems Inc.
> + *
> + * 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 __HVM_IOREQ_H__
> +#define __HVM_IOREQ_H__
> +
> +#include <xen/sched.h>
> +
> +#include <asm/hvm/ioreq.h>
> +
> +#define GET_IOREQ_SERVER(d, id) \
> +    (d)->arch.hvm.ioreq_server.server[id]
> +
> +static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> +                                                        unsigned int id)
> +{
> +    if ( id >= MAX_NR_IOREQ_SERVERS )
> +        return NULL;
> +
> +    return GET_IOREQ_SERVER(d, id);
> +}
> +
> +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
> +{
> +    return ioreq->state == STATE_IOREQ_READY &&
> +           !ioreq->data_is_ptr &&
> +           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
> +}

I don't think having this in common code is correct. The short-cut of not completing PIO reads seems somewhat x86 specific. Does ARM even have the concept of PIO?

  Paul

> +
> +bool hvm_io_pending(struct vcpu *v);
> +bool handle_hvm_io_completion(struct vcpu *v);
> +bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
> +
> +int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
> +                            ioservid_t *id);
> +int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id);
> +int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
> +                              unsigned long *ioreq_gfn,
> +                              unsigned long *bufioreq_gfn,
> +                              evtchn_port_t *bufioreq_port);
> +int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
> +                               unsigned long idx, mfn_t *mfn);
> +int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
> +                                     uint32_t type, uint64_t start,
> +                                     uint64_t end);
> +int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
> +                                         uint32_t type, uint64_t start,
> +                                         uint64_t end);
> +int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
> +                               bool enabled);
> +
> +int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v);
> +void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v);
> +void hvm_destroy_all_ioreq_servers(struct domain *d);
> +
> +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> +                                                 ioreq_t *p);
> +int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
> +                   bool buffered);
> +unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
> +
> +void hvm_ioreq_init(struct domain *d);
> +
> +#endif /* __HVM_IOREQ_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.7.4