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

From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.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 11:28:45 +0200	[thread overview]
Message-ID: <195a50316efde86113f6f5df269c754a90490c57.camel@gmail.com> (raw)
In-Reply-To: <405c2d66-a6f4-4bbb-ada5-2ac49c8c9744@suse.com>

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.

But after your words about one place, I realized that it would be
better to update a place where a common option is defined.

The only thing I would like to change is probably it will be better to
do the opposite, add "depends on" arches that support
CONFIG_GRANT_TABLE now so it will not need to update "depends on" for
new arches or arches that don't support CONFIG_GRANT_TABLE.

> 
> > One more question I have do we really need this randconfig? On
> > RISC-V
> > side, I launched several time this patch series ( from v1 to v4 +
> > runs
> > during test of patch series ) and I haven't faced case
> > when CONFIG_GRANT_TABLE=n. ( but I turned the config off in
> > defconfig +
> > EXTRA_XEN_CONFIG ).
> 
> That override is why in CI you wouldn't see an issue. But as said -
> CI
> isn't everything.
From this point of view it will be better to add "depends on !PPC &&
!RISCV" to "Config GRANT_TABLE".

~ Oleksii