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

[Alejandro Vallejo <agarciav@amd.com>]

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.
>
> Jan

Because we do need it or the code doesn't compile. gnttab_dom0_frames(), for
instance, exists and is used in unguarded statements. 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.

Cheers,
Alejandro