Re: [PATCH v2 02/15] arm/gnttab: Break links between asm/grant_table.h and xen/grant_table.h

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Fri Jun 6, 2025 at 12:07 PM CEST, Jan Beulich wrote:
> On 06.06.2025 12:02, Alejandro Vallejo wrote:
>> On Fri Jun 6, 2025 at 8:52 AM CEST, Jan Beulich wrote:
>>> On 05.06.2025 21:47, Alejandro Vallejo wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -42,8 +42,10 @@
>>>>  #include <xen/xvmalloc.h>
>>>>  #include <xen/nospec.h>
>>>>  #include <xsm/xsm.h>
>>>> +
>>>>  #include <asm/flushtlb.h>
>>>>  #include <asm/guest_atomics.h>
>>>> +#include <asm/grant_table.h>
>>>>  
>>>>  #ifdef CONFIG_PV_SHIM
>>>>  #include <asm/guest.h>
>>>> --- a/xen/include/xen/grant_table.h
>>>> +++ b/xen/include/xen/grant_table.h
>>>> @@ -27,10 +27,6 @@
>>>>  #include <xen/rwlock.h>
>>>>  #include <public/grant_table.h>
>>>>  
>>>> -#ifdef CONFIG_GRANT_TABLE
>>>> -#include <asm/grant_table.h>
>>>> -#endif
>>>> -
>>>>  struct grant_table;
>>>>  
>>>>  /* Seed a gnttab entry for Hyperlaunch/dom0less. */
>>>
>>> The description doesn't make clear why these two files need changing.
>> 
>> What sort of description? I removed a conditional include  and added it to 
>> one
>> of the few places it didn't include it already along with xen/grant_table.h.
>> 
>> The title does say the patch removes the crossed includes in 
>> asm/grant_table.h
>> and xen/grant_table.h.
>> 
>> It's, I hope, self-explanatory regular spring cleanup.
>
> Then I'm sorry, to me it isn't. "Break links" has an entirely different (file
> system) meaning to me, in the common case. Plus that says what is being done,
> but not why. And it's the "why" that I'm seeking clarification on. From your
> response to my remarks on v1 I was concluding that the issue is that in a few
> places asm/grant_table.h would need including additionally. I didn't expect
> any #include to (need to) go away.
>
> Jan

Let me take a step back then. How about this commit message for this same patch?

  xen/gnttab: Remove cyclic includes in xen/grant_table.h and arm's 
asm/grant_table.h

  The way they currently include each other, with one of the includes being 
conditional
  on CONFIG_GRANT_TABLE, makes it hard to know which contents are included when.

  Seeing how nothing in either header depends on the other, let the include 
sites
  include both if both are needed.

  Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>

Cheers,
Alejandro