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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

> Trying to fight against the behaviour of Kconfig is not a good use of
> anyone's time.
> 
> ~Andrew
> 
> [1] default to unrelated symbols is also broken for a related reason. 
> The result you get is sensitive to the order of processing of symbols.

Is it? It has been my understanding that defaults get re-evaluated as user
input is processed.

Jan