Re: [PATCH] tcg/arm: Increase stack alignment for function generation

[Richard Henderson <richard.henderson@linaro.org>]

On 9/1/21 8:41 PM, Peter Maydell wrote:
> On Wed, 1 Sept 2021 at 19:30, Richard W.M. Jones <rjones@redhat.com> wrote:
>>
>> On Wed, Sep 01, 2021 at 07:18:03PM +0100, Peter Maydell wrote:
>>> On Wed, 1 Sept 2021 at 18:01, Richard W.M. Jones <rjones@redhat.com> wrote:
>>>>
>>>> This avoids the following assertion when the kernel initializes X.509
>>>> certificates:
>>>>
>>>> [    7.315373] Loading compiled-in X.509 certificates
>>>> qemu-system-arm: ../tcg/tcg.c:3063: temp_allocate_frame: Assertion `align <= TCG_TARGET_STACK_ALIGN' failed.
>>>>
>>>> Fixes: commit c1c091948ae
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1999878
>>>> Cc: qemu-stable@nongnu.org
>>>> Tested-by: Richard W.M. Jones <rjones@redhat.com>
>>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>>>> ---
>>>>   tcg/arm/tcg-target.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h
>>>> index d113b7f8db..09df3b39a1 100644
>>>> --- a/tcg/arm/tcg-target.h
>>>> +++ b/tcg/arm/tcg-target.h
>>>> @@ -115,7 +115,7 @@ extern bool use_neon_instructions;
>>>>   #endif
>>>>
>>>>   /* used for function call generation */
>>>> -#define TCG_TARGET_STACK_ALIGN         8
>>>> +#define TCG_TARGET_STACK_ALIGN          16
>>>>   #define TCG_TARGET_CALL_ALIGN_ARGS     1
>>>>   #define TCG_TARGET_CALL_STACK_OFFSET   0
>>>
>>> The 32-bit Arm procedure call standard only guarantees 8-alignment
>>> of SP, not 16-alignment, so I suspect this is not the correct fix.
>>
>> Wouldn't it be a good idea if asserts in TCG dumped out something
>> useful about the guest code?  Because I can only reproduce this bug in
>> a very awkward batch environment I need to collect as much information
>> from log messages as possible.
> 
> Is the failure case short enough to allow -d ... logging to
> be taken? That's usually the most useful info, but it's so huge
> it's often not feasible.
> 
> Anyway, the problem here is that the arm backend now supports
> Neon, and it will try to do some operations on 128 bit types,
> where at least the loads and stores require 16-alignment. But
> nothing is enforcing that we have 16 alignment. (Without the assert
> we'd probably blunder on and fault on an unaligned access.)

Correct.

And while we could actively align the stack, that makes the prologue and epilogue overly 
complicated, and we can't just use pop {reg-list}.

My preferred solution is to add another per-backend define that says what the required 
alignment for vector stack spills, and then *don't* add the :128 bit alignment specifier 
to the vector load/store insns we emit.

I'll wait til I get home in 10 days or so before I tackle this.


r~