[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition
From: |
Taylor Simpson |
Subject: |
RE: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition |
Date: |
Wed, 26 Aug 2020 23:51:58 +0000 |
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
- [RFC PATCH v3 00/34] Hexagon patch series, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 10/34] Hexagon (target/hexagon) instruction and packet types, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 08/34] Hexagon (target/hexagon) GDB Stub, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 01/34] Hexagon Update MAINTAINERS file, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 15/34] Hexagon (target/hexagon) instruction printing, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 05/34] Hexagon (target/hexagon) register names, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 06/34] Hexagon (disas) disassembler, Taylor Simpson, 2020/08/18