> -----Original Message-----
> From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> Sent: Wednesday, March 12, 2025 11:27 AM
> To: 'Brian Cain' <brian.c...@oss.qualcomm.com>; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus Bernardino
> (QUIC) <quic_mathb...@quicinc.com>; a...@rev.ng; a...@rev.ng; Marco
> Liebel (QUIC) <quic_mlie...@quicinc.com>; alex.ben...@linaro.org; Mark
> Burton (QUIC) <quic_mbur...@quicinc.com>; Sid Manning
> <sidn...@quicinc.com>; Brian Cain <bc...@quicinc.com>
> Subject: RE: [PATCH 33/38] target/hexagon: Add gdb support for sys regs
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> > -----Original Message-----
> > From: Brian Cain <brian.c...@oss.qualcomm.com>
> > Sent: Friday, February 28, 2025 11:26 PM
> > To: qemu-devel@nongnu.org
> > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng;
> a...@rev.ng;
> > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> > alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com;
> > Brian Cain <bc...@quicinc.com>
> > Subject: [PATCH 33/38] target/hexagon: Add gdb support for sys regs
> >
> > From: Brian Cain <bc...@quicinc.com>
> >
> > Co-authored-by: Matheus Tavares Bernardino
> <quic_mathb...@quicinc.com>
> > Signed-off-by: Brian Cain <brian.c...@oss.qualcomm.com>
>
>
> > +int hexagon_sys_gdb_write_register(CPUState *cs, uint8_t *mem_buf,
> > +int
> > +n) {
> > + CPUHexagonState *env = cpu_env(cs);
> > +
> > + if (n < NUM_SREGS) {
> > + hexagon_gdb_sreg_write(env, n, ldtul_p(mem_buf));
> > + return sizeof(target_ulong);
> > + }
> > + n -= NUM_SREGS;
> > +
> > + if (n < NUM_GREGS) {
> > + return env->greg[n] = ldtul_p(mem_buf);
>
> Are all of these writable directly without any checks?
[Sid Manning]
I checked the iset and all guest registers have READ/WRITE permissions.
>
> > + }
> > + n -= NUM_GREGS;
> > +
> > + g_assert_not_reached();
> > +}
> > +#endif
> > static int gdb_get_vreg(CPUHexagonState *env, GByteArray *mem_buf,
> > int
> > n) {
> > int total = 0;
> > diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> > index 76b2475d88..fd9caafefc 100644
> > --- a/target/hexagon/op_helper.c
> > +++ b/target/hexagon/op_helper.c
> > @@ -1465,6 +1465,17 @@ void HELPER(sreg_write)(CPUHexagonState
> *env,
> > uint32_t reg, uint32_t val)
> > sreg_write(env, reg, val);
> > }
> >
> > +void hexagon_gdb_sreg_write(CPUHexagonState *env, uint32_t reg,
> > +uint32_t val) {
> > + BQL_LOCK_GUARD();
> > + sreg_write(env, reg, val);
> > + /*
> > + * The above is needed to run special logic for regs like syscfg, but
> > it
> > + * won't set read-only bits. This will:
> > + */
> > + arch_set_system_reg(env, reg, val); }
>
> Does the hardware allow the debugger to do this?
[Sid Manning]
On hardware, if we are talking about T32, something like "r.s syscfg &xxx" can
be done, but I think this would involve instruction stuffing to update the
register.
If we are running Linux, system registers would not be exposed in the thread's
context and I think the debugger's knowledge would be limited to just those
registers.
This behavior matches the legacy simulator, hexagon-sim.
>
> > +
> > void HELPER(sreg_write_pair)(CPUHexagonState *env, uint32_t reg,
> > uint64_t val) {
> > BQL_LOCK_GUARD();
> > @@ -1508,6 +1519,11 @@ uint32_t HELPER(sreg_read)(CPUHexagonState
> > *env, uint32_t reg)
> > return sreg_read(env, reg);
> > }
> >
> > +uint32_t hexagon_sreg_read(CPUHexagonState *env, uint32_t reg) {
> > + return sreg_read(env, reg);
> > +}
> > +
>
> Why not just use sreg_read?
[Sid Manning]
The usage of this is in gdbstub.c and I don't think the extra layer is needed
so it can be removed and the stub updated.
>
