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
