| Message ID | 20201026143028.3034018-1-pbonzini@redhat.com |
|---|---|
| Headers | show |
| Series | remove bios_name variable | expand |
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
Patchew URL: https://patchew.org/QEMU/20201026143028.3034018-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201026143028.3034018-1-pbonzini@redhat.com
Subject: [PATCH 00/15] remove bios_name variable
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
288a1cc..e75de83 master -> master
* [new tag] patchew/20201026143028.3034018-1-pbonzini@redhat.com -> patchew/20201026143028.3034018-1-pbonzini@redhat.com
Switched to a new branch 'test'
e6a3975 vl: remove bios_name
ff23ca8 sparc: remove bios_name
4b04e35 sh4: remove bios_name
35de573 s390: remove bios_name
a352220 rx: move BIOS load from MCU to board
83c3173 ppc: remove bios_name
c020272 moxie: remove bios_name
09bcdbc mips: remove bios_name
c618c9e m68k: remove bios_name
4ff17f7 lm32: remove bios_name
c6609bf i386: remove bios_name
2f2d724 hppa: remove bios_name
da68a51 arm: remove bios_name
5687239 digic: stash firmware into DigicState
2fae50e alpha: remove bios_name
=== OUTPUT BEGIN ===
1/15 Checking commit 2fae50e6defc (alpha: remove bios_name)
2/15 Checking commit 56872395809e (digic: stash firmware into DigicState)
3/15 Checking commit da68a516b853 (arm: remove bios_name)
WARNING: line over 80 characters
#37: FILE: hw/arm/highbank.c:299:
+ sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, machine->firmware);
total: 0 errors, 1 warnings, 105 lines checked
Patch 3/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/15 Checking commit 2f2d724e2d12 (hppa: remove bios_name)
WARNING: line over 80 characters
#21: FILE: hw/hppa/machine.c:216:
+ machine->firmware ?: "hppa-firmware.img");
total: 0 errors, 1 warnings, 9 lines checked
Patch 4/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/15 Checking commit c6609bf59a25 (i386: remove bios_name)
WARNING: line over 80 characters
#33: FILE: hw/i386/microvm.c:266:
+ x86_bios_rom_init(MACHINE(mms), default_firmware, get_system_memory(), true);
total: 0 errors, 1 warnings, 75 lines checked
Patch 5/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/15 Checking commit 4ff17f7afa49 (lm32: remove bios_name)
ERROR: spaces required around that ':' (ctx:VxE)
#19: FILE: hw/lm32/milkymist.c:111:
+ const char *bios_name = machine->firmware ? BIOS_FILENAME:
^
total: 1 errors, 0 warnings, 16 lines checked
Patch 6/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/15 Checking commit c618c9eb69d3 (m68k: remove bios_name)
8/15 Checking commit 09bcdbcc744e (mips: remove bios_name)
WARNING: line over 80 characters
#44: FILE: hw/mips/jazz.c:221:
+ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, machine->firmware ?: BIOS_FILENAME);
WARNING: line over 80 characters
#79: FILE: hw/mips/malta.c:1346:
+ error_report("Could not load MIPS bios '%s'", machine->firmware);
WARNING: line over 80 characters
#92: FILE: hw/mips/mipssim.c:180:
+ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, machine->firmware ?: BIOS_FILENAME);
total: 0 errors, 3 warnings, 89 lines checked
Patch 8/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/15 Checking commit c0202725c5bf (moxie: remove bios_name)
WARNING: line over 80 characters
#22: FILE: hw/moxie/moxiesim.c:137:
+ if (load_image_targphys(machine->firmware, FIRMWARE_BASE, FIRMWARE_SIZE) < 0) {
total: 0 errors, 1 warnings, 12 lines checked
Patch 9/15 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/15 Checking commit 83c31730c9bd (ppc: remove bios_name)
11/15 Checking commit a3522204200e (rx: move BIOS load from MCU to board)
12/15 Checking commit 35de573a9382 (s390: remove bios_name)
13/15 Checking commit 4b04e35c1605 (sh4: remove bios_name)
14/15 Checking commit ff23ca862682 (sparc: remove bios_name)
15/15 Checking commit e6a39752a393 (vl: remove bios_name)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20201026143028.3034018-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 26/10/20 15:44, Peter Maydell wrote: > 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. Much better indeed, thanks! Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: Michael Walle <michael@walle.cc> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes:
Might be worth mentioning the trixy path from qdev_prop_set_string to
end up in ipl->firmware because it's not obvious just from the diff.
Anyway:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> <snip> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Paolo Bonzini <pbonzini@redhat.com> writes: > bios_name was a legacy variable used by machine code, but it is > no more. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> well it all built which is as far as I've tested it: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On 10/26/20 3:30 PM, Paolo Bonzini wrote: > Cc: Michael Walle <michael@walle.cc> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/lm32/milkymist.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c > index 9f8fe9fef1..c5d3d77a2b 100644 > --- a/hw/lm32/milkymist.c > +++ b/hw/lm32/milkymist.c > @@ -108,6 +108,7 @@ static void > milkymist_init(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > + const char *bios_name = machine->firmware ? BIOS_FILENAME: Does that build? > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > @@ -162,9 +163,6 @@ milkymist_init(MachineState *machine) > } > > /* load bios rom */ > - if (bios_name == NULL) { > - bios_name = BIOS_FILENAME; > - } > bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > > if (bios_filename) { >
On 10/26/20 3:30 PM, Paolo Bonzini wrote: > Cc: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/m68k/mcf5208.c | 10 +++++----- > hw/m68k/next-cube.c | 4 +--- > hw/m68k/q800.c | 4 +--- > 3 files changed, 7 insertions(+), 11 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 26/10/2020 15.30, Paolo Bonzini wrote: > Cc: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/m68k/mcf5208.c | 10 +++++----- > hw/m68k/next-cube.c | 4 +--- > hw/m68k/q800.c | 4 +--- > 3 files changed, 7 insertions(+), 11 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 10/26/20 3:30 PM, Paolo Bonzini wrote: > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/sh4/shix.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 10/26/20 3:30 PM, Paolo Bonzini wrote: > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/sparc/leon3.c | 4 +--- > hw/sparc/sun4m.c | 2 +- > hw/sparc64/sun4u.c | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 26/10/2020 15.30, Paolo Bonzini wrote: > Cc: Thomas Huth <thuth@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/s390x/ipl.c | 8 ++------ > hw/s390x/s390-virtio-ccw.c | 3 ++- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 3d2652d75a..61e8c967d3 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -128,11 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > if (!ipl->kernel || ipl->enforce_bios) { > uint64_t fwbase = (MIN(ram_size, 0x80000000U) - 0x200000) & ~0xffffUL; > > - if (bios_name == NULL) { > - bios_name = ipl->firmware; > - } > - > - bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > + bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->firmware); > if (bios_filename == NULL) { > error_setg(errp, "could not find stage1 bootloader"); > return; > @@ -154,7 +150,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > g_free(bios_filename); > > if (bios_size == -1) { > - error_setg(errp, "could not load bootloader '%s'", bios_name); > + error_setg(errp, "could not load bootloader '%s'", ipl->firmware); > return; > } > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e52182f946..a521eba673 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -258,7 +258,8 @@ static void ccw_init(MachineState *machine) > /* get a BUS */ > css_bus = virtual_css_bus_init(); > s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, > - machine->initrd_filename, "s390-ccw.img", > + machine->initrd_filename, > + machine->firmware ?: "s390-ccw.img", > "s390-netboot.img", true); > > dev = qdev_new(TYPE_S390_PCI_HOST_BRIDGE); > Reviewed-by: Thomas Huth <thuth@redhat.com>
Le 26/10/2020 à 15:30, Paolo Bonzini a écrit : > Cc: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/m68k/mcf5208.c | 10 +++++----- > hw/m68k/next-cube.c | 4 +--- > hw/m68k/q800.c | 4 +--- > 3 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c > index d310a98e7b..7c8ca5ddf6 100644 > --- a/hw/m68k/mcf5208.c > +++ b/hw/m68k/mcf5208.c > @@ -301,17 +301,17 @@ static void mcf5208evb_init(MachineState *machine) > /* 0xfc0a8000 SDRAM controller. */ > > /* Load firmware */ > - if (bios_name) { > + if (machine->firmware) { > char *fn; > uint8_t *ptr; > > - fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > + fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, machine->firmware); > if (!fn) { > - error_report("Could not find ROM image '%s'", bios_name); > + error_report("Could not find ROM image '%s'", machine->firmware); > exit(1); > } > if (load_image_targphys(fn, 0x0, ROM_SIZE) < 8) { > - error_report("Could not load ROM image '%s'", bios_name); > + error_report("Could not load ROM image '%s'", machine->firmware); > exit(1); > } > g_free(fn); > @@ -323,7 +323,7 @@ static void mcf5208evb_init(MachineState *machine) > > /* Load kernel. */ > if (!kernel_filename) { > - if (qtest_enabled() || bios_name) { > + if (qtest_enabled() || machine->firmware) { > return; > } > error_report("Kernel image must be specified"); Why do you do differently for mcf5208 than the others? const char *bios_name = machine->firmware; and no other changes? With or without this: Acked-by: Laurent Vivier <laurent@vivier.eu> > diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c > index e7045980b7..37bc35dfa4 100644 > --- a/hw/m68k/next-cube.c > +++ b/hw/m68k/next-cube.c > @@ -868,6 +868,7 @@ static void next_cube_init(MachineState *machine) > MemoryRegion *bmapm1 = g_new(MemoryRegion, 1); > MemoryRegion *bmapm2 = g_new(MemoryRegion, 1); > MemoryRegion *sysmem = get_system_memory(); > + const char *bios_name = machine->firmware ?: ROM_FILE; > NeXTState *ns = NEXT_MACHINE(machine); > DeviceState *dev; > > @@ -924,9 +925,6 @@ static void next_cube_init(MachineState *machine) > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x0200e000); > > /* Load ROM here */ > - if (bios_name == NULL) { > - bios_name = ROM_FILE; > - } > /* still not sure if the rom should also be mapped at 0x0*/ > memory_region_init_rom(rom, NULL, "next.rom", 0x20000, &error_fatal); > memory_region_add_subregion(sysmem, 0x01000000, rom); > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index ce4b47c3e3..6ebcddcfb2 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -167,6 +167,7 @@ static void q800_init(MachineState *machine) > const char *kernel_filename = machine->kernel_filename; > const char *initrd_filename = machine->initrd_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > + const char *bios_name = machine->firmware ?: MACROM_FILENAME; > hwaddr parameters_base; > CPUState *cs; > DeviceState *dev; > @@ -400,9 +401,6 @@ static void q800_init(MachineState *machine) > rom = g_malloc(sizeof(*rom)); > memory_region_init_rom(rom, NULL, "m68k_mac.rom", MACROM_SIZE, > &error_abort); > - if (bios_name == NULL) { > - bios_name = MACROM_FILENAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > memory_region_add_subregion(get_system_memory(), MACROM_ADDR, rom); > >
On Mon, Oct 26, 2020 at 10:30:23AM -0400, Paolo Bonzini wrote: > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/e500.c | 4 ++-- > hw/ppc/mac_newworld.c | 4 +--- > hw/ppc/mac_oldworld.c | 4 +--- > hw/ppc/pnv.c | 5 +---- > hw/ppc/ppc405_boards.c | 6 ++---- > hw/ppc/prep.c | 4 +--- > hw/ppc/spapr.c | 4 +--- > 7 files changed, 9 insertions(+), 22 deletions(-) > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index ae39b9358e..153a74c98c 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -1035,7 +1035,7 @@ void ppce500_init(MachineState *machine) > * -kernel to users but allows them to run through u-boot as well. > */ > kernel_as_payload = false; > - if (bios_name == NULL) { > + if (machine->firmware == NULL) { > if (machine->kernel_filename) { > payload_name = machine->kernel_filename; > kernel_as_payload = true; > @@ -1043,7 +1043,7 @@ void ppce500_init(MachineState *machine) > payload_name = "u-boot.e500"; > } > } else { > - payload_name = bios_name; > + payload_name = machine->firmware; > } > > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name); > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c > index f9a1cc8944..61c63819df 100644 > --- a/hw/ppc/mac_newworld.c > +++ b/hw/ppc/mac_newworld.c > @@ -109,6 +109,7 @@ static void ppc_core99_reset(void *opaque) > static void ppc_core99_init(MachineState *machine) > { > ram_addr_t ram_size = machine->ram_size; > + const char *bios_name = machine->firmware ?: PROM_FILENAME; > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > @@ -161,9 +162,6 @@ static void ppc_core99_init(MachineState *machine) > &error_fatal); > memory_region_add_subregion(get_system_memory(), PROM_BASE, bios); > > - if (!bios_name) { > - bios_name = PROM_FILENAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (filename) { > /* Load OpenBIOS (ELF) */ > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c > index 6c59aa5601..11623e8e67 100644 > --- a/hw/ppc/mac_oldworld.c > +++ b/hw/ppc/mac_oldworld.c > @@ -83,6 +83,7 @@ static void ppc_heathrow_reset(void *opaque) > static void ppc_heathrow_init(MachineState *machine) > { > ram_addr_t ram_size = machine->ram_size; > + const char *bios_name = machine->firmware ?: PROM_FILENAME; > const char *boot_device = machine->boot_order; > PowerPCCPU *cpu = NULL; > CPUPPCState *env = NULL; > @@ -130,9 +131,6 @@ static void ppc_heathrow_init(MachineState *machine) > &error_fatal); > memory_region_add_subregion(get_system_memory(), PROM_BASE, bios); > > - if (!bios_name) { > - bios_name = PROM_FILENAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (filename) { > /* Load OpenBIOS (ELF) */ > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index d9e52873ea..f2b1ee83d3 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -713,6 +713,7 @@ static void pnv_chip_power10_pic_print_info(PnvChip *chip, Monitor *mon) > > static void pnv_init(MachineState *machine) > { > + const char *bios_name = machine->firmware ?: FW_FILE_NAME; > PnvMachineState *pnv = PNV_MACHINE(machine); > MachineClass *mc = MACHINE_GET_CLASS(machine); > char *fw_filename; > @@ -739,10 +740,6 @@ static void pnv_init(MachineState *machine) > pnv->pnor = PNV_PNOR(dev); > > /* load skiboot firmware */ > - if (bios_name == NULL) { > - bios_name = FW_FILE_NAME; > - } > - > fw_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (!fw_filename) { > error_report("Could not find OPAL firmware '%s'", bios_name); > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c > index 4687715b15..c867e46330 100644 > --- a/hw/ppc/ppc405_boards.c > +++ b/hw/ppc/ppc405_boards.c > @@ -141,6 +141,7 @@ static void ref405ep_fpga_init(MemoryRegion *sysmem, uint32_t base) > static void ref405ep_init(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > + const char *bios_name = machine->firmware ?: BIOS_FILENAME; > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > @@ -206,8 +207,6 @@ static void ref405ep_init(MachineState *machine) > memory_region_init_rom(bios, NULL, "ef405ep.bios", BIOS_SIZE, > &error_fatal); > > - if (bios_name == NULL) > - bios_name = BIOS_FILENAME; > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (filename) { > bios_size = load_image_size(filename, > @@ -425,6 +424,7 @@ static void taihu_cpld_init(MemoryRegion *sysmem, uint32_t base) > static void taihu_405ep_init(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > + const char *bios_name = machine->firmware ?: BIOS_FILENAME; > const char *kernel_filename = machine->kernel_filename; > const char *initrd_filename = machine->initrd_filename; > char *filename; > @@ -475,8 +475,6 @@ static void taihu_405ep_init(MachineState *machine) > } else > #endif > { > - if (bios_name == NULL) > - bios_name = BIOS_FILENAME; > bios = g_new(MemoryRegion, 1); > memory_region_init_rom(bios, NULL, "taihu_405ep.bios", BIOS_SIZE, > &error_fatal); > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c > index 4a0cb434a6..c6b9d1ddcb 100644 > --- a/hw/ppc/prep.c > +++ b/hw/ppc/prep.c > @@ -237,6 +237,7 @@ static int prep_set_cmos_checksum(DeviceState *dev, void *opaque) > > static void ibm_40p_init(MachineState *machine) > { > + const char *bios_name = machine->firmware ?: "openbios-ppc"; > CPUPPCState *env = NULL; > uint16_t cmos_checksum; > PowerPCCPU *cpu; > @@ -271,9 +272,6 @@ static void ibm_40p_init(MachineState *machine) > > /* PCI host */ > dev = qdev_new("raven-pcihost"); > - if (!bios_name) { > - bios_name = "openbios-ppc"; > - } > qdev_prop_set_string(dev, "bios-name", bios_name); > qdev_prop_set_uint32(dev, "elf-machine", PPC_ELF_MACHINE); > pcihost = SYS_BUS_DEVICE(dev); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 63315f2d0f..667d59e5ad 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2647,6 +2647,7 @@ static void spapr_machine_init(MachineState *machine) > SpaprMachineState *spapr = SPAPR_MACHINE(machine); > SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); > MachineClass *mc = MACHINE_GET_CLASS(machine); > + const char *bios_name = machine->firmware ?: FW_FILE_NAME; > const char *kernel_filename = machine->kernel_filename; > const char *initrd_filename = machine->initrd_filename; > PCIHostState *phb; > @@ -2970,9 +2971,6 @@ static void spapr_machine_init(MachineState *machine) > } > } > > - if (bios_name == NULL) { > - bios_name = FW_FILE_NAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (!filename) { > error_report("Could not find LPAR firmware '%s'", bios_name); -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Mon, 26 Oct 2020 10:30:25 -0400 Paolo Bonzini <pbonzini@redhat.com> wrote: > Cc: Thomas Huth <thuth@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/s390x/ipl.c | 8 ++------ > hw/s390x/s390-virtio-ccw.c | 3 ++- > 2 files changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 26.10.20 15:30, Paolo Bonzini wrote: > Cc: Thomas Huth <thuth@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/s390x/ipl.c | 8 ++------ > hw/s390x/s390-virtio-ccw.c | 3 ++- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 3d2652d75a..61e8c967d3 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -128,11 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > if (!ipl->kernel || ipl->enforce_bios) { > uint64_t fwbase = (MIN(ram_size, 0x80000000U) - 0x200000) & ~0xffffUL; > > - if (bios_name == NULL) { > - bios_name = ipl->firmware; > - } > - > - bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > + bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->firmware); > if (bios_filename == NULL) { > error_setg(errp, "could not find stage1 bootloader"); > return; > @@ -154,7 +150,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > g_free(bios_filename); > > if (bios_size == -1) { > - error_setg(errp, "could not load bootloader '%s'", bios_name); > + error_setg(errp, "could not load bootloader '%s'", ipl->firmware); > return; > } > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e52182f946..a521eba673 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -258,7 +258,8 @@ static void ccw_init(MachineState *machine) > /* get a BUS */ > css_bus = virtual_css_bus_init(); > s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, > - machine->initrd_filename, "s390-ccw.img", > + machine->initrd_filename, > + machine->firmware ?: "s390-ccw.img", Adding the elvis operator is actually a fix, no?
On 27/10/20 09:38, Christian Borntraeger wrote: >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index e52182f946..a521eba673 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -258,7 +258,8 @@ static void ccw_init(MachineState *machine) >> /* get a BUS */ >> css_bus = virtual_css_bus_init(); >> s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline, >> - machine->initrd_filename, "s390-ccw.img", >> + machine->initrd_filename, >> + machine->firmware ?: "s390-ccw.img", > Adding the elvis operator is actually a fix, no? > I think it was already doing the equivalent here in s390_ipl_realize if (bios_name == NULL) { bios_name = ipl->firmware; } bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); so it was just an encapsulation violation on part of the IPL device. Paolo
On 26/10/20 20:11, Laurent Vivier wrote: >> /* Load kernel. */ >> if (!kernel_filename) { >> - if (qtest_enabled() || bios_name) { >> + if (qtest_enabled() || machine->firmware) { >> return; >> } >> error_report("Kernel image must be specified"); > Why do you do differently for mcf5208 than the others? > > const char *bios_name = machine->firmware; > > and no other changes? because in this case you can use machine->firmware, I'm keeping bios_name for the cases where machine->firmware cannot be used directly (e.g. there is a default). Paolo
On 26/10/2020 15.30, Paolo Bonzini wrote: > bios_name was a legacy variable used by machine code, but it is > no more. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > include/sysemu/sysemu.h | 1 - > softmmu/vl.c | 2 -- > 2 files changed, 3 deletions(-) > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 817ff4cf75..1336b4264a 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -8,7 +8,6 @@ > > /* vl.c */ > > -extern const char *bios_name; > extern int only_migratable; > extern const char *qemu_name; > extern QemuUUID qemu_uuid; > diff --git a/softmmu/vl.c b/softmmu/vl.c > index b7d7f43c88..7909709879 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -119,7 +119,6 @@ > > static const char *data_dir[16]; > static int data_dir_idx; > -const char *bios_name = NULL; > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > int display_opengl; > const char* keyboard_layout = NULL; > @@ -4205,7 +4204,6 @@ void qemu_init(int argc, char **argv, char **envp) > kernel_filename = qemu_opt_get(machine_opts, "kernel"); > initrd_filename = qemu_opt_get(machine_opts, "initrd"); > kernel_cmdline = qemu_opt_get(machine_opts, "append"); > - bios_name = qemu_opt_get(machine_opts, "firmware"); > > opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL); > if (opts) { > Reviewed-by: Thomas Huth <thuth@redhat.com>
On 10/26/20 3:30 PM, Paolo Bonzini wrote: > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/mips/fuloong2e.c | 6 +++--- > hw/mips/jazz.c | 6 +++--- > hw/mips/malta.c | 6 +++--- > hw/mips/mipssim.c | 6 +++--- > hw/mips/r4k.c | 4 +--- > 5 files changed, 13 insertions(+), 15 deletions(-) ... > diff --git a/hw/mips/r4k.c b/hw/mips/r4k.c > index 3830854342..b27be138a4 100644 > --- a/hw/mips/r4k.c > +++ b/hw/mips/r4k.c > @@ -168,6 +168,7 @@ static const int sector_len = 32 * KiB; > static > void mips_r4k_init(MachineState *machine) > { > + const char *bios_name = machine->firmware ?: BIOS_FILENAME; Don't we have a "redefinition of global variable" warning here? Anyway, Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > const char *kernel_filename = machine->kernel_filename; > const char *kernel_cmdline = machine->kernel_cmdline; > const char *initrd_filename = machine->initrd_filename; > @@ -221,9 +222,6 @@ void mips_r4k_init(MachineState *machine) > * run. > */ > > - if (bios_name == NULL) { > - bios_name = BIOS_FILENAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > if (filename) { > bios_size = get_image_size(filename); >
On 26/10/2020 14:30, Paolo Bonzini wrote: > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/sparc/leon3.c | 4 +--- > hw/sparc/sun4m.c | 2 +- > hw/sparc64/sun4u.c | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c > index d40b7891f6..1c50b02f81 100644 > --- a/hw/sparc/leon3.c > +++ b/hw/sparc/leon3.c > @@ -185,6 +185,7 @@ static void leon3_set_pil_in(void *opaque, int n, int level) > static void leon3_generic_hw_init(MachineState *machine) > { > ram_addr_t ram_size = machine->ram_size; > + const char *bios_name = machine->firmware ?: LEON3_PROM_FILENAME; > const char *kernel_filename = machine->kernel_filename; > SPARCCPU *cpu; > CPUSPARCState *env; > @@ -259,9 +260,6 @@ static void leon3_generic_hw_init(MachineState *machine) > memory_region_add_subregion(address_space_mem, LEON3_PROM_OFFSET, prom); > > /* Load boot prom */ > - if (bios_name == NULL) { > - bios_name = LEON3_PROM_FILENAME; > - } > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > > if (filename) { > diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c > index 38d1e0fd12..81d4ae9385 100644 > --- a/hw/sparc/sun4m.c > +++ b/hw/sparc/sun4m.c > @@ -878,7 +878,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, > hwdef->max_mem - machine->ram_size); > } > > - prom_init(hwdef->slavio_base, bios_name); > + prom_init(hwdef->slavio_base, machine->firmware); > > slavio_intctl = slavio_intctl_init(hwdef->intctl_base, > hwdef->intctl_base + 0x10000ULL, > diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c > index 05e659c8a4..50230d261a 100644 > --- a/hw/sparc64/sun4u.c > +++ b/hw/sparc64/sun4u.c > @@ -578,7 +578,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, > /* set up devices */ > ram_init(0, machine->ram_size); > > - prom_init(hwdef->prom_addr, bios_name); > + prom_init(hwdef->prom_addr, machine->firmware); > > /* Init sabre (PCI host bridge) */ > sabre = SABRE(qdev_new(TYPE_SABRE)); Looks good to me: Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.