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

From: Jan Beulich <jbeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.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 15:28:25 +0100	[thread overview]
Message-ID: <11f0e1dd-01b4-4d13-8137-e86b0975ad80@suse.com> (raw)
In-Reply-To: <20240228135908.13319-1-roger.pau@citrix.com>

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?

Jan