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

[Jan Beulich <jbeulich@xxxxxxxx>]

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