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 13:53 +0100, Jan Beulich wrote:
> 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.
On RISC-V side, I can run guests in dom0less and still don't use
grant_tables. And I am not sure when I'll start to implement it. Only
one thing I defined in grant_table.h is:
#define gnttab_dom0_frames()                                          
\
    min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext -
_stext))

But I think it can moved somewhere or dropped as it was defined because
of:

void __init create_dom0(void)
{
    struct domain *dom0;
    struct xen_domctl_createdomain dom0_cfg = {
        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
        .max_evtchn_port = -1,
        .max_grant_frames = gnttab_dom0_frames(),
        .max_maptrack_frames = -1,
        .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
    };
...

Which was copied from Arm.

Taking into account that opt_max_grant_frames is 0 in case when
CONFIG_GRANT_TABLE=n then ".max_grant_frames =" will be 0 in case of
!CONFIG_GRANT_TABLE so for time being the macros gnttab_dom0_frames can
be dropped until CONFIG_GRANT_TABLE will be introduced.

But I am not aware of PPC plans of usage this config.

~ Oleksii