diff mbox series

[RFC,PATCH-for-9.1,13/29] hw/i386/pc: Remove non-PCI code from pc_system_firmware_init()

Message ID 20240328155439.58719-14-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw/i386/pc: Decouple ISA vs PCI-based machines | expand

Commit Message

Philippe Mathieu-Daudé March 28, 2024, 3:54 p.m. UTC 
x86_bios_rom_init() is the single non-PCI-machine call
from pc_system_firmware_init(). Extract it to the caller.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c       | 6 +++++-
 hw/i386/pc_sysfw.c | 5 +----
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

BALATON Zoltan March 28, 2024, 6:50 p.m. UTC | #1 
On Thu, 28 Mar 2024, Philippe Mathieu-Daudé wrote:
> x86_bios_rom_init() is the single non-PCI-machine call
> from pc_system_firmware_init(). Extract it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/i386/pc.c       | 6 +++++-
> hw/i386/pc_sysfw.c | 5 +----
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f184808e3e..5b96daa414 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -956,7 +956,11 @@ void pc_memory_init(PCMachineState *pcms,
>     }
>
>     /* Initialize PC system firmware */
> -    pc_system_firmware_init(pcms, rom_memory);
> +    if (pci_enabled) {
> +        pc_system_firmware_init(pcms, rom_memory);
> +    } else {
> +        x86_bios_rom_init(machine, "bios.bin", rom_memory, true);
> +    }
>
>     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 862a082b0a..541dcaef71 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -202,10 +202,7 @@ void pc_system_firmware_init(PCMachineState *pcms,

Maybe also rename to pc_pci_firmware_init() to make  it clear this is only 
for PCI PC machine now?

Regards,
BALATON Zoltan

>     int i;
>     BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
>
> -    if (!pc_machine_is_pci_enabled(pcms)) {
> -        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, true);
> -        return;
> -    }
> +    assert(pc_machine_is_pci_enabled(pcms));
>
>     /* Map legacy -drive if=pflash to machine properties */
>     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
>
Bernhard Beschow April 6, 2024, 10:35 a.m. UTC | #2 
Am 28. März 2024 15:54:21 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>x86_bios_rom_init() is the single non-PCI-machine call
>from pc_system_firmware_init(). Extract it to the caller.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/i386/pc.c       | 6 +++++-
> hw/i386/pc_sysfw.c | 5 +----
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index f184808e3e..5b96daa414 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -956,7 +956,11 @@ void pc_memory_init(PCMachineState *pcms,
>     }
> 
>     /* Initialize PC system firmware */
>-    pc_system_firmware_init(pcms, rom_memory);
>+    if (pci_enabled) {
>+        pc_system_firmware_init(pcms, rom_memory);
>+    } else {
>+        x86_bios_rom_init(machine, "bios.bin", rom_memory, true);
>+    }
> 
>     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
>diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>index 862a082b0a..541dcaef71 100644
>--- a/hw/i386/pc_sysfw.c
>+++ b/hw/i386/pc_sysfw.c
>@@ -202,10 +202,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
>     int i;
>     BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
> 
>-    if (!pc_machine_is_pci_enabled(pcms)) {
>-        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, true);
>-        return;
>-    }
>+    assert(pc_machine_is_pci_enabled(pcms));

AFAICS nothing refers to pci in the whole file any longer. The only reason for checking pci_enabled before seems for filtering out the x86_bios_rom_init() case. This has been moved to the caller. Can we thus drop the assert? This allows for further removal of code in this patch and avoids superficial barriers for reusing this code. Or do I miss something?

Anyway, this patch looks like good material on its own and could be tagged independently.

With dropping the assert considered:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

> 
>     /* Map legacy -drive if=pflash to machine properties */
>     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
diff mbox series

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f184808e3e..5b96daa414 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -956,7 +956,11 @@  void pc_memory_init(PCMachineState *pcms,
     }
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(pcms, rom_memory);
+    if (pci_enabled) {
+        pc_system_firmware_init(pcms, rom_memory);
+    } else {
+        x86_bios_rom_init(machine, "bios.bin", rom_memory, true);
+    }
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 862a082b0a..541dcaef71 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -202,10 +202,7 @@  void pc_system_firmware_init(PCMachineState *pcms,
     int i;
     BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)];
 
-    if (!pc_machine_is_pci_enabled(pcms)) {
-        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, true);
-        return;
-    }
+    assert(pc_machine_is_pci_enabled(pcms));
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {