diff mbox

RISC-V: Correct legacy misa register number.

Message ID 20180704001334.27460-1-jimw@sifive.com
State New, archived
Headers

Commit Message

Jim Wilson July 4, 2018, 12:13 a.m. UTC 
  The main purpose of this patch is to fix the legacy misa register number, which
is missing the +65 added to all of the other CSRs.

This also changes DECLARE_CSR to use RISCV_FIRST_CSR_REGNUM instead of
RISCV_LAST_FP_REGNUM+1 to be consistent with riscv-tdep.c.

I don't have access to legacy hardware that I can test the misa number change
with, but it has been tested on a riscv64-linux system using patched gdb and
patched kernel, since it isn't usable otherwise.

Jim

	gdb/
	* riscv-tdep.h (DECLARE_CSR): Use RISCV_FIRST_CSR_REGNUM instead of
	RISCV_LAST_FP_REGNUM + 1.
	(RSICV_CSR_LEGACY_MISA_REGNUM): Add RISCV_FIRST_CSR_REGNUM.
---
 gdb/riscv-tdep.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Palmer Dabbelt July 4, 2018, 12:35 a.m. UTC | #1 
On Tue, 03 Jul 2018 17:13:34 PDT (-0700), Jim Wilson wrote:
> The main purpose of this patch is to fix the legacy misa register number, which
> is missing the +65 added to all of the other CSRs.
>
> This also changes DECLARE_CSR to use RISCV_FIRST_CSR_REGNUM instead of
> RISCV_LAST_FP_REGNUM+1 to be consistent with riscv-tdep.c.
>
> I don't have access to legacy hardware that I can test the misa number change
> with, but it has been tested on a riscv64-linux system using patched gdb and
> patched kernel, since it isn't usable otherwise.

FWIW, there's no legacy RISC-V hardware that can run a native GDB.  Thus the 
only mechanism to access this would be via a JTAG debugger, and there's no ABI 
spec for those.

Is there even a reason to have a legacy MISA CSR exposed to GDB?  I feel like 
we can just handle this in the JTAG debugger and keep this oddity from slipping 
into an ABI.

I'm adding Tim as he might have more context.

> Jim
>
> 	gdb/
> 	* riscv-tdep.h (DECLARE_CSR): Use RISCV_FIRST_CSR_REGNUM instead of
> 	RISCV_LAST_FP_REGNUM + 1.
> 	(RSICV_CSR_LEGACY_MISA_REGNUM): Add RISCV_FIRST_CSR_REGNUM.
> ---
>  gdb/riscv-tdep.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index ab5e278759..4fc05976ba 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -39,11 +39,11 @@ enum
>    RISCV_LAST_FP_REGNUM = 64,	/* Last Floating Point Register */
>
>    RISCV_FIRST_CSR_REGNUM = 65,  /* First CSR */
> -#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_LAST_FP_REGNUM + 1 + num,
> +#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_FIRST_CSR_REGNUM + num,
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
>    RISCV_LAST_CSR_REGNUM = 4160,
> -  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10,
> +  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10 + RISCV_FIRST_CSR_REGNUM,
>
>    RISCV_PRIV_REGNUM = 4161,

If we can't nuke RISCV_CSR_LEGACY_MISA_REGNUM them I'm fine with either way.  
Sorry to keep throwing a wrench in the works :)
Andrew Burgess July 4, 2018, 8:34 a.m. UTC | #2 
* Jim Wilson <jimw@sifive.com> [2018-07-03 17:13:34 -0700]:

> The main purpose of this patch is to fix the legacy misa register number, which
> is missing the +65 added to all of the other CSRs.
> 
> This also changes DECLARE_CSR to use RISCV_FIRST_CSR_REGNUM instead of
> RISCV_LAST_FP_REGNUM+1 to be consistent with riscv-tdep.c.
> 
> I don't have access to legacy hardware that I can test the misa number change
> with, but it has been tested on a riscv64-linux system using patched gdb and
> patched kernel, since it isn't usable otherwise.
> 
> Jim
> 
> 	gdb/
> 	* riscv-tdep.h (DECLARE_CSR): Use RISCV_FIRST_CSR_REGNUM instead of
> 	RISCV_LAST_FP_REGNUM + 1.
> 	(RSICV_CSR_LEGACY_MISA_REGNUM): Add RISCV_FIRST_CSR_REGNUM.
> ---
>  gdb/riscv-tdep.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index ab5e278759..4fc05976ba 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -39,11 +39,11 @@ enum
>    RISCV_LAST_FP_REGNUM = 64,	/* Last Floating Point Register */
>  
>    RISCV_FIRST_CSR_REGNUM = 65,  /* First CSR */
> -#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_LAST_FP_REGNUM + 1 + num,
> +#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_FIRST_CSR_REGNUM + num,

