On 12.06.2025 10:52, Penny, Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeul...@suse.com> >> Sent: Thursday, June 12, 2025 3:02 PM >> >> 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: >>>>> --- 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
