Re: [PATCHv2 8/9] gdb/riscv: write CSRs into baremetal core dumps

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 1/20/21 5:23 PM, Andrew Burgess wrote:
> Use the current target description to include CSRs into the RISC-V
> baremetal core dumps.
> 
> Every CSR declared in the current target description will be included
> in the core dump.  If a CSR fails to read then the value 0 will be
> placed into the core dump instead.
> 
> It will be critical for users that they have the same target
> description in use when loading the core file as was in use when
> writing the core file.  This should be fine if the user allows the
> target description to be written into the core file.
> 
> In more detail, this commit adds a NT_RISCV_CSR note type.  The
> contents of this section is a series of either 4-byte (on RV32
> targets), or 8-byte (on RV64 targets) regions.  Every CSR that
> is mentioned in the current target description is written into one of
> these regions in the order that the registers appear in the target
> description.  As a consequence it is critical that the exact same
> target description, including the same register order, is in use when
> the CSRs are loaded from the core file.
> 
> gdb/ChangeLog:
> 
> 	* riscv-non-tdep.c (riscv_csrset): New static global.
> 	(riscv_update_csrmap): New function.
> 	(riscv_iterate_over_regset_sections): Process CSRs.
> ---
>   gdb/ChangeLog         |  7 +++++
>   gdb/riscv-none-tdep.c | 60 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+)
> 
> diff --git a/gdb/riscv-none-tdep.c b/gdb/riscv-none-tdep.c
> index e06ee0981fe..2392c132e69 100644
> --- a/gdb/riscv-none-tdep.c
> +++ b/gdb/riscv-none-tdep.c
> @@ -24,6 +24,8 @@
>   #include "elf-bfd.h"
>   #include "regset.h"
>   #include "none-tdep.h"
> +#include "user-regs.h"
> +#include "target-descriptions.h"
>   
>   /* Define the general register mapping.  This follows the same format as
>      the RISC-V linux corefile.  The linux kernel puts the PC at offset 0,
> @@ -62,6 +64,42 @@ static const struct regset riscv_fregset =
>     riscv_fregmap, riscv_supply_regset, regcache_collect_regset
>   };
>   
> +/* Define the CSR regset, this is not constant as the regmap field is
> +   updated dynamically based on the current target description.  */
> +
> +static struct regset riscv_csrset =
> +{
> +  nullptr, regcache_supply_regset, regcache_collect_regset
> +};
> +
> +/* Update the regmap field of RISCV_CSRSET based on the CSRs available in
> +   the current target description.  */
> +
> +static void
> +riscv_update_csrmap (struct gdbarch *gdbarch,
> +		     const struct tdesc_feature *feature_csr)
> +{
> +  int i = 0;
> +
> +  /* Release any previously defined map.  */
> +  delete[] ((struct regcache_map_entry *) riscv_csrset.regmap);
> +
> +  /* Now create a register map for every csr found in the target
> +     description.  */
> +  struct regcache_map_entry *riscv_csrmap
> +    = new struct regcache_map_entry[feature_csr->registers.size() + 1];
> +  for (auto &csr : feature_csr->registers)
> +    {
> +      int regnum = user_reg_map_name_to_regnum (gdbarch, csr->name.c_str(),
> +						csr->name.length());
> +      riscv_csrmap[i++] = {1, regnum, 0};
> +    }
> +
> +  /* Mark the end of the array.  */
> +  riscv_csrmap[i] = {0};
> +  riscv_csrset.regmap = riscv_csrmap;
> +}
> +
>   /* Implement the "iterate_over_regset_sections" gdbarch method.  */
>   
>   static void
> @@ -81,6 +119,28 @@ riscv_iterate_over_regset_sections (struct gdbarch *gdbarch,
>   	    + register_size (gdbarch, RISCV_CSR_FCSR_REGNUM));
>         cb (".reg2", sz, sz, &riscv_fregset, NULL, cb_data);
>       }
> +
> +  /* Read or write the CSRs.  The set of CSRs is defined by the current
> +     target description.  The user is responsible for ensuring that the
> +     same target description is in use when reading the core file as was
> +     in use when writing the core file.  */
> +  const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);
> +
> +  /* Do not dump/load any CSRs if there is no target description or the target
> +     description does not contain any CSRs.  */
> +  if (tdesc != nullptr)
> +    {
> +      const struct tdesc_feature *feature_csr
> +        = tdesc_find_feature (tdesc, riscv_feature_name_csr);
> +      if (feature_csr != nullptr && feature_csr->registers.size () > 0)
> +	{
> +	  riscv_update_csrmap (gdbarch, feature_csr);
> +	  cb (".reg-riscv-csr",
> +	      (feature_csr->registers.size() * riscv_isa_xlen (gdbarch)),
> +	      (feature_csr->registers.size() * riscv_isa_xlen (gdbarch)),
> +	      &riscv_csrset, NULL, cb_data);
> +	}
> +    }
>   }
>   
>   /* Initialize RISC-V bare-metal ABI info.  */
> 

Looks OK to me.