Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Alistair Francis" <alistair23@gmail.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
	alistair.francis@wdc.com, Bin Meng <bin.meng@windriver.com>,
	Palmer Dabbelt <palmer@dabbelt.com>
Subject: Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
Date: Fri, 13 Jan 2023 07:21:23 -0300	[thread overview]
Message-ID: <0ba72b27-0c3d-2d3d-adec-899717f40594@ventanamicro.com> (raw)
In-Reply-To: <b5cab84f-4bb9-694b-80e9-0b5aeac9de6c@linaro.org>

On 1/13/23 04:16, Philippe Mathieu-Daudé wrote:
> On 12/1/23 01:34, Alistair Francis wrote:
>> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>>> the same steps when '-kernel' is used:
>>>
>>> - execute load_kernel()
>>> - load init_rd()
>>> - write kernel_cmdline
>>>
>>> Let's fold everything inside riscv_load_kernel() to avoid code
>>> repetition. To not change the behavior of boards that aren't calling
>>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>>> allow these boards to opt out from initrd loading.
>>>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>>   hw/riscv/opentitan.c       |  2 +-
>>>   hw/riscv/sifive_e.c        |  3 ++-
>>>   hw/riscv/sifive_u.c        | 12 ++----------
>>>   hw/riscv/spike.c           | 11 +----------
>>>   hw/riscv/virt.c            | 12 ++----------
>>>   include/hw/riscv/boot.h    |  1 +
>>>   8 files changed, 30 insertions(+), 45 deletions(-)
>
>>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>>                            NULL, &kernel_load_base, NULL, NULL, 0,
>>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>>> -        return kernel_load_base;
>>> +        kernel_entry = kernel_load_base;
>>
>> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
>> we get a value of 0xffffffff80000000.
>>
>> Previously the top bits would be lost as we return a target_ulong from
>> this function, but with this change we pass the value
>> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
>>
>> This diff fixes the failure for me
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 4888d5c1e0..f08ed44b97 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>                           NULL, &kernel_load_base, NULL, NULL, 0,
>>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>> -        kernel_entry = kernel_load_base;
>> +        kernel_entry = (target_ulong) kernel_load_base;
>>          goto out;
>>      }
>>
>>
>> but I don't think that's the right fix. We should instead look at the
>> CPU XLEN and drop the high bits if required.
>
> Ah, that is what should be done in load_elf_ram_sym()'s missing
> translate_fn() handler.

Interesting. I'll try it again and re-send.

Daniel