Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next

[Daniel Henrique Barboza]

Hi Bin!

On 1/12/23 22:28, Bin Meng wrote:
> Hi Daniel,
>
> On Wed, Jan 11, 2023 at 1:03 PM Alistair Francis <alistair23@gmail.com> wrote:
> > On Wed, Jan 11, 2023 at 6:17 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> > > Hi,
> > >
> > > I found this bug when testing my avocado changes in riscv-to-apply.next.
> > > The sifive_u board, both 32 and 64 bits, stopped booting OpenSBI. The
> > > guest hangs indefinitely.
> > >
> > > Git bisect points that this patch broke things:
> > >
> > > 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8 is the first bad commit
> > > commit 8c3f35d25e7e98655c609b6c1e9f103b9240f8f8
> > > Author: Weiwei Li <liweiwei@iscas.ac.cn>
> > > Date:   Wed Dec 28 14:20:21 2022 +0800
> > >
> > >      target/riscv: add support for Zca extension
> > >
> > >      Modify the check for C extension to Zca (C implies Zca)
> > > (https://github.com/alistair23/qemu/commit/8c3f35d25e7e98655c609b6c1e9f103b9240f8f8)
> > >
> > >
> > > But this patch per se isn't doing anything wrong. The root of the
> > > problem is that this patch makes assumptions based on the previous
> > > patch:
> > >
> > > commit a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85
> > > Author: Weiwei Li <liweiwei@iscas.ac.cn>
> > > Date:   Wed Dec 28 14:20:20 2022 +0800
> > >
> > >      target/riscv: add cfg properties for Zc* extension
> > > (https://github.com/alistair23/qemu/commit/a2b409aa6cadc1ed9715e1ab916ddd3dade0ba85)
> > >
> > > Which added a lot of logic and assumptions that are being skipped by all
> > > the SiFive boards because, during riscv_cpu_realize(), we have this
> > > code:
> > >
> > >      /* If only MISA_EXT is unset for misa, then set it from properties */
> > >      if (env->misa_ext == 0) {
> > >          uint32_t ext = 0;
> > >          (...)
> > >      }
> > >
> > > In short, we have a lot of code that are being skipped by all SiFive
> > > CPUs because these CPUs are setting a non-zero value in set_misa() in
> > > their respective cpu_init() functions.
> > >
> > > It's possible to just hack in and fix the SiFive problem in isolate, but
> > > I believe we can do better and allow all riscv_cpu_realize() to be executed
> > > for all CPUs, regardless of what they've done during their cpu_init().
> > >
> > >
> > > Daniel Henrique Barboza (2):
> > >    target/riscv/cpu: set cpu->cfg in register_cpu_props()
> > >    target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize()
> > Thanks for the patches
> >
> > I have rebased these onto the latest master and dropped the other
> > series. That way when the other series is applied we don't break
> > bisectability.
> It seems these 2 patches are already in Alistair's tree.
>
> Richard had a suggestion for patch 1 and I had some minor comments
> too. Do you plan to resend a v2 for that?

I'll re-send the v2 with your comments addressed.

About Richard's suggestion, I believe I replied that it would require more 
thought
because, as it is now, it would break boards that are setting their properties 
after
register_cpu_props(). The overall simplification of the cpu_init() code across 
all
RISC-V boards is good thing to do in the future as a follow up, IMO.


Thanks,

Daniel



> Regards,
> Bin