Hi, > -----Original Message----- > From: Richard Henderson <richard.hender...@linaro.org> > Sent: Wednesday, January 5, 2022 0:02 > To: Schwarz, Konrad (T CED SES-DE) <konrad.schw...@siemens.com>; > qemu-devel@nongnu.org > Cc: Alistair Francis <alistair.fran...@wdc.com>; Bin Meng > <bin.m...@windriver.com>; Palmer Dabbelt > <pal...@dabbelt.com>; Ralf Ramsauer <ralf.ramsa...@oth-regensburg.de> > Subject: Re: [PATCH v2 4/5] RISC-V: Typed CSRs in gdbserver > > On 1/4/22 7:51 AM, Konrad Schwarz wrote: > > +++ b/target/riscv/csr32-op-gdbserver.h > ... > > +++ b/target/riscv/csr64-op-gdbserver.h > > There is a *lot* of overlap between these two files. > Why not add this data to the main csr_ops array? > That would put all the info for each csr in one place.
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. I did actually originally add the data directly to the csr_ops array. Because of the large number of CSRs and the generational aspect, it was infeasible (for me at least) to create an editing merge script to intermingle the existing definitions and the new data. 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. This is also the reason the overlap is not handled more elegantly: GDB target description XML doesn't support alternates for different machine word lengths and so I lose that information when transforming from the 64 and 32-bit target description sources. > > + [CSR_CYCLE] { .gdb_type = "uint64", .gdb_group = "user" }, > > 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. 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'm pretty sure my target description is correct for this case, I've rechecked several times. Also, GDB is not picking up all of the new CSRs for some reason, but I'm sure is reading the target description from QEMU. Since target descriptions are rare in practice, a bug would not surprise me; for example, the DTD for target descriptions supplied with GDB is wrong: it is missing the `enum' element (which was probably added later) and the xsi:include element. Ultimately, this is a chicken and egg problem: bugs in GDB can only be flushed out if it gets interesting target descriptions to read. > > > +char const riscv_gdb_csr_types[] = > > +#ifdef TARGET_RISCV32 > ... > > +#elif defined TARGET_RISCV64 > ... > > +# endif > > +; > > Ideally we shouldn't use ifdefs for this -- we should choose one or the other > depending on > the startup env->misa_mxl_max. We are still using an ifdef for the main > riscv-*-virtual.xml, but that could be considered a bug to fix. 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. 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? Even if we did, the GDB remote protocol does not have the ability to say "API change, please reread the target description" (AFAIK). And in any case, users can easily side-step the issue by using a 32-bit target QEMU if they are only interested in 32-bit code. Regards Konrad