Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally
equivalent "if !...") in Kconfig file, since negative dependancy will badly
affect allyesconfig. To make sure unchanging produced config file based
on "pvshim_defconfig", we shall explicitly state according Kconfig is not set
Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have unset
values when running make defconfig based on "x86_64_defconfig".
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v2 -> v3:
- remove comment for PV_SHIM_EXCLUSIVE
---
v3 -> v4:
- explicitly state "CONFIG_xxx is not set" in "pvshim_defconfig"
- Add "default y" for SHADOW_PAGING and TBOOT
- refactor commit message
---
xen/arch/x86/Kconfig | 6 ++----
xen/arch/x86/configs/pvshim_defconfig | 5 +++++
xen/arch/x86/hvm/Kconfig | 1 -
xen/drivers/video/Kconfig | 4 ++--
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 7afe879710..8c8e661d53 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -143,7 +143,7 @@ config XEN_IBT
config SHADOW_PAGING
bool "Shadow Paging"
- default !PV_SHIM_EXCLUSIVE
+ default y
depends on PV || HVM
help
@@ -175,7 +175,7 @@ config BIGMEM
config TBOOT
bool "Xen tboot support (UNSUPPORTED)"
depends on INTEL && UNSUPPORTED
- default !PV_SHIM_EXCLUSIVE
+ default y
select CRYPTO
help
Allows support for Trusted Boot using the Intel(R) Trusted Execution
@@ -288,7 +288,6 @@ config PV_SHIM_EXCLUSIVE
If unsure, say N.
-if !PV_SHIM_EXCLUSIVE
config HYPERV_GUEST
bool "Hyper-V Guest"
@@ -298,7 +297,6 @@ config HYPERV_GUEST
If unsure, say N.
-endif
config REQUIRE_NX
bool "Require NX (No eXecute) support"
diff --git a/xen/arch/x86/configs/pvshim_defconfig b/xen/arch/x86/configs/pvshim_defconfig
index 2ad27f898e..6f652e145e 100644
--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -26,3 +26,8 @@ CONFIG_EXPERT=y
# CONFIG_INTEL_IOMMU is not set
# CONFIG_DEBUG is not set
# CONFIG_GDBSX is not set
+# CONFIG_SHADOW_PAGING is not set
+# CONFIG_TBOOT is not set
+# HYPERV_HYPERV_GUEST is not set
+# CONFIG_HVM is not set
+# CONFIG_VGA is not set
diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig
index 2def0f98e2..b903764bda 100644
--- a/xen/arch/x86/hvm/Kconfig
+++ b/xen/arch/x86/hvm/Kconfig
@@ -1,6 +1,5 @@
menuconfig HVM
bool "HVM support"
- depends on !PV_SHIM_EXCLUSIVE
default !PV_SHIM
select COMPAT
select IOREQ_SERVER
diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig
index 245030beea..66ee1e7c9c 100644
--- a/xen/drivers/video/Kconfig
+++ b/xen/drivers/video/Kconfig
@@ -3,10 +3,10 @@ config VIDEO
bool
config VGA
- bool "VGA support" if !PV_SHIM_EXCLUSIVE
+ bool "VGA support"
select VIDEO
depends on X86
- default y if !PV_SHIM_EXCLUSIVE
+ default y
help
Enable VGA output for the Xen hypervisor.
--
2.34.1On 28.05.2025 11:16, Penny Zheng wrote: > Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally > equivalent "if !...") in Kconfig file, since negative dependancy will badly > affect allyesconfig. To make sure unchanging produced config file based > on "pvshim_defconfig", we shall explicitly state according Kconfig is not set > > Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have unset > values when running make defconfig based on "x86_64_defconfig". I fear I don't understand this, perhaps related to me also not seeing how ... > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -143,7 +143,7 @@ config XEN_IBT > > config SHADOW_PAGING > bool "Shadow Paging" > - default !PV_SHIM_EXCLUSIVE > + default y > depends on PV || HVM > help > > @@ -175,7 +175,7 @@ config BIGMEM > config TBOOT > bool "Xen tboot support (UNSUPPORTED)" > depends on INTEL && UNSUPPORTED > - default !PV_SHIM_EXCLUSIVE > + default y > select CRYPTO > help > Allows support for Trusted Boot using the Intel(R) Trusted Execution ... these two fit with title and description. The justification for removing the !PV_SHIM_EXCLUSIVE here is not "breaks allyesconfig". > @@ -288,7 +288,6 @@ config PV_SHIM_EXCLUSIVE > > If unsure, say N. > > -if !PV_SHIM_EXCLUSIVE > > config HYPERV_GUEST > bool "Hyper-V Guest" > @@ -298,7 +297,6 @@ config HYPERV_GUEST > > If unsure, say N. > > -endif > > config REQUIRE_NX > bool "Require NX (No eXecute) support" Please don't leave double blank lines. > --- a/xen/arch/x86/configs/pvshim_defconfig > +++ b/xen/arch/x86/configs/pvshim_defconfig > @@ -26,3 +26,8 @@ CONFIG_EXPERT=y > # CONFIG_INTEL_IOMMU is not set > # CONFIG_DEBUG is not set > # CONFIG_GDBSX is not set > +# CONFIG_SHADOW_PAGING is not set > +# CONFIG_TBOOT is not set > +# HYPERV_HYPERV_GUEST is not set This one doesn't look right, simply by its name. > +# CONFIG_HVM is not set > +# CONFIG_VGA is not set Just to mention it - I'm unsure whether adding such at the end isn't going to cause issues. But maybe I'm paranoid ... > --- a/xen/drivers/video/Kconfig > +++ b/xen/drivers/video/Kconfig > @@ -3,10 +3,10 @@ config VIDEO > bool > > config VGA > - bool "VGA support" if !PV_SHIM_EXCLUSIVE > + bool "VGA support" > select VIDEO > depends on X86 > - default y if !PV_SHIM_EXCLUSIVE > + default y > help > Enable VGA output for the Xen hypervisor. Like above, this change also doesn't really fit with title and description. Jan
[Public] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, June 10, 2025 9:01 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; > Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v4 03/20] xen/x86: remove "depends > on !PV_SHIM_EXCLUSIVE" > > On 28.05.2025 11:16, Penny Zheng wrote: > > Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally > > equivalent "if !...") in Kconfig file, since negative dependancy will > > badly affect allyesconfig. To make sure unchanging produced config > > file based on "pvshim_defconfig", we shall explicitly state according > > Kconfig is not set > > > > Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have > > unset values when running make defconfig based on "x86_64_defconfig". > > I fear I don't understand this, perhaps related to me also not seeing how ... > If we just removed "default !PV_SHIM_EXCLUSIVE", .config file generated by "make defconfig" will change, having unsetting values for SHADOW_PAGING (# CONFIG_SHADOW_PAGING is not set) If changing it to "default y" is too casual, maybe we shall add "CONFIG_ SHADOW_PAGING=y" in x86_64_defconfig, to at least retain consistent .config file. My fault, Only considering "SHADOW_PAGING" is enough, as TBOOT depends on UNSUPPORTED firstly > > --- a/xen/arch/x86/Kconfig > > +++ b/xen/arch/x86/Kconfig > > @@ -143,7 +143,7 @@ config XEN_IBT > > > > config SHADOW_PAGING > > bool "Shadow Paging" > > - default !PV_SHIM_EXCLUSIVE > > + default y > > depends on PV || HVM > > help > > > > @@ -175,7 +175,7 @@ config BIGMEM > > config TBOOT > > bool "Xen tboot support (UNSUPPORTED)" > > depends on INTEL && UNSUPPORTED > > - default !PV_SHIM_EXCLUSIVE > > + default y > > select CRYPTO > > help > > Allows support for Trusted Boot using the Intel(R) Trusted > > Execution > > ... these two fit with title and description. The justification for removing > the !PV_SHIM_EXCLUSIVE here is not "breaks allyesconfig". > Hmmm, it is the consequence of "removing the !PV_SHIM_EXCLUSIVE" Maybe I shall add more explanation in commit message? > > @@ -288,7 +288,6 @@ config PV_SHIM_EXCLUSIVE > > > > If unsure, say N. > > > > -if !PV_SHIM_EXCLUSIVE > > > > config HYPERV_GUEST > > bool "Hyper-V Guest" > > @@ -298,7 +297,6 @@ config HYPERV_GUEST > > > > If unsure, say N. > > > > -endif > > > > config REQUIRE_NX > > bool "Require NX (No eXecute) support" > > Please don't leave double blank lines. > > > --- a/xen/arch/x86/configs/pvshim_defconfig > > +++ b/xen/arch/x86/configs/pvshim_defconfig > > @@ -26,3 +26,8 @@ CONFIG_EXPERT=y > > # CONFIG_INTEL_IOMMU is not set > > # CONFIG_DEBUG is not set > > # CONFIG_GDBSX is not set > > +# CONFIG_SHADOW_PAGING is not set > > +# CONFIG_TBOOT is not set > > +# HYPERV_HYPERV_GUEST is not set > > This one doesn't look right, simply by its name. > > > +# CONFIG_HVM is not set > > +# CONFIG_VGA is not set > > Just to mention it - I'm unsure whether adding such at the end isn't going to cause > issues. But maybe I'm paranoid ... > It could be too casual.. I will only leave VGA here, as we're sure that with removing "!PV_SHIM_EXCLUSIVE", CONFIG_VGA is setting as y in pvshim_defconfig > > --- a/xen/drivers/video/Kconfig > > +++ b/xen/drivers/video/Kconfig > > @@ -3,10 +3,10 @@ config VIDEO > > bool > > > > config VGA > > - bool "VGA support" if !PV_SHIM_EXCLUSIVE > > + bool "VGA support" > > select VIDEO > > depends on X86 > > - default y if !PV_SHIM_EXCLUSIVE > > + default y > > help > > Enable VGA output for the Xen hypervisor. > > Like above, this change also doesn't really fit with title and description. I have added " (also the functionally equivalent "if !...") " in commit message to also cover above change > > Jan
On 12.06.2025 06:09, Penny, Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, June 10, 2025 9:01 PM >> >> On 28.05.2025 11:16, Penny Zheng wrote: >>> Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally >>> equivalent "if !...") in Kconfig file, since negative dependancy will >>> badly affect allyesconfig. To make sure unchanging produced config >>> file based on "pvshim_defconfig", we shall explicitly state according >>> Kconfig is not set >>> >>> Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have >>> unset values when running make defconfig based on "x86_64_defconfig". >> >> I fear I don't understand this, perhaps related to me also not seeing how ... > > If we just removed "default !PV_SHIM_EXCLUSIVE", .config file generated by "make defconfig" > will change, having unsetting values for SHADOW_PAGING (# CONFIG_SHADOW_PAGING is not set) > If changing it to "default y" is too casual, maybe we shall add "CONFIG_ SHADOW_PAGING=y" in x86_64_defconfig, > to at least retain consistent .config file. > My fault, Only considering "SHADOW_PAGING" is enough, as TBOOT depends on UNSUPPORTED firstly > >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -143,7 +143,7 @@ config XEN_IBT >>> >>> config SHADOW_PAGING >>> bool "Shadow Paging" >>> - default !PV_SHIM_EXCLUSIVE >>> + default y >>> depends on PV || HVM >>> help >>> >>> @@ -175,7 +175,7 @@ config BIGMEM >>> config TBOOT >>> bool "Xen tboot support (UNSUPPORTED)" >>> depends on INTEL && UNSUPPORTED >>> - default !PV_SHIM_EXCLUSIVE >>> + default y >>> select CRYPTO >>> help >>> Allows support for Trusted Boot using the Intel(R) Trusted >>> Execution >> >> ... these two fit with title and description. The justification for removing >> the !PV_SHIM_EXCLUSIVE here is not "breaks allyesconfig". > > Hmmm, it is the consequence of "removing the !PV_SHIM_EXCLUSIVE" > Maybe I shall add more explanation in commit message? Just to clarify - my questions here were about the changes altogether, i.e.: Why are these two change - as a whole - needed, given the subject? And just to try to avoid any misunderstanding: My point is that "depends on ..." and "default ..." are different things, when the commit message discusses only the former. So yes, extending the commit message may be one way to address my remarks. But really I think changes to defaults (if needed at all) would better be separate from changes to "depends on ...". >>> --- a/xen/arch/x86/configs/pvshim_defconfig >>> +++ b/xen/arch/x86/configs/pvshim_defconfig >>> @@ -26,3 +26,8 @@ CONFIG_EXPERT=y >>> # CONFIG_INTEL_IOMMU is not set >>> # CONFIG_DEBUG is not set >>> # CONFIG_GDBSX is not set >>> +# CONFIG_SHADOW_PAGING is not set >>> +# CONFIG_TBOOT is not set >>> +# HYPERV_HYPERV_GUEST is not set >> >> This one doesn't look right, simply by its name. >> >>> +# CONFIG_HVM is not set >>> +# CONFIG_VGA is not set >> >> Just to mention it - I'm unsure whether adding such at the end isn't going to cause >> issues. But maybe I'm paranoid ... >> > > It could be too casual.. > I will only leave VGA here, as we're sure that with removing "!PV_SHIM_EXCLUSIVE", > CONFIG_VGA is setting as y in pvshim_defconfig I don't follow: Why would a shim need VGA support compiled in? >>> --- a/xen/drivers/video/Kconfig >>> +++ b/xen/drivers/video/Kconfig >>> @@ -3,10 +3,10 @@ config VIDEO >>> bool >>> >>> config VGA >>> - bool "VGA support" if !PV_SHIM_EXCLUSIVE >>> + bool "VGA support" >>> select VIDEO >>> depends on X86 >>> - default y if !PV_SHIM_EXCLUSIVE >>> + default y >>> help >>> Enable VGA output for the Xen hypervisor. >> >> Like above, this change also doesn't really fit with title and description. > > I have added " (also the functionally equivalent "if !...") " in commit message to also > cover above change Well. There are multiple uses of "if ...". The one matching "depends on ..." is covered in the description, yes. But the two uses here don't fall in this same group. One is a prompt visibility change, and the other is a change to yet another default. See above for my recommendation (to split things properly). Jan
[Public] > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Thursday, June 12, 2025 3:02 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; > Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal > <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v4 03/20] xen/x86: remove "depends > on !PV_SHIM_EXCLUSIVE" > > On 12.06.2025 06:09, Penny, Zheng wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, June 10, 2025 9:01 PM > >> > >> On 28.05.2025 11:16, Penny Zheng wrote: > >>> Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally > >>> equivalent "if !...") in Kconfig file, since negative dependancy > >>> will badly affect allyesconfig. To make sure unchanging produced > >>> config file based on "pvshim_defconfig", we shall explicitly state > >>> according Kconfig is not set > >>> > >>> Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have > >>> unset values when running make defconfig based on "x86_64_defconfig". > >> > >> I fear I don't understand this, perhaps related to me also not seeing how ... > > > > If we just removed "default !PV_SHIM_EXCLUSIVE", .config file generated by > "make defconfig" > > will change, having unsetting values for SHADOW_PAGING (# > > CONFIG_SHADOW_PAGING is not set) If changing it to "default y" is too > > casual, maybe we shall add "CONFIG_ SHADOW_PAGING=y" in > x86_64_defconfig, to at least retain consistent .config file. > > My fault, Only considering "SHADOW_PAGING" is enough, as TBOOT depends > > on UNSUPPORTED firstly > > > >>> --- a/xen/arch/x86/Kconfig > >>> +++ b/xen/arch/x86/Kconfig > >>> @@ -143,7 +143,7 @@ config XEN_IBT > >>> > >>> config SHADOW_PAGING > >>> bool "Shadow Paging" > >>> - default !PV_SHIM_EXCLUSIVE > >>> + default y > >>> depends on PV || HVM > >>> help > >>> > >>> @@ -175,7 +175,7 @@ config BIGMEM > >>> config TBOOT > >>> bool "Xen tboot support (UNSUPPORTED)" > >>> depends on INTEL && UNSUPPORTED > >>> - default !PV_SHIM_EXCLUSIVE > >>> + default y > >>> select CRYPTO > >>> help > >>> Allows support for Trusted Boot using the Intel(R) Trusted > >>> Execution > >> > >> ... these two fit with title and description. The justification for > >> removing the !PV_SHIM_EXCLUSIVE here is not "breaks allyesconfig". > > > > Hmmm, it is the consequence of "removing the !PV_SHIM_EXCLUSIVE" > > Maybe I shall add more explanation in commit message? > > Just to clarify - my questions here were about the changes altogether, i.e.: > Why are these two change - as a whole - needed, given the subject? And just to try > to avoid any misunderstanding: My point is that "depends on ..." and "default ..." are > different things, when the commit message discusses only the former. So yes, > extending the commit message may be one way to address my remarks. But really > I think changes to defaults (if needed at all) would better be separate from changes > to "depends on ...". > The reason why I added an extra default y for CONFIG_SHADOW_PAGING is that the .config file generated from x86_64_defconfig has changed after removing the "default !PV_SHIM_EXCLUSIVE", from "CONFIG_SHADOW_PAGING=y" to " CONFIG_SHADOW_PAGING is not set ". To fix it, I casually added a "default y" here. I understand that like you said, it doesn't fit with the title and description... I'll create a new commit for it. And instead of adding "default y", maybe just adding " CONFIG_SHADOW_PAGING=y" in x86_64_defconfig is enough. > >>> --- a/xen/arch/x86/configs/pvshim_defconfig > >>> +++ b/xen/arch/x86/configs/pvshim_defconfig > >>> @@ -26,3 +26,8 @@ CONFIG_EXPERT=y > >>> # CONFIG_INTEL_IOMMU is not set > >>> # CONFIG_DEBUG is not set > >>> # CONFIG_GDBSX is not set > >>> +# CONFIG_SHADOW_PAGING is not set > >>> +# CONFIG_TBOOT is not set > >>> +# HYPERV_HYPERV_GUEST is not set > >> > >> This one doesn't look right, simply by its name. > >> > >>> +# CONFIG_HVM is not set > >>> +# CONFIG_VGA is not set > >> > >> Just to mention it - I'm unsure whether adding such at the end isn't > >> going to cause issues. But maybe I'm paranoid ... > >> > > > > It could be too casual.. > > I will only leave VGA here, as we're sure that with removing > > "!PV_SHIM_EXCLUSIVE", CONFIG_VGA is setting as y in pvshim_defconfig > > I don't follow: Why would a shim need VGA support compiled in? > Yes, VGA shall not be compiled for a shim. And it is the reason why I added "# CONFIG_VGA is not set" in pvshim_defconfig. Without it, the consequence of removing " if !PV_SHIM_EXCLUSIVE " for VGA is that when we run "make defconfig pvshim_defconfig", we will get CONFIG_VGA=y in .config Like you said, this change belongs to the group of changing kconfig default values, and will later be included in a new commit > >>> --- a/xen/drivers/video/Kconfig > >>> +++ b/xen/drivers/video/Kconfig > >>> @@ -3,10 +3,10 @@ config VIDEO > >>> bool > >>> > >>> config VGA > >>> - bool "VGA support" if !PV_SHIM_EXCLUSIVE > >>> + bool "VGA support" > >>> select VIDEO > >>> depends on X86 > >>> - default y if !PV_SHIM_EXCLUSIVE > >>> + default y > >>> help > >>> Enable VGA output for the Xen hypervisor. > >> > >> Like above, this change also doesn't really fit with title and description. > > > > I have added " (also the functionally equivalent "if !...") " in > > commit message to also cover above change > > Well. There are multiple uses of "if ...". The one matching "depends on ..." is covered > in the description, yes. But the two uses here don't fall in this same group. One is a > prompt visibility change, and the other is a change to yet another default. See above > for my recommendation (to split things properly). > Correct me if I understand wrongly: "if ..." for HYPERV_GUEST is falling the group where prompt visibility changes, and fits with the title and description "if ...." for VGA is a change regarding kconfig default value, and shall be covered in a new commit. Yet my changes on pvshim_defconfig and x86_64_defconfig shall also be included in the new commit. As they are all talking about changing kconfig default value. > Jan
On 12.06.2025 10:52, Penny, Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Thursday, June 12, 2025 3:02 PM >> >> On 12.06.2025 06:09, Penny, Zheng wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Tuesday, June 10, 2025 9:01 PM >>>> >>>> On 28.05.2025 11:16, Penny Zheng wrote: >>>>> --- a/xen/arch/x86/Kconfig >>>>> +++ b/xen/arch/x86/Kconfig >>>>> @@ -143,7 +143,7 @@ config XEN_IBT >>>>> >>>>> config SHADOW_PAGING >>>>> bool "Shadow Paging" >>>>> - default !PV_SHIM_EXCLUSIVE >>>>> + default y >>>>> depends on PV || HVM >>>>> help >>>>> >>>>> @@ -175,7 +175,7 @@ config BIGMEM >>>>> config TBOOT >>>>> bool "Xen tboot support (UNSUPPORTED)" >>>>> depends on INTEL && UNSUPPORTED >>>>> - default !PV_SHIM_EXCLUSIVE >>>>> + default y >>>>> select CRYPTO >>>>> help >>>>> Allows support for Trusted Boot using the Intel(R) Trusted >>>>> Execution >>>> >>>> ... these two fit with title and description. The justification for >>>> removing the !PV_SHIM_EXCLUSIVE here is not "breaks allyesconfig". >>> >>> Hmmm, it is the consequence of "removing the !PV_SHIM_EXCLUSIVE" >>> Maybe I shall add more explanation in commit message? >> >> Just to clarify - my questions here were about the changes altogether, i.e.: >> Why are these two change - as a whole - needed, given the subject? And just to try >> to avoid any misunderstanding: My point is that "depends on ..." and "default ..." are >> different things, when the commit message discusses only the former. So yes, >> extending the commit message may be one way to address my remarks. But really >> I think changes to defaults (if needed at all) would better be separate from changes >> to "depends on ...". >> > > The reason why I added an extra default y for CONFIG_SHADOW_PAGING is that > the .config file generated from x86_64_defconfig has changed after removing the "default !PV_SHIM_EXCLUSIVE", from "CONFIG_SHADOW_PAGING=y" to " CONFIG_SHADOW_PAGING is not set ". To fix it, I casually added a "default y" here. > I understand that like you said, it doesn't fit with the title and description... I'll create a new commit for it. And instead of adding "default y", maybe just adding " CONFIG_SHADOW_PAGING=y" in x86_64_defconfig is enough. No, the change (if it is really wanted / needed, which I question at least for the time being) would need to be done to the default (unless there are other reasons to alter the default presently used). >>>>> --- a/xen/arch/x86/configs/pvshim_defconfig >>>>> +++ b/xen/arch/x86/configs/pvshim_defconfig >>>>> @@ -26,3 +26,8 @@ CONFIG_EXPERT=y >>>>> # CONFIG_INTEL_IOMMU is not set >>>>> # CONFIG_DEBUG is not set >>>>> # CONFIG_GDBSX is not set >>>>> +# CONFIG_SHADOW_PAGING is not set >>>>> +# CONFIG_TBOOT is not set >>>>> +# HYPERV_HYPERV_GUEST is not set >>>> >>>> This one doesn't look right, simply by its name. >>>> >>>>> +# CONFIG_HVM is not set >>>>> +# CONFIG_VGA is not set >>>> >>>> Just to mention it - I'm unsure whether adding such at the end isn't >>>> going to cause issues. But maybe I'm paranoid ... >>>> >>> >>> It could be too casual.. >>> I will only leave VGA here, as we're sure that with removing >>> "!PV_SHIM_EXCLUSIVE", CONFIG_VGA is setting as y in pvshim_defconfig >> >> I don't follow: Why would a shim need VGA support compiled in? > > Yes, VGA shall not be compiled for a shim. And it is the reason why I added "# CONFIG_VGA is not set" in pvshim_defconfig. Without it, the consequence of removing " if !PV_SHIM_EXCLUSIVE " for VGA is that when we run "make defconfig pvshim_defconfig", we will get CONFIG_VGA=y in .config > Like you said, this change belongs to the group of changing kconfig default values, and will later be included in a new commit I keep being confused by what you say; will need to see how v5 is going to look like. >>>>> --- a/xen/drivers/video/Kconfig >>>>> +++ b/xen/drivers/video/Kconfig >>>>> @@ -3,10 +3,10 @@ config VIDEO >>>>> bool >>>>> >>>>> config VGA >>>>> - bool "VGA support" if !PV_SHIM_EXCLUSIVE >>>>> + bool "VGA support" >>>>> select VIDEO >>>>> depends on X86 >>>>> - default y if !PV_SHIM_EXCLUSIVE >>>>> + default y >>>>> help >>>>> Enable VGA output for the Xen hypervisor. >>>> >>>> Like above, this change also doesn't really fit with title and description. >>> >>> I have added " (also the functionally equivalent "if !...") " in >>> commit message to also cover above change >> >> Well. There are multiple uses of "if ...". The one matching "depends on ..." is covered >> in the description, yes. But the two uses here don't fall in this same group. One is a >> prompt visibility change, and the other is a change to yet another default. See above >> for my recommendation (to split things properly). >> > > Correct me if I understand wrongly: > "if ..." for HYPERV_GUEST is falling the group where prompt visibility changes, and fits with the title and description No and yes. A file scope "if" acts like a "depends on" on every enclosed option. Hence it fits with the title here, but _not_ because prompt visibility changes. What you may be mixing up is the fact that a prompt of course _also_ becomes invisible when an option's dependencies aren't met. Yet normally when talking about prompt visibility, at least I would always mean the prompt for an active option (i.e. where by hiding the prompt we just take away the user's ability to control the value). > "if ...." for VGA is a change regarding kconfig default value, and shall be covered in a new commit. > Yet my changes on pvshim_defconfig and x86_64_defconfig shall also be included in the new commit. As they are all > talking about changing kconfig default value. I'm not sure. When you remove a "depends on" yet at the same time you want to retain the "off" setting for the default shim configuration, pvshim_defconfig may still need adding to. So edits may be necessary there in more than one of the split patches. Jan
On Wed, 27 May 2025, Penny Zheng wrote: > Remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally > equivalent "if !...") in Kconfig file, since negative dependancy will badly > affect allyesconfig. To make sure unchanging produced config file based > on "pvshim_defconfig", we shall explicitly state according Kconfig is not set > > Add "default y" for SHADOW_PAGING and TBOOT, otherwise we will have unset > values when running make defconfig based on "x86_64_defconfig". > > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > v2 -> v3: > - remove comment for PV_SHIM_EXCLUSIVE > --- > v3 -> v4: > - explicitly state "CONFIG_xxx is not set" in "pvshim_defconfig" > - Add "default y" for SHADOW_PAGING and TBOOT > - refactor commit message > --- > xen/arch/x86/Kconfig | 6 ++---- > xen/arch/x86/configs/pvshim_defconfig | 5 +++++ > xen/arch/x86/hvm/Kconfig | 1 - > xen/drivers/video/Kconfig | 4 ++-- > 4 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 7afe879710..8c8e661d53 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -143,7 +143,7 @@ config XEN_IBT > > config SHADOW_PAGING > bool "Shadow Paging" > - default !PV_SHIM_EXCLUSIVE > + default y > depends on PV || HVM > help > > @@ -175,7 +175,7 @@ config BIGMEM > config TBOOT > bool "Xen tboot support (UNSUPPORTED)" > depends on INTEL && UNSUPPORTED > - default !PV_SHIM_EXCLUSIVE > + default y > select CRYPTO > help > Allows support for Trusted Boot using the Intel(R) Trusted Execution > @@ -288,7 +288,6 @@ config PV_SHIM_EXCLUSIVE > > If unsure, say N. > > -if !PV_SHIM_EXCLUSIVE > > config HYPERV_GUEST > bool "Hyper-V Guest" > @@ -298,7 +297,6 @@ config HYPERV_GUEST > > If unsure, say N. > > -endif > > config REQUIRE_NX > bool "Require NX (No eXecute) support" > diff --git a/xen/arch/x86/configs/pvshim_defconfig b/xen/arch/x86/configs/pvshim_defconfig > index 2ad27f898e..6f652e145e 100644 > --- a/xen/arch/x86/configs/pvshim_defconfig > +++ b/xen/arch/x86/configs/pvshim_defconfig > @@ -26,3 +26,8 @@ CONFIG_EXPERT=y > # CONFIG_INTEL_IOMMU is not set > # CONFIG_DEBUG is not set > # CONFIG_GDBSX is not set > +# CONFIG_SHADOW_PAGING is not set > +# CONFIG_TBOOT is not set > +# HYPERV_HYPERV_GUEST is not set > +# CONFIG_HVM is not set > +# CONFIG_VGA is not set > diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig > index 2def0f98e2..b903764bda 100644 > --- a/xen/arch/x86/hvm/Kconfig > +++ b/xen/arch/x86/hvm/Kconfig > @@ -1,6 +1,5 @@ > menuconfig HVM > bool "HVM support" > - depends on !PV_SHIM_EXCLUSIVE > default !PV_SHIM > select COMPAT > select IOREQ_SERVER > diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig > index 245030beea..66ee1e7c9c 100644 > --- a/xen/drivers/video/Kconfig > +++ b/xen/drivers/video/Kconfig > @@ -3,10 +3,10 @@ config VIDEO > bool > > config VGA > - bool "VGA support" if !PV_SHIM_EXCLUSIVE > + bool "VGA support" > select VIDEO > depends on X86 > - default y if !PV_SHIM_EXCLUSIVE > + default y > help > Enable VGA output for the Xen hypervisor. > > -- > 2.34.1 >
© 2016 - 2025 Red Hat, Inc.