> -----Original Message-----
> From: Oleksandr <[email protected]>
> Sent: 04 August 2020 12:51
> To: [email protected]; [email protected]
> Cc: 'Oleksandr Tyshchenko' <[email protected]>; 'Jan Beulich'
> <[email protected]>; 'Andrew
> Cooper' <[email protected]>; 'Wei Liu' <[email protected]>; 'Roger Pau
> Monné' <[email protected]>;
> 'George Dunlap' <[email protected]>; 'Ian Jackson'
> <[email protected]>; 'Julien Grall'
> <[email protected]>; 'Stefano Stabellini' <[email protected]>; 'Jun
> Nakajima'
> <[email protected]>; 'Kevin Tian' <[email protected]>; 'Tim Deegan'
> <[email protected]>; 'Julien
> Grall' <[email protected]>
> Subject: Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common
>
>
> 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
>
Yes, but that code is clearly not finished since, after setting io_completion
it says:
/* XXX: Decide what to do */
if ( rc == IO_RETRY )
rc = IO_HANDLED;
So, it's not clear what the eventual implementation will be and whether it will
need to make that call.
Paul
>
> --
> Regards,
>
> Oleksandr Tyshchenko
