Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 22.09.20 09:33, Jan Beulich wrote:

Hi Jan

> On 21.09.2020 21:02, Oleksandr wrote:
> > On 14.09.20 17:17, Jan Beulich wrote:
> > > On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/xen/ioreq.h
> > > > @@ -0,0 +1,82 @@
> > > > +/*
> > > > + * ioreq.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 __IOREQ_H__
> > > > +#define __IOREQ_H__
> > > __XEN_IOREQ_H__ please.
> > ok
> >
> >
> > > > +#include <xen/sched.h>
> > > > +
> > > > +#include <asm/hvm/ioreq.h>
> > > Is this include really needed here (i.e. by the code further down in
> > > the file, and hence by everyone including this header), or rather
> > > just in a few specific .c files?
> > I think, just in few specific .c files. Which are x86/hvm/ioreq.c and
> > common/ioreq.c now and several other files later on (x86/hvm/dm.c,
> > arm/io.c, etc)
> > Shall I include that header in these files instead?
> Yes please, and please take this as a common guideline. While
> private headers are often used to include things needed by all
> of the (typically few) users of the header, non-private ones
> shouldn't create unnecessary dependencies on other headers. As
> you've said further up - you did run into hard to resolve
> header dependencies yourself, and the practice of including
> headers without strict need is one of the reasons of such
> problems.
Got it.


> > > > +#define GET_IOREQ_SERVER(d, id) \
> > > > +    (d)->arch.hvm.ioreq_server.server[id]
> > > arch.hvm.* feels like a layering violation when used in this header.
> > Got it. The only reason why GET_IOREQ_SERVER is here is inline
> > get_ioreq_server(). I will make it non-inline and move both to
> > common/ioreq.c.
> Which won't make the layering violation go away. It's still
> common rather than per-arch code then. As suggested elsewhere,
> I think the whole ioreq_server struct wants to move into
> struct domain itself, perhaps inside a new #ifdef (iirc one of
> the patches introduces a suitable Kconfig option).
Well, your advise regarding ioreq_server sounds reasonable, but the 
common ioreq.c
still will have other *arch.hvm.* for both vcpu and domain. So looks 
like other involved structs should be moved
into *common* struct domain/vcpu itself, correct? Some of them could be 
moved easily since contain the same fields (arch.hvm.ioreq_gfn),
but some of them couldn't and seems to require to pull a lot of changes 
to the Xen code (arch.hvm.params, arch.hvm.hvm_io), I am afraid.
Or I missed something?


> This goes
> alongside my suggestion to drop the "hvm" prefixes and infixes
> from involved function names.
Well, I assume this request as well as the request above should be 
addressed in the follow-up patches, as we want to keep the code movement 
in current patch as (almost) verbatim copy,
Am I correct?

--
Regards,

Oleksandr Tyshchenko