> -----Original Message-----
> From: Sid Manning <sidn...@quicinc.com>
> Sent: Wednesday, March 12, 2025 2:10 PM
> To: ltaylorsimp...@gmail.com; '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>; Brian Cain
> <bc...@quicinc.com>
> Subject: RE: [PATCH 33/38] target/hexagon: Add gdb support for sys regs
>
>
>
> > -----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-
> de...@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.
[Sid Manning]
OK there is a reason this is like this, sreg_read is declared statically above
and is not available to the debug stub.
>
> >
