RE: [PATCH v5 01/18] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"

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

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, June 30, 2025 4:21 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 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@xxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> > 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
I'll add them back to emphasize intended setting for the shim is different from 
the general default

> And then there's still a non-"depends on" change left ...
>
> > --- 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
```

> 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?

>
> Jan