Re: [PATCH v2 1/4] x86: provide an inverted Kconfig control for shim-exclusive mode

[Jan Beulich <jbeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>
>>>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> 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