RE: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition

[Taylor Simpson]

Thanks for the feedback, Richard!!



> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, August 26, 2020 7:36 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core
> definition
>
> On 8/18/20 8:50 AM, Taylor Simpson wrote:
> > +#include <fenv.h>
>
> This should not be in cpu.h.  What's up?

We're not using qemu softfloat (it's on the TODO list), so there's a fenv_t 
field in CPUHexagonState.

> > +#define TARGET_PAGE_BITS 16     /* 64K pages */
> > +#define TARGET_LONG_BITS 32
>
> Belongs in cpu-param.h

OK

> > +#ifdef CONFIG_USER_ONLY
> > +#define TOTAL_PER_THREAD_REGS 64
> > +#else
> ...
> > +    target_ulong gpr[TOTAL_PER_THREAD_REGS];
>
> Do I not understand hexagon enough to know why the number of general
> registers
> would vary with system mode?  Why is the define conditional on user-only?

Yes, there are some registers that are only available in system mode.  Since 
this series is only for linux-user mode, I'll remove the ifdef for now.

We're working on system mode.  When that series is ready, we can put the ifdef 
back in.  At that time, you'll also see the extra registers in hex_regs.h.

> No.  There are plenty of bad examples of this in qemu, let's not add another.
>
> First, the lock doesn't do what you think.
> Second, stderr is never right.
> Third, just about any time you want this, there's a tracepoint that you could
> add that would be better, correct, and toggleable from the command-line.

OK

> I understand your desire for this sort of comparison.  What I don't
> understand
> is the method.  Surely it would be preferable to actually change the stack
> location in qemu, rather than constantly adjust for it.
>
> Add a per-target hook to linux-user/hexagon/target_elf.h that controls the
> allocation of the stack in elfload.c, setup_arg_pages().

OK, will look into this.  Thanks for the advice, I wasn't aware this was 
possible.

>
> > +static void hexagon_dump(CPUHexagonState *env, FILE *f)
> > +{
> > +    static target_ulong last_pc;
> > +    int i;
> > +
> > +    /*
> > +     * When comparing with LLDB, it doesn't step through single-cycle
> > +     * hardware loops the same way.  So, we just skip them here
> > +     */
> > +    if (env->gpr[HEX_REG_PC] == last_pc) {
> > +        return;
> > +    }
>
> Multi-threaded data race.  Might as well move last_pc to env->dump_last_pc
> or
> something.
>
> But I'd also suggest that all of this lldb compatibility stuff be optional,
> switchable from the command-line with a cpu property.  Because there are
> going
> to be real cases where *not* single-stepping will result in dumps from the
> same
> PC, and you've just squashed all of those.
>
> Call the property x-lldb-compat, or something, and default it to off.  You 
> then
> turn it on with "-cpu v67,x-lldb-compat=on".

OK