On 01.07.2025 09:00, Penny, Zheng wrote:
> [Public]
>
> Hi,
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, June 30, 2025 4:21 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
>> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v5 01/18] xen/x86: remove "depends
>> on !PV_SHIM_EXCLUSIVE"
>>
>> On 16.06.2025 08:41, 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.
>>>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>> v2 -> v3:
>>> - remove comment for PV_SHIM_EXCLUSIVE
>>> ---
>>> v3 -> v4:
>>> - explicitly state "CONFIG_xxx is not set" in "pvshim_defconfig"
>>
>> Where did these changes go? Nothing is said about ...
>>
>>> - Add "default y" for SHADOW_PAGING and TBOOT
>>> - refactor commit message
>>> ---
>>> v4 -> v5:
>>> - For not breaking allyesconfig, changes to defaults are actually not needed.
>>> So remove them all
>>> - Leave one blank lines
>>
>> ... their (complete) dropping here. Aiui overrides for anything where you remove the
>> dependency (and where the intended setting for the shim is different from the
>> general default) would still be needed here.
>>
>
> Since I checked, before and after this commit, result of "make defconfig pvshim_defconfig" doesn't really change for above options, so I remove them
Hmm, I'm puzzled by that. But if so, ...
> I'll add them back to emphasize intended setting for the shim is different from the general default
... nothing should indeed be added (back). What's there isn't to emphasize
anything, but to override what otherwise is the default.
(I can see my mistake there at the example of HVM: That option itself has a
suitable default, and hence anything enclosed in the subsequent "if HVM" is
indeed suitable covered.)
>>> --- a/xen/drivers/video/Kconfig
>>> +++ b/xen/drivers/video/Kconfig
>>> @@ -3,7 +3,7 @@ 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
>>
>> ... here, which (as indicated before) imo doesn't belong here, but at the very least
>> would need covering in the description.
>>
>
> Hmmm. Although " bool "VGA support" if !PV_SHIM_EXCLUSIVE " doesn't make CONFIG_VGA the option disappear when PV_SHIM_EXCLUSIVE=y, it still make it unconfigurable. So I treat it dependency too here...
> Maybe I shall add the following in the description:
> ```
> Although " if !PV_SHIM_EXCLUSIVE " for CONFIG_VGA is not truly a dependency, setting PV_SHIM_EXCLUSIVE y still makes it unconfigurable. So we remove it here too
> ```
Hmm, now that you say this I wonder why this was written that way. I notice it
was me who suggested this form, but I don't remember anymore why I didn't
suggest the simpler "depends on", which - afaict - would have the exact same
effect. What you mean to add to the description may want to reflect that (as
long as you agree, of course).
>> Also, just to repeat what I said in reply to the cover letter: Imo this change needs to
>> move 2nd to last in the series, and it then wants committing together with the last
>> patch (which you will want to put in as a remark to the eventual committer).
>
> Yes, I'll move it to 2nd to last. Shall I mention "It shall be committed together with ...." in commit message or change log?
Somewhere below the first --- separator (and maybe best also in the cover letter).
Such doesn't belong in the eventual commit.
Jan