Re: [PATCH v5 5/7] xen: ifdef inclusion of <asm/grant_table.h> in <xen/grant_table.h>

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-12-11 at 17:02 +0100, Jan Beulich wrote:
> On 11.12.2023 15:43, Oleksii wrote:
> > On Mon, 2023-12-04 at 11:39 +0100, Jan Beulich wrote:
> > > On 04.12.2023 11:34, Oleksii wrote:
> > > > If you ( or anyone else ) don't mind, I'll update the patch
> > > > with an
> > > > introduction of HAS_GRANT_TABLE.
> > > 
> > > I won't NAK such a patch, but unless convincing arguments appear
> > > I
> > > also
> > > won't ACK it.
> > I am going to disable GRANT_TABLE config for RISC-V ( and PPC? ) by
> > providing a separate YAML file ( riscv-fixed-randconfig.yaml ) with
> > the
> > following content:
> > .riscv-fixed-randconfig:
> >   variables:
> >     EXTRA_FIXED_RANDCONFIG:
> >       CONFIG_COVERAGE=n
> >       CONFIG_GRANT_TABLE=n
> >       CONFIG_MEM_ACCESS=n # this I'll add in the next patch where
> > asm-
> > geneic for mem_access.h is introduced
> > 
> > And then for riscv*randconfig jobs do the following:
> > archlinux-current-gcc-riscv64-randconfig:
> >   extends:
> >     - .gcc-riscv64-cross-build
> >   variables:
> >     CONTAINER: archlinux:current-riscv64
> >     KBUILD_DEFCONFIG: tiny64_defconfig
> >     RANDCONFIG: y
> >     EXTRA_FIXED_RANDCONFIG: !reference [.riscv-fixed-randconfig,
> > variables, EXTRA_FIXED_RANDCONFIG]
> > 
> > For RISC-V, I prefer having a separate file for all the
> > EXTRA_FIXED_RANDCONFIG because in another patch series [1], I'll
> > introduce a large number of disabled configs for randconfig.
> > 
> > For PPC, I don't think it's necessary to introduce a separate YAML
> > file
> > for EXTRA_FIXED_RANDCONFIG. For the time being, it is only
> > necessary to
> > disable two configs: CONFIG_GRANT_TABLE and CONFIG_MEM_ACCESS (in
> > the
> > next patch of this series).
> 
> Why would this be different for PPC and RISC-V?
I haven't investigated that. Perhaps Shawn covered more stubs for a larger 
numberof configs, so he didn't encounter issues with some configs.
For example, during my tests with the inclusion of riscv-fixed-
randconfig.yaml,
randconfig jobs for RISC-V failed several times on perf.c.
At least, during the inclusion of #include <asm/perfc.h>, which is not
provided
for RISC-V, so CONFIG_PERFC_COUNTER is disabled for RISC-V.

However, Shawn either provided necessary stubs for
CONFIG_PERF_COUNTERS,
or it is just luck that such an issue didn't occur for PPC."

> 
> > If this solution is acceptable to you, can I retain your
> > 'Suggested-
> > by'?"
> 
> No, please don't. I've replied to Andrew on the other thread - I
> don't
> see why helping just gitlab-CI is desirable. I'm actually surprised
Well, now I understand your point better, and it makes sense. I was
more confident in the GitLab-CI solution before you replied on the
other thread.

However, it seems to me that randconfig exists only for testing
purposes and usually doesn't require running locally. Therefore, it
makes sense to perform all these tasks only for GitLab-CI to avoid
complicating things around the Makefile in case anything related to
KCONFIG_ALLCONFIG needs to be in sync with Linux.

In any case, I believe it's a good idea to wait for Andrew's feedback.

> Linux have no solution ready for use, as the underlying problem of
> not
> all settings necessarily being valid to use ought to affect them as
> well. Then again perhaps this really only is a transient issue during
> arch bringup ...
"It is challenging for me, at least, to predict whether all options
mentioned in EXTRA_FIXED_RANDCONFIG will be disabled in the future.
Most of them will likely be implemented, but certainty is difficult to
ascertain."

>  In which case the approach taken here may be fine, but
> it still wouldn't be what I suggested. It may then be Stefano or
> Andrew
> who you could consider for such a tag.
I'm a bit confused again. In this case, it seems that both you andStefano or 
Andrew should be on the suggested list.
You proposed the approach with "#ifdef CONFIG_GRANT_TABLE #include
<asm/grant_table.h> #endif".
Stefano and Andrew suggested how to disable CONFIG_GRANT_TABLE for the
config.

Could you please clarify where my understanding is incorrect?

~ Oleksii