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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr <olekstysh@xxxxxxxxx>
> Sent: 04 August 2020 12:10
> To: paul@xxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: 'Oleksandr Tyshchenko' <oleksandr_tyshchenko@xxxxxxxx>; 'Jan Beulich' 
> <jbeulich@xxxxxxxx>; 'Andrew
> Cooper' <andrew.cooper3@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>; 'Roger Pau 
> Monné' <roger.pau@xxxxxxxxxx>;
> 'George Dunlap' <george.dunlap@xxxxxxxxxx>; 'Ian Jackson' 
> <ian.jackson@xxxxxxxxxxxxx>; 'Julien Grall'
> <julien@xxxxxxx>; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Jun 
> Nakajima'
> <jun.nakajima@xxxxxxxxx>; 'Kevin Tian' <kevin.tian@xxxxxxxxx>; 'Tim Deegan' 
> <tim@xxxxxxx>; 'Julien
> Grall' <julien.grall@xxxxxxx>
> Subject: Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common
> 
> 
> On 04.08.20 10:45, Paul Durrant wrote:
> 
> Hi Paul
> 
> >> -----Original Message-----
> >> From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
> >> Sent: 03 August 2020 19:21
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Jan Beulich 
> >> <jbeulich@xxxxxxxx>; Andrew
> >> Cooper <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné 
> >> <roger.pau@xxxxxxxxxx>;
> >> George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson 
> >> <ian.jackson@xxxxxxxxxxxxx>; Julien Grall
> >> <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Paul 
> >> Durrant <paul@xxxxxxx>; Jun
> >> Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Tim 
> >> Deegan <tim@xxxxxxx>;
> Julien
> >> Grall <julien.grall@xxxxxxx>
> >> Subject: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common
> >>
> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >>
> >> 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@xxxxxxx>
> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >> ---
> >>   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.
> 
> Oh, I completely forgot about MAINTAINERS file. Sure, I will update file
> and retain you.
> 
> 
> >
> > [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.
> 
> ok
> 
> 
> >> +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.
> 
> ok, will update.
> 
> 
> > 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?
> 
> I am not 100% sure here, but it seems that doesn't have.
> 
> Shall I make hvm_ioreq_needs_completion() per arch? Arm variant would
> have the same implementation, but without "ioreq->type !=
> IOREQ_TYPE_PIO" check...
> 

With your series applied, does any common code actually call 
hvm_ioreq_needs_completion()? I suspect it will remain x86 specific, without 
any need for an Arm variant.

  Paul

> --
> Regards,
> 
> Oleksandr Tyshchenko