RE: [PATCH v4 03/20] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, June 12, 2025 3:02 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxx>
> >> 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