Re: [PATCH 2/3] x86/pv-shim: don't even allow enabling GRANT_TABLE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2022 21:26, Andrew Cooper wrote:
> On 06/12/2022 14:30, Jan Beulich wrote:
>> Grant table code is unused in shim mode, so there's no point in
>> building it in the first place for shim-exclusive mode.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> nack.
> 
> This is bogus, as is every other "depends on !PV_SHIM_EXCLUSIVE".

But why? Doing things like this in Kconfig is exactly ...

> The only reason I haven't reverting the others so `make allyesconfig`
> doesn't disable CONFIG_HVM, is because I haven't had time.  This change
> further breaks allyesconfig by disabling GRANT_TABLE too.
> 
> PV_SHIM_EXCLUSIVE is a simple option for a bit of dead code
> elimination.  It is not valid to be used like this.

... for the purpose of dead code elimination. By nack-ing a change like
this (and by having voiced opposition to earlier ones) you simply call
for yet more entirely unhelpful #ifdef-ary. See the last paragraph of
the description of patch 1, half of which this change rectifies. The
solution on the evtchn side, unfortunately, looks to be #ifdef-ary,
short of there being a suitable Kconfig option.

Furthermore Kconfig, in my view, is specifically intended to allow to
prevent the user from selecting entirely bogus option combinations
(and even more so suggest entirely bogus configurations by bogus
default settings). If you disagree, then I'm afraid we have a 2nd
Kconfig usage topic which we need to settle on in a project-wide
manner. If only we ever made any progress on such ...

As to allyesconfig - I can see your point there, but then arrangements
need to be invented to avoid this kind of unhelpful behavior.

Which raises two questions then: Would you view

 config GRANT_TABLE
        bool "Grant table support" if EXPERT
        default y if !PV_SHIM_EXCLUSIVE

as acceptable? I could accept this as a (perhaps) temporary compromise,
until the general aspect was settled. And what about the different kind
of change in patch 3, where allyesconfig isn't affected (afaict)?

Also, as a wider scope remark: In general - and this particularly
applies to cases where you feel like nack-ing things - comments on
patches want to be constructive (and thus actionable, pointing out
possible ways for the author to re-work things such they would become
acceptable while achieving the same goal; see the first of the two
questions above for a possible example). There's nothing constructive
in your response here, nor was there ever (afair) in earlier nacks that
I received from you.

Jan