Re: [PATCH for-4.17 v4 2/2] amd: remove VIRT_SC_MSR_HVM synthetic feature

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 15/11/2022 16:44, Jan Beulich wrote:
> On 15.11.2022 17:21, Andrew Cooper wrote:
>> On 15/11/2022 13:26, Roger Pau Monne wrote:
>>> Since the VIRT_SPEC_CTRL.SSBD selection is no longer context switched
>>> on vm{entry,exit} there's no need to use a synthetic feature bit for
>>> it anymore.
>>>
>>> Remove the bit and instead use a global variable.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>
>> This is definitely not appropriate for 4.17, but it's a performance
>> regression in general, hence my firm and repeated objection to this
>> style of patch.
>>
>> General synthetic bits have existed for several decades longer than
>> alternatives.  It has never ever been a rule, or even a recommendation,
>> to aggressively purge the non-alternative bits, because it's a provably
>> bad thing to do.
> There we are again - you state something as bad without really saying
> why it is bad.

You may not agree with the reasoning, but you are lying to yourself, if
no-one else, by claiming that no justification was presented.


> My view is that synthetic bits were wrong to introduce
> when they don't stand a chance of being used in an alternative.

Your view is incompatible with a linear interpretation of history, as
has been pointed repeatedly before by the fact that 1/3 of Xen's
synthetic features full predate the introduction of alternatives.

"I don't like using synthetic bits in this way" is a point of view, but
is not something that counters technical reasoning about the tradeoff in
question.

>
> I agree though that there's no strong need for this to make 4.17. It
> may end up making backports slightly easier, as no such bit existed
> in 4.16.

*This* is a good justification to take the change.

Equally, Roger's subsequent observation that it can actually live in
__initdata.

>> You are attempting a micro-optimisation, that won't produce any
>> improvement at all in centuries, while...
>>
>>> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
>>> index a332087604..9e3b9094d3 100644
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -49,6 +49,7 @@ boolean_param("allow_unsafe", opt_allow_unsafe);
>>>  /* Signal whether the ACPI C1E quirk is required. */
>>>  bool __read_mostly amd_acpi_c1e_quirk;
>>>  bool __ro_after_init amd_legacy_ssbd;
>>> +bool __ro_after_init amd_virt_spec_ctrl;
>> ... actually expending .rodata with something 8 times less efficiently
>> packed, and ...
> ... as long as you're talking of just a single CPU. The break-even is
> at 8 CPUs (8 bits used either way).

And still irrelevant when the size of the per-cpu data area doesn't
change for several centuries in the argued case.

> I think we need to settle on at least halfway firm rules on when to use
> synthetic feature bits and when to use plain global booleans. Without
> that the tastes of the three of us are going to collide again every once
> in a while.

Its very easy.  All other things being equal, synthetic features are the
most efficient option.

In most cases, things aren't all equal, and literally any
technically-credible justification will do.


If a tradeoff doesn't plausibly work within a decade, then it's probably
a waste of time raising, and definitely not a point to legitimately
object with.  Especially as in the past, I've already given you an
alternative course of action where the synthetic features aren't per-cpu...

~Andrew