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
