Re: [Qemu-devel] [PATCH] target/xtensa: linux-user: add call0 ABI support

[Max Filippov]

On Mon, Aug 26, 2019 at 3:27 AM Laurent Vivier <address@hidden> wrote:
>
> Le 26/08/2019 à 00:17, Max Filippov a écrit :
> > Xtensa binaries built for call0 ABI don't rotate register window on
> > function calls and returns. Invocation of signal handlers from the
> > kernel is therefore different in windowed and call0 ABIs.
> > There's currently no way to determine xtensa ELF binary ABI from the
> > binary itself. Provide an environment variable QEMU_XTENSA_ABI_CALL0 and
> > use it to initialize PS.WOE in xtensa_cpu_reset. Check this flag in
> > setup_rt_frame to determine how a signal should be delivered.
> >
> > Signed-off-by: Max Filippov <address@hidden>
> > ---
> >  linux-user/xtensa/signal.c | 25 +++++++++++++++++--------
> >  target/xtensa/cpu.c        | 22 ++++++++++++++++++----
> >  2 files changed, 35 insertions(+), 12 deletions(-)
> >
> > diff --git a/linux-user/xtensa/signal.c b/linux-user/xtensa/signal.c
> > index 8d54ef3ae34b..590f0313ffe9 100644
> > --- a/linux-user/xtensa/signal.c
> > +++ b/linux-user/xtensa/signal.c
> > @@ -134,6 +134,8 @@ void setup_rt_frame(int sig, struct target_sigaction 
> > *ka,
> >      abi_ulong frame_addr;
> >      struct target_rt_sigframe *frame;
> >      uint32_t ra;
> > +    bool abi_call0;
> > +    unsigned base;
> >      int i;
> >
> >      frame_addr = get_sigframe(ka, env, sizeof(*frame));
> > @@ -182,20 +184,27 @@ void setup_rt_frame(int sig, struct target_sigaction 
> > *ka,
> >          __put_user(0x00, &frame->retcode[5]);
> >  #endif
> >      }
> > -    env->sregs[PS] = PS_UM | (3 << PS_RING_SHIFT);
> > -    if (xtensa_option_enabled(env->config, 
> > XTENSA_OPTION_WINDOWED_REGISTER)) {
> > -        env->sregs[PS] |= PS_WOE | (1 << PS_CALLINC_SHIFT);
> > -    }
> >      memset(env->regs, 0, sizeof(env->regs));
> >      env->pc = ka->_sa_handler;
> >      env->regs[1] = frame_addr;
> >      env->sregs[WINDOW_BASE] = 0;
> >      env->sregs[WINDOW_START] = 1;
> >
> > -    env->regs[4] = (ra & 0x3fffffff) | 0x40000000;
> > -    env->regs[6] = sig;
> > -    env->regs[7] = frame_addr + offsetof(struct target_rt_sigframe, info);
> > -    env->regs[8] = frame_addr + offsetof(struct target_rt_sigframe, uc);
> > +    abi_call0 = (env->sregs[PS] & PS_WOE) == 0;
>
> Is this safe to rely on content of sregs[PS]?

It should be: PS is a privileged register, PS.WOE can only
be changed explicitly and windowed registers are not supposed to work
when PS.WOE is cleared.

> Why don't you use xtensa_abi_call0()?

I actually used it in early version of this change, but then
decided that PS.WOE might be a better indicator for the call0 ABI.

> > +    env->sregs[PS] = PS_UM | (3 << PS_RING_SHIFT);
> > +
> > +    if (abi_call0) {
> > +        base = 0;
> > +        env->regs[base] = ra;
> > +    } else {
> > +        env->sregs[PS] |= PS_WOE | (1 << PS_CALLINC_SHIFT);
> > +        base = 4;
> > +        env->regs[base] = (ra & 0x3fffffff) | 0x40000000;
> > +    }
> > +    env->regs[base + 2] = sig;
> > +    env->regs[base + 3] = frame_addr + offsetof(struct target_rt_sigframe,
> > +                                                info);
> > +    env->regs[base + 4] = frame_addr + offsetof(struct target_rt_sigframe, 
> > uc);
> >      unlock_user_struct(frame, frame_addr, 1);
> >      return;
> >
> > diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> > index 76db1741a796..791c061880e7 100644
> > --- a/target/xtensa/cpu.c
> > +++ b/target/xtensa/cpu.c
> > @@ -53,6 +53,18 @@ static bool xtensa_cpu_has_work(CPUState *cs)
> >  #endif
> >  }
> >
> > +#ifdef CONFIG_USER_ONLY
> > +static int xtensa_abi_call0(void)
> > +{
> > +    static int abi_call0 = -1;
> > +
> > +    if (abi_call0 == -1) {
> > +        abi_call0 = getenv("QEMU_XTENSA_ABI_CALL0") != NULL;
>
> Wouldn't it be cleaner to add this in the arg_table[] in
> linux-user/main.c and then use directly the variable?

Probably. Will do so in v2.

-- 
Thanks.
-- Max