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

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?

Cheers,
Alejandro