diff mbox series

[v2,05/18] target/riscv: gdbstub: Do not generate CSR XML if Zicsr is disabled

Message ID 20230228104035.1879882-6-bmeng@tinylab.org (mailing list archive)
State New, archived
Headers show
Series target/riscv: Various fixes to gdbstub and CSR access | expand

Commit Message

Bin Meng Feb. 28, 2023, 10:40 a.m. UTC 
There is no need to generate the CSR XML if the Zicsr extension
is not enabled.

Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---

(no changes since v1)

 target/riscv/gdbstub.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

LIU Zhiwei March 1, 2023, 9:52 a.m. UTC | #1 
On 2023/2/28 18:40, Bin Meng wrote:
> There is no need to generate the CSR XML if the Zicsr extension
> is not enabled.

Should we generate the FPU XML or Vector XML when Zicsr is not enabled?

Zhiwei

>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>
> (no changes since v1)
>
>   target/riscv/gdbstub.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 704f3d6922..294f0ceb1c 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -406,7 +406,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>           g_assert_not_reached();
>       }
>   
> -    gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                             riscv_gen_dynamic_csr_xml(cs, cs->gdb_num_regs),
> -                             "riscv-csr.xml", 0);
> +    if (cpu->cfg.ext_icsr) {
> +        int base_reg = cs->gdb_num_regs;
> +        gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> +                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
> +                                 "riscv-csr.xml", 0);
> +    }
>   }
Bin Meng March 1, 2023, 9:55 a.m. UTC | #2 
On Wed, Mar 1, 2023 at 5:52 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
>
> On 2023/2/28 18:40, Bin Meng wrote:
> > There is no need to generate the CSR XML if the Zicsr extension
> > is not enabled.
>
> Should we generate the FPU XML or Vector XML when Zicsr is not enabled?

Good point. I think we should disable that too.

>
> Zhiwei
>

Regards,
Bin
Palmer Dabbelt March 1, 2023, 11:43 p.m. UTC | #3 
On Wed, 01 Mar 2023 01:55:34 PST (-0800), Bin Meng wrote:
> On Wed, Mar 1, 2023 at 5:52 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>>
>>
>> On 2023/2/28 18:40, Bin Meng wrote:
>> > There is no need to generate the CSR XML if the Zicsr extension
>> > is not enabled.
>>
>> Should we generate the FPU XML or Vector XML when Zicsr is not enabled?
>
> Good point. I think we should disable that too.

Seems reasonable.  Did you want to do that as part of a v3, or just as a 
follow-on fix?

>> Zhiwei
>>
>
> Regards,
> Bin
Bin Meng March 2, 2023, 12:30 a.m. UTC | #4 
On Thu, Mar 2, 2023 at 7:43 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 01 Mar 2023 01:55:34 PST (-0800), Bin Meng wrote:
> > On Wed, Mar 1, 2023 at 5:52 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> >>
> >>
> >> On 2023/2/28 18:40, Bin Meng wrote:
> >> > There is no need to generate the CSR XML if the Zicsr extension
> >> > is not enabled.
> >>
> >> Should we generate the FPU XML or Vector XML when Zicsr is not enabled?
> >
> > Good point. I think we should disable that too.
>
> Seems reasonable.  Did you want to do that as part of a v3, or just as a
> follow-on fix?
>

I looked at this further.

The FPU / Vector XML is guarded by the " env->misa_ext" check. If
Zicsr is disabled while F or V extension is off, QEMU will error out
in riscv_cpu_realize() earlier before the gdbstub init.

So current patch should be fine.

Regards,
Bin
Palmer Dabbelt March 2, 2023, 12:58 a.m. UTC | #5 
On Wed, 01 Mar 2023 16:30:52 PST (-0800), Bin Meng wrote:
> On Thu, Mar 2, 2023 at 7:43 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 01 Mar 2023 01:55:34 PST (-0800), Bin Meng wrote:
>> > On Wed, Mar 1, 2023 at 5:52 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>> >>
>> >>
>> >> On 2023/2/28 18:40, Bin Meng wrote:
>> >> > There is no need to generate the CSR XML if the Zicsr extension
>> >> > is not enabled.
>> >>
>> >> Should we generate the FPU XML or Vector XML when Zicsr is not enabled?
>> >
>> > Good point. I think we should disable that too.
>>
>> Seems reasonable.  Did you want to do that as part of a v3, or just as a
>> follow-on fix?
>>
>
> I looked at this further.
>
> The FPU / Vector XML is guarded by the " env->misa_ext" check. If
> Zicsr is disabled while F or V extension is off, QEMU will error out
> in riscv_cpu_realize() earlier before the gdbstub init.
>
> So current patch should be fine.

There's a merge conflict that git auto-resolved as

diff --cc target/riscv/csr.c
index a1ecf62305,3a7e0217e2..a2cf3536f0
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@@ -90,10 -53,10 +53,9 @@@ static RISCVException fs(CPURISCVState 
  
  static RISCVException vs(CPURISCVState *env, int csrno)
  {
-     CPUState *cs = env_cpu(env);
-     RISCVCPU *cpu = RISCV_CPU(cs);
+     RISCVCPU *cpu = env_archcpu(env);
  
 -    if (env->misa_ext & RVV ||
 -        cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) {
 +    if (cpu->cfg.ext_zve32f) {
  #if !defined(CONFIG_USER_ONLY)
          if (!env->debugger && !riscv_cpu_vector_enabled(env)) {
              return RISCV_EXCP_ILLEGAL_INST;

which looks correct to me.  It's passing my tests and queued up, but LMK if
something looks wrong.

Thanks!
LIU Zhiwei March 2, 2023, 2:43 a.m. UTC | #6 
On 2023/3/2 8:30, Bin Meng wrote:
> On Thu, Mar 2, 2023 at 7:43 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Wed, 01 Mar 2023 01:55:34 PST (-0800), Bin Meng wrote:
>>> On Wed, Mar 1, 2023 at 5:52 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>>>>
>>>> On 2023/2/28 18:40, Bin Meng wrote:
>>>>> There is no need to generate the CSR XML if the Zicsr extension
>>>>> is not enabled.
>>>> Should we generate the FPU XML or Vector XML when Zicsr is not enabled?
>>> Good point. I think we should disable that too.
>> Seems reasonable.  Did you want to do that as part of a v3, or just as a
>> follow-on fix?
>>
> I looked at this further.
>
> The FPU / Vector XML is guarded by the " env->misa_ext" check. If
> Zicsr is disabled while F or V extension is off, QEMU will error out
> in riscv_cpu_realize() earlier before the gdbstub init.

Make sense.

Zhiwei

>
> So current patch should be fine.
>
> Regards,
> Bin
diff mbox series

Patch

diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 704f3d6922..294f0ceb1c 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -406,7 +406,10 @@  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
         g_assert_not_reached();
     }
 
-    gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
-                             riscv_gen_dynamic_csr_xml(cs, cs->gdb_num_regs),
-                             "riscv-csr.xml", 0);
+    if (cpu->cfg.ext_icsr) {
+        int base_reg = cs->gdb_num_regs;
+        gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
+                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
+                                 "riscv-csr.xml", 0);
+    }
 }