Re: [PATCH v4 13/14] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

From: Jan Beulich <jbeulich@suse.com>
To: Oleksii <oleksii.kurochko@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	Shawn Anastasio <sanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 13/14] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>
Date: Tue, 28 Nov 2023 10:58:30 +0100	[thread overview]
Message-ID: <b81607e0-409f-4805-a4ee-8be8fdd959b8@suse.com> (raw)
In-Reply-To: <195a50316efde86113f6f5df269c754a90490c57.camel@gmail.com>

On 28.11.2023 10:28, Oleksii wrote:
> On Tue, 2023-11-28 at 08:57 +0100, Jan Beulich wrote:
>> On 27.11.2023 20:38, Oleksii wrote:
>>> On Mon, 2023-11-27 at 15:41 +0100, Jan Beulich wrote:
>>>> On 27.11.2023 15:13, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/ppc/include/asm/grant_table.h
>>>>> +++ /dev/null
>>>>> @@ -1,5 +0,0 @@
>>>>> -/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> -#ifndef __ASM_PPC_GRANT_TABLE_H__
>>>>> -#define __ASM_PPC_GRANT_TABLE_H__
>>>>> -
>>>>> -#endif /* __ASM_PPC_GRANT_TABLE_H__ */
>>>>
>>>> Removing this header would be correct only if GRANT_TABLE had a
>>>> "depends on
>>>> !PPC", I'm afraid. Recall that the earlier randconfig adjustment
>>>> in
>>>> CI was
>>>> actually requested to be undone, at which point what an arch's
>>>> defconfig
>>>> says isn't necessarily what a randconfig should use.
>>> We can do depends on !PPC && !RISCV but shouldn't it be enough only
>>> to
>>> turn CONFIG_GRANT_TABLE off in defconfig and set
>>> CONFIG_GRANT_TABLE=n
>>> in EXTRA_XEN_CONFIG?
>>
>> That _might_ be sufficient for CI, but we shouldn't take CI as the
>> only
>> thing in the world. Personally I consider any kind of overriding for,
>> in particular, randconfig at bets bogus. Whatever may not be selected
>> (or must be selected) should be arranged for by Kconfig files
>> themselves.
>> That said, there may be _rare_ exceptions. But what PPC's and RISC-
>> V's
>> defconfig-s have right now is more than "rare" imo.
>>
>>> Some time ago I also tried to redefine "Config GRANT_TABLE" in
>>> arch-
>>> specific Kconfig + defconfig + EXTRA_XEN_CONFIG and it works for
>>> me.
>>> Could it be solution instead of "depends on..." ?
>>
>> Why would we want to re-define an setting? There wants to be one
>> single
>> place where a common option is defined. Or maybe I don't understand
>> what you're suggesting ...
> I just thought this change is temporary because grant_table.h will be
> introduced later or earlier, and it will be needed to remove "depends
> on !PPC && !RISCV". And not to litter common KConfig with the change
> which will be removed, it will be better to redefine it in arch-
> specific Kconfig with default n.

Right. But if it's indeed temporary, what's the point of this patch?
The entire series is (supposed to be) to improve code structure for
longer term purposes. If a non-generic grant_table.h is going to
appear for PPC and RISC-V, I don't see why the present dummy would
need removing. That's only useful if an arch can be expected to live
with GRANT_TABLE=n even when otherwise feature-complete, and at that
point modifying the Kconfig dependencies would (imo) be appropriate.

Jan