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

[Oleksandr <olekstysh@xxxxxxxxx>]

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...
--
Regards,

Oleksandr Tyshchenko