> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, March 12, 2025 11:27 AM
> To: 'Brian Cain' <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; Matheus Bernardino
> (QUIC) <[email protected]>; [email protected]; [email protected]; Marco
> Liebel (QUIC) <[email protected]>; [email protected]; Mark
> Burton (QUIC) <[email protected]>; Sid Manning
> <[email protected]>; Brian Cain <[email protected]>
> 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 <[email protected]>
> > Sent: Friday, February 28, 2025 11:26 PM
> > To: [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> [email protected];
> > Brian Cain <[email protected]>
> > Subject: [PATCH 33/38] target/hexagon: Add gdb support for sys regs
> >
> > From: Brian Cain <[email protected]>
> >
> > Co-authored-by: Matheus Tavares Bernardino
> <[email protected]>
> > Signed-off-by: Brian Cain <[email protected]>
>
>
> > +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.
>
