Re: [PATCH 07/19] arm/gnttab: Break cycle between asm/grant_table.h and xen/grant_table.h

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Tue Jun 3, 2025 at 9:04 AM CEST, Jan Beulich wrote:
> On 02.06.2025 18:39, Alejandro Vallejo wrote:
>> On Mon Jun 2, 2025 at 4:51 PM CEST, Jan Beulich wrote:
>>> On 02.06.2025 16:30, Alejandro Vallejo wrote:
>>>> On Mon Jun 2, 2025 at 9:53 AM CEST, Jan Beulich wrote:
>>>>> On 30.05.2025 14:02, Alejandro Vallejo wrote:> --- 
>>>>> a/xen/include/xen/grant_table.h
>>>>>> +++ b/xen/include/xen/grant_table.h
>>>>>> @@ -27,7 +27,7 @@
>>>>>>  #include <xen/rwlock.h>
>>>>>>  #include <public/grant_table.h>
>>>>>>  
>>>>>> -#ifdef CONFIG_GRANT_TABLE
>>>>>> +#if __has_include("asm/grant_table.h")
>>>>>>  #include <asm/grant_table.h>
>>>>>>  #endif
>>>>>
>>>>> This change looks wrong (or otherwise is lacking justification): With 
>>>>> GRANT_TABLE=n
>>>>> the arch header isn't supposed to be included.
>>>>
>>>> It's not equivalent to the previous code; but that's a feature, not a bug.
>>>>
>>>> Not including the header with GRANT_TABLE=n  was the best we could with
>>>> the older toolchains in order to not try to include a header that might not
>>>> exist. The high number of sequential inclusions of xen/grant_table.h and
>>>> asm/grant_table.h seem to attest to that.
>>>>
>>>> I can ammend the commit message to be clearer, but IMO this is what it was 
>>>> always
>>>> meant to be. I can replace the current commit message with:
>>>>
>>>>   "The previous toolchain base version didn't provide __has_include(), 
>>>> which
>>>>    allows conditional inclusion based on a header's existence. Lacking that
>>>>    feature the inclusion was guarded by the GRANT_TABLE option being 
>>>> present
>>>>    but even then sometimes the arch-specific header is required even when
>>>>    the option is not selected. This causes inclusion sites to needlessly
>>>>    include both asm/grant_table.h and xen/grant_table.h.
>>>>
>>>>    Using __has_include() removes this requirement at inclusion sites."
>>>>
>>>> Thoughts?
>>>
>>> So why would we include a header we don't need when GRANT_TABLE=n? Overall
>>> we're trying to reduce dependency trees rather than growing them further.
>> 
>> Because we do need it or the code doesn't compile. gnttab_dom0_frames(), for
>> instance, exists and is used in unguarded statements.
>
> I fear I don't understand this: Things are building fine right now, aren't
> they?
>
>> There's more case like
>> that. It may be possible to break those dependencies so the inclusion is
>> not always expected, but the reality is that you tend to need the other 
>> header
>> for pretty much the same class of reasons you needed xen/grant_table.h to 
>> begin
>> with.
>> 
>> Forcing the code to include both seems counter-productive to me.
>
> Depends on how frequent the double inclusion is, imo.
>
> Jan

Not as much as I feared. I've removed both linkages for v2.

Cheers,
Alejandro