[Public] > -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Sent: Thursday, June 12, 2025 3:02 PM > To: Penny, Zheng <penny.zh...@amd.com> > Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper > <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; > Anthony PERARD <anthony.per...@vates.tech>; Orzel, Michal > <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini > <sstabell...@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 <jbeul...@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
