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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] x86/altcall: always use a temporary parameter stashing variable
Date: Wed, 28 Feb 2024 17:56:28 +0100	[thread overview]
Message-ID: <Zd9lvN2tUrmEt2pg@macbook> (raw)
In-Reply-To: <11f0e1dd-01b4-4d13-8137-e86b0975ad80@suse.com>

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@citrix.com>
> > Fixes: 2ce562b2a413 ('x86/altcall: use a union as register type for function parameters on clang')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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.

Thanks, Roger.