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

[Jan Beulich <jbeulich@xxxxxxxx>]

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