Regardless of the discussion about RISCV_CSR_LEGACY_MISA_REGNUM
could you push the change to DECLARE_CSR anyway please.  I think this
is a good clean up and it's unrelated to the other change.

Thanks,
Andrew





>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
>    RISCV_LAST_CSR_REGNUM = 4160,
> -  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10,
> +  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10 + RISCV_FIRST_CSR_REGNUM,
>  
>    RISCV_PRIV_REGNUM = 4161,
>  
> -- 
> 2.17.1
>
Jim Wilson July 4, 2018, 4:17 p.m. UTC | #3 
On Tue, Jul 3, 2018 at 5:35 PM, Palmer Dabbelt <palmer@sifive.com> wrote:
> FWIW, there's no legacy RISC-V hardware that can run a native GDB.  Thus the
> only mechanism to access this would be via a JTAG debugger, and there's no
> ABI spec for those.
>
> Is there even a reason to have a legacy MISA CSR exposed to GDB?  I feel
> like we can just handle this in the JTAG debugger and keep this oddity from
> slipping into an ABI.
>
> I'm adding Tim as he might have more context.

This is code that gets used for both linux native and embedded cross.
So the fact that there is no legacy hardware that can run linux isn't
relevant.

Gdb is trying to read misa to discover what features exist on the
target, C, F, D, etc.  It first tries to read the 1.10 misa, and if
that fails, it then tries to read the 1.9 misa.  If the 1.9 misa
fails, then you get a cryptic error about a missing register.  But the
priv spec says that misa should always exist and always be readable,
so I think you can only get the cryptic error from the broken linux
support in github riscv/riscv-binutils-gdb.  If the misa read returns
0, which is allowed by the priv spec, then gdb looks at the program
elf header flags, and assumes the target hardware matches the ELF
file.

My linux support for now just returns 0, but in the future it might be
nice to have ptrace support for reading misa.  This would only need to
support the 1.10 misa.

As far as I know, the legacy misa support should remain.  But I don't
know how the OpenOCD support works.  I suppose you want to make
OpenOCD try reading both register numbers if we ask for the 1.10 misa?
 I don't see the benefit of that.  There isn't really any ABI exposure
here.  It is just a few lines of code in gdb to try to read misa,
falling back to the old number if the new number doesn't work.

Jim
Tim Newsome July 6, 2018, 3:16 a.m. UTC | #4 
As Jim says, gdb connecting through OpenOCD to a legacy RISC-V core
benefits from the fallback to reading misa at the old address. Pushing that
behavior into OpenOCD feels like it's going to make OpenOCD behave in
unexpected ways by special-casing just that one CSR. (It also opens up the
door for doing that fora number of other CSRs which used to live at
different numbers.) I much prefer the current behavior, where OpenOCD
simply reads the CSR requested, and returns an error if it's unavailable.

Tim

On Wed, Jul 4, 2018 at 9:17 AM, Jim Wilson <jimw@sifive.com> wrote:

