Refresh the dynamic CSR xml after updating the state of the cpu.

[PATCH 2/2] hw/intc: riscv_imsic: Refresh the CSRs xml after updating the state of the cpu.

Posted by Tommy Wu 2 years, 8 months ago

Originally, when we set the ext_smaia to true, we still cannot print the
AIA CSRs in the remote gdb debugger, because the dynamic CSR xml is
generated when the cpu is realized.

This patch refreshes the dynamic CSR xml after we update the ext_smaia,
so that we can print the AIA CSRs in the remote gdb debugger.

Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/intc/riscv_imsic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
index fea3385b51..97a51d535b 100644
--- a/hw/intc/riscv_imsic.c
+++ b/hw/intc/riscv_imsic.c
@@ -350,6 +350,10 @@ static void riscv_imsic_realize(DeviceState *dev, Error **errp)
         } else {
             rcpu->cfg.ext_smaia = true;
         }
+
+        /* Refresh the dynamic csr xml for the gdbstub. */
+        riscv_refresh_dynamic_csr_xml(cpu);
+
         riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ? PRV_M : PRV_S,
                                       riscv_imsic_rmw, imsic);
     }
-- 
2.38.1

Re: [PATCH 2/2] hw/intc: riscv_imsic: Refresh the CSRs xml after updating the state of the cpu.

Posted by Weiwei Li 2 years, 8 months ago

On 2023/5/23 19:44, Tommy Wu wrote:
> Originally, when we set the ext_smaia to true, we still cannot print the
> AIA CSRs in the remote gdb debugger, because the dynamic CSR xml is
> generated when the cpu is realized.
>
> This patch refreshes the dynamic CSR xml after we update the ext_smaia,
> so that we can print the AIA CSRs in the remote gdb debugger.
>
> Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>   hw/intc/riscv_imsic.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> index fea3385b51..97a51d535b 100644
> --- a/hw/intc/riscv_imsic.c
> +++ b/hw/intc/riscv_imsic.c
> @@ -350,6 +350,10 @@ static void riscv_imsic_realize(DeviceState *dev, Error **errp)
>           } else {
>               rcpu->cfg.ext_smaia = true;
>           }
> +
> +        /* Refresh the dynamic csr xml for the gdbstub. */
> +        riscv_refresh_dynamic_csr_xml(cpu);
> +

There is an assert in riscv_refresh_dynamic_csr_xml():

+    if (!cpu->dyn_csr_xml) {
+        g_assert_not_reached();
+    }

So I think riscv_refresh_dynamic_csr_xml() can only be called when 
cpu->dyn_csr_xml is true here.

Regards,

Weiwei Li

>           riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ? PRV_M : PRV_S,
>                                         riscv_imsic_rmw, imsic);
>       }

Re: [PATCH 2/2] hw/intc: riscv_imsic: Refresh the CSRs xml after updating the state of the cpu.

Posted by Tommy Wu 2 years, 8 months ago

Hi Weiwei Li,
Yes, you're right,  `riscv_refresh_dynamic_csr_xml()`  can only be called
when
cpu->dyn_csr_xml isn't a NULL pointer here.

The cpu->dyn_csr_xml will be set when the cpu is realized.

Best Regards,
Tommy


On Tue, May 23, 2023 at 10:44 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:

>
> On 2023/5/23 19:44, Tommy Wu wrote:
> > Originally, when we set the ext_smaia to true, we still cannot print the
> > AIA CSRs in the remote gdb debugger, because the dynamic CSR xml is
> > generated when the cpu is realized.
> >
> > This patch refreshes the dynamic CSR xml after we update the ext_smaia,
> > so that we can print the AIA CSRs in the remote gdb debugger.
> >
> > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > ---
> >   hw/intc/riscv_imsic.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> > index fea3385b51..97a51d535b 100644
> > --- a/hw/intc/riscv_imsic.c
> > +++ b/hw/intc/riscv_imsic.c
> > @@ -350,6 +350,10 @@ static void riscv_imsic_realize(DeviceState *dev,
> Error **errp)
> >           } else {
> >               rcpu->cfg.ext_smaia = true;
> >           }
> > +
> > +        /* Refresh the dynamic csr xml for the gdbstub. */
> > +        riscv_refresh_dynamic_csr_xml(cpu);
> > +
>
> There is an assert in riscv_refresh_dynamic_csr_xml():
>
> +    if (!cpu->dyn_csr_xml) {
> +        g_assert_not_reached();
> +    }
>
> So I think riscv_refresh_dynamic_csr_xml() can only be called when
> cpu->dyn_csr_xml is true here.
>
> Regards,
>
> Weiwei Li
>
> >           riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ? PRV_M :
> PRV_S,
> >                                         riscv_imsic_rmw, imsic);
> >       }
>
>

Re: [PATCH 2/2] hw/intc: riscv_imsic: Refresh the CSRs xml after updating the state of the cpu.

Posted by Weiwei Li 2 years, 8 months ago

On 2023/5/24 09:51, Tommy Wu wrote:
> Hi Weiwei Li,
> Yes, you're right,  `riscv_refresh_dynamic_csr_xml()`  can only be 
> called when
> cpu->dyn_csr_xml isn't a NULL pointer here.
>
> The cpu->dyn_csr_xml will be set when the cpu is realized.

