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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 04.08.20 14:23, Paul Durrant wrote:
> > > 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.
Yes, it does. Please see common usage in hvm_io_assist() and 
handle_hvm_io_completion() (current patch) and usage in Arm code 
(arch/arm/io.c: io_state try_fwd_ioserv) [1]

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg00072.html

--
Regards,

Oleksandr Tyshchenko