> On Tue, Jul 3, 2018 at 5:35 PM, Palmer Dabbelt <palmer@sifive.com> wrote:
> > FWIW, there's no legacy RISC-V hardware that can run a native GDB.  Thus
> the
> > only mechanism to access this would be via a JTAG debugger, and there's
> no
> > ABI spec for those.
> >
> > Is there even a reason to have a legacy MISA CSR exposed to GDB?  I feel
> > like we can just handle this in the JTAG debugger and keep this oddity
> from
> > slipping into an ABI.
> >
> > I'm adding Tim as he might have more context.
>
> This is code that gets used for both linux native and embedded cross.
> So the fact that there is no legacy hardware that can run linux isn't
> relevant.
>
> Gdb is trying to read misa to discover what features exist on the
> target, C, F, D, etc.  It first tries to read the 1.10 misa, and if
> that fails, it then tries to read the 1.9 misa.  If the 1.9 misa
> fails, then you get a cryptic error about a missing register.  But the
> priv spec says that misa should always exist and always be readable,
> so I think you can only get the cryptic error from the broken linux
> support in github riscv/riscv-binutils-gdb.  If the misa read returns
> 0, which is allowed by the priv spec, then gdb looks at the program
> elf header flags, and assumes the target hardware matches the ELF
> file.
>
> My linux support for now just returns 0, but in the future it might be
> nice to have ptrace support for reading misa.  This would only need to
> support the 1.10 misa.
>
> As far as I know, the legacy misa support should remain.  But I don't
> know how the OpenOCD support works.  I suppose you want to make
> OpenOCD try reading both register numbers if we ask for the 1.10 misa?
>  I don't see the benefit of that.  There isn't really any ABI exposure
> here.  It is just a few lines of code in gdb to try to read misa,
> falling back to the old number if the new number doesn't work.
>
> Jim
>
Jim Wilson July 16, 2018, 10:01 p.m. UTC | #5 
On Wed, Jul 4, 2018 at 1:34 AM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Jim Wilson <jimw@sifive.com> [2018-07-03 17:13:34 -0700]:
>
>> The main purpose of this patch is to fix the legacy misa register number, which
>> is missing the +65 added to all of the other CSRs.
>>
>> This also changes DECLARE_CSR to use RISCV_FIRST_CSR_REGNUM instead of
>> RISCV_LAST_FP_REGNUM+1 to be consistent with riscv-tdep.c.
>>
>> I don't have access to legacy hardware that I can test the misa number change
>> with, but it has been tested on a riscv64-linux system using patched gdb and
>> patched kernel, since it isn't usable otherwise.
>>
>> Jim
>>
>>       gdb/
>>       * riscv-tdep.h (DECLARE_CSR): Use RISCV_FIRST_CSR_REGNUM instead of
>>       RISCV_LAST_FP_REGNUM + 1.
>>       (RSICV_CSR_LEGACY_MISA_REGNUM): Add RISCV_FIRST_CSR_REGNUM.
>
> Regardless of the discussion about RISCV_CSR_LEGACY_MISA_REGNUM
> could you push the change to DECLARE_CSR anyway please.  I think this
> is a good clean up and it's unrelated to the other change.

Tim Newsome confirmed that there should be no special behavior in
OpenOCD to handle the legacy misa register number.  I'd like approval
to commit the whole patch.  It is a bug fix for live code, and a minor
consistency cleanup.

Jim
Andrew Burgess July 17, 2018, 3:39 p.m. UTC | #6 
* Jim Wilson <jimw@sifive.com> [2018-07-16 15:01:41 -0700]:

> On Wed, Jul 4, 2018 at 1:34 AM, Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > * Jim Wilson <jimw@sifive.com> [2018-07-03 17:13:34 -0700]:
> >
> >> The main purpose of this patch is to fix the legacy misa register number, which
> >> is missing the +65 added to all of the other CSRs.
> >>
> >> This also changes DECLARE_CSR to use RISCV_FIRST_CSR_REGNUM instead of
> >> RISCV_LAST_FP_REGNUM+1 to be consistent with riscv-tdep.c.
> >>
> >> I don't have access to legacy hardware that I can test the misa number change
> >> with, but it has been tested on a riscv64-linux system using patched gdb and
> >> patched kernel, since it isn't usable otherwise.
> >>
> >> Jim
> >>
> >>       gdb/
> >>       * riscv-tdep.h (DECLARE_CSR): Use RISCV_FIRST_CSR_REGNUM instead of
> >>       RISCV_LAST_FP_REGNUM + 1.
> >>       (RSICV_CSR_LEGACY_MISA_REGNUM): Add RISCV_FIRST_CSR_REGNUM.
> >
> > Regardless of the discussion about RISCV_CSR_LEGACY_MISA_REGNUM
> > could you push the change to DECLARE_CSR anyway please.  I think this
> > is a good clean up and it's unrelated to the other change.
> 
> Tim Newsome confirmed that there should be no special behavior in
> OpenOCD to handle the legacy misa register number.  I'd like approval
> to commit the whole patch.  It is a bug fix for live code, and a minor
> consistency cleanup.

I approve then :)

Thanks,

Andrew
diff mbox

Patch

diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index ab5e278759..4fc05976ba 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -39,11 +39,11 @@  enum
   RISCV_LAST_FP_REGNUM = 64,	/* Last Floating Point Register */
 
   RISCV_FIRST_CSR_REGNUM = 65,  /* First CSR */
-#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_LAST_FP_REGNUM + 1 + num,
+#define DECLARE_CSR(name, num) RISCV_ ## num ## _REGNUM = RISCV_FIRST_CSR_REGNUM + num,
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
   RISCV_LAST_CSR_REGNUM = 4160,
-  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10,
+  RISCV_CSR_LEGACY_MISA_REGNUM = 0xf10 + RISCV_FIRST_CSR_REGNUM,
 
   RISCV_PRIV_REGNUM = 4161,