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

[Peter Maydell]

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.)

Rereading the TCG code, maybe your patch here is right. It's
not clear to me whether TCG_TARGET_STACK_ALIGN is intended
to specify "alignment the calling convention requires" (though
that's what the comment above it suggests) or "alignment that
TCG requires". The prologue does seem to actively align to the
specified value, not merely assume-and-preserve that alignment.

-- PMM