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.