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

[Oleksandr <olekstysh@gmail.com>]

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