[PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs

Daniel Henrique Barboza posted 3 patches 1 year, 4 months ago
hw/riscv/boot.c            | 96 +++++++++++++++++++++++---------------
hw/riscv/microchip_pfsoc.c | 12 +----
hw/riscv/opentitan.c       |  4 +-
hw/riscv/sifive_e.c        |  4 +-
hw/riscv/sifive_u.c        | 12 +----
hw/riscv/spike.c           | 14 ++----
hw/riscv/virt.c            | 12 +----
include/hw/riscv/boot.h    |  3 +-
8 files changed, 76 insertions(+), 81 deletions(-)
[PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
Posted by Daniel Henrique Barboza 1 year, 4 months ago
Hi,

This new version removed the translate_fn() from patch 1 because it
wasn't removing the sign-extension for pentry as we thought it would.
A more detailed explanation is given in the commit msg of patch 1.

We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
using it when we're running a 32-bit CPU. This worked with 32 bit
'virt' machine booting with the -kernel option.

If this approach doesn't work for the Xvisor use case, IMO  we should
just filter kernel_load_addr bits directly as we were doing a handful of
versions ago.

Patches are based on current riscv-to-apply.next. 

Changes from v9:
- patch 1:
  - removed the translate_fn() callback
  - return 'kernel_low' when running a 32-bit CPU
- v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html

Daniel Henrique Barboza (3):
  hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
  hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
  hw/riscv/boot.c: make riscv_load_initrd() static

 hw/riscv/boot.c            | 96 +++++++++++++++++++++++---------------
 hw/riscv/microchip_pfsoc.c | 12 +----
 hw/riscv/opentitan.c       |  4 +-
 hw/riscv/sifive_e.c        |  4 +-
 hw/riscv/sifive_u.c        | 12 +----
 hw/riscv/spike.c           | 14 ++----
 hw/riscv/virt.c            | 12 +----
 include/hw/riscv/boot.h    |  3 +-
 8 files changed, 76 insertions(+), 81 deletions(-)

-- 
2.39.1
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
Posted by Conor Dooley 1 year, 4 months ago
On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This new version removed the translate_fn() from patch 1 because it
> wasn't removing the sign-extension for pentry as we thought it would.
> A more detailed explanation is given in the commit msg of patch 1.
> 
> We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
> using it when we're running a 32-bit CPU. This worked with 32 bit
> 'virt' machine booting with the -kernel option.
> 
> If this approach doesn't work for the Xvisor use case, IMO  we should
> just filter kernel_load_addr bits directly as we were doing a handful of
> versions ago.
> 
> Patches are based on current riscv-to-apply.next. 
> 
> Changes from v9:
> - patch 1:
>   - removed the translate_fn() callback
>   - return 'kernel_low' when running a 32-bit CPU
> - v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html

I think my T-b got lost last time around, but I gave this version a
whirl too & things are working for me as they were before on Icicle.
For the series:
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> 
> Daniel Henrique Barboza (3):
>   hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
>   hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
>   hw/riscv/boot.c: make riscv_load_initrd() static
> 
>  hw/riscv/boot.c            | 96 +++++++++++++++++++++++---------------
>  hw/riscv/microchip_pfsoc.c | 12 +----
>  hw/riscv/opentitan.c       |  4 +-
>  hw/riscv/sifive_e.c        |  4 +-
>  hw/riscv/sifive_u.c        | 12 +----
>  hw/riscv/spike.c           | 14 ++----
>  hw/riscv/virt.c            | 12 +----
>  include/hw/riscv/boot.h    |  3 +-
>  8 files changed, 76 insertions(+), 81 deletions(-)
> 
> -- 
> 2.39.1
> 
> 
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
Posted by Daniel Henrique Barboza 1 year, 4 months ago

On 2/2/23 14:25, Conor Dooley wrote:
> On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This new version removed the translate_fn() from patch 1 because it
>> wasn't removing the sign-extension for pentry as we thought it would.
>> A more detailed explanation is given in the commit msg of patch 1.
>>
>> We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
>> using it when we're running a 32-bit CPU. This worked with 32 bit
>> 'virt' machine booting with the -kernel option.
>>
>> If this approach doesn't work for the Xvisor use case, IMO  we should
>> just filter kernel_load_addr bits directly as we were doing a handful of
>> versions ago.
>>
>> Patches are based on current riscv-to-apply.next.
>>
>> Changes from v9:
>> - patch 1:
>>    - removed the translate_fn() callback
>>    - return 'kernel_low' when running a 32-bit CPU
>> - v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html
> 
> I think my T-b got lost last time around, but I gave this version a
> whirl too & things are working for me as they were before on Icicle.

That was my bad. I forgot to add the test-by after doing the changes for
the next version.

But I don't think this is the series you're talking about. The tested-by tag
you gave was on these patches:

"[PATCH v5 0/3] riscv_load_fdt() semantics change"

I believe you can add a Tested-by there. And feel free to give it a go - the
patches are on riscv-to-apply.next already.



Daniel


> For the series:
> Tested-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks,
> Conor.
> 
>>
>> Daniel Henrique Barboza (3):
>>    hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
>>    hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
>>    hw/riscv/boot.c: make riscv_load_initrd() static
>>
>>   hw/riscv/boot.c            | 96 +++++++++++++++++++++++---------------
>>   hw/riscv/microchip_pfsoc.c | 12 +----
>>   hw/riscv/opentitan.c       |  4 +-
>>   hw/riscv/sifive_e.c        |  4 +-
>>   hw/riscv/sifive_u.c        | 12 +----
>>   hw/riscv/spike.c           | 14 ++----
>>   hw/riscv/virt.c            | 12 +----
>>   include/hw/riscv/boot.h    |  3 +-
>>   8 files changed, 76 insertions(+), 81 deletions(-)
>>
>> -- 
>> 2.39.1
>>
>>
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
Posted by Conor Dooley 1 year, 4 months ago
On Thu, Feb 02, 2023 at 03:37:17PM -0300, Daniel Henrique Barboza wrote:
> On 2/2/23 14:25, Conor Dooley wrote:
> > On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote:
> > > This new version removed the translate_fn() from patch 1 because it
> > > wasn't removing the sign-extension for pentry as we thought it would.
> > > A more detailed explanation is given in the commit msg of patch 1.
> > > 
> > > We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
> > > using it when we're running a 32-bit CPU. This worked with 32 bit
> > > 'virt' machine booting with the -kernel option.
> > > 
> > > If this approach doesn't work for the Xvisor use case, IMO  we should
> > > just filter kernel_load_addr bits directly as we were doing a handful of
> > > versions ago.
> > > 
> > > Patches are based on current riscv-to-apply.next.
> > > 
> > > Changes from v9:
> > > - patch 1:
> > >    - removed the translate_fn() callback
> > >    - return 'kernel_low' when running a 32-bit CPU
> > > - v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html
> > 
> > I think my T-b got lost last time around, but I gave this version a
> > whirl too & things are working for me as they were before on Icicle.
> 
> That was my bad. I forgot to add the test-by after doing the changes for
> the next version.

Oh, I'm sorry. I saw a new version of the series a few days ago and
noticed the missing tags, and then saw this one today, touching MPFS,
and conflated the two.

> But I don't think this is the series you're talking about. The tested-by tag
> you gave was on these patches:
> 
> "[PATCH v5 0/3] riscv_load_fdt() semantics change"
> 
> I believe you can add a Tested-by there. And feel free to give it a go - the
> patches are on riscv-to-apply.next already.

Tested-by stands here though, I replied to the same message-id that I
shazamed and tried ;)
And I did so on top of the HEAD of riscv-to-apply.next, so I am happy
with the version that got applied too.

Sorry!

Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs
Posted by Daniel Henrique Barboza 1 year, 4 months ago

On 2/2/23 15:46, Conor Dooley wrote:
> On Thu, Feb 02, 2023 at 03:37:17PM -0300, Daniel Henrique Barboza wrote:
>> On 2/2/23 14:25, Conor Dooley wrote:
>>> On Thu, Feb 02, 2023 at 10:58:07AM -0300, Daniel Henrique Barboza wrote:
>>>> This new version removed the translate_fn() from patch 1 because it
>>>> wasn't removing the sign-extension for pentry as we thought it would.
>>>> A more detailed explanation is given in the commit msg of patch 1.
>>>>
>>>> We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
>>>> using it when we're running a 32-bit CPU. This worked with 32 bit
>>>> 'virt' machine booting with the -kernel option.
>>>>
>>>> If this approach doesn't work for the Xvisor use case, IMO  we should
>>>> just filter kernel_load_addr bits directly as we were doing a handful of
>>>> versions ago.
>>>>
>>>> Patches are based on current riscv-to-apply.next.
>>>>
>>>> Changes from v9:
>>>> - patch 1:
>>>>     - removed the translate_fn() callback
>>>>     - return 'kernel_low' when running a 32-bit CPU
>>>> - v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html
>>>
>>> I think my T-b got lost last time around, but I gave this version a
>>> whirl too & things are working for me as they were before on Icicle.
>>
>> That was my bad. I forgot to add the test-by after doing the changes for
>> the next version.
> 
> Oh, I'm sorry. I saw a new version of the series a few days ago and
> noticed the missing tags, and then saw this one today, touching MPFS,
> and conflated the two.
> 
>> But I don't think this is the series you're talking about. The tested-by tag
>> you gave was on these patches:
>>
>> "[PATCH v5 0/3] riscv_load_fdt() semantics change"
>>
>> I believe you can add a Tested-by there. And feel free to give it a go - the
>> patches are on riscv-to-apply.next already.
> 
> Tested-by stands here though, I replied to the same message-id that I
> shazamed and tried ;)

Alright then. Tested-by on both series :D


Thanks!

> And I did so on top of the HEAD of riscv-to-apply.next, so I am happy
> with the version that got applied too.
> 
> Sorry!
>