| Message ID | 20230110201405.247785-1-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next | expand |
Hi, I mentioned that the bug were found in riscv-to-apply.next but forgot to mentioned that the patches were also based on top of it as well: https://github.com/alistair23/qemu/tree/riscv-to-apply.next Thanks, Daniel On 1/10/23 17:14, Daniel Henrique Barboza 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() > > target/riscv/cpu.c | 525 +++++++++++++++++++++++++-------------------- > target/riscv/cpu.h | 4 + > 2 files changed, 292 insertions(+), 237 deletions(-) >
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. Alistair > > target/riscv/cpu.c | 525 +++++++++++++++++++++++++-------------------- > target/riscv/cpu.h | 4 + > 2 files changed, 292 insertions(+), 237 deletions(-) > > -- > 2.39.0 > >
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? Regards, Bin
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
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() target/riscv/cpu.c | 525 +++++++++++++++++++++++++-------------------- target/riscv/cpu.h | 4 + 2 files changed, 292 insertions(+), 237 deletions(-)