Re: [PATCH 02/15] digic: stash firmware into DigicState

[Peter Maydell]

On Mon, 26 Oct 2020 at 14:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Prepare for removing bios_name.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/arm/digic_boards.c  | 5 +++--
>  include/hw/arm/digic.h | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
> index d5524d3e72..d320b54c44 100644
> --- a/hw/arm/digic_boards.c
> +++ b/hw/arm/digic_boards.c
> @@ -55,6 +55,7 @@ static void digic4_board_init(MachineState *machine, 
> DigicBoard *board)
>      DigicState *s = DIGIC(object_new(TYPE_DIGIC));
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>
> +    s->firmware = machine->firmware;
>      if (machine->ram_size != mc->default_ram_size) {
>          char *sz = size_to_str(mc->default_ram_size);
>          error_report("Invalid RAM size, should be %s", sz);
> @@ -91,8 +92,8 @@ static void digic_load_rom(DigicState *s, hwaddr addr,
>          return;
>      }
>
> -    if (bios_name) {
> -        filename = bios_name;
> +    if (s->firmware) {
> +        filename = s->firmware;
>      } else {
>          filename = def_filename;
>      }

The existing code is a little odd, because if the user passes -bios
then we use it in both the add_rom0 and add_rom1 callbacks;
however this ends up not mattering for the moment because the
only supported Digic board has just the rom1 and no rom0.

Anyway, rather than stashing the firmware filename in the
DigicState, you could lift the "decide whether to use
machine->firmware or the def_filename" choice up to
the callsites in digic4_board_init():

    if (board->add_rom0) {
        board->add_rom0(s, DIGIC4_ROM0_BASE,
                        machine->firmware ?: board->rom0_def_filename);
    }
(and similarly for rom1).

Then you can delete the
    if (bios_name) {
        filename = bios_name;
    } else {
        filename = def_filename;
    }
block from digic_load_rom() and rename the arguments of
digic_load_rom() and digic4_add_k8p3215uqb_rom() to just
"filename" rather than "def_filename".

Doing it that way avoids passing things around that we don't
need to, and makes it clear in the digic4_board_init() code
that we're doing something a bit suspect in possibly using
the machine->firmware file twice -- if we ever need to fix
that bug then it'll be a simple change to the logic in that
one function.

thanks
-- PMM