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

[Jan Beulich <jbeulich@suse.com>]

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