On 21.01.2025 00:27, Stefano Stabellini wrote: > On Mon, 20 Jan 2025, Jan Beulich wrote: >> On 18.01.2025 00:41, Andrew Cooper wrote: >>> On 17/01/2025 10:43 pm, Stefano Stabellini wrote: >>>> On Fri, 17 Jan 2025, Jan Beulich wrote: >>>>> On 17.01.2025 13:24, Alejandro Vallejo wrote: >>>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote: >>>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote: >>>>>>>> On Wed, 1 Mar 2023, Jan Beulich wrote: >>>>>>>>> While we want certain things turned off in shim-exclusive mode, doing >>>>>>>>> so via "depends on !PV_SHIM_EXCLUSIVE" badly affects allyesconfig: >>>>>>>>> Since >>>>>>>>> that will turn on PV_SHIM_EXCLUSIVE, other options will be turned off >>>>>>>>> as >>>>>>>>> a result. Yet allyesconfig wants to enable as much of the >>>>>>>>> functionality >>>>>>>>> as possible. >>>>>>>>> >>>>>>>>> Retain PV_SHIM_EXCLUSIVE as a prompt-less option such that first of >>>>>>>>> all >>>>>>>>> C code using it can remain as is. This isn't just for less code churn, >>>>>>>>> but also because I think that symbol is more logical to use in many >>>>>>>>> (all?) places. >>>>>>>>> >>>>>>>>> Requested-by: Andrew Cooper <andrew.coop...@citrix.com> >>>>>>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> The new Kconfig control's name is up for improvement suggestions, but >>>>>>>>> I >>>>>>>>> think it's already better than the originally thought of >>>>>>>>> FULL_HYPERVISOR. >>>>>>>> I think the approach in general is OK, maybe we can improve the naming >>>>>>>> further. What about one of the following? >>>>>>>> >>>>>>>> NO_PV_SHIM_EXCLUSIVE >>>>>>>> PV_SHIM_NOT_EXCLUSIVE >>>>>>> IMO negated options are confusing, and if possible I think we should >>>>>>> avoid using them unless strictly necessary. >>>>>> The problem is that the option is negative in nature. It's asking for >>>>>> hypervisor without _something_. I do agree with Stefano that shim would >>>>>> be >>>>>> better in the name. Otherwise readers are forced to play divination >>>>>> tricks. >>>>>> >>>>>> How about something like: >>>>>> >>>>>> SHIMLESS_HYPERVISOR >>>>>> >>>>>> That's arguably not negated, preserves "shim" in the name and has the >>>>>> correct >>>>>> polarity for allyesconfig to yield the right thing. >>>>> Except that a hypervisor with this option enabled isn't shim-less, but >>>>> permits >>>>> working in shim as well as in non-shim mode. >>>> First, let's recognize that we have two opposing requirements. One >>>> requirement is to make it as easy as possible to configure for the user. >>>> Ideally without using negative CONFIG options, such as >>>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly, >>>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others, >>>> I think. >>>> >>>> On the other hand, we have the requirement that we don't want >>>> allyesconfig to end up disabling a bunch of CONFIG options. Now this >>>> requirement can be satisfied easily by adding something like >>>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous >>>> requirement. >>>> >>>> So we need a compromise, something that doesn't end up disabling other >>>> CONFIG options, to make allyesconfig happy, but also not too confusing >>>> for the user (which is a matter of personal opinion). >>>> >>>> In short, expect that people will have different opinions on this and >>>> will find different compromises better or worse. For one, I prefer to >>>> compromise on "no negative CONFIG options" and use >>>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and >>>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a >>>> completely generic FULL_HYPERVISOR option, which means nothing to me. >>>> >>>> I cannot see a way to have a good and clear non-negated CONFIG option, >>>> and also satisfy the allyesconfig requirement. So I prefer to compromise >>>> on the "non-negated" part. >>> >>> Debating the naming is missing the point. >>> >>> >>> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way >>> that Kconfig is not capable of expressing. Specifically, what is broken >>> is having "lower level" options inhibit unrelated "higher level" options >>> when the graph gets rescanned[1]. That's why we're in the laughable >>> position of `make allyesconfig` turning off CONFIG_HVM. >>> >>> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean >>> "reset me back to a PV Shim". >> >> Isn't this an independent goal? Or is this a statement on what you see >> my change (kind of) doing, indicating you consider this wrong? > > The way I understood it, it is the latter > > >>> Kconfig spells this "make $foo_defconfig" for an appropriately given foo. >>> >>> >>> There should be: >>> >>> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than >>> making the boolean be a compile time constant. >> >> I fear it remains unclear to me what exactly you mean here. It feels like >> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be >> dropped, without replacement. That seems wrong to me, though. In >> particular I'm of the opinion that besides using "make pvshim_defconfig" >> people ought to also have the option to properly configure a shim build >> from scratch (or from any random .config they hold in hands), requiring >> respective "depends on" and/or "select" / "imply" to be in place. > > That should be done with "make pvshim_defconfig"
Why? Specifically, why should people use only one entirely nailed down configuration for a shim. Like a "normal" hypervisor, there are aspects which very well can be left to the person doing the configuration. >> Or else they may end up with a lot of dead code. (Just consider e.g. >> HVM=n: We also don't permit HVM-only stuff to be enabled in that case, >> as any of that would again be dead code then.) > > It will always be possible to come up with poor configurations. I do not > believe it is necessarily our responsibility to go out of our way to > prevent them. Well - if so, why would we do this in some cases but not in others? You may recall that I'm a proponent of consistency and predictability. >>> 2) a pvshim_defconfig target which expresses what a pvshim ought to look >>> like. >> >> We have this file already. I consider it debatable though whether this file >> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the >> name as either "works just as shim" or "can also work as shim". > > If I understood it right, I like Andrew's suggestion. He is suggesting > to do the following: > > - turning PV_SHIM_EXCLUSIVE into something that does nothing FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but "nothing other than making the boolean be a compile time constant". > - adding "make pvshim_defconfig" Why do you say "adding"? We have this already. Jan > So that: > > - people use "make pvshim_defconfig" to get what today is enabled by > PV_SHIM_EXCLUSIVE > - but "make allyesconfig" doesn't end up disabling things > - the Kconfig menu makes sense because PV_SHIM_EXCLUSIVE goes away and > it is not replaced by anything > > If I got it right, I am in favor.