Yeah, It will  be set only when Zicsr is supported.  And I think aia 
requires Zicsr.

However, another question:  whether we should add a check in 
riscv_imsic.c that it requires Zicsr?

Regards,

Weiwei Li

>
> Best Regards,
> Tommy
>
>
> On Tue, May 23, 2023 at 10:44 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
>     On 2023/5/23 19:44, Tommy Wu wrote:
>     > Originally, when we set the ext_smaia to true, we still cannot
>     print the
>     > AIA CSRs in the remote gdb debugger, because the dynamic CSR xml is
>     > generated when the cpu is realized.
>     >
>     > This patch refreshes the dynamic CSR xml after we update the
>     ext_smaia,
>     > so that we can print the AIA CSRs in the remote gdb debugger.
>     >
>     > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
>     > Reviewed-by: Frank Chang <frank.chang@sifive.com>
>     > ---
>     >   hw/intc/riscv_imsic.c | 4 ++++
>     >   1 file changed, 4 insertions(+)
>     >
>     > diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
>     > index fea3385b51..97a51d535b 100644
>     > --- a/hw/intc/riscv_imsic.c
>     > +++ b/hw/intc/riscv_imsic.c
>     > @@ -350,6 +350,10 @@ static void riscv_imsic_realize(DeviceState
>     *dev, Error **errp)
>     >           } else {
>     >               rcpu->cfg.ext_smaia = true;
>     >           }
>     > +
>     > +        /* Refresh the dynamic csr xml for the gdbstub. */
>     > +        riscv_refresh_dynamic_csr_xml(cpu);
>     > +
>
>     There is an assert in riscv_refresh_dynamic_csr_xml():
>
>     +    if (!cpu->dyn_csr_xml) {
>     +        g_assert_not_reached();
>     +    }
>
>     So I think riscv_refresh_dynamic_csr_xml() can only be called when
>     cpu->dyn_csr_xml is true here.
>
>     Regards,
>
>     Weiwei Li
>
>     >           riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ?
>     PRV_M : PRV_S,
>     >                                         riscv_imsic_rmw, imsic);
>     >       }
>

Re: [PATCH 2/2] hw/intc: riscv_imsic: Refresh the CSRs xml after updating the state of the cpu.

Posted by Tommy Wu 2 years, 8 months ago

Hi Weiwei Li,
You're right, it seems that we need to add a check in riscv_imsic.c
Thanks for the advice!

Best Regards,
Tommy

On Wed, May 24, 2023 at 10:35 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:

>
> On 2023/5/24 09:51, Tommy Wu wrote:
> > Hi Weiwei Li,
> > Yes, you're right,  `riscv_refresh_dynamic_csr_xml()`  can only be
> > called when
> > cpu->dyn_csr_xml isn't a NULL pointer here.
> >
> > The cpu->dyn_csr_xml will be set when the cpu is realized.
>
> Yeah, It will  be set only when Zicsr is supported.  And I think aia
> requires Zicsr.
>
> However, another question:  whether we should add a check in
> riscv_imsic.c that it requires Zicsr?
>
> Regards,
>
> Weiwei Li
>
> >
> > Best Regards,
> > Tommy
> >
> >
> > On Tue, May 23, 2023 at 10:44 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >
> >
> >     On 2023/5/23 19:44, Tommy Wu wrote:
> >     > Originally, when we set the ext_smaia to true, we still cannot
> >     print the
> >     > AIA CSRs in the remote gdb debugger, because the dynamic CSR xml is
> >     > generated when the cpu is realized.
> >     >
> >     > This patch refreshes the dynamic CSR xml after we update the
> >     ext_smaia,
> >     > so that we can print the AIA CSRs in the remote gdb debugger.
> >     >
> >     > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> >     > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> >     > ---
> >     >   hw/intc/riscv_imsic.c | 4 ++++
> >     >   1 file changed, 4 insertions(+)
> >     >
> >     > diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> >     > index fea3385b51..97a51d535b 100644
> >     > --- a/hw/intc/riscv_imsic.c
> >     > +++ b/hw/intc/riscv_imsic.c
> >     > @@ -350,6 +350,10 @@ static void riscv_imsic_realize(DeviceState
> >     *dev, Error **errp)
> >     >           } else {
> >     >               rcpu->cfg.ext_smaia = true;
> >     >           }
> >     > +
> >     > +        /* Refresh the dynamic csr xml for the gdbstub. */
> >     > +        riscv_refresh_dynamic_csr_xml(cpu);
> >     > +
> >
> >     There is an assert in riscv_refresh_dynamic_csr_xml():
> >
> >     +    if (!cpu->dyn_csr_xml) {
> >     +        g_assert_not_reached();
> >     +    }
> >
> >     So I think riscv_refresh_dynamic_csr_xml() can only be called when
> >     cpu->dyn_csr_xml is true here.
> >
> >     Regards,
> >
> >     Weiwei Li
> >
> >     >           riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ?
> >     PRV_M : PRV_S,
> >     >                                         riscv_imsic_rmw, imsic);
> >     >       }
> >
>
>