Re: [PATCH v2 4/5] RISC-V: Typed CSRs in gdbserver

[Richard Henderson <richard.henderson@linaro.org>]

On 1/5/22 6:04 AM, Schwarz, Konrad wrote:
> So the problem is that these files are generated -- somewhat ironically
> via XSLT from complete GDB target descriptions, which are themselves
> generated from a mixture of AWK and shell scripts that I have in a
> different project and which you would probably not want to have
> contributed.  Those scripts generate a variety of other definitions
> for C and assembly besides the GDB XML target descriptions, so would
> probably need to be reduced for just QEMU usage.

You may be right that we don't want your original scripts, but that also  implies that the 
fact your files are generated is irrelevant to us.  Why should we care?  I think it makes 
more sense to manually edit csr.c and be done with it.

>  I tried to
> work around this by using C99's designated initializer syntax,
> adding in the new data at the end of the table, and using specific
> enough initializers to not disturb the existing data.
> 
> However, this did not work out: despite using very specific initializers,
> the previously initialized CSR structures in the csr_ops array
> were reset to their default values, i.e., 0, breaking the code.
> This was not the way I expected this feature to work in C99 and
> my reading of the C99 standard does not support this either.  But
> that’s what GCC does, at least on my machine.

I'm sure your syntax was incorrect.  You probably used

>>> +  [CSR_CYCLE] { .gdb_type = "uint64", .gdb_group = "user" },

which does indeed overwrite the entire entry in the array.  You could have used

     [CSR_CYCLE].gdb_type = "uint64"

which will just set the one field.

That said, I don't think that we want this distributed initialization.

>> I think you should be able to use "unsigned long" as a proxy for the native register size.
> 
> `unsigned long' is not listed in section `G.3 Predefined Target Types'
> of the GDB manual.

Hmm.  I didn't look at the docs; I looked at gdbtypes.h and saw the existence of 
builtin_unsigned_long.  I suppose this might not be plumbed into the xml.

In which case, since we're generating everything dynamically anyway, we could just as 
easily make NULL map to "uint<XLEN>" when generating the xml.  Or instead of a string, 
perhaps some enum which distinguishes data_ptr/code_ptr/uint<xlen>/uint<min(n,xlen)> which 
is then filled in during generation.

> I also have to say that GDB does not handle the target descriptions
> correctly in all cases.  In particular, I suspect a bug when
> a field crosses a 32-bit boundary: GDB is showing twice the
> field value.

I wonder if this is an issue of using "uint32" for the field type?  You could, for the 
moment, drop all of the bitfield descriptions and leave the interpretation to the user. 
Giving an accurate integral value for the register as a whole is more helpful than giving 
inaccurate field values.

> As I wrote Alistair, I'm not sure this reasoning is correct.  Even if a 64-bit
> machine is running in 32-bit mode, the machine itself remains a 64-bit machine,
> and it's CSRs are so too.

And that's where we're mis-communicating.

We're working toward having exactly one qemu-system-riscv binary, which will emulate both 
RV32 and RV64 (and RV128) cpus, selected by -cpu foo.  At which point there will be no 
CONFIG_RISCV32 define at all.  The way to distinguish the three cases is 
cpu->env.misa_mxl_max.

> We can have the situation of a 64-bit kernel and 32-bit
> user-mode process; would we want the CSRs to change when switching between the two?

Well ideally yes...

> Even if we did, the GDB remote protocol does not have the ability to say "API change,
> please reread the target description" (AFAIK).

... but as you rightly note, gdb can't handle it.


r~