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

[Schwarz, Konrad]

Hi,

> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Tuesday, January 4, 2022 23:12
> To: Schwarz, Konrad (T CED SES-DE) <konrad.schwarz@siemens.com>
> Subject: Re: [PATCH v2 4/5] RISC-V: Typed CSRs in gdbserver
> 
> On Wed, Jan 5, 2022 at 1:56 AM Konrad Schwarz
> <konrad.schwarz@siemens.com> wrote:
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 9f41954894..557b4afe0e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -3,6 +3,7 @@
> >   *
> >   * Copyright (c) 2016-2017 Sagar Karandikar, sagark@eecs.berkeley.edu
> >   * Copyright (c) 2017-2018 SiFive, Inc.
> > + * Copyright (c) 2021 Siemens AG, konrad.schwarz@siemens.com
> 
> Please don't add these to existing files. In this case you have just
> added a newline to this file

Sorry, I don't know how that slipped through.
I originally didn't have any copyright messages, to which my company objected,
and I guess I overcompensated.
 
> diff --git a/target/riscv/csr32-op-gdbserver.h 
> b/target/riscv/csr32-op-gdbserver.h
> > new file mode 100644
> > index 0000000000..e8ec527f23
> > --- /dev/null
> > +++ b/target/riscv/csr32-op-gdbserver.h
> > @@ -0,0 +1,109 @@
> > +/* Copyright (c) 2021 Siemens AG, konrad.schwarz@siemens.com */
> 
> All of these files should have the usual file boiler plate

OK, I'll try to figure out something.

> > +#if !defined(CONFIG_USER_ONLY)
> > +#  ifdef TARGET_RISCV64
> > +#    include "csr64-op-gdbserver.h"
> > +#  elif defined TARGET_RISCV64
> > +#    include "csr32-op-gdbserver.h"
> 
> This doesn't look right. `if defined TARGET_RISCV64` -> `include
> "csr32-op-gdbserver.h"`?

You are quite right. 

> Also this should be dynamic instead of based on the build time CPU, as
> the user could use a 32-bit CPU on a 64-bit target build.

I'm not sure.  The machine itself is still a 64-bit machine; its CSRs are 64 
bits long.
`csr.c', which implements the CSRs contains an instance of # ifdef 
TARGET_RISCV64,
so that part of the implementation is not dynamic either.

Also, someone who is developing 32-bit system software can easily
sidestep the issue by using the 32-bit target.

Regards
Konrad