Re: [PATCH] x86/altcall: always use a temporary parameter stashing variable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2024 17:56, Roger Pau Monné wrote:
> On Wed, Feb 28, 2024 at 03:28:25PM +0100, Jan Beulich wrote:
>> On 28.02.2024 14:59, Roger Pau Monne wrote:
>>> The usage in ALT_CALL_ARG() on clang of:
>>>
>>> register union {
>>>     typeof(arg) e;
>>>     const unsigned long r;
>>> } ...
>>>
>>> When `arg` is the first argument to alternative_{,v}call() and
>>> const_vlapic_vcpu() is used results in clang 3.5.0 complaining with:
>>>
>>> arch/x86/hvm/vlapic.c:141:47: error: non-const static data member must be 
>>> initialized out of line
>>>          alternative_call(hvm_funcs.test_pir, const_vlapic_vcpu(vlapic), 
>>> vec) )
>>>
>>> Workaround this by pulling `arg1` into a local variable, like it's done for
>>> further arguments (arg2, arg3...)
>>>
>>> Originally arg1 wasn't pulled into a variable because for the a1_ register
>>> local variable the possible clobbering as a result of operators on other
>>> variables don't matter:
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables
>>>
>>> Note clang version 3.8.1 seems to already be fixed and don't require the
>>> workaround, but since it's harmless do it uniformly everywhere.
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Fixes: 2ce562b2a413 ('x86/altcall: use a union as register type for 
>>> function parameters on clang')
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> I'm okay with this change, but since you don't mention anything in this
>> regard: Did you look at whether / how generated code (with gcc) changes?
> 
> So the specific example of vlapic_test_irq() shows no changes to the
> generated code. bloat-o-meter shows a 0 delta:
> 
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> Function                                     old     new   delta
> Total: Before=3570098, After=3570098, chg +0.00%
> 
> I assume there can be some reordering of instructions, as we now force
> arg1 temporary variable (v1_) to be initialized ahead of the rest of
> the arguments.

Yeah, re-ordering is okay. I was more worried of new unnecessary register
copying, for example. Thanks for confirming.

Jan