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:51
> 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 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