1. Patchew
  2. QEMU
  3. View series

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

Daniel Henrique Barboza posted 2 patches 1 year, 3 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
target/riscv/cpu.c | 525 +++++++++++++++++++++++++--------------------
target/riscv/cpu.h |   4 +
2 files changed, 292 insertions(+), 237 deletions(-)
[PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
Posted by Daniel Henrique Barboza 1 year, 3 months ago 
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(-)

-- 
2.39.0
Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
Posted by Alistair Francis 1 year, 3 months ago 
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
>
>
Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
Posted by Bin Meng 1 year, 3 months ago 
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
Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
Posted by Daniel Henrique Barboza 1 year, 3 months ago 
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
Re: [PATCH 0/2] target/riscv/cpu: fix sifive_u 32/64bits boot in riscv-to-apply.next
Posted by Daniel Henrique Barboza 1 year, 3 months ago 
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(-)
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.