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
