On 21.01.2025 09:52, Roger Pau Monné wrote: > On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote: >> 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. > > But nothing prevents a user from starting from a shim defconfig, and > then tweaking it as desired: > > $ make pvshim_defconfig > $ make menuconfig > > Or there's something I'm missing here?
Well, no, you don't. But if the above is okay, why would not starting from pvshim_defconfig not also be okay? Plus whichever way tweaks are done, sensible dependencies should still be enforced imo. >>>> 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". > > Won't making the boolean a compile time constant would also result in > DCO kicking in and removing a fair amount of code? So even if you > have enabled everything in Kconfig, the resulting hypervisor would > only be suitable to be used as a shim? Of course. Jan
