Message ID | 20180704001334.27460-1-jimw@sifive.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 15945 invoked by alias); 4 Jul 2018 00:14:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 15931 invoked by uid 89); 4 Jul 2018 00:14:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:1434, HX-Received:sk:c8-v6mr, riscvtdepc, riscv-tdep.c X-HELO: mail-pl0-f67.google.com Received: from mail-pl0-f67.google.com (HELO mail-pl0-f67.google.com) (209.85.160.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Jul 2018 00:14:07 +0000 Received: by mail-pl0-f67.google.com with SMTP id 31-v6so1773632plc.4 for <gdb-patches@sourceware.org>; Tue, 03 Jul 2018 17:14:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=from:to:cc:subject:date:message-id; bh=bKxvYK+uWHq6Q/c48f+NtrB2aRLCl+gCwp9ZZxL+GGg=; b=bFsMwk3GBxGdw66Ufd3S1iXoXI42G05LjI3KBederskCUzlJn07Up2FfzOXR8Kvk9w xm2fbLaCeTxtoCktgy3YHPgLLsxlYxINszlta/ioaCWHF91getd0MrH2A+9o1N5NCtQT nvlGdo2pmZb13c8bQ/e65fTQC40jjD76WcSjZR0xQPkViP347e8n0PWb56tDfstkT4KZ 7FRztcSzkhwwB4Rsog/TTNTvouM/HcDiRrHE6ngfwjzwfaX4R6N4vSv7Lv72YZ37ZK8z 7Z8+sHs7tMl++d8smMLBKX4cbkW8Xgm9if6eweTuk0tifXM+hIwiDnuL/hFIym6TF6EA ngvQ== Return-Path: <jimw@sifive.com> Received: from rohan.internal.sifive.com ([12.206.222.5]) by smtp.gmail.com with ESMTPSA id r77-v6sm4414766pfr.117.2018.07.03.17.14.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Jul 2018 17:14:05 -0700 (PDT) From: Jim Wilson <jimw@sifive.com> To: gdb-patches@sourceware.org Cc: andrew.burgess@embecosm.com, Jim Wilson <jimw@sifive.com> Subject: [PATCH] RISC-V: Correct legacy misa register number. Date: Tue, 3 Jul 2018 17:13:34 -0700 Message-Id: <20180704001334.27460-1-jimw@sifive.com> |
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
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 :)
* 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 >
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
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 >
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
* 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 --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,