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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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.

2) a pvshim_defconfig target which expresses what a pvshim ought to look
like.

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.