Re: [PATCH v4 18/18] hw/i386/isapc.c: replace rom_memory with system_memory

[Mark Cave-Ayland <mark.caveayland@nutanix.com>]

On 10/07/2025 12:05, Philippe Mathieu-Daudé wrote:

> On 10/7/25 12:53, Philippe Mathieu-Daudé wrote:
>> On 10/7/25 10:52, Mark Cave-Ayland wrote:
>>> Now that we can guarantee the isapc machine will never have a PCI 
>>> bus, any
>>> instances of rom_memory can be replaced by system_memory and rom_memory
>>> removed completely.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>> ---
>>>   hw/i386/isapc.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/isapc.c b/hw/i386/isapc.c
>>> index bb22083821..27c075b5f3 100644
>>> --- a/hw/i386/isapc.c
>>> +++ b/hw/i386/isapc.c
>>> @@ -35,7 +35,6 @@ static void pc_init_isa(MachineState *machine)
>>>       ISABus *isa_bus;
>>>       GSIState *gsi_state;
>>>       MemoryRegion *ram_memory;
>>> -    MemoryRegion *rom_memory = system_memory;
>>>       DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>>       uint32_t irq;
>>>       int i;
>>> @@ -73,7 +72,7 @@ static void pc_init_isa(MachineState *machine)
>>>       /* allocate ram and load rom/bios */
>>>       if (!xen_enabled()) {
>>> -        pc_memory_init(pcms, system_memory, rom_memory, 0);
>>> +        pc_memory_init(pcms, system_memory, system_memory, 0);
>>
>> I'd prefer just call here:
>>
>>    x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, true);
>>
>> and in pc_system_firmware_init(): assert(pcmc->pci_enabled).
>>
>> WDYT?
> 
> What I have in mind (untested):
> 
> -- >8 --
> diff --git a/hw/i386/isapc.c b/hw/i386/isapc.c
> index 27c075b5f32..a7c2146916c 100644
> --- a/hw/i386/isapc.c
> +++ b/hw/i386/isapc.c
> @@ -74,3 +74,4 @@ static void pc_init_isa(MachineState *machine)
>       if (!xen_enabled()) {
> -        pc_memory_init(pcms, system_memory, system_memory, 0);
> +        pc_memory_init(pcms, system_memory, NULL, 0);
> +        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", system_memory, 
> true);
>       } else {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b2116335752..2952d3ee4ff 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -811,3 +811,3 @@ void pc_memory_init(PCMachineState *pcms,
>                       MemoryRegion *system_memory,
> -                    MemoryRegion *rom_memory,
> +                    MemoryRegion *pci_memory,
>                       uint64_t pci_hole64_size)
> @@ -826,2 +826,3 @@ void pc_memory_init(PCMachineState *pcms,
> 
> +    assert(pcmc->pci_enabled ^ !!pci_memory);
>       assert(machine->ram_size == x86ms->below_4g_mem_size +
> @@ -955,3 +956,5 @@ void pc_memory_init(PCMachineState *pcms,
>       /* Initialize PC system firmware */
> -    pc_system_firmware_init(pcms, rom_memory);
> +    if (pcmc->pci_enabled) {
> +        pc_system_firmware_init(pcms, pci_memory);
> +    }
> 
> @@ -969,3 +972,3 @@ void pc_memory_init(PCMachineState *pcms,
>           }
> -        memory_region_add_subregion_overlap(rom_memory,
> +        memory_region_add_subregion_overlap(pci_memory,
>                                               PC_ROM_MIN_VGA,
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 821396c16e9..0c29e4188fc 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -221,6 +221,3 @@ void pc_system_firmware_init(PCMachineState *pcms,
> 
> -    if (!pcmc->pci_enabled) {
> -        x86_bios_rom_init(X86_MACHINE(pcms), "bios.bin", rom_memory, 
> true);
> -        return;
> -    }
> +    assert(pcmc->pci_enabled);

I think that's a good idea, however the original aim of this series was 
just to do the basic split and tidy-up work (hopefully in time for 10.1).

There is certainly more tidy-up that is possible w.r.t. pc.c, but I 
didn't want to start unraveling that thread right now for fear of this 
series getting too large :/


ATB,

Mark.