| Message ID | cdc20255540a66ba0b6946ac6d48c11029cd3385.1701453087.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce generic headers | expand |
On 01.12.2023 21:48, Oleksii Kurochko wrote: > Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid > generation of empty <asm/grant_table.h> for cases when > CONFIG_GRANT_TABLE is not enabled. > > The following changes were done for Arm: > <asm/grant_table.h> should be included directly because it contains > gnttab_dom0_frames() macros which is unique for Arm and is used in > arch/arm/domain_build.c. > <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in > <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames > won't be available for use in arch/arm/domain_build.c. > > Suggested-by: Jan Beulich <jbeulich@suse.com> Not really, no: In particular ... > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -15,6 +15,7 @@ config CORE_PARKING > config GRANT_TABLE > bool "Grant table support" if EXPERT > default y > + depends on ARM || X86 ... this I explicitly said I consider wrong to add. Jan
On Mon, 2023-12-04 at 09:41 +0100, Jan Beulich wrote: > On 01.12.2023 21:48, Oleksii Kurochko wrote: > > Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid > > generation of empty <asm/grant_table.h> for cases when > > CONFIG_GRANT_TABLE is not enabled. > > > > The following changes were done for Arm: > > <asm/grant_table.h> should be included directly because it contains > > gnttab_dom0_frames() macros which is unique for Arm and is used in > > arch/arm/domain_build.c. > > <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in > > <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE > > gnttab_dom0_frames > > won't be available for use in arch/arm/domain_build.c. > > > > Suggested-by: Jan Beulich <jbeulich@suse.com> > > Not really, no: In particular ... > > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -15,6 +15,7 @@ config CORE_PARKING > > config GRANT_TABLE > > bool "Grant table support" if EXPERT > > default y > > + depends on ARM || X86 > > ... this I explicitly said I consider wrong to add. Then I misunderstood you. What about to do the same as with MEM_ACCESS config and introduce HAS_GRANT_TABLE? Or would it be better just update "depends on" to !RISCV && !PPC? ~ Oleksii
On 04.12.2023 10:39, Oleksii wrote: > On Mon, 2023-12-04 at 09:41 +0100, Jan Beulich wrote: >> On 01.12.2023 21:48, Oleksii Kurochko wrote: >>> Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid >>> generation of empty <asm/grant_table.h> for cases when >>> CONFIG_GRANT_TABLE is not enabled. >>> >>> The following changes were done for Arm: >>> <asm/grant_table.h> should be included directly because it contains >>> gnttab_dom0_frames() macros which is unique for Arm and is used in >>> arch/arm/domain_build.c. >>> <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in >>> <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE >>> gnttab_dom0_frames >>> won't be available for use in arch/arm/domain_build.c. >>> >>> Suggested-by: Jan Beulich <jbeulich@suse.com> >> >> Not really, no: In particular ... >> >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -15,6 +15,7 @@ config CORE_PARKING >>> config GRANT_TABLE >>> bool "Grant table support" if EXPERT >>> default y >>> + depends on ARM || X86 >> >> ... this I explicitly said I consider wrong to add. > Then I misunderstood you. > > What about to do the same as with MEM_ACCESS config and introduce > HAS_GRANT_TABLE? That's an option, provided (and I put that under question before) there realistically can be ports which don't mean to support grant tables. You mentioned that things are fine for the dom0less setup you're testing, but I don't think a fully-functional Xen port makes sense to only support dom0less. But of course I'm willing to hear arguments to the contrary. > Or would it be better just update "depends on" to !RISCV && !PPC? Definitely not. Jan
On Mon, 2023-12-04 at 10:46 +0100, Jan Beulich wrote: > On 04.12.2023 10:39, Oleksii wrote: > > On Mon, 2023-12-04 at 09:41 +0100, Jan Beulich wrote: > > > On 01.12.2023 21:48, Oleksii Kurochko wrote: > > > > Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid > > > > generation of empty <asm/grant_table.h> for cases when > > > > CONFIG_GRANT_TABLE is not enabled. > > > > > > > > The following changes were done for Arm: > > > > <asm/grant_table.h> should be included directly because it > > > > contains > > > > gnttab_dom0_frames() macros which is unique for Arm and is used > > > > in > > > > arch/arm/domain_build.c. > > > > <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in > > > > <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE > > > > gnttab_dom0_frames > > > > won't be available for use in arch/arm/domain_build.c. > > > > > > > > Suggested-by: Jan Beulich <jbeulich@suse.com> > > > > > > Not really, no: In particular ... If it is comment was addressed to Suggested-by. Then it was added when we didn't have a discussion about config GRANT_TABLE and depends on things. Probably I had to remove it after I started to update Kconfig things. I am really sorry if I had to remove that before send this patch version. > > > > > > > --- a/xen/common/Kconfig > > > > +++ b/xen/common/Kconfig > > > > @@ -15,6 +15,7 @@ config CORE_PARKING > > > > config GRANT_TABLE > > > > bool "Grant table support" if EXPERT > > > > default y > > > > + depends on ARM || X86 > > > > > > ... this I explicitly said I consider wrong to add. > > Then I misunderstood you. > > > > What about to do the same as with MEM_ACCESS config and introduce > > HAS_GRANT_TABLE? > > That's an option, provided (and I put that under question before) > there > realistically can be ports which don't mean to support grant tables. > You mentioned that things are fine for the dom0less setup you're > testing, > but I don't think a fully-functional Xen port makes sense to only > support > dom0less. But of course I'm willing to hear arguments to the > contrary. Unfortunately, I am not experienced in the depths of Xen, but I used grant tables in Arm to share frames between dom0 and guest in PV drivers. It should be another usage of grant tables. I assume it is possible not to use grant tables and come up with another solution, but it isn't the best idea, and I don't have any reason not to use what already exists and works. Are there any cases where something else, instead of grant tables, is used? Therefore, I agree that a fully functional Xen port will support only dom0less, but it can live for a long time only with dom0less. And in non-dom0less grant tables will be used somewhere sooner or later. > > > Or would it be better just update "depends on" to !RISCV && !PPC? > > Definitely not. So we have a "weird" situation. Considering the message above, grant tables are likely to be used anyway. From this point of view, there is not a lot of sense in introducing "temporary" HAS_GRANT_TABLE as, at some point, every architecture will use grant tables ( the same is with "depends on !RISCV && &!PPC or any other combination ), and HAS_GRANT_TABLE won't be needed any more except the time when support for new architecture will be started and it will live without grant tables for some time. But an introduction of HAS_GRANT_TABLE makes sense ( at least, when the work on support for new architectures will be started ) for me. If you ( or anyone else ) don't mind, I'll update the patch with an introduction of HAS_GRANT_TABLE. ~ Oleksii
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. Jan
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). If this solution is acceptable to you, can I retain your 'Suggested- by'?" [1]: https://lore.kernel.org/xen-devel/b4e85f8f58787b4d179022973ce25673d6b56e36.1700761381.git.oleksii.kurochko@gmail.com/ ~ Oleksii
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? > 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 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 ... 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. Jan
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
On 11.12.2023 18:37, Oleksii wrote: > On Mon, 2023-12-11 at 17:02 +0100, Jan Beulich wrote: >> 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". But you're not meaning to use that approach anymore, are you? Jan > Stefano and Andrew suggested how to disable CONFIG_GRANT_TABLE for the > config. > > Could you please clarify where my understanding is incorrect? > > ~ Oleksii
On Mon, 2023-12-11 at 18:49 +0100, Jan Beulich wrote: > On 11.12.2023 18:37, Oleksii wrote: > > On Mon, 2023-12-11 at 17:02 +0100, Jan Beulich wrote: > > > 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". > > But you're not meaning to use that approach anymore, are you? No, I am going to use it because there is still a need to use #ifdef for #include <asm/grant_table.h> in <xen/grant_table.h> to avoid providing a useless empty asm/grant_table.h header if CONFIG_GRANT_TABLE isn't supported. ~ Oleksii
On 11.12.2023 20:40, Oleksii wrote: > On Mon, 2023-12-11 at 18:49 +0100, Jan Beulich wrote: >> On 11.12.2023 18:37, Oleksii wrote: >>> On Mon, 2023-12-11 at 17:02 +0100, Jan Beulich wrote: >>>> 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". >> >> But you're not meaning to use that approach anymore, are you? > No, I am going to use it because there is still a need to use #ifdef > for #include <asm/grant_table.h> in <xen/grant_table.h> to avoid > providing a useless empty asm/grant_table.h header if > CONFIG_GRANT_TABLE isn't supported. Then _there_ keeping the tag is okay of course. But the CI change (or whatever is come of it) will need treating in whichever way it is going to move. Jan
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index df66fb88d8..28df515a3d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -36,6 +36,7 @@ #include <xen/irq.h> #include <xen/grant_table.h> +#include <asm/grant_table.h> #include <xen/serial.h> #define STATIC_EVTCHN_NODE_SIZE_CELLS 2 diff --git a/xen/arch/ppc/include/asm/grant_table.h b/xen/arch/ppc/include/asm/grant_table.h deleted file mode 100644 index d0ff58dd3d..0000000000 --- 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__ */ diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 310ad4229c..13e26ca06f 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -15,6 +15,7 @@ config CORE_PARKING config GRANT_TABLE bool "Grant table support" if EXPERT default y + depends on ARM || X86 ---help--- Grant table provides a generic mechanism to memory sharing between domains. This shared memory interface underpins the diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 85fe6b7b5e..50edfecfb6 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -26,7 +26,10 @@ #include <xen/mm-frame.h> #include <xen/rwlock.h> #include <public/grant_table.h> + +#ifdef CONFIG_GRANT_TABLE #include <asm/grant_table.h> +#endif struct grant_table;
Ifdef-ing inclusion of <asm/grant_table.h> allows to avoid generation of empty <asm/grant_table.h> for cases when CONFIG_GRANT_TABLE is not enabled. The following changes were done for Arm: <asm/grant_table.h> should be included directly because it contains gnttab_dom0_frames() macros which is unique for Arm and is used in arch/arm/domain_build.c. <asm/grant_table.h> is #ifdef-ed with CONFIG_GRANT_TABLE in <xen/grant_table.h> so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames won't be available for use in arch/arm/domain_build.c. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: - Added dependencies for "Config GRANT_TABLE" to be sure that randconfig will not turn on the config. --- Changes in V4: - Nothing changed. Only rebase. --- Changes in V3: - Remove unnecessary comment. --- xen/arch/arm/domain_build.c | 1 + xen/arch/ppc/include/asm/grant_table.h | 5 ----- xen/common/Kconfig | 1 + xen/include/xen/grant_table.h | 3 +++ 4 files changed, 5 insertions(+), 5 deletions(-) delete mode 100644 xen/arch/ppc/include/asm/grant_table.h