Re: [PATCH v3 07/10] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Alex Bennée]

Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:

> On 12/28/22 12:51, Philippe Mathieu-Daudé wrote:
>> On 28/12/22 14:33, Daniel Henrique Barboza wrote:
>>> 'filename', 'mem_size' and 'fdt' from riscv_load_initrd() can all be
>>> retrieved by the MachineState object for all callers.
>>>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>> ---
>>>   hw/riscv/boot.c            | 6 ++++--
>>>   hw/riscv/microchip_pfsoc.c | 3 +--
>>>   hw/riscv/sifive_u.c        | 3 +--
>>>   hw/riscv/spike.c           | 3 +--
>>>   hw/riscv/virt.c            | 3 +--
>>>   include/hw/riscv/boot.h    | 3 +--
>>>   6 files changed, 9 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>> index d3c71b3f0b..f7e806143a 100644
>>> --- a/hw/riscv/boot.c
>>> +++ b/hw/riscv/boot.c
>>> @@ -204,9 +204,11 @@ target_ulong riscv_load_kernel(const char 
>>> *kernel_filename,
>>>       exit(1);
>>>   }
>>>   -void riscv_load_initrd(const char *filename, uint64_t mem_size,
>>> -                       uint64_t kernel_entry, void *fdt)
>>> +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>>>   {
>>> +    const char *filename = machine->initrd_filename;
>>
>> Do we want to check for missing -initrd here? Or simply return quietly
>> if not provided?
>
> This function is always called after a "if (!machine->initrd_filename)", so 
> we can
> be certain that -initrd will always be present if the function is
> called.

If that is an API guarantee we should assert that is the case then as
calling without machine->initrd_filename would be a bug. 

>
> Perhaps one thing that we could do is to remove this check and fold it inside 
> the
> function, right at the start, to make the function behavior independent of 
> what
> the caller is doing. We could do that at patch 9/10 where we'll end up with a 
> single
> caller instead of 4-5.
>
>
> Daniel
>
>>
>>> +    uint64_t mem_size = machine->ram_size;
>>> +    void *fdt = machine->fdt;
>>>       hwaddr start, end;
>>>       ssize_t size;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro