Re: [XEN PATCH] ioreq: include arch-specific ioreq header in <xen/ioreq.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2023 11:13, Nicola Vetrini wrote:
> On 01/09/2023 09:36, Jan Beulich wrote:
>> On 01.09.2023 09:13, Nicola Vetrini wrote:
>>> On 28/08/2023 09:59, Jan Beulich wrote:
>>>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>>>> The common header file for ioreq should include the arch-specific 
>>>>> one.
>>>>
>>>> To be honest I'm not convinced of "should" here. There are two 
>>>> aspects
>>>> to consider: On one hand it is good practice to do what you say. Otoh
>>>> it
>>>> introduces a needless dependency, and it'll require new arch-es
>>>> (RISC-V,
>>>> PPC at present) to introduce another dummy header just to satisfy the
>>>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go 
>>>> this
>>>> route, besides a wording change here I think ...
>>>>
>>>
>>> Since including both <xen/ioreq.h> and the arch-specific one was
>>> requested to be changed
>>> in previous series, I was trying to apply the same pattern here.
>>> If you prefer not to change the current layout then the fix is even
>>> simpler: I'll just
>>> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's
>>> missing.
>>>
>>>>> --- a/xen/include/xen/ioreq.h
>>>>> +++ b/xen/include/xen/ioreq.h
>>>>> @@ -20,6 +20,7 @@
>>>>>  #define __XEN_IOREQ_H__
>>>>>
>>>>>  #include <xen/sched.h>
>>>>> +#include <asm/ioreq.h>
>>>>
>>>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER
>>>> being
>>>> defined. (I'm actually surprised that there's no respective #ifdef in
>>>> that header, meaning types defined there are available even when the
>>>> functionality was turned off.)
>>>
>>> The two functions in question are actually guarded by an #ifdef
>>> CONFIG_IOREQ_SERVER
>>> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are
>>> defined)
>>
>> Well, I don't see how an #ifdef there helps with the aspect mentioned
>> earlier (new arch-es needing to needlessly provide such a header as 
>> long
>> as the #include here is unconditional).
> 
> As far as I can tell, including the asm header in the arm implementation 
> file does not imply
> that new archs will need such an header. Of course, if the solution 
> proposed in the patch is
> chosen then I agree with you.

Hmm, maybe then I misunderstood your earlier reply: I thought you were
arguing against the change I had suggested.

Jan