RE: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition
Thanks for the feedback, Richard!!
> -Original Message-
> From: Richard Henderson
> Sent: Wednesday, August 26, 2020 7:36 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi;
> aleksandar.m.m...@gmail.com; a...@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
>
> 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
Re: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition
On 8/18/20 8:50 AM, Taylor Simpson wrote:
> +#include
This should not be in cpu.h. What's up?
> +#define TARGET_PAGE_BITS 16 /* 64K pages */
> +#define TARGET_LONG_BITS 32
Belongs in cpu-param.h
> +#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?
> +/*
> + * Change HEX_DEBUG to 1 to turn on debugging output
> + */
> +#define HEX_DEBUG 0
> +#define HEX_DEBUG_LOG(...) \
> +do { \
> +if (HEX_DEBUG) { \
> +rcu_read_lock(); \
> +fprintf(stderr, __VA_ARGS__); \
> +rcu_read_unlock(); \
> +} \
> +} while (0)
> +
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.
> +/*
> + * One of the main debugging techniques is to use "-d cpu" and compare
> against
> + * LLDB output when single stepping. However, the target and qemu put the
> + * stacks at different locations. This is used to compensate so the diff is
> + * cleaner.
> + */
> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
> + target_ulong addr)
> +{
> +static bool first = true;
> +if (first) {
> +first = false;
> +env->stack_start = env->gpr[HEX_REG_SP];
> +env->gpr[HEX_REG_USR] = 0x56000;
> +
> +#define ADJUST_STACK 0
> +#if ADJUST_STACK
> +/*
> + * Change the two numbers below to
> + * 1qemu stack location
> + * 2hardware stack location
> + * Or set to zero for normal mode (no stack adjustment)
> + */
> +env->stack_adjust = 0xfffeeb80 - 0xbf89f980;
> +#else
> +env->stack_adjust = 0;
> +#endif
> +}
> +
> +target_ulong stack_start = env->stack_start;
> +target_ulong stack_size = 0x1;
> +target_ulong stack_adjust = env->stack_adjust;
> +
> +if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
> +return addr - stack_adjust;
> +}
> +return addr;
> +}
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().
> +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".
r~
