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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2023 12:49, Oleksii wrote:
> 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

Agreeing and then making this suggestion contradicts one another. Unless
the long-term plan is for PPC and RISC-V to not use grant tables.

Jan