Re: [PATCH V5 14/22] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH V5 14/22] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
Date: Wed, 27 Jan 2021 11:54:49 +0200	[thread overview]
Message-ID: <9016d379-2b79-0b1c-ff25-9c5f7284e3c9@gmail.com> (raw)
In-Reply-To: <ff05bfd8-150a-1dbc-8c25-c922ee863146@suse.com>

On 26.01.21 11:15, Jan Beulich wrote:

Hi Jan

> On 25.01.2021 20:08, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/xen/dm.h
>> +++ b/xen/include/xen/dm.h
>> @@ -19,6 +19,8 @@
>>   
>>   #include <xen/sched.h>
>>   
>> +#include <public/hvm/dm_op.h>
>> +
>>   struct dmop_args {
>>       domid_t domid;
>>       unsigned int nr_bufs;
> How come this becomes necessary at this point in the series, when
> nothing else in this header changes, and nothing changes in the
> public headers at all? Is it perhaps a .c file that needs the
> #include instead? Headers shouldn't pull in other headers without
> clear need - as indicated in reply to a prior version, we have
> way too many bad examples (causing headaches in certain cases),
> and we'd like to avoid gaining more.

Yes, I understand this and completely agree. I remember last discussion 
on that, this is not forgotten. The only reason I made this (non 
entirely correct) change is to make
series compilable on Arm with IOREQ support enabled. If I considered 
this change as a correct one I would make it from the very beginning (in 
patch #9 which adds this common header)...
I added remark to draw reviewer's attention on the fact that I got stuck 
with resolving that, what I did wrong and how it should be done 
properly. The problem is that I didn't manage to make it properly.

Of course, I tried to include it directly by dm.c, but this didn't help 
much without violating "alphabetical order" rule. Details here:
https://lore.kernel.org/xen-devel/e0bc7f80-974e-945d-4605-173bd05302af@gmail.com/

I would appreciate any input on that.

> (Oh, I notice you actually
> have a post-commit-message remark about this, but then this
> patch should be marked RFC until the issue was resolved.)

Agree, I should have marked this patch as RFC to avoid misunderstanding.

-- 
Regards,

Oleksandr Tyshchenko