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

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

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, June 10, 2025 9:01 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 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?


> > @@ -288,7 +288,6 @@ config PV_SHIM_EXCLUSIVE
> >
> >       If unsure, say N.
> >
> > -if !PV_SHIM_EXCLUSIVE
> >
> >  config HYPERV_GUEST
> >     bool "Hyper-V Guest"
> > @@ -298,7 +297,6 @@ config HYPERV_GUEST
> >
> >       If unsure, say N.
> >
> > -endif
> >
> >  config REQUIRE_NX
> >     bool "Require NX (No eXecute) support"
>
> Please don't leave double blank lines.
>
> > --- 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

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

>
> Jan