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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2021 18:01, Julien Grall wrote:
> Hi Jan,
> 
> On 26/01/2021 09:15, Jan Beulich wrote:
>> 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. (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.)
> 
> dm.h contains the following:
> 
> struct dmop_args {
>      domid_t domid;
>      unsigned int nr_bufs;
>      /* Reserve enough buf elements for all current hypercalls. */
>      struct xen_dm_op_buf buf[2];
> };

Which means the public header should be included, yes, but
right away at the point this dependency appears, not at a
random point later in the series.

Jan

> The struct xen_dm_op_buf is defined public/hvm/dm_op.h. On x86, this 
> will be indirectly included via:
>     ->  xen/sched.h
>      ->  xen/domain.h
>       ->  asm/domain.h
>        ->  asm/hvm/domain.h
>         ->  public/hvm/dm_op.h
> 
> It looks like this was include from asm/hvm/domain.h because, before 
> this series, NR_IO_RANGE_TYPES made use of a DMOP definition.
> 
> With this series, the type is now moved in ioreq.h. So I think we may be 
> able to drop the include from asm/hvm/domain.h (this would avoid to 
> include it everywhere...).
> 
> I also think we want to include public/hvm/dm_op.h in xen/dm.h because 
> it is included directly by *.c.
> 
> Cheers,
>