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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-11-28 at 10:58 +0100, Jan Beulich wrote:
> 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.
I agree. Let's proceed by adding the dependency in the KConfig file.

So which one option will be better:
1. depends on !PPC && !RISCV
2. depends on ARM || X86

~ Oleksii