[PATCH v6 00/19] hw/i386: separate isapc out from pc_piix

[PATCH v6 00/19] hw/i386: separate isapc out from pc_piix

[Mark Cave-Ayland]

For various historical reasons the initialisation of the isapc machine is closely
intertwined with the initialisation of the pc machine, which is preventing some
future improvements to the pc machine initialisation logic.

Since the consensus [1] was that the isapc is still useful for testing and running
older OSs, this series splits the isapc machine and its main initialisation
routine pc_init_isa() into a separate isapc.c file to reduce the maintenance
burden on pc machine developers.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>

[1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg06137.html

(Patches still needing review: 9)

v6:
- Rebase onto master
- Add R-B/S-o-B tags from Phil
- Specify isapc max_ram_below_4g as 4GB as suggested by Phil

v5:
- Add warn_report() to patch 1 as requested by Phil
- Add patch 3 to inline pc_xen_hvm_init_pci() as suggested by Phil
- Add R-B tags from Phil

v4:
- Rebase onto master
- Add patch 2 to remove unused loader.h header as suggested by Bernhard
- Add patches 16-18 to tidy up the pc_piix headers and the pc_piix/isapc
  rom_memory variable after the isapc split also suggested by Bernhard

v3:
- Rebase onto master
- Add patch 1 to restrict isapc machine to 32-bit x86 CPUs as suggested by
  Philippe
- Include logic in patch 1 to handle the case if an isapc machine is
  launched with -cpu max as suggested by Daniel
- Add patch 13 to tidy-up pc_init1() for the i440fx-pc machine in the same
  way as patch 11 does for the isapc machine as suggested by Bernhard

v2:
- Rebase onto master to account for the fix in commit 0b006153b7
  ("hw/i386/pc_piix: Fix RTC ISA IRQ wiring of isapc machine")
- Replace verbatim MIT licence text with SPDX identifier as discussed
  with Daniel


Mark Cave-Ayland (19):
  hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs
  hw/i386/pc_piix.c: remove include for loader.h
  hw/i386/pc_piix.c: inline pc_xen_hvm_init_pci() into pc_xen_hvm_init()
  hw/i386/pc_piix.c: duplicate pc_init1() into pc_isa_init()
  hw/i386/pc_piix.c: remove pcmc->pci_enabled dependent initialisation
    from pc_init_isa()
  hw/i386/pc_piix.c: remove SMI and piix4_pm initialisation from
    pc_init_isa()
  hw/i386/pc_piix.c: remove SGX initialisation from pc_init_isa()
  hw/i386/pc_piix.c: remove nvdimm initialisation from pc_init_isa()
  hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()
  hw/i386/pc_piix.c: hardcode hole64_size to 0 in pc_init_isa()
  hw/i386/pc_piix.c: remove pc_system_flash_cleanup_unused() from
    pc_init_isa()
  hw/i386/pc_piix.c: always initialise ISA IDE drives in pc_init_isa()
  hw/i386/pc_piix.c: assume pcmc->pci_enabled is always false in
    pc_init_isa()
  hw/i386/pc_piix.c: hardcode pcms->pci_bus to NULL in pc_init_isa()
  hw/i386/pc_piix.c: assume pcmc->pci_enabled is always true in
    pc_init1()
  hw/i386: move isapc machine to separate isapc.c file
  hw/i386/pc_piix.c: remove unused headers after isapc machine split
  hw/i386/pc_piix.c: replace rom_memory with pci_memory
  hw/i386/isapc.c: replace rom_memory with system_memory

 hw/i386/Kconfig     |   3 -
 hw/i386/isapc.c     | 170 ++++++++++++++++++++++++++++++
 hw/i386/meson.build |   1 +
 hw/i386/pc_piix.c   | 250 ++++++++++++++------------------------------
 4 files changed, 251 insertions(+), 173 deletions(-)
 create mode 100644 hw/i386/isapc.c

-- 
2.43.0

[PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Mark Cave-Ayland]

The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
possible to specify any CPU via -cpu on the command line, it makes no
sense to allow modern 64-bit CPUs to be used.

Restrict the isapc machine to the available 32-bit CPUs, taking care to
handle the case where if a user inadvertently uses -cpu max then the "best"
32-bit CPU is used (in this case the pentium3).

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c03324281b..5720b6b556 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
 #ifdef CONFIG_ISAPC
 static void pc_init_isa(MachineState *machine)
 {
+    /*
+     * There is a small chance that someone unintentionally passes "-cpu max"
+     * for the isapc machine, which will provide a much more modern 32-bit
+     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
+     * been specified, choose the "best" 32-bit cpu possible which we consider
+     * be the pentium3 (deliberately choosing an Intel CPU given that the
+     * default 486 CPU for the isapc machine is also an Intel CPU).
+     */
+    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
+        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
+        warn_report("-cpu max is invalid for isapc machine, using pentium3");
+    }
+
     pc_init1(machine, NULL);
 }
 #endif
@@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
 #ifdef CONFIG_ISAPC
 static void isapc_machine_options(MachineClass *m)
 {
+    static const char * const valid_cpu_types[] = {
+        X86_CPU_TYPE_NAME("486"),
+        X86_CPU_TYPE_NAME("athlon"),
+        X86_CPU_TYPE_NAME("kvm32"),
+        X86_CPU_TYPE_NAME("pentium"),
+        X86_CPU_TYPE_NAME("pentium2"),
+        X86_CPU_TYPE_NAME("pentium3"),
+        X86_CPU_TYPE_NAME("qemu32"),
+        X86_CPU_TYPE_NAME("max"),
+        NULL
+    };
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     m->desc = "ISA-only PC";
     m->max_cpus = 1;
     m->option_rom_has_mr = true;
@@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
     pcmc->has_reserved_memory = false;
     m->default_nic = "ne2k_isa";
     m->default_cpu_type = X86_CPU_TYPE_NAME("486");
+    m->valid_cpu_types = valid_cpu_types;
     m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
     m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
 }
-- 
2.43.0

[PATCH v6 02/19] hw/i386/pc_piix.c: remove include for loader.h

[Mark Cave-Ayland]

This header is not required since the loader functionality is handled separately
by pc_memory_init() in pc.c.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5720b6b556..bc8fbe46a1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -28,7 +28,6 @@
 #include "qemu/units.h"
 #include "hw/char/parallel-isa.h"
 #include "hw/dma/i8257.h"
-#include "hw/loader.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
-- 
2.43.0

[PATCH v6 03/19] hw/i386/pc_piix.c: inline pc_xen_hvm_init_pci() into pc_xen_hvm_init()

[Mark Cave-Ayland]

This helps to simplify the initialisation of the Xen hvm machine.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
---
 hw/i386/pc_piix.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bc8fbe46a1..071bf7412e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -453,14 +453,6 @@ static void pc_init_isa(MachineState *machine)
 #endif
 
 #ifdef CONFIG_XEN
-static void pc_xen_hvm_init_pci(MachineState *machine)
-{
-    const char *pci_type = xen_igd_gfx_pt_enabled() ?
-                TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : TYPE_I440FX_PCI_DEVICE;
-
-    pc_init1(machine, pci_type);
-}
-
 static void pc_xen_hvm_init(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
@@ -470,7 +462,10 @@ static void pc_xen_hvm_init(MachineState *machine)
         exit(1);
     }
 
-    pc_xen_hvm_init_pci(machine);
+    pc_init1(machine, xen_igd_gfx_pt_enabled()
+                      ? TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE
+                      : TYPE_I440FX_PCI_DEVICE);
+
     xen_igd_reserve_slot(pcms->pcibus);
     pci_create_simple(pcms->pcibus, -1, "xen-platform");
 }
-- 
2.43.0

[PATCH v6 04/19] hw/i386/pc_piix.c: duplicate pc_init1() into pc_isa_init()

[Mark Cave-Ayland]

This is to prepare for splitting the isapc machine into its own separate file.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 261 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 260 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 071bf7412e..5b97ad2cea 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,6 +435,87 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
 #ifdef CONFIG_ISAPC
 static void pc_init_isa(MachineState *machine)
 {
+    const char *pci_type = NULL;
+    PCMachineState *pcms = PC_MACHINE(machine);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    X86MachineState *x86ms = X86_MACHINE(machine);
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    Object *phb = NULL;
+    ISABus *isa_bus;
+    Object *piix4_pm = NULL;
+    qemu_irq smi_irq;
+    GSIState *gsi_state;
+    MemoryRegion *ram_memory;
+    MemoryRegion *pci_memory = NULL;
+    MemoryRegion *rom_memory = system_memory;
+    ram_addr_t lowmem;
+    uint64_t hole64_size = 0;
+
+    /*
+     * Calculate ram split, for memory below and above 4G.  It's a bit
+     * complicated for backward compatibility reasons ...
+     *
+     *  - Traditional split is 3.5G (lowmem = 0xe0000000).  This is the
+     *    default value for max_ram_below_4g now.
+     *
+     *  - Then, to gigabyte align the memory, we move the split to 3G
+     *    (lowmem = 0xc0000000).  But only in case we have to split in
+     *    the first place, i.e. ram_size is larger than (traditional)
+     *    lowmem.  And for new machine types (gigabyte_align = true)
+     *    only, for live migration compatibility reasons.
+     *
+     *  - Next the max-ram-below-4g option was added, which allowed to
+     *    reduce lowmem to a smaller value, to allow a larger PCI I/O
+     *    window below 4G.  qemu doesn't enforce gigabyte alignment here,
+     *    but prints a warning.
+     *
+     *  - Finally max-ram-below-4g got updated to also allow raising lowmem,
+     *    so legacy non-PAE guests can get as much memory as possible in
+     *    the 32bit address space below 4G.
+     *
+     *  - Note that Xen has its own ram setup code in xen_ram_init(),
+     *    called via xen_hvm_init_pc().
+     *
+     * Examples:
+     *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  512M high
+     *    qemu -M pc -m 4G        (new default)    -> 3072M low, 1024M high
+     *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 2048M high
+     *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
+     */
+    if (xen_enabled()) {
+        xen_hvm_init_pc(pcms, &ram_memory);
+    } else {
+        ram_memory = machine->ram;
+        if (!pcms->max_ram_below_4g) {
+            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
+        }
+        lowmem = pcms->max_ram_below_4g;
+        if (machine->ram_size >= pcms->max_ram_below_4g) {
+            if (pcmc->gigabyte_align) {
+                if (lowmem > 0xc0000000) {
+                    lowmem = 0xc0000000;
+                }
+                if (lowmem & (1 * GiB - 1)) {
+                    warn_report("Large machine and max_ram_below_4g "
+                                "(%" PRIu64 ") not a multiple of 1G; "
+                                "possible bad performance.",
+                                pcms->max_ram_below_4g);
+                }
+            }
+        }
+
+        if (machine->ram_size >= lowmem) {
+            x86ms->above_4g_mem_size = machine->ram_size - lowmem;
+            x86ms->below_4g_mem_size = lowmem;
+        } else {
+            x86ms->above_4g_mem_size = 0;
+            x86ms->below_4g_mem_size = machine->ram_size;
+        }
+    }
+
+    pc_machine_init_sgx_epc(pcms);
+
     /*
      * There is a small chance that someone unintentionally passes "-cpu max"
      * for the isapc machine, which will provide a much more modern 32-bit
@@ -448,7 +529,185 @@ static void pc_init_isa(MachineState *machine)
         warn_report("-cpu max is invalid for isapc machine, using pentium3");
     }
 
-    pc_init1(machine, NULL);
+    x86_cpus_init(x86ms, pcmc->default_cpu_version);
+
+    if (kvm_enabled()) {
+        kvmclock_create(pcmc->kvmclock_create_always);
+    }
+
+    if (pcmc->pci_enabled) {
+        pci_memory = g_new(MemoryRegion, 1);
+        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
+        rom_memory = pci_memory;
+
+        phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
+        object_property_add_child(OBJECT(machine), "i440fx", phb);
+        object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
+                                 OBJECT(ram_memory), &error_fatal);
+        object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
+                                 OBJECT(pci_memory), &error_fatal);
+        object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
+                                 OBJECT(system_memory), &error_fatal);
+        object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
+                                 OBJECT(system_io), &error_fatal);
+        object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
+                                 x86ms->below_4g_mem_size, &error_fatal);
+        object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
+                                 x86ms->above_4g_mem_size, &error_fatal);
+        object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
+                                &error_fatal);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
+
+        pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
+        pci_bus_map_irqs(pcms->pcibus,
+                         xen_enabled() ? xen_pci_slot_get_pirq
+                                       : pc_pci_slot_get_pirq);
+
+        hole64_size = object_property_get_uint(phb,
+                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
+                                               &error_abort);
+    }
+
+    /* allocate ram and load rom/bios */
+    if (!xen_enabled()) {
+        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
+    } else {
+        assert(machine->ram_size == x86ms->below_4g_mem_size +
+                                    x86ms->above_4g_mem_size);
+
+        pc_system_flash_cleanup_unused(pcms);
+        if (machine->kernel_filename != NULL) {
+            /* For xen HVM direct kernel boot, load linux here */
+            xen_load_linux(pcms);
+        }
+    }
+
+    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
+
+    if (pcmc->pci_enabled) {
+        PCIDevice *pci_dev;
+        DeviceState *dev;
+        size_t i;
+
+        pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
+        object_property_set_bool(OBJECT(pci_dev), "has-usb",
+                                 machine_usb(machine), &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
+                                 x86_machine_is_acpi_enabled(x86ms),
+                                 &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
+                                 &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
+                                 &error_abort);
+        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
+        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
+                                 x86_machine_is_smm_enabled(x86ms),
+                                 &error_abort);
+        dev = DEVICE(pci_dev);
+        for (i = 0; i < ISA_NUM_IRQS; i++) {
+            qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
+        }
+        pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
+
+        if (xen_enabled()) {
+            pci_device_set_intx_routing_notifier(
+                        pci_dev, piix_intx_routing_notifier_xen);
+
+            /*
+             * Xen supports additional interrupt routes from the PCI devices to
+             * the IOAPIC: the four pins of each PCI device on the bus are also
+             * connected to the IOAPIC directly.
+             * These additional routes can be discovered through ACPI.
+             */
+            pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
+                         XEN_IOAPIC_NUM_PIRQS);
+        }
+
+        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
+        x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
+                                                              "rtc"));
+        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
+        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
+        pci_ide_create_devs(PCI_DEVICE(dev));
+        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
+        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
+    } else {
+        isa_bus = isa_bus_new(NULL, system_memory, system_io,
+                              &error_abort);
+        isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
+
+        x86ms->rtc = isa_new(TYPE_MC146818_RTC);
+        qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
+        isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
+
+        i8257_dma_init(OBJECT(machine), isa_bus, 0);
+        pcms->hpet_enabled = false;
+    }
+
+    if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
+        pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+    }
+
+    if (phb) {
+        ioapic_init_gsi(gsi_state, phb);
+    }
+
+    if (tcg_enabled()) {
+        x86_register_ferr_irq(x86ms->gsi[13]);
+    }
+
+    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
+
+    /* init basic PC hardware */
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
+                         !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
+
+    pc_nic_init(pcmc, isa_bus, pcms->pcibus);
+
+#ifdef CONFIG_IDE_ISA
+    if (!pcmc->pci_enabled) {
+        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+        int i;
+
+        ide_drive_get(hd, ARRAY_SIZE(hd));
+        for (i = 0; i < MAX_IDE_BUS; i++) {
+            ISADevice *dev;
+            char busname[] = "ide.0";
+            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
+                               ide_irq[i],
+                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
+            /*
+             * The ide bus name is ide.0 for the first bus and ide.1 for the
+             * second one.
+             */
+            busname[4] = '0' + i;
+            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
+        }
+    }
+#endif
+
+    if (piix4_pm) {
+        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
+
+        qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
+        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
+        /* TODO: Populate SPD eeprom data.  */
+        smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
+
+        object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
+                                 TYPE_HOTPLUG_HANDLER,
+                                 (Object **)&x86ms->acpi_dev,
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_STRONG);
+        object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
+                                 piix4_pm, &error_abort);
+    }
+
+    if (machine->nvdimms_state->is_enabled) {
+        nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+                               x86_nvdimm_acpi_dsmio,
+                               x86ms->fw_cfg, OBJECT(pcms));
+    }
 }
 #endif
 
-- 
2.43.0

[PATCH v6 05/19] hw/i386/pc_piix.c: remove pcmc->pci_enabled dependent initialisation from pc_init_isa()

[Mark Cave-Ayland]

PCI code will never be used for an isapc machine.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 105 ++++------------------------------------------
 1 file changed, 8 insertions(+), 97 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5b97ad2cea..da2de88383 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,19 +435,16 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
 #ifdef CONFIG_ISAPC
 static void pc_init_isa(MachineState *machine)
 {
-    const char *pci_type = NULL;
     PCMachineState *pcms = PC_MACHINE(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     X86MachineState *x86ms = X86_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
-    Object *phb = NULL;
     ISABus *isa_bus;
     Object *piix4_pm = NULL;
     qemu_irq smi_irq;
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
-    MemoryRegion *pci_memory = NULL;
     MemoryRegion *rom_memory = system_memory;
     ram_addr_t lowmem;
     uint64_t hole64_size = 0;
@@ -535,39 +532,6 @@ static void pc_init_isa(MachineState *machine)
         kvmclock_create(pcmc->kvmclock_create_always);
     }
 
-    if (pcmc->pci_enabled) {
-        pci_memory = g_new(MemoryRegion, 1);
-        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
-        rom_memory = pci_memory;
-
-        phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
-        object_property_add_child(OBJECT(machine), "i440fx", phb);
-        object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
-                                 OBJECT(ram_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
-                                 OBJECT(pci_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
-                                 OBJECT(system_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
-                                 OBJECT(system_io), &error_fatal);
-        object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
-                                 x86ms->below_4g_mem_size, &error_fatal);
-        object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
-                                 x86ms->above_4g_mem_size, &error_fatal);
-        object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
-                                &error_fatal);
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
-
-        pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
-        pci_bus_map_irqs(pcms->pcibus,
-                         xen_enabled() ? xen_pci_slot_get_pirq
-                                       : pc_pci_slot_get_pirq);
-
-        hole64_size = object_property_get_uint(phb,
-                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
-                                               &error_abort);
-    }
-
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
@@ -584,74 +548,21 @@ static void pc_init_isa(MachineState *machine)
 
     gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
 
-    if (pcmc->pci_enabled) {
-        PCIDevice *pci_dev;
-        DeviceState *dev;
-        size_t i;
-
-        pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
-        object_property_set_bool(OBJECT(pci_dev), "has-usb",
-                                 machine_usb(machine), &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
-                                 x86_machine_is_acpi_enabled(x86ms),
-                                 &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
-                                 &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
-                                 &error_abort);
-        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
-        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
-                                 x86_machine_is_smm_enabled(x86ms),
-                                 &error_abort);
-        dev = DEVICE(pci_dev);
-        for (i = 0; i < ISA_NUM_IRQS; i++) {
-            qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
-        }
-        pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
-
-        if (xen_enabled()) {
-            pci_device_set_intx_routing_notifier(
-                        pci_dev, piix_intx_routing_notifier_xen);
-
-            /*
-             * Xen supports additional interrupt routes from the PCI devices to
-             * the IOAPIC: the four pins of each PCI device on the bus are also
-             * connected to the IOAPIC directly.
-             * These additional routes can be discovered through ACPI.
-             */
-            pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
-                         XEN_IOAPIC_NUM_PIRQS);
-        }
-
-        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
-        x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
-                                                              "rtc"));
-        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
-        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
-        pci_ide_create_devs(PCI_DEVICE(dev));
-        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
-        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
-    } else {
-        isa_bus = isa_bus_new(NULL, system_memory, system_io,
-                              &error_abort);
-        isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
+    isa_bus = isa_bus_new(NULL, system_memory, system_io,
+                            &error_abort);
+    isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
 
-        x86ms->rtc = isa_new(TYPE_MC146818_RTC);
-        qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
-        isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
+    x86ms->rtc = isa_new(TYPE_MC146818_RTC);
+    qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
+    isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
 
-        i8257_dma_init(OBJECT(machine), isa_bus, 0);
-        pcms->hpet_enabled = false;
-    }
+    i8257_dma_init(OBJECT(machine), isa_bus, 0);
+    pcms->hpet_enabled = false;
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
     }
 
-    if (phb) {
-        ioapic_init_gsi(gsi_state, phb);
-    }
-
     if (tcg_enabled()) {
         x86_register_ferr_irq(x86ms->gsi[13]);
     }
-- 
2.43.0

[PATCH v6 06/19] hw/i386/pc_piix.c: remove SMI and piix4_pm initialisation from pc_init_isa()

[Mark Cave-Ayland]

These are based upon the PIIX4 PCI chipset and so can never be used on an isapc machine.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index da2de88383..e58a7b3977 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -441,8 +441,6 @@ static void pc_init_isa(MachineState *machine)
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     ISABus *isa_bus;
-    Object *piix4_pm = NULL;
-    qemu_irq smi_irq;
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
     MemoryRegion *rom_memory = system_memory;
@@ -597,23 +595,6 @@ static void pc_init_isa(MachineState *machine)
     }
 #endif
 
-    if (piix4_pm) {
-        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
-
-        qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
-        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
-        /* TODO: Populate SPD eeprom data.  */
-        smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
-
-        object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
-                                 TYPE_HOTPLUG_HANDLER,
-                                 (Object **)&x86ms->acpi_dev,
-                                 object_property_allow_set_link,
-                                 OBJ_PROP_LINK_STRONG);
-        object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
-                                 piix4_pm, &error_abort);
-    }
-
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
                                x86_nvdimm_acpi_dsmio,
-- 
2.43.0

[PATCH v6 07/19] hw/i386/pc_piix.c: remove SGX initialisation from pc_init_isa()

[Mark Cave-Ayland]

The Intel SGX instructions only exist on recent CPUs and so would never be available
on a CPU from the pre-PCI era.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
---
 hw/i386/pc_piix.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e58a7b3977..129a3e2524 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -509,8 +509,6 @@ static void pc_init_isa(MachineState *machine)
         }
     }
 
-    pc_machine_init_sgx_epc(pcms);
-
     /*
      * There is a small chance that someone unintentionally passes "-cpu max"
      * for the isapc machine, which will provide a much more modern 32-bit
-- 
2.43.0

[PATCH v6 08/19] hw/i386/pc_piix.c: remove nvdimm initialisation from pc_init_isa()

[Mark Cave-Ayland]

NVDIMMs cannot be used by PCs from a pre-PCI era.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 129a3e2524..816124c027 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -592,12 +592,6 @@ static void pc_init_isa(MachineState *machine)
         }
     }
 #endif
-
-    if (machine->nvdimms_state->is_enabled) {
-        nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
-                               x86_nvdimm_acpi_dsmio,
-                               x86ms->fw_cfg, OBJECT(pcms));
-    }
 }
 #endif
 
-- 
2.43.0

[PATCH v6 09/19] hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()

[Mark Cave-Ayland]

All isapc machines must have 32-bit CPUs and so the RAM split logic can be hardcoded
accordingly.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
---
 hw/i386/pc_piix.c | 58 ++++-------------------------------------------
 1 file changed, 4 insertions(+), 54 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 816124c027..fc94937ad4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -444,69 +444,19 @@ static void pc_init_isa(MachineState *machine)
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
     MemoryRegion *rom_memory = system_memory;
-    ram_addr_t lowmem;
     uint64_t hole64_size = 0;
 
     /*
-     * Calculate ram split, for memory below and above 4G.  It's a bit
-     * complicated for backward compatibility reasons ...
-     *
-     *  - Traditional split is 3.5G (lowmem = 0xe0000000).  This is the
-     *    default value for max_ram_below_4g now.
-     *
-     *  - Then, to gigabyte align the memory, we move the split to 3G
-     *    (lowmem = 0xc0000000).  But only in case we have to split in
-     *    the first place, i.e. ram_size is larger than (traditional)
-     *    lowmem.  And for new machine types (gigabyte_align = true)
-     *    only, for live migration compatibility reasons.
-     *
-     *  - Next the max-ram-below-4g option was added, which allowed to
-     *    reduce lowmem to a smaller value, to allow a larger PCI I/O
-     *    window below 4G.  qemu doesn't enforce gigabyte alignment here,
-     *    but prints a warning.
-     *
-     *  - Finally max-ram-below-4g got updated to also allow raising lowmem,
-     *    so legacy non-PAE guests can get as much memory as possible in
-     *    the 32bit address space below 4G.
-     *
-     *  - Note that Xen has its own ram setup code in xen_ram_init(),
-     *    called via xen_hvm_init_pc().
-     *
-     * Examples:
-     *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  512M high
-     *    qemu -M pc -m 4G        (new default)    -> 3072M low, 1024M high
-     *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 2048M high
-     *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
+     * There is no RAM split for the isapc machine
      */
     if (xen_enabled()) {
         xen_hvm_init_pc(pcms, &ram_memory);
     } else {
         ram_memory = machine->ram;
-        if (!pcms->max_ram_below_4g) {
-            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
-        }
-        lowmem = pcms->max_ram_below_4g;
-        if (machine->ram_size >= pcms->max_ram_below_4g) {
-            if (pcmc->gigabyte_align) {
-                if (lowmem > 0xc0000000) {
-                    lowmem = 0xc0000000;
-                }
-                if (lowmem & (1 * GiB - 1)) {
-                    warn_report("Large machine and max_ram_below_4g "
-                                "(%" PRIu64 ") not a multiple of 1G; "
-                                "possible bad performance.",
-                                pcms->max_ram_below_4g);
-                }
-            }
-        }
 
-        if (machine->ram_size >= lowmem) {
-            x86ms->above_4g_mem_size = machine->ram_size - lowmem;
-            x86ms->below_4g_mem_size = lowmem;
-        } else {
-            x86ms->above_4g_mem_size = 0;
-            x86ms->below_4g_mem_size = machine->ram_size;
-        }
+        pcms->max_ram_below_4g = 4 * GiB;
+        x86ms->above_4g_mem_size = 0;
+        x86ms->below_4g_mem_size = machine->ram_size;
     }
 
     /*
-- 
2.43.0

[PATCH v6 10/19] hw/i386/pc_piix.c: hardcode hole64_size to 0 in pc_init_isa()

[Mark Cave-Ayland]

All isapc machines must have 32-bit CPUs and have no PCI 64-bit hole so it can be
hardcoded to 0.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fc94937ad4..7c9f4b78b0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -444,7 +444,6 @@ static void pc_init_isa(MachineState *machine)
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
     MemoryRegion *rom_memory = system_memory;
-    uint64_t hole64_size = 0;
 
     /*
      * There is no RAM split for the isapc machine
@@ -480,7 +479,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, hole64_size);
+        pc_memory_init(pcms, system_memory, rom_memory, 0);
     } else {
         assert(machine->ram_size == x86ms->below_4g_mem_size +
                                     x86ms->above_4g_mem_size);
-- 
2.43.0

[PATCH v6 11/19] hw/i386/pc_piix.c: remove pc_system_flash_cleanup_unused() from pc_init_isa()

[Mark Cave-Ayland]

This function contains 'assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled)' and so we can
safely assume that it should never be used for the isapc machine.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7c9f4b78b0..bbd5f9d17f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -484,7 +484,6 @@ static void pc_init_isa(MachineState *machine)
         assert(machine->ram_size == x86ms->below_4g_mem_size +
                                     x86ms->above_4g_mem_size);
 
-        pc_system_flash_cleanup_unused(pcms);
         if (machine->kernel_filename != NULL) {
             /* For xen HVM direct kernel boot, load linux here */
             xen_load_linux(pcms);
-- 
2.43.0

[PATCH v6 12/19] hw/i386/pc_piix.c: always initialise ISA IDE drives in pc_init_isa()

[Mark Cave-Ayland]

By definition an isapc machine must always use ISA IDE drives so ensure that they
are always enabled. At the same time also remove the surrounding CONFIG_IDE_ISA
define since it will be enabled via the ISAPC Kconfig.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bbd5f9d17f..924888b59d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -444,6 +444,8 @@ static void pc_init_isa(MachineState *machine)
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
     MemoryRegion *rom_memory = system_memory;
+    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+    int i;
 
     /*
      * There is no RAM split for the isapc machine
@@ -519,27 +521,20 @@ static void pc_init_isa(MachineState *machine)
 
     pc_nic_init(pcmc, isa_bus, pcms->pcibus);
 
-#ifdef CONFIG_IDE_ISA
-    if (!pcmc->pci_enabled) {
-        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-        int i;
-
-        ide_drive_get(hd, ARRAY_SIZE(hd));
-        for (i = 0; i < MAX_IDE_BUS; i++) {
-            ISADevice *dev;
-            char busname[] = "ide.0";
-            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
-                               ide_irq[i],
-                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
-            /*
-             * The ide bus name is ide.0 for the first bus and ide.1 for the
-             * second one.
-             */
-            busname[4] = '0' + i;
-            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
-        }
+    ide_drive_get(hd, ARRAY_SIZE(hd));
+    for (i = 0; i < MAX_IDE_BUS; i++) {
+        ISADevice *dev;
+        char busname[] = "ide.0";
+        dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
+                            ide_irq[i],
+                            hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
+        /*
+         * The ide bus name is ide.0 for the first bus and ide.1 for the
+         * second one.
+         */
+        busname[4] = '0' + i;
+        pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
     }
-#endif
 }
 #endif
 
-- 
2.43.0

[PATCH v6 13/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always false in pc_init_isa()

[Mark Cave-Ayland]

By definition PCI can never be enabled on an isapc machine so hardcode the relevant values
set via pcmc->pci_enabled.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 924888b59d..e8603b585a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -492,7 +492,7 @@ static void pc_init_isa(MachineState *machine)
         }
     }
 
-    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
+    gsi_state = pc_gsi_create(&x86ms->gsi, false);
 
     isa_bus = isa_bus_new(NULL, system_memory, system_io,
                             &error_abort);
@@ -513,7 +513,7 @@ static void pc_init_isa(MachineState *machine)
         x86_register_ferr_irq(x86ms->gsi[13]);
     }
 
-    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
+    pc_vga_init(isa_bus, NULL);
 
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
-- 
2.43.0

[PATCH v6 14/19] hw/i386/pc_piix.c: hardcode pcms->pci_bus to NULL in pc_init_isa()

[Mark Cave-Ayland]

By definition PCI can never be enabled on an isapc machine so hardcode the PCIBus argument
of pc_nic_init() to NULL.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e8603b585a..49bd1a41e7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -519,7 +519,7 @@ static void pc_init_isa(MachineState *machine)
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
                          !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
 
-    pc_nic_init(pcmc, isa_bus, pcms->pcibus);
+    pc_nic_init(pcmc, isa_bus, NULL);
 
     ide_drive_get(hd, ARRAY_SIZE(hd));
     for (i = 0; i < MAX_IDE_BUS; i++) {
-- 
2.43.0

[PATCH v6 15/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always true in pc_init1()

[Mark Cave-Ayland]

PCI is always enabled on the pc-i440fx machine so hardcode the relevant logic
in pc_init1().

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 194 ++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 118 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 49bd1a41e7..a776998504 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -71,7 +71,7 @@
 
 #define XEN_IOAPIC_NUM_PIRQS 128ULL
 
-#ifdef CONFIG_IDE_ISA
+#ifdef CONFIG_ISAPC
 static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
@@ -125,6 +125,9 @@ static void pc_init1(MachineState *machine, const char *pci_type)
     MemoryRegion *rom_memory = system_memory;
     ram_addr_t lowmem;
     uint64_t hole64_size = 0;
+    PCIDevice *pci_dev;
+    DeviceState *dev;
+    size_t i;
 
     /*
      * Calculate ram split, for memory below and above 4G.  It's a bit
@@ -195,38 +198,36 @@ static void pc_init1(MachineState *machine, const char *pci_type)
         kvmclock_create(pcmc->kvmclock_create_always);
     }
 
-    if (pcmc->pci_enabled) {
-        pci_memory = g_new(MemoryRegion, 1);
-        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
-        rom_memory = pci_memory;
-
-        phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
-        object_property_add_child(OBJECT(machine), "i440fx", phb);
-        object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
-                                 OBJECT(ram_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
-                                 OBJECT(pci_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
-                                 OBJECT(system_memory), &error_fatal);
-        object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
-                                 OBJECT(system_io), &error_fatal);
-        object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
-                                 x86ms->below_4g_mem_size, &error_fatal);
-        object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
-                                 x86ms->above_4g_mem_size, &error_fatal);
-        object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
-                                &error_fatal);
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
-
-        pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
-        pci_bus_map_irqs(pcms->pcibus,
-                         xen_enabled() ? xen_pci_slot_get_pirq
-                                       : pc_pci_slot_get_pirq);
-
-        hole64_size = object_property_get_uint(phb,
-                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
-                                               &error_abort);
-    }
+    pci_memory = g_new(MemoryRegion, 1);
+    memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
+    rom_memory = pci_memory;
+
+    phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
+    object_property_add_child(OBJECT(machine), "i440fx", phb);
+    object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
+                                OBJECT(ram_memory), &error_fatal);
+    object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
+                                OBJECT(pci_memory), &error_fatal);
+    object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
+                                OBJECT(system_memory), &error_fatal);
+    object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
+                                OBJECT(system_io), &error_fatal);
+    object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
+                                x86ms->below_4g_mem_size, &error_fatal);
+    object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
+                                x86ms->above_4g_mem_size, &error_fatal);
+    object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
+                            &error_fatal);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
+
+    pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
+    pci_bus_map_irqs(pcms->pcibus,
+                        xen_enabled() ? xen_pci_slot_get_pirq
+                                    : pc_pci_slot_get_pirq);
+
+    hole64_size = object_property_get_uint(phb,
+                                            PCI_HOST_PROP_PCI_HOLE64_SIZE,
+                                            &error_abort);
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
@@ -242,72 +243,51 @@ static void pc_init1(MachineState *machine, const char *pci_type)
         }
     }
 
-    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
-
-    if (pcmc->pci_enabled) {
-        PCIDevice *pci_dev;
-        DeviceState *dev;
-        size_t i;
-
-        pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
-        object_property_set_bool(OBJECT(pci_dev), "has-usb",
-                                 machine_usb(machine), &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
-                                 x86_machine_is_acpi_enabled(x86ms),
-                                 &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
-                                 &error_abort);
-        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
-                                 &error_abort);
-        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
-        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
-                                 x86_machine_is_smm_enabled(x86ms),
-                                 &error_abort);
-        dev = DEVICE(pci_dev);
-        for (i = 0; i < ISA_NUM_IRQS; i++) {
-            qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
-        }
-        pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
-
-        if (xen_enabled()) {
-            pci_device_set_intx_routing_notifier(
-                        pci_dev, piix_intx_routing_notifier_xen);
-
-            /*
-             * Xen supports additional interrupt routes from the PCI devices to
-             * the IOAPIC: the four pins of each PCI device on the bus are also
-             * connected to the IOAPIC directly.
-             * These additional routes can be discovered through ACPI.
-             */
-            pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
-                         XEN_IOAPIC_NUM_PIRQS);
-        }
+    gsi_state = pc_gsi_create(&x86ms->gsi, true);
+
+    pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
+    object_property_set_bool(OBJECT(pci_dev), "has-usb",
+                                machine_usb(machine), &error_abort);
+    object_property_set_bool(OBJECT(pci_dev), "has-acpi",
+                                x86_machine_is_acpi_enabled(x86ms),
+                                &error_abort);
+    object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
+                                &error_abort);
+    object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
+                                &error_abort);
+    qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
+    object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
+                                x86_machine_is_smm_enabled(x86ms),
+                                &error_abort);
+    dev = DEVICE(pci_dev);
+    for (i = 0; i < ISA_NUM_IRQS; i++) {
+        qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
+    }
+    pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
 
-        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
-        x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
-                                                              "rtc"));
-        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
-        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
-        pci_ide_create_devs(PCI_DEVICE(dev));
-        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
-        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
-    } else {
-        uint32_t irq;
+    if (xen_enabled()) {
+        pci_device_set_intx_routing_notifier(
+                    pci_dev, piix_intx_routing_notifier_xen);
 
-        isa_bus = isa_bus_new(NULL, system_memory, system_io,
-                              &error_abort);
-        isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
+        /*
+         * Xen supports additional interrupt routes from the PCI devices to
+         * the IOAPIC: the four pins of each PCI device on the bus are also
+         * connected to the IOAPIC directly.
+         * These additional routes can be discovered through ACPI.
+         */
+        pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
+                        XEN_IOAPIC_NUM_PIRQS);
+    }
 
-        x86ms->rtc = isa_new(TYPE_MC146818_RTC);
-        qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
-        isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
-        irq = object_property_get_uint(OBJECT(x86ms->rtc), "irq",
-                                       &error_fatal);
-        isa_connect_gpio_out(ISA_DEVICE(x86ms->rtc), 0, irq);
+    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
+    x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
+                                                            "rtc"));
+    piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
+    dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
+    pci_ide_create_devs(PCI_DEVICE(dev));
+    pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
+    pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
 
-        i8257_dma_init(OBJECT(machine), isa_bus, 0);
-        pcms->hpet_enabled = false;
-    }
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
@@ -321,7 +301,7 @@ static void pc_init1(MachineState *machine, const char *pci_type)
         x86_register_ferr_irq(x86ms->gsi[13]);
     }
 
-    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
+    pc_vga_init(isa_bus, pcms->pcibus);
 
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
@@ -329,28 +309,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
 
     pc_nic_init(pcmc, isa_bus, pcms->pcibus);
 
-#ifdef CONFIG_IDE_ISA
-    if (!pcmc->pci_enabled) {
-        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-        int i;
-
-        ide_drive_get(hd, ARRAY_SIZE(hd));
-        for (i = 0; i < MAX_IDE_BUS; i++) {
-            ISADevice *dev;
-            char busname[] = "ide.0";
-            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
-                               ide_irq[i],
-                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
-            /*
-             * The ide bus name is ide.0 for the first bus and ide.1 for the
-             * second one.
-             */
-            busname[4] = '0' + i;
-            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
-        }
-    }
-#endif
-
     if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
 
-- 
2.43.0

[PATCH v6 16/19] hw/i386: move isapc machine to separate isapc.c file

[Mark Cave-Ayland]

Now that pc_init_isa() is independent of any PCI initialisation, move it into a
separate isapc.c file. This enables us to finally fix the dependency of ISAPC on
I440FX in hw/i386/Kconfig.

Note that as part of the move to a separate file we can see that the licence text
is a verbatim copy of the MIT licence. The text originates from commit 1df912cf9e
("VL license of the day is MIT/BSD") so we can be sure that this was the original
intent. As a consequence we can update the file header to use a SPDX tag as per
the current project contribution guidelines.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/Kconfig     |   3 -
 hw/i386/isapc.c     | 171 ++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/meson.build |   1 +
 hw/i386/pc_piix.c   | 149 --------------------------------------
 4 files changed, 172 insertions(+), 152 deletions(-)
 create mode 100644 hw/i386/isapc.c

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 3a0e2b8ebb..6a0ab54bea 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -96,9 +96,6 @@ config ISAPC
     select ISA_BUS
     select PC
     select IDE_ISA
-    # FIXME: it is in the same file as i440fx, and does not compile
-    # if separated
-    depends on I440FX
 
 config Q35
     bool
diff --git a/hw/i386/isapc.c b/hw/i386/isapc.c
new file mode 100644
index 0000000000..62acdada3e
--- /dev/null
+++ b/hw/i386/isapc.c
@@ -0,0 +1,171 @@
+/*
+ * QEMU PC System Emulator
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/units.h"
+#include "qemu/error-report.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/dma/i8257.h"
+#include "hw/i386/pc.h"
+#include "hw/ide/isa.h"
+#include "hw/ide/ide-bus.h"
+#include "system/kvm.h"
+#include "hw/i386/kvm/clock.h"
+#include "hw/xen/xen-x86.h"
+#include "system/xen.h"
+#include "hw/rtc/mc146818rtc.h"
+#include "target/i386/cpu.h"
+
+static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
+static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
+static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
+
+
+static void pc_init_isa(MachineState *machine)
+{
+    PCMachineState *pcms = PC_MACHINE(machine);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    X86MachineState *x86ms = X86_MACHINE(machine);
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
+    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;
+
+    /*
+     * There is no RAM split for the isapc machine
+     */
+    if (xen_enabled()) {
+        xen_hvm_init_pc(pcms, &ram_memory);
+    } else {
+        ram_memory = machine->ram;
+
+        pcms->max_ram_below_4g = 4 * GiB;
+        x86ms->above_4g_mem_size = 0;
+        x86ms->below_4g_mem_size = machine->ram_size;
+    }
+
+    /*
+     * There is a small chance that someone unintentionally passes "-cpu max"
+     * for the isapc machine, which will provide a much more modern 32-bit
+     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
+     * been specified, choose the "best" 32-bit cpu possible which we consider
+     * be the pentium3 (deliberately choosing an Intel CPU given that the
+     * default 486 CPU for the isapc machine is also an Intel CPU).
+     */
+    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
+        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
+        warn_report("-cpu max is invalid for isapc machine, using pentium3");
+    }
+
+    x86_cpus_init(x86ms, pcmc->default_cpu_version);
+
+    if (kvm_enabled()) {
+        kvmclock_create(pcmc->kvmclock_create_always);
+    }
+
+    /* allocate ram and load rom/bios */
+    if (!xen_enabled()) {
+        pc_memory_init(pcms, system_memory, rom_memory, 0);
+    } else {
+        assert(machine->ram_size == x86ms->below_4g_mem_size +
+                                    x86ms->above_4g_mem_size);
+
+        if (machine->kernel_filename != NULL) {
+            /* For xen HVM direct kernel boot, load linux here */
+            xen_load_linux(pcms);
+        }
+    }
+
+    gsi_state = pc_gsi_create(&x86ms->gsi, false);
+
+    isa_bus = isa_bus_new(NULL, system_memory, system_io,
+                            &error_abort);
+    isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
+
+    x86ms->rtc = isa_new(TYPE_MC146818_RTC);
+    qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
+    isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
+    irq = object_property_get_uint(OBJECT(x86ms->rtc), "irq",
+                                   &error_fatal);
+    isa_connect_gpio_out(ISA_DEVICE(x86ms->rtc), 0, irq);
+
+    i8257_dma_init(OBJECT(machine), isa_bus, 0);
+    pcms->hpet_enabled = false;
+
+    if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
+        pc_i8259_create(isa_bus, gsi_state->i8259_irq);
+    }
+
+    if (tcg_enabled()) {
+        x86_register_ferr_irq(x86ms->gsi[13]);
+    }
+
+    pc_vga_init(isa_bus, NULL);
+
+    /* init basic PC hardware */
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
+                         !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
+
+    pc_nic_init(pcmc, isa_bus, NULL);
+
+    ide_drive_get(hd, ARRAY_SIZE(hd));
+    for (i = 0; i < MAX_IDE_BUS; i++) {
+        ISADevice *dev;
+        char busname[] = "ide.0";
+        dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
+                           ide_irq[i],
+                           hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
+        /*
+         * The ide bus name is ide.0 for the first bus and ide.1 for the
+         * second one.
+         */
+        busname[4] = '0' + i;
+        pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
+    }
+}
+
+static void isapc_machine_options(MachineClass *m)
+{
+    static const char * const valid_cpu_types[] = {
+        X86_CPU_TYPE_NAME("486"),
+        X86_CPU_TYPE_NAME("athlon"),
+        X86_CPU_TYPE_NAME("kvm32"),
+        X86_CPU_TYPE_NAME("pentium"),
+        X86_CPU_TYPE_NAME("pentium2"),
+        X86_CPU_TYPE_NAME("pentium3"),
+        X86_CPU_TYPE_NAME("qemu32"),
+        X86_CPU_TYPE_NAME("max"),
+        NULL
+    };
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
+    m->desc = "ISA-only PC";
+    m->max_cpus = 1;
+    m->option_rom_has_mr = true;
+    m->rom_file_has_mr = false;
+    pcmc->pci_enabled = false;
+    pcmc->has_acpi_build = false;
+    pcmc->smbios_defaults = false;
+    pcmc->gigabyte_align = false;
+    pcmc->smbios_legacy_mode = true;
+    pcmc->has_reserved_memory = false;
+    m->default_nic = "ne2k_isa";
+    m->default_cpu_type = X86_CPU_TYPE_NAME("486");
+    m->valid_cpu_types = valid_cpu_types;
+    m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
+    m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
+}
+
+DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
+                  isapc_machine_options);
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index 7896f348cf..436b3ce52d 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -14,6 +14,7 @@ i386_ss.add(when: 'CONFIG_X86_IOMMU', if_true: files('x86-iommu.c'),
 i386_ss.add(when: 'CONFIG_AMD_IOMMU', if_true: files('amd_iommu.c'),
                                       if_false: files('amd_iommu-stub.c'))
 i386_ss.add(when: 'CONFIG_I440FX', if_true: files('pc_piix.c'))
+i386_ss.add(when: 'CONFIG_ISAPC', if_true: files('isapc.c'))
 i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c'))
 i386_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: files('nitro_enclave.c'))
 i386_ss.add(when: 'CONFIG_Q35', if_true: files('pc_q35.c'))
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a776998504..05713662e4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -71,12 +71,6 @@
 
 #define XEN_IOAPIC_NUM_PIRQS 128ULL
 
-#ifdef CONFIG_ISAPC
-static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
-static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
-static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
-#endif
-
 static GlobalProperty pc_piix_compat_defaults[] = {
     { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "true" },
     { TYPE_VFIO_PCI_NOHOTPLUG, "use-legacy-x86-rom", "true" },
@@ -390,112 +384,6 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
     pcms->south_bridge = PCSouthBridgeOption_lookup.array[value];
 }
 
-#ifdef CONFIG_ISAPC
-static void pc_init_isa(MachineState *machine)
-{
-    PCMachineState *pcms = PC_MACHINE(machine);
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-    X86MachineState *x86ms = X86_MACHINE(machine);
-    MemoryRegion *system_memory = get_system_memory();
-    MemoryRegion *system_io = get_system_io();
-    ISABus *isa_bus;
-    GSIState *gsi_state;
-    MemoryRegion *ram_memory;
-    MemoryRegion *rom_memory = system_memory;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-    int i;
-
-    /*
-     * There is no RAM split for the isapc machine
-     */
-    if (xen_enabled()) {
-        xen_hvm_init_pc(pcms, &ram_memory);
-    } else {
-        ram_memory = machine->ram;
-
-        pcms->max_ram_below_4g = 4 * GiB;
-        x86ms->above_4g_mem_size = 0;
-        x86ms->below_4g_mem_size = machine->ram_size;
-    }
-
-    /*
-     * There is a small chance that someone unintentionally passes "-cpu max"
-     * for the isapc machine, which will provide a much more modern 32-bit
-     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
-     * been specified, choose the "best" 32-bit cpu possible which we consider
-     * be the pentium3 (deliberately choosing an Intel CPU given that the
-     * default 486 CPU for the isapc machine is also an Intel CPU).
-     */
-    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
-        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
-        warn_report("-cpu max is invalid for isapc machine, using pentium3");
-    }
-
-    x86_cpus_init(x86ms, pcmc->default_cpu_version);
-
-    if (kvm_enabled()) {
-        kvmclock_create(pcmc->kvmclock_create_always);
-    }
-
-    /* allocate ram and load rom/bios */
-    if (!xen_enabled()) {
-        pc_memory_init(pcms, system_memory, rom_memory, 0);
-    } else {
-        assert(machine->ram_size == x86ms->below_4g_mem_size +
-                                    x86ms->above_4g_mem_size);
-
-        if (machine->kernel_filename != NULL) {
-            /* For xen HVM direct kernel boot, load linux here */
-            xen_load_linux(pcms);
-        }
-    }
-
-    gsi_state = pc_gsi_create(&x86ms->gsi, false);
-
-    isa_bus = isa_bus_new(NULL, system_memory, system_io,
-                            &error_abort);
-    isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
-
-    x86ms->rtc = isa_new(TYPE_MC146818_RTC);
-    qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
-    isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
-
-    i8257_dma_init(OBJECT(machine), isa_bus, 0);
-    pcms->hpet_enabled = false;
-
-    if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
-        pc_i8259_create(isa_bus, gsi_state->i8259_irq);
-    }
-
-    if (tcg_enabled()) {
-        x86_register_ferr_irq(x86ms->gsi[13]);
-    }
-
-    pc_vga_init(isa_bus, NULL);
-
-    /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
-                         !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
-
-    pc_nic_init(pcmc, isa_bus, NULL);
-
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-    for (i = 0; i < MAX_IDE_BUS; i++) {
-        ISADevice *dev;
-        char busname[] = "ide.0";
-        dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
-                            ide_irq[i],
-                            hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
-        /*
-         * The ide bus name is ide.0 for the first bus and ide.1 for the
-         * second one.
-         */
-        busname[4] = '0' + i;
-        pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
-    }
-}
-#endif
-
 #ifdef CONFIG_XEN
 static void pc_xen_hvm_init(MachineState *machine)
 {
@@ -854,43 +742,6 @@ static void pc_i440fx_machine_2_6_options(MachineClass *m)
 
 DEFINE_I440FX_MACHINE(2, 6);
 
-#ifdef CONFIG_ISAPC
-static void isapc_machine_options(MachineClass *m)
-{
-    static const char * const valid_cpu_types[] = {
-        X86_CPU_TYPE_NAME("486"),
-        X86_CPU_TYPE_NAME("athlon"),
-        X86_CPU_TYPE_NAME("kvm32"),
-        X86_CPU_TYPE_NAME("pentium"),
-        X86_CPU_TYPE_NAME("pentium2"),
-        X86_CPU_TYPE_NAME("pentium3"),
-        X86_CPU_TYPE_NAME("qemu32"),
-        X86_CPU_TYPE_NAME("max"),
-        NULL
-    };
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
-
-    m->desc = "ISA-only PC";
-    m->max_cpus = 1;
-    m->option_rom_has_mr = true;
-    m->rom_file_has_mr = false;
-    pcmc->pci_enabled = false;
-    pcmc->has_acpi_build = false;
-    pcmc->smbios_defaults = false;
-    pcmc->gigabyte_align = false;
-    pcmc->smbios_legacy_mode = true;
-    pcmc->has_reserved_memory = false;
-    m->default_nic = "ne2k_isa";
-    m->default_cpu_type = X86_CPU_TYPE_NAME("486");
-    m->valid_cpu_types = valid_cpu_types;
-    m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
-    m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
-}
-
-DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
-                  isapc_machine_options);
-#endif
-
 #ifdef CONFIG_XEN
 static void xenfv_machine_4_2_options(MachineClass *m)
 {
-- 
2.43.0

[PATCH v6 17/19] hw/i386/pc_piix.c: remove unused headers after isapc machine split

[Mark Cave-Ayland]

The headers for isapc-only devices can be removed from pc_piix.c since they are
no longer used by the i440fx-pc machine.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 05713662e4..5babdc3e4c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -27,19 +27,16 @@
 
 #include "qemu/units.h"
 #include "hw/char/parallel-isa.h"
-#include "hw/dma/i8257.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
 #include "hw/pci-host/i440fx.h"
-#include "hw/rtc/mc146818rtc.h"
 #include "hw/southbridge/piix.h"
 #include "hw/display/ramfb.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
 #include "net/net.h"
-#include "hw/ide/isa.h"
 #include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "system/kvm.h"
-- 
2.43.0

[PATCH v6 18/19] hw/i386/pc_piix.c: replace rom_memory with pci_memory

[Mark Cave-Ayland]

Now that we can guarantee the i440fx-pc machine will always have a PCI bus, any
instances of rom_memory can be replaced by pci_memory and rom_memory removed
completely.

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5babdc3e4c..b736f0f236 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -113,7 +113,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
     GSIState *gsi_state;
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory = NULL;
-    MemoryRegion *rom_memory = system_memory;
     ram_addr_t lowmem;
     uint64_t hole64_size = 0;
     PCIDevice *pci_dev;
@@ -191,7 +190,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
 
     pci_memory = g_new(MemoryRegion, 1);
     memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
-    rom_memory = pci_memory;
 
     phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
     object_property_add_child(OBJECT(machine), "i440fx", phb);
@@ -222,7 +220,7 @@ static void pc_init1(MachineState *machine, const char *pci_type)
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
+        pc_memory_init(pcms, system_memory, pci_memory, hole64_size);
     } else {
         assert(machine->ram_size == x86ms->below_4g_mem_size +
                                     x86ms->above_4g_mem_size);
-- 
2.43.0

[PATCH v6 19/19] hw/i386/isapc.c: replace rom_memory with system_memory

[Mark Cave-Ayland]

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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 62acdada3e..87193beb44 100644
--- a/hw/i386/isapc.c
+++ b/hw/i386/isapc.c
@@ -37,7 +37,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;
@@ -76,7 +75,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);
     } else {
         assert(machine->ram_size == x86ms->below_4g_mem_size +
                                     x86ms->above_4g_mem_size);
-- 
2.43.0

Re: [PATCH v6 09/19] hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()

[Philippe Mathieu-Daudé]

On 22/8/25 14:11, Mark Cave-Ayland wrote:
> All isapc machines must have 32-bit CPUs and so the RAM split logic can be hardcoded
> accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> ---
>   hw/i386/pc_piix.c | 58 ++++-------------------------------------------
>   1 file changed, 4 insertions(+), 54 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> possible to specify any CPU via -cpu on the command line, it makes no
> sense to allow modern 64-bit CPUs to be used.
> 
> Restrict the isapc machine to the available 32-bit CPUs, taking care to
> handle the case where if a user inadvertently uses -cpu max then the "best"
> 32-bit CPU is used (in this case the pentium3).
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c03324281b..5720b6b556 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
>   #ifdef CONFIG_ISAPC
>   static void pc_init_isa(MachineState *machine)
>   {
> +    /*
> +     * There is a small chance that someone unintentionally passes "-cpu max"
> +     * for the isapc machine, which will provide a much more modern 32-bit
> +     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
> +     * been specified, choose the "best" 32-bit cpu possible which we consider
> +     * be the pentium3 (deliberately choosing an Intel CPU given that the
> +     * default 486 CPU for the isapc machine is also an Intel CPU).
> +     */
> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> +        warn_report("-cpu max is invalid for isapc machine, using pentium3");
> +    }

Do we need to handle the case of "-cpu host"?

>       pc_init1(machine, NULL);
>   }
>   #endif
> @@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
>   #ifdef CONFIG_ISAPC
>   static void isapc_machine_options(MachineClass *m)
>   {
> +    static const char * const valid_cpu_types[] = {
> +        X86_CPU_TYPE_NAME("486"),
> +        X86_CPU_TYPE_NAME("athlon"),
> +        X86_CPU_TYPE_NAME("kvm32"),
> +        X86_CPU_TYPE_NAME("pentium"),
> +        X86_CPU_TYPE_NAME("pentium2"),
> +        X86_CPU_TYPE_NAME("pentium3"),
> +        X86_CPU_TYPE_NAME("qemu32"),
> +        X86_CPU_TYPE_NAME("max"),
> +        NULL
> +    };
>       PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>       m->desc = "ISA-only PC";
>       m->max_cpus = 1;
>       m->option_rom_has_mr = true;
> @@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
>       pcmc->has_reserved_memory = false;
>       m->default_nic = "ne2k_isa";
>       m->default_cpu_type = X86_CPU_TYPE_NAME("486");
> +    m->valid_cpu_types = valid_cpu_types;
>       m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
>       m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>   }

Re: [PATCH v6 02/19] hw/i386/pc_piix.c: remove include for loader.h

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> This header is not required since the loader functionality is handled separately
> by pc_memory_init() in pc.c.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5720b6b556..bc8fbe46a1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -28,7 +28,6 @@
>   #include "qemu/units.h"
>   #include "hw/char/parallel-isa.h"
>   #include "hw/dma/i8257.h"
> -#include "hw/loader.h"
>   #include "hw/i386/x86.h"
>   #include "hw/i386/pc.h"
>   #include "hw/i386/apic.h"

Re: [PATCH v6 03/19] hw/i386/pc_piix.c: inline pc_xen_hvm_init_pci() into pc_xen_hvm_init()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> This helps to simplify the initialisation of the Xen hvm machine.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index bc8fbe46a1..071bf7412e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -453,14 +453,6 @@ static void pc_init_isa(MachineState *machine)
>   #endif
>   
>   #ifdef CONFIG_XEN
> -static void pc_xen_hvm_init_pci(MachineState *machine)
> -{
> -    const char *pci_type = xen_igd_gfx_pt_enabled() ?
> -                TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : TYPE_I440FX_PCI_DEVICE;
> -
> -    pc_init1(machine, pci_type);
> -}
> -
>   static void pc_xen_hvm_init(MachineState *machine)
>   {
>       PCMachineState *pcms = PC_MACHINE(machine);
> @@ -470,7 +462,10 @@ static void pc_xen_hvm_init(MachineState *machine)
>           exit(1);
>       }
>   
> -    pc_xen_hvm_init_pci(machine);
> +    pc_init1(machine, xen_igd_gfx_pt_enabled()
> +                      ? TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE
> +                      : TYPE_I440FX_PCI_DEVICE);
> +
>       xen_igd_reserve_slot(pcms->pcibus);
>       pci_create_simple(pcms->pcibus, -1, "xen-platform");
>   }

Re: [PATCH v6 04/19] hw/i386/pc_piix.c: duplicate pc_init1() into pc_isa_init()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> This is to prepare for splitting the isapc machine into its own separate file.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 261 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 260 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 071bf7412e..5b97ad2cea 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c

...

> +    if (piix4_pm) {
> +        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
> +
> +        qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
> +        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
> +        /* TODO: Populate SPD eeprom data.  */
> +        smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
> +
> +        object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
> +                                 TYPE_HOTPLUG_HANDLER,
> +                                 (Object **)&x86ms->acpi_dev,
> +                                 object_property_allow_set_link,
> +                                 OBJ_PROP_LINK_STRONG);
> +        object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
> +                                 piix4_pm, &error_abort);
> +    }
> +
> +    if (machine->nvdimms_state->is_enabled) {
> +        nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> +                               x86_nvdimm_acpi_dsmio,
> +                               x86ms->fw_cfg, OBJECT(pcms));
> +    }

IGVM support was merged recently and the rebase of this series doesn't 
catch it.

I think we need to copy the IGVM related code as well, in this patch.
We can remove it later in a separate patch if it's unnecessary for isapc.

>   }
>   #endif
>   

Re: [PATCH v6 05/19] hw/i386/pc_piix.c: remove pcmc->pci_enabled dependent initialisation from pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> PCI code will never be used for an isapc machine.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

Re: [PATCH v6 13/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always false in pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> By definition PCI can never be enabled on an isapc machine so hardcode the relevant values
> set via pcmc->pci_enabled.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

It seems to me that this patch can be squashed into patch 05

> ---
>   hw/i386/pc_piix.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 924888b59d..e8603b585a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -492,7 +492,7 @@ static void pc_init_isa(MachineState *machine)
>           }
>       }
>   
> -    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> +    gsi_state = pc_gsi_create(&x86ms->gsi, false);
>   
>       isa_bus = isa_bus_new(NULL, system_memory, system_io,
>                               &error_abort);
> @@ -513,7 +513,7 @@ static void pc_init_isa(MachineState *machine)
>           x86_register_ferr_irq(x86ms->gsi[13]);
>       }
>   
> -    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
> +    pc_vga_init(isa_bus, NULL);
>   
>       /* init basic PC hardware */
>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,

Re: [PATCH v6 06/19] hw/i386/pc_piix.c: remove SMI and piix4_pm initialisation from pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> These are based upon the PIIX4 PCI chipset and so can never be used on an isapc machine.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 19 -------------------
>   1 file changed, 19 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index da2de88383..e58a7b3977 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -441,8 +441,6 @@ static void pc_init_isa(MachineState *machine)
>       MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *system_io = get_system_io();
>       ISABus *isa_bus;
> -    Object *piix4_pm = NULL;
> -    qemu_irq smi_irq;
>       GSIState *gsi_state;
>       MemoryRegion *ram_memory;
>       MemoryRegion *rom_memory = system_memory;
> @@ -597,23 +595,6 @@ static void pc_init_isa(MachineState *machine)
>       }
>   #endif
>   
> -    if (piix4_pm) {
> -        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
> -
> -        qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
> -        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
> -        /* TODO: Populate SPD eeprom data.  */
> -        smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
> -
> -        object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
> -                                 TYPE_HOTPLUG_HANDLER,
> -                                 (Object **)&x86ms->acpi_dev,
> -                                 object_property_allow_set_link,
> -                                 OBJ_PROP_LINK_STRONG);
> -        object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
> -                                 piix4_pm, &error_abort);
> -    }
> -
>       if (machine->nvdimms_state->is_enabled) {
>           nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
>                                  x86_nvdimm_acpi_dsmio,

Re: [PATCH v6 09/19] hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> All isapc machines must have 32-bit CPUs and so the RAM split logic can be hardcoded
> accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> ---
>   hw/i386/pc_piix.c | 58 ++++-------------------------------------------
>   1 file changed, 4 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 816124c027..fc94937ad4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -444,69 +444,19 @@ static void pc_init_isa(MachineState *machine)
>       GSIState *gsi_state;
>       MemoryRegion *ram_memory;
>       MemoryRegion *rom_memory = system_memory;
> -    ram_addr_t lowmem;
>       uint64_t hole64_size = 0;
>   
>       /*
> -     * Calculate ram split, for memory below and above 4G.  It's a bit
> -     * complicated for backward compatibility reasons ...
> -     *
> -     *  - Traditional split is 3.5G (lowmem = 0xe0000000).  This is the
> -     *    default value for max_ram_below_4g now.
> -     *
> -     *  - Then, to gigabyte align the memory, we move the split to 3G
> -     *    (lowmem = 0xc0000000).  But only in case we have to split in
> -     *    the first place, i.e. ram_size is larger than (traditional)
> -     *    lowmem.  And for new machine types (gigabyte_align = true)
> -     *    only, for live migration compatibility reasons.
> -     *
> -     *  - Next the max-ram-below-4g option was added, which allowed to
> -     *    reduce lowmem to a smaller value, to allow a larger PCI I/O
> -     *    window below 4G.  qemu doesn't enforce gigabyte alignment here,
> -     *    but prints a warning.
> -     *
> -     *  - Finally max-ram-below-4g got updated to also allow raising lowmem,
> -     *    so legacy non-PAE guests can get as much memory as possible in
> -     *    the 32bit address space below 4G.
> -     *
> -     *  - Note that Xen has its own ram setup code in xen_ram_init(),
> -     *    called via xen_hvm_init_pc().
> -     *
> -     * Examples:
> -     *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  512M high
> -     *    qemu -M pc -m 4G        (new default)    -> 3072M low, 1024M high
> -     *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 2048M high
> -     *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low (=4G-128M)
> +     * There is no RAM split for the isapc machine
>        */
>       if (xen_enabled()) {
>           xen_hvm_init_pc(pcms, &ram_memory);
>       } else {
>           ram_memory = machine->ram;
> -        if (!pcms->max_ram_below_4g) {
> -            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
> -        }
> -        lowmem = pcms->max_ram_below_4g;
> -        if (machine->ram_size >= pcms->max_ram_below_4g) {
> -            if (pcmc->gigabyte_align) {
> -                if (lowmem > 0xc0000000) {
> -                    lowmem = 0xc0000000;
> -                }
> -                if (lowmem & (1 * GiB - 1)) {
> -                    warn_report("Large machine and max_ram_below_4g "
> -                                "(%" PRIu64 ") not a multiple of 1G; "
> -                                "possible bad performance.",
> -                                pcms->max_ram_below_4g);
> -                }
> -            }
> -        }
>   
> -        if (machine->ram_size >= lowmem) {
> -            x86ms->above_4g_mem_size = machine->ram_size - lowmem;
> -            x86ms->below_4g_mem_size = lowmem;
> -        } else {
> -            x86ms->above_4g_mem_size = 0;
> -            x86ms->below_4g_mem_size = machine->ram_size;
> -        }
> +        pcms->max_ram_below_4g = 4 * GiB;
> +        x86ms->above_4g_mem_size = 0;
> +        x86ms->below_4g_mem_size = machine->ram_size;

I think we need to sanity check the machine->ram_size is not bigger than 
4G, and error out if it exceeds.

>       }
>   
>       /*

Re: [PATCH v6 07/19] hw/i386/pc_piix.c: remove SGX initialisation from pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> The Intel SGX instructions only exist on recent CPUs and so would never be available
> on a CPU from the pre-PCI era.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e58a7b3977..129a3e2524 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -509,8 +509,6 @@ static void pc_init_isa(MachineState *machine)
>           }
>       }
>   
> -    pc_machine_init_sgx_epc(pcms);
> -
>       /*
>        * There is a small chance that someone unintentionally passes "-cpu max"
>        * for the isapc machine, which will provide a much more modern 32-bit

Re: [PATCH v6 08/19] hw/i386/pc_piix.c: remove nvdimm initialisation from pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> NVDIMMs cannot be used by PCs from a pre-PCI era.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 129a3e2524..816124c027 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -592,12 +592,6 @@ static void pc_init_isa(MachineState *machine)
>           }
>       }
>   #endif
> -
> -    if (machine->nvdimms_state->is_enabled) {
> -        nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> -                               x86_nvdimm_acpi_dsmio,
> -                               x86ms->fw_cfg, OBJECT(pcms));
> -    }
>   }
>   #endif
>   

Re: [PATCH v6 10/19] hw/i386/pc_piix.c: hardcode hole64_size to 0 in pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> All isapc machines must have 32-bit CPUs and have no PCI 64-bit hole so it can be
> hardcoded to 0.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index fc94937ad4..7c9f4b78b0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -444,7 +444,6 @@ static void pc_init_isa(MachineState *machine)
>       GSIState *gsi_state;
>       MemoryRegion *ram_memory;
>       MemoryRegion *rom_memory = system_memory;
> -    uint64_t hole64_size = 0;
>   
>       /*
>        * There is no RAM split for the isapc machine
> @@ -480,7 +479,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, hole64_size);
> +        pc_memory_init(pcms, system_memory, rom_memory, 0);
>       } else {
>           assert(machine->ram_size == x86ms->below_4g_mem_size +
>                                       x86ms->above_4g_mem_size);

Re: [PATCH v6 12/19] hw/i386/pc_piix.c: always initialise ISA IDE drives in pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> By definition an isapc machine must always use ISA IDE drives so ensure that they
> are always enabled. At the same time also remove the surrounding CONFIG_IDE_ISA
> define since it will be enabled via the ISAPC Kconfig.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 35 +++++++++++++++--------------------
>   1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index bbd5f9d17f..924888b59d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -444,6 +444,8 @@ static void pc_init_isa(MachineState *machine)
>       GSIState *gsi_state;
>       MemoryRegion *ram_memory;
>       MemoryRegion *rom_memory = system_memory;
> +    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> +    int i;
>   
>       /*
>        * There is no RAM split for the isapc machine
> @@ -519,27 +521,20 @@ static void pc_init_isa(MachineState *machine)
>   
>       pc_nic_init(pcmc, isa_bus, pcms->pcibus);
>   
> -#ifdef CONFIG_IDE_ISA
> -    if (!pcmc->pci_enabled) {
> -        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> -        int i;
> -
> -        ide_drive_get(hd, ARRAY_SIZE(hd));
> -        for (i = 0; i < MAX_IDE_BUS; i++) {
> -            ISADevice *dev;
> -            char busname[] = "ide.0";
> -            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
> -                               ide_irq[i],
> -                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> -            /*
> -             * The ide bus name is ide.0 for the first bus and ide.1 for the
> -             * second one.
> -             */
> -            busname[4] = '0' + i;
> -            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
> -        }
> +    ide_drive_get(hd, ARRAY_SIZE(hd));
> +    for (i = 0; i < MAX_IDE_BUS; i++) {
> +        ISADevice *dev;
> +        char busname[] = "ide.0";
> +        dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
> +                            ide_irq[i],
> +                            hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> +        /*
> +         * The ide bus name is ide.0 for the first bus and ide.1 for the
> +         * second one.
> +         */
> +        busname[4] = '0' + i;
> +        pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
>       }
> -#endif
>   }
>   #endif
>   

Re: [PATCH v6 14/19] hw/i386/pc_piix.c: hardcode pcms->pci_bus to NULL in pc_init_isa()

[Xiaoyao Li]

On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
> By definition PCI can never be enabled on an isapc machine so hardcode the PCIBus argument
> of pc_nic_init() to NULL.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e8603b585a..49bd1a41e7 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -519,7 +519,7 @@ static void pc_init_isa(MachineState *machine)
>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
>                            !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
>   
> -    pc_nic_init(pcmc, isa_bus, pcms->pcibus);
> +    pc_nic_init(pcmc, isa_bus, NULL);
>   
>       ide_drive_get(hd, ARRAY_SIZE(hd));
>       for (i = 0; i < MAX_IDE_BUS; i++) {

Re: [PATCH v6 15/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always true in pc_init1()

[Xiaoyao Li]

On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
> PCI is always enabled on the pc-i440fx machine so hardcode the relevant logic
> in pc_init1().
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

some nits below, otherwise

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 194 ++++++++++++++++++----------------------------
>   1 file changed, 76 insertions(+), 118 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 49bd1a41e7..a776998504 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -71,7 +71,7 @@
>   
>   #define XEN_IOAPIC_NUM_PIRQS 128ULL
>   
> -#ifdef CONFIG_IDE_ISA
> +#ifdef CONFIG_ISAPC

This seems unrelated to this patch?

>   static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
>   static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>   static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
> @@ -125,6 +125,9 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>       MemoryRegion *rom_memory = system_memory;
>       ram_addr_t lowmem;
>       uint64_t hole64_size = 0;
> +    PCIDevice *pci_dev;
> +    DeviceState *dev;
> +    size_t i;

Suggest adding

	assert(pcmc->pci_enabled);

like what pc_q35_init() does.

>       /*
>        * Calculate ram split, for memory below and above 4G.  It's a bit
> @@ -195,38 +198,36 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>           kvmclock_create(pcmc->kvmclock_create_always);
>       }
>   
> -    if (pcmc->pci_enabled) {
> -        pci_memory = g_new(MemoryRegion, 1);
> -        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> -        rom_memory = pci_memory;
> -
> -        phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
> -        object_property_add_child(OBJECT(machine), "i440fx", phb);
> -        object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
> -                                 OBJECT(ram_memory), &error_fatal);
> -        object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
> -                                 OBJECT(pci_memory), &error_fatal);
> -        object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
> -                                 OBJECT(system_memory), &error_fatal);
> -        object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
> -                                 OBJECT(system_io), &error_fatal);
> -        object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
> -                                 x86ms->below_4g_mem_size, &error_fatal);
> -        object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
> -                                 x86ms->above_4g_mem_size, &error_fatal);
> -        object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
> -                                &error_fatal);
> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
> -
> -        pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
> -        pci_bus_map_irqs(pcms->pcibus,
> -                         xen_enabled() ? xen_pci_slot_get_pirq
> -                                       : pc_pci_slot_get_pirq);
> -
> -        hole64_size = object_property_get_uint(phb,
> -                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
> -                                               &error_abort);
> -    }
> +    pci_memory = g_new(MemoryRegion, 1);
> +    memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> +    rom_memory = pci_memory;
> +
> +    phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
> +    object_property_add_child(OBJECT(machine), "i440fx", phb);
> +    object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
> +                                OBJECT(ram_memory), &error_fatal);
> +    object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
> +                                OBJECT(pci_memory), &error_fatal);
> +    object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
> +                                OBJECT(system_memory), &error_fatal);
> +    object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
> +                                OBJECT(system_io), &error_fatal);
> +    object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
> +                                x86ms->below_4g_mem_size, &error_fatal);
> +    object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
> +                                x86ms->above_4g_mem_size, &error_fatal);
> +    object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
> +                            &error_fatal);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
> +
> +    pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
> +    pci_bus_map_irqs(pcms->pcibus,
> +                        xen_enabled() ? xen_pci_slot_get_pirq
> +                                    : pc_pci_slot_get_pirq);
> +
> +    hole64_size = object_property_get_uint(phb,
> +                                            PCI_HOST_PROP_PCI_HOLE64_SIZE,
> +                                            &error_abort);
>   
>       /* allocate ram and load rom/bios */
>       if (!xen_enabled()) {
> @@ -242,72 +243,51 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>           }
>       }
>   
> -    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> -
> -    if (pcmc->pci_enabled) {
> -        PCIDevice *pci_dev;
> -        DeviceState *dev;
> -        size_t i;
> -
> -        pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
> -        object_property_set_bool(OBJECT(pci_dev), "has-usb",
> -                                 machine_usb(machine), &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
> -                                 x86_machine_is_acpi_enabled(x86ms),
> -                                 &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
> -                                 &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
> -                                 &error_abort);
> -        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
> -        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
> -                                 x86_machine_is_smm_enabled(x86ms),
> -                                 &error_abort);
> -        dev = DEVICE(pci_dev);
> -        for (i = 0; i < ISA_NUM_IRQS; i++) {
> -            qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
> -        }
> -        pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
> -
> -        if (xen_enabled()) {
> -            pci_device_set_intx_routing_notifier(
> -                        pci_dev, piix_intx_routing_notifier_xen);
> -
> -            /*
> -             * Xen supports additional interrupt routes from the PCI devices to
> -             * the IOAPIC: the four pins of each PCI device on the bus are also
> -             * connected to the IOAPIC directly.
> -             * These additional routes can be discovered through ACPI.
> -             */
> -            pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
> -                         XEN_IOAPIC_NUM_PIRQS);
> -        }
> +    gsi_state = pc_gsi_create(&x86ms->gsi, true);
> +
> +    pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
> +    object_property_set_bool(OBJECT(pci_dev), "has-usb",
> +                                machine_usb(machine), &error_abort);
> +    object_property_set_bool(OBJECT(pci_dev), "has-acpi",
> +                                x86_machine_is_acpi_enabled(x86ms),
> +                                &error_abort);
> +    object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
> +                                &error_abort);
> +    object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
> +                                &error_abort);
> +    qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
> +    object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
> +                                x86_machine_is_smm_enabled(x86ms),
> +                                &error_abort);
> +    dev = DEVICE(pci_dev);
> +    for (i = 0; i < ISA_NUM_IRQS; i++) {
> +        qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
> +    }
> +    pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
>   
> -        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
> -        x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
> -                                                              "rtc"));
> -        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
> -        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
> -        pci_ide_create_devs(PCI_DEVICE(dev));
> -        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
> -        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
> -    } else {
> -        uint32_t irq;
> +    if (xen_enabled()) {
> +        pci_device_set_intx_routing_notifier(
> +                    pci_dev, piix_intx_routing_notifier_xen);
>   
> -        isa_bus = isa_bus_new(NULL, system_memory, system_io,
> -                              &error_abort);
> -        isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
> +        /*
> +         * Xen supports additional interrupt routes from the PCI devices to
> +         * the IOAPIC: the four pins of each PCI device on the bus are also
> +         * connected to the IOAPIC directly.
> +         * These additional routes can be discovered through ACPI.
> +         */
> +        pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
> +                        XEN_IOAPIC_NUM_PIRQS);
> +    }
>   
> -        x86ms->rtc = isa_new(TYPE_MC146818_RTC);
> -        qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
> -        isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
> -        irq = object_property_get_uint(OBJECT(x86ms->rtc), "irq",
> -                                       &error_fatal);
> -        isa_connect_gpio_out(ISA_DEVICE(x86ms->rtc), 0, irq);
> +    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
> +    x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
> +                                                            "rtc"));
> +    piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
> +    dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
> +    pci_ide_create_devs(PCI_DEVICE(dev));
> +    pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
> +    pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
>   
> -        i8257_dma_init(OBJECT(machine), isa_bus, 0);
> -        pcms->hpet_enabled = false;
> -    }
>   
>       if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
>           pc_i8259_create(isa_bus, gsi_state->i8259_irq);
> @@ -321,7 +301,7 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>           x86_register_ferr_irq(x86ms->gsi[13]);
>       }
>   
> -    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
> +    pc_vga_init(isa_bus, pcms->pcibus);
>   
>       /* init basic PC hardware */
>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
> @@ -329,28 +309,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>   
>       pc_nic_init(pcmc, isa_bus, pcms->pcibus);
>   
> -#ifdef CONFIG_IDE_ISA
> -    if (!pcmc->pci_enabled) {
> -        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> -        int i;
> -
> -        ide_drive_get(hd, ARRAY_SIZE(hd));
> -        for (i = 0; i < MAX_IDE_BUS; i++) {
> -            ISADevice *dev;
> -            char busname[] = "ide.0";
> -            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
> -                               ide_irq[i],
> -                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
> -            /*
> -             * The ide bus name is ide.0 for the first bus and ide.1 for the
> -             * second one.
> -             */
> -            busname[4] = '0' + i;
> -            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
> -        }
> -    }
> -#endif
> -
>       if (piix4_pm) {
>           smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
>   

Re: [PATCH v6 16/19] hw/i386: move isapc machine to separate isapc.c file

[Xiaoyao Li]

On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
> Now that pc_init_isa() is independent of any PCI initialisation, move it into a
> separate isapc.c file. This enables us to finally fix the dependency of ISAPC on
> I440FX in hw/i386/Kconfig.
> 
> Note that as part of the move to a separate file we can see that the licence text
> is a verbatim copy of the MIT licence. The text originates from commit 1df912cf9e
> ("VL license of the day is MIT/BSD") so we can be sure that this was the original
> intent. As a consequence we can update the file header to use a SPDX tag as per
> the current project contribution guidelines.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

Re: [PATCH v6 17/19] hw/i386/pc_piix.c: remove unused headers after isapc machine split

[Xiaoyao Li]

On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
> The headers for isapc-only devices can be removed from pc_piix.c since they are
> no longer used by the i440fx-pc machine.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 05713662e4..5babdc3e4c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -27,19 +27,16 @@
>   
>   #include "qemu/units.h"
>   #include "hw/char/parallel-isa.h"
> -#include "hw/dma/i8257.h"
>   #include "hw/i386/x86.h"
>   #include "hw/i386/pc.h"
>   #include "hw/i386/apic.h"
>   #include "hw/pci-host/i440fx.h"
> -#include "hw/rtc/mc146818rtc.h"
>   #include "hw/southbridge/piix.h"
>   #include "hw/display/ramfb.h"
>   #include "hw/pci/pci.h"
>   #include "hw/pci/pci_ids.h"
>   #include "hw/usb.h"
>   #include "net/net.h"
> -#include "hw/ide/isa.h"
>   #include "hw/ide/pci.h"
>   #include "hw/irq.h"
>   #include "system/kvm.h"

Re: [PATCH v6 18/19] hw/i386/pc_piix.c: replace rom_memory with pci_memory

[Xiaoyao Li]

On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
> Now that we can guarantee the i440fx-pc machine will always have a PCI bus, any
> instances of rom_memory can be replaced by pci_memory and rom_memory removed
> completely.
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   hw/i386/pc_piix.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5babdc3e4c..b736f0f236 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -113,7 +113,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>       GSIState *gsi_state;
>       MemoryRegion *ram_memory;
>       MemoryRegion *pci_memory = NULL;
> -    MemoryRegion *rom_memory = system_memory;
>       ram_addr_t lowmem;
>       uint64_t hole64_size = 0;
>       PCIDevice *pci_dev;
> @@ -191,7 +190,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>   
>       pci_memory = g_new(MemoryRegion, 1);
>       memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> -    rom_memory = pci_memory;
>   
>       phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
>       object_property_add_child(OBJECT(machine), "i440fx", phb);
> @@ -222,7 +220,7 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>   
>       /* allocate ram and load rom/bios */
>       if (!xen_enabled()) {
> -        pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
> +        pc_memory_init(pcms, system_memory, pci_memory, hole64_size);
>       } else {
>           assert(machine->ram_size == x86ms->below_4g_mem_size +
>                                       x86ms->above_4g_mem_size);

Re: [PATCH v6 19/19] hw/i386/isapc.c: replace rom_memory with system_memory

[Xiaoyao Li]

On 8/22/2025 8:12 PM, 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.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 62acdada3e..87193beb44 100644
> --- a/hw/i386/isapc.c
> +++ b/hw/i386/isapc.c
> @@ -37,7 +37,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;
> @@ -76,7 +75,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);
>       } else {
>           assert(machine->ram_size == x86ms->below_4g_mem_size +
>                                       x86ms->above_4g_mem_size);

Re: [PATCH v6 04/19] hw/i386/pc_piix.c: duplicate pc_init1() into pc_isa_init()

[Mark Cave-Ayland]

On 26/08/2025 10:21, Xiaoyao Li wrote:

> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>> This is to prepare for splitting the isapc machine into its own 
>> separate file.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c | 261 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 260 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 071bf7412e..5b97ad2cea 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
> 
> ...
> 
>> +    if (piix4_pm) {
>> +        smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 
>> 0);
>> +
>> +        qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, 
>> smi_irq);
>> +        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), 
>> "i2c"));
>> +        /* TODO: Populate SPD eeprom data.  */
>> +        smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
>> +
>> +        object_property_add_link(OBJECT(machine), 
>> PC_MACHINE_ACPI_DEVICE_PROP,
>> +                                 TYPE_HOTPLUG_HANDLER,
>> +                                 (Object **)&x86ms->acpi_dev,
>> +                                 object_property_allow_set_link,
>> +                                 OBJ_PROP_LINK_STRONG);
>> +        object_property_set_link(OBJECT(machine), 
>> PC_MACHINE_ACPI_DEVICE_PROP,
>> +                                 piix4_pm, &error_abort);
>> +    }
>> +
>> +    if (machine->nvdimms_state->is_enabled) {
>> +        nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
>> +                               x86_nvdimm_acpi_dsmio,
>> +                               x86ms->fw_cfg, OBJECT(pcms));
>> +    }
> 
> IGVM support was merged recently and the rebase of this series doesn't 
> catch it.

Indeed, thanks for spotting this!

> I think we need to copy the IGVM related code as well, in this patch.
> We can remove it later in a separate patch if it's unnecessary for isapc.

Yes, it looks like we can just simply remove it in a separate patch 
since according to the docs at 
https://gitlab.com/qemu-project/qemu/-/blob/master/qemu-options.hx?ref_type=heads#L6186 
IGVM is only supported for the q35 and pc machines.

>>   }
>>   #endif


ATB,

Mark.

Re: [PATCH v6 09/19] hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()

[Mark Cave-Ayland]

On 26/08/2025 11:01, Xiaoyao Li wrote:

> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>> All isapc machines must have 32-bit CPUs and so the RAM split logic 
>> can be hardcoded
>> accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> ---
>>   hw/i386/pc_piix.c | 58 ++++-------------------------------------------
>>   1 file changed, 4 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 816124c027..fc94937ad4 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -444,69 +444,19 @@ static void pc_init_isa(MachineState *machine)
>>       GSIState *gsi_state;
>>       MemoryRegion *ram_memory;
>>       MemoryRegion *rom_memory = system_memory;
>> -    ram_addr_t lowmem;
>>       uint64_t hole64_size = 0;
>>       /*
>> -     * Calculate ram split, for memory below and above 4G.  It's a bit
>> -     * complicated for backward compatibility reasons ...
>> -     *
>> -     *  - Traditional split is 3.5G (lowmem = 0xe0000000).  This is the
>> -     *    default value for max_ram_below_4g now.
>> -     *
>> -     *  - Then, to gigabyte align the memory, we move the split to 3G
>> -     *    (lowmem = 0xc0000000).  But only in case we have to split in
>> -     *    the first place, i.e. ram_size is larger than (traditional)
>> -     *    lowmem.  And for new machine types (gigabyte_align = true)
>> -     *    only, for live migration compatibility reasons.
>> -     *
>> -     *  - Next the max-ram-below-4g option was added, which allowed to
>> -     *    reduce lowmem to a smaller value, to allow a larger PCI I/O
>> -     *    window below 4G.  qemu doesn't enforce gigabyte alignment 
>> here,
>> -     *    but prints a warning.
>> -     *
>> -     *  - Finally max-ram-below-4g got updated to also allow raising 
>> lowmem,
>> -     *    so legacy non-PAE guests can get as much memory as possible in
>> -     *    the 32bit address space below 4G.
>> -     *
>> -     *  - Note that Xen has its own ram setup code in xen_ram_init(),
>> -     *    called via xen_hvm_init_pc().
>> -     *
>> -     * Examples:
>> -     *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low,  
>> 512M high
>> -     *    qemu -M pc -m 4G        (new default)    -> 3072M low, 
>> 1024M high
>> -     *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 
>> 2048M high
>> -     *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low 
>> (=4G-128M)
>> +     * There is no RAM split for the isapc machine
>>        */
>>       if (xen_enabled()) {
>>           xen_hvm_init_pc(pcms, &ram_memory);
>>       } else {
>>           ram_memory = machine->ram;
>> -        if (!pcms->max_ram_below_4g) {
>> -            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
>> -        }
>> -        lowmem = pcms->max_ram_below_4g;
>> -        if (machine->ram_size >= pcms->max_ram_below_4g) {
>> -            if (pcmc->gigabyte_align) {
>> -                if (lowmem > 0xc0000000) {
>> -                    lowmem = 0xc0000000;
>> -                }
>> -                if (lowmem & (1 * GiB - 1)) {
>> -                    warn_report("Large machine and max_ram_below_4g "
>> -                                "(%" PRIu64 ") not a multiple of 1G; "
>> -                                "possible bad performance.",
>> -                                pcms->max_ram_below_4g);
>> -                }
>> -            }
>> -        }
>> -        if (machine->ram_size >= lowmem) {
>> -            x86ms->above_4g_mem_size = machine->ram_size - lowmem;
>> -            x86ms->below_4g_mem_size = lowmem;
>> -        } else {
>> -            x86ms->above_4g_mem_size = 0;
>> -            x86ms->below_4g_mem_size = machine->ram_size;
>> -        }
>> +        pcms->max_ram_below_4g = 4 * GiB;
>> +        x86ms->above_4g_mem_size = 0;
>> +        x86ms->below_4g_mem_size = machine->ram_size;
> 
> I think we need to sanity check the machine->ram_size is not bigger than 
> 4G, and error out if it exceeds.

Amazingly there is currently no limit for the isapc machine, but I shall 
add it in for v7.

>>       }
>>       /*


ATB,

Mark.

Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Mark Cave-Ayland]

On 26/08/2025 08:25, Xiaoyao Li wrote:

> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
>> possible to specify any CPU via -cpu on the command line, it makes no
>> sense to allow modern 64-bit CPUs to be used.
>>
>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>> handle the case where if a user inadvertently uses -cpu max then the 
>> "best"
>> 32-bit CPU is used (in this case the pentium3).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index c03324281b..5720b6b556 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int 
>> value, Error **errp)
>>   #ifdef CONFIG_ISAPC
>>   static void pc_init_isa(MachineState *machine)
>>   {
>> +    /*
>> +     * There is a small chance that someone unintentionally passes "- 
>> cpu max"
>> +     * for the isapc machine, which will provide a much more modern 
>> 32-bit
>> +     * CPU than would be expected for an ISA-era PC. If the "max" cpu 
>> type has
>> +     * been specified, choose the "best" 32-bit cpu possible which we 
>> consider
>> +     * be the pentium3 (deliberately choosing an Intel CPU given that 
>> the
>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>> +     */
>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>> +        warn_report("-cpu max is invalid for isapc machine, using 
>> pentium3");
>> +    }
> 
> Do we need to handle the case of "-cpu host"?

I don't believe so. I wasn't originally planning to support "-cpu max" 
for isapc, however Daniel mentioned that it could possibly be generated 
from libvirt so it makes sense to add the above check to warn in this 
case and then continue.

>>       pc_init1(machine, NULL);
>>   }
>>   #endif
>> @@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
>>   #ifdef CONFIG_ISAPC
>>   static void isapc_machine_options(MachineClass *m)
>>   {
>> +    static const char * const valid_cpu_types[] = {
>> +        X86_CPU_TYPE_NAME("486"),
>> +        X86_CPU_TYPE_NAME("athlon"),
>> +        X86_CPU_TYPE_NAME("kvm32"),
>> +        X86_CPU_TYPE_NAME("pentium"),
>> +        X86_CPU_TYPE_NAME("pentium2"),
>> +        X86_CPU_TYPE_NAME("pentium3"),
>> +        X86_CPU_TYPE_NAME("qemu32"),
>> +        X86_CPU_TYPE_NAME("max"),
>> +        NULL
>> +    };
>>       PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>> +
>>       m->desc = "ISA-only PC";
>>       m->max_cpus = 1;
>>       m->option_rom_has_mr = true;
>> @@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
>>       pcmc->has_reserved_memory = false;
>>       m->default_nic = "ne2k_isa";
>>       m->default_cpu_type = X86_CPU_TYPE_NAME("486");
>> +    m->valid_cpu_types = valid_cpu_types;
>>       m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
>>       m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>>   }


ATB,

Mark.

Re: [PATCH v6 13/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always false in pc_init_isa()

[Mark Cave-Ayland]

On 26/08/2025 10:50, Xiaoyao Li wrote:

> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>> By definition PCI can never be enabled on an isapc machine so hardcode 
>> the relevant values
>> set via pcmc->pci_enabled.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> It seems to me that this patch can be squashed into patch 05
> 
>> ---
>>   hw/i386/pc_piix.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 924888b59d..e8603b585a 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -492,7 +492,7 @@ static void pc_init_isa(MachineState *machine)
>>           }
>>       }
>> -    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
>> +    gsi_state = pc_gsi_create(&x86ms->gsi, false);
>>       isa_bus = isa_bus_new(NULL, system_memory, system_io,
>>                               &error_abort);
>> @@ -513,7 +513,7 @@ static void pc_init_isa(MachineState *machine)
>>           x86_register_ferr_irq(x86ms->gsi[13]);
>>       }
>> -    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
>> +    pc_vga_init(isa_bus, NULL);
>>       /* init basic PC hardware */
>>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,

Indeed, that seems to work fine so I will make this change in the next 
version.


ATB,

Mark.

Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Daniel P. Berrangé]

On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
> On 26/08/2025 08:25, Xiaoyao Li wrote:
> 
> > On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> > > The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> > > possible to specify any CPU via -cpu on the command line, it makes no
> > > sense to allow modern 64-bit CPUs to be used.
> > > 
> > > Restrict the isapc machine to the available 32-bit CPUs, taking care to
> > > handle the case where if a user inadvertently uses -cpu max then the
> > > "best"
> > > 32-bit CPU is used (in this case the pentium3).
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index c03324281b..5720b6b556 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> > > int value, Error **errp)
> > >   #ifdef CONFIG_ISAPC
> > >   static void pc_init_isa(MachineState *machine)
> > >   {
> > > +    /*
> > > +     * There is a small chance that someone unintentionally passes
> > > "- cpu max"
> > > +     * for the isapc machine, which will provide a much more modern
> > > 32-bit
> > > +     * CPU than would be expected for an ISA-era PC. If the "max"
> > > cpu type has
> > > +     * been specified, choose the "best" 32-bit cpu possible which
> > > we consider
> > > +     * be the pentium3 (deliberately choosing an Intel CPU given
> > > that the
> > > +     * default 486 CPU for the isapc machine is also an Intel CPU).
> > > +     */
> > > +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> > > +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> > > +        warn_report("-cpu max is invalid for isapc machine, using
> > > pentium3");
> > > +    }
> > 
> > Do we need to handle the case of "-cpu host"?
> 
> I don't believe so. I wasn't originally planning to support "-cpu max" for
> isapc, however Daniel mentioned that it could possibly be generated from
> libvirt so it makes sense to add the above check to warn in this case and
> then continue.

Libvirt will support sending any valid -cpu flag, including both
'max' (any config) and 'host' (if KVM).

If 'isapc' still expects to support KVM, then it would be odd to
reject 'host', but KVM presumably has no built-in way to limit to
32-bit without QEMU manually masking many features ?

I'm a little worried about implications of libvirt sending '-cpu max'
and QEMU secretly turning that into '-cpu pentium3', as opposed to
having '-cpu max' expand to equiv to 'pentium3', which might cauase
confusion when libvirt queries the expanded CPU ? Copying Jiri for
an opinion from libvirt side, as I might be worrying about nothing.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Xiaoyao Li]

On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
> On 26/08/2025 08:25, Xiaoyao Li wrote:
> 
>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst 
>>> it is
>>> possible to specify any CPU via -cpu on the command line, it makes no
>>> sense to allow modern 64-bit CPUs to be used.
>>>
>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>> handle the case where if a user inadvertently uses -cpu max then the 
>>> "best"
>>> 32-bit CPU is used (in this case the pentium3).
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index c03324281b..5720b6b556 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int 
>>> value, Error **errp)
>>>   #ifdef CONFIG_ISAPC
>>>   static void pc_init_isa(MachineState *machine)
>>>   {
>>> +    /*
>>> +     * There is a small chance that someone unintentionally passes 
>>> "- cpu max"
>>> +     * for the isapc machine, which will provide a much more modern 
>>> 32-bit
>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>> cpu type has
>>> +     * been specified, choose the "best" 32-bit cpu possible which 
>>> we consider
>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>> that the
>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>> +     */
>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>> pentium3");
>>> +    }
>>
>> Do we need to handle the case of "-cpu host"?
> 
> I don't believe so. I wasn't originally planning to support "-cpu max" 
> for isapc, however Daniel mentioned that it could possibly be generated 
> from libvirt so it makes sense to add the above check to warn in this 
> case and then continue.

"host" cpu type is the child of "max", so "-cpu host" will pass in the 
is_cpu_type_supported(), the same as "max".

While we are changing "max" to "pentium3", I think it needs to do the 
same for "host". Otherwise, "-cpu host" won't get any warning and expose 
the native features to the VMs that are mostly not supposed to exist for 
isapc, e.g., the LM CUPID bit.

Re: [PATCH v6 15/19] hw/i386/pc_piix.c: assume pcmc->pci_enabled is always true in pc_init1()

[Mark Cave-Ayland]

On 26/08/2025 13:09, Xiaoyao Li wrote:

> On 8/22/2025 8:12 PM, Mark Cave-Ayland wrote:
>> PCI is always enabled on the pc-i440fx machine so hardcode the 
>> relevant logic
>> in pc_init1().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> some nits below, otherwise
> 
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
>> ---
>>   hw/i386/pc_piix.c | 194 ++++++++++++++++++----------------------------
>>   1 file changed, 76 insertions(+), 118 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 49bd1a41e7..a776998504 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -71,7 +71,7 @@
>>   #define XEN_IOAPIC_NUM_PIRQS 128ULL
>> -#ifdef CONFIG_IDE_ISA
>> +#ifdef CONFIG_ISAPC
> 
> This seems unrelated to this patch?

Ooops I think this must have slipped in when working on an earlier 
version of the series, but I believe it can just be removed.

>>   static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
>>   static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>>   static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>> @@ -125,6 +125,9 @@ static void pc_init1(MachineState *machine, const 
>> char *pci_type)
>>       MemoryRegion *rom_memory = system_memory;
>>       ram_addr_t lowmem;
>>       uint64_t hole64_size = 0;
>> +    PCIDevice *pci_dev;
>> +    DeviceState *dev;
>> +    size_t i;
> 
> Suggest adding
> 
>      assert(pcmc->pci_enabled);
> 
> like what pc_q35_init() does.

Good idea! I'll add this into the next version.

>>       /*
>>        * Calculate ram split, for memory below and above 4G.  It's a bit
>> @@ -195,38 +198,36 @@ static void pc_init1(MachineState *machine, 
>> const char *pci_type)
>>           kvmclock_create(pcmc->kvmclock_create_always);
>>       }
>> -    if (pcmc->pci_enabled) {
>> -        pci_memory = g_new(MemoryRegion, 1);
>> -        memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>> -        rom_memory = pci_memory;
>> -
>> -        phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
>> -        object_property_add_child(OBJECT(machine), "i440fx", phb);
>> -        object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
>> -                                 OBJECT(ram_memory), &error_fatal);
>> -        object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
>> -                                 OBJECT(pci_memory), &error_fatal);
>> -        object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
>> -                                 OBJECT(system_memory), &error_fatal);
>> -        object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
>> -                                 OBJECT(system_io), &error_fatal);
>> -        object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
>> -                                 x86ms->below_4g_mem_size, 
>> &error_fatal);
>> -        object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
>> -                                 x86ms->above_4g_mem_size, 
>> &error_fatal);
>> -        object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, 
>> pci_type,
>> -                                &error_fatal);
>> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
>> -
>> -        pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), 
>> "pci.0"));
>> -        pci_bus_map_irqs(pcms->pcibus,
>> -                         xen_enabled() ? xen_pci_slot_get_pirq
>> -                                       : pc_pci_slot_get_pirq);
>> -
>> -        hole64_size = object_property_get_uint(phb,
>> -                                               
>> PCI_HOST_PROP_PCI_HOLE64_SIZE,
>> -                                               &error_abort);
>> -    }
>> +    pci_memory = g_new(MemoryRegion, 1);
>> +    memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>> +    rom_memory = pci_memory;
>> +
>> +    phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));
>> +    object_property_add_child(OBJECT(machine), "i440fx", phb);
>> +    object_property_set_link(phb, PCI_HOST_PROP_RAM_MEM,
>> +                                OBJECT(ram_memory), &error_fatal);
>> +    object_property_set_link(phb, PCI_HOST_PROP_PCI_MEM,
>> +                                OBJECT(pci_memory), &error_fatal);
>> +    object_property_set_link(phb, PCI_HOST_PROP_SYSTEM_MEM,
>> +                                OBJECT(system_memory), &error_fatal);
>> +    object_property_set_link(phb, PCI_HOST_PROP_IO_MEM,
>> +                                OBJECT(system_io), &error_fatal);
>> +    object_property_set_uint(phb, PCI_HOST_BELOW_4G_MEM_SIZE,
>> +                                x86ms->below_4g_mem_size, &error_fatal);
>> +    object_property_set_uint(phb, PCI_HOST_ABOVE_4G_MEM_SIZE,
>> +                                x86ms->above_4g_mem_size, &error_fatal);
>> +    object_property_set_str(phb, I440FX_HOST_PROP_PCI_TYPE, pci_type,
>> +                            &error_fatal);
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
>> +
>> +    pcms->pcibus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
>> +    pci_bus_map_irqs(pcms->pcibus,
>> +                        xen_enabled() ? xen_pci_slot_get_pirq
>> +                                    : pc_pci_slot_get_pirq);
>> +
>> +    hole64_size = object_property_get_uint(phb,
>> +                                            
>> PCI_HOST_PROP_PCI_HOLE64_SIZE,
>> +                                            &error_abort);
>>       /* allocate ram and load rom/bios */
>>       if (!xen_enabled()) {
>> @@ -242,72 +243,51 @@ static void pc_init1(MachineState *machine, 
>> const char *pci_type)
>>           }
>>       }
>> -    gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
>> -
>> -    if (pcmc->pci_enabled) {
>> -        PCIDevice *pci_dev;
>> -        DeviceState *dev;
>> -        size_t i;
>> -
>> -        pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
>> -        object_property_set_bool(OBJECT(pci_dev), "has-usb",
>> -                                 machine_usb(machine), &error_abort);
>> -        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
>> -                                 x86_machine_is_acpi_enabled(x86ms),
>> -                                 &error_abort);
>> -        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
>> -                                 &error_abort);
>> -        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
>> -                                 &error_abort);
>> -        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
>> -        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
>> -                                 x86_machine_is_smm_enabled(x86ms),
>> -                                 &error_abort);
>> -        dev = DEVICE(pci_dev);
>> -        for (i = 0; i < ISA_NUM_IRQS; i++) {
>> -            qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms- 
>> >gsi[i]);
>> -        }
>> -        pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
>> -
>> -        if (xen_enabled()) {
>> -            pci_device_set_intx_routing_notifier(
>> -                        pci_dev, piix_intx_routing_notifier_xen);
>> -
>> -            /*
>> -             * Xen supports additional interrupt routes from the PCI 
>> devices to
>> -             * the IOAPIC: the four pins of each PCI device on the 
>> bus are also
>> -             * connected to the IOAPIC directly.
>> -             * These additional routes can be discovered through ACPI.
>> -             */
>> -            pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
>> -                         XEN_IOAPIC_NUM_PIRQS);
>> -        }
>> +    gsi_state = pc_gsi_create(&x86ms->gsi, true);
>> +
>> +    pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
>> +    object_property_set_bool(OBJECT(pci_dev), "has-usb",
>> +                                machine_usb(machine), &error_abort);
>> +    object_property_set_bool(OBJECT(pci_dev), "has-acpi",
>> +                                x86_machine_is_acpi_enabled(x86ms),
>> +                                &error_abort);
>> +    object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
>> +                                &error_abort);
>> +    object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
>> +                                &error_abort);
>> +    qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
>> +    object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
>> +                                x86_machine_is_smm_enabled(x86ms),
>> +                                &error_abort);
>> +    dev = DEVICE(pci_dev);
>> +    for (i = 0; i < ISA_NUM_IRQS; i++) {
>> +        qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
>> +    }
>> +    pci_realize_and_unref(pci_dev, pcms->pcibus, &error_fatal);
>> -        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
>> -        x86ms->rtc = 
>> ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
>> -                                                              "rtc"));
>> -        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
>> -        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), 
>> "ide"));
>> -        pci_ide_create_devs(PCI_DEVICE(dev));
>> -        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
>> -        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
>> -    } else {
>> -        uint32_t irq;
>> +    if (xen_enabled()) {
>> +        pci_device_set_intx_routing_notifier(
>> +                    pci_dev, piix_intx_routing_notifier_xen);
>> -        isa_bus = isa_bus_new(NULL, system_memory, system_io,
>> -                              &error_abort);
>> -        isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
>> +        /*
>> +         * Xen supports additional interrupt routes from the PCI 
>> devices to
>> +         * the IOAPIC: the four pins of each PCI device on the bus 
>> are also
>> +         * connected to the IOAPIC directly.
>> +         * These additional routes can be discovered through ACPI.
>> +         */
>> +        pci_bus_irqs(pcms->pcibus, xen_intx_set_irq, pci_dev,
>> +                        XEN_IOAPIC_NUM_PIRQS);
>> +    }
>> -        x86ms->rtc = isa_new(TYPE_MC146818_RTC);
>> -        qdev_prop_set_int32(DEVICE(x86ms->rtc), "base_year", 2000);
>> -        isa_realize_and_unref(x86ms->rtc, isa_bus, &error_fatal);
>> -        irq = object_property_get_uint(OBJECT(x86ms->rtc), "irq",
>> -                                       &error_fatal);
>> -        isa_connect_gpio_out(ISA_DEVICE(x86ms->rtc), 0, irq);
>> +    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
>> +    x86ms->rtc = 
>> ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
>> +                                                            "rtc"));
>> +    piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
>> +    dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>> +    pci_ide_create_devs(PCI_DEVICE(dev));
>> +    pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
>> +    pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
>> -        i8257_dma_init(OBJECT(machine), isa_bus, 0);
>> -        pcms->hpet_enabled = false;
>> -    }
>>       if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == 
>> ON_OFF_AUTO_AUTO) {
>>           pc_i8259_create(isa_bus, gsi_state->i8259_irq);
>> @@ -321,7 +301,7 @@ static void pc_init1(MachineState *machine, const 
>> char *pci_type)
>>           x86_register_ferr_irq(x86ms->gsi[13]);
>>       }
>> -    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
>> +    pc_vga_init(isa_bus, pcms->pcibus);
>>       /* init basic PC hardware */
>>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
>> @@ -329,28 +309,6 @@ static void pc_init1(MachineState *machine, const 
>> char *pci_type)
>>       pc_nic_init(pcmc, isa_bus, pcms->pcibus);
>> -#ifdef CONFIG_IDE_ISA
>> -    if (!pcmc->pci_enabled) {
>> -        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>> -        int i;
>> -
>> -        ide_drive_get(hd, ARRAY_SIZE(hd));
>> -        for (i = 0; i < MAX_IDE_BUS; i++) {
>> -            ISADevice *dev;
>> -            char busname[] = "ide.0";
>> -            dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
>> -                               ide_irq[i],
>> -                               hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS 
>> * i + 1]);
>> -            /*
>> -             * The ide bus name is ide.0 for the first bus and ide.1 
>> for the
>> -             * second one.
>> -             */
>> -            busname[4] = '0' + i;
>> -            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
>> -        }
>> -    }
>> -#endif
>> -
>>       if (piix4_pm) {
>>           smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, 
>> first_cpu, 0);


ATB,

Mark.

Re: [PATCH v6 09/19] hw/i386/pc_piix.c: simplify RAM size logic in pc_init_isa()

[Mark Cave-Ayland]

On 27/08/2025 12:00, Mark Cave-Ayland wrote:

> On 26/08/2025 11:01, Xiaoyao Li wrote:
> 
>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>> All isapc machines must have 32-bit CPUs and so the RAM split logic 
>>> can be hardcoded
>>> accordingly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>> ---
>>>   hw/i386/pc_piix.c | 58 ++++-------------------------------------------
>>>   1 file changed, 4 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 816124c027..fc94937ad4 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -444,69 +444,19 @@ static void pc_init_isa(MachineState *machine)
>>>       GSIState *gsi_state;
>>>       MemoryRegion *ram_memory;
>>>       MemoryRegion *rom_memory = system_memory;
>>> -    ram_addr_t lowmem;
>>>       uint64_t hole64_size = 0;
>>>       /*
>>> -     * Calculate ram split, for memory below and above 4G.  It's a bit
>>> -     * complicated for backward compatibility reasons ...
>>> -     *
>>> -     *  - Traditional split is 3.5G (lowmem = 0xe0000000).  This is the
>>> -     *    default value for max_ram_below_4g now.
>>> -     *
>>> -     *  - Then, to gigabyte align the memory, we move the split to 3G
>>> -     *    (lowmem = 0xc0000000).  But only in case we have to split in
>>> -     *    the first place, i.e. ram_size is larger than (traditional)
>>> -     *    lowmem.  And for new machine types (gigabyte_align = true)
>>> -     *    only, for live migration compatibility reasons.
>>> -     *
>>> -     *  - Next the max-ram-below-4g option was added, which allowed to
>>> -     *    reduce lowmem to a smaller value, to allow a larger PCI I/O
>>> -     *    window below 4G.  qemu doesn't enforce gigabyte alignment 
>>> here,
>>> -     *    but prints a warning.
>>> -     *
>>> -     *  - Finally max-ram-below-4g got updated to also allow raising 
>>> lowmem,
>>> -     *    so legacy non-PAE guests can get as much memory as 
>>> possible in
>>> -     *    the 32bit address space below 4G.
>>> -     *
>>> -     *  - Note that Xen has its own ram setup code in xen_ram_init(),
>>> -     *    called via xen_hvm_init_pc().
>>> -     *
>>> -     * Examples:
>>> -     *    qemu -M pc-1.7 -m 4G    (old default)    -> 3584M low, 
>>> 512M high
>>> -     *    qemu -M pc -m 4G        (new default)    -> 3072M low, 
>>> 1024M high
>>> -     *    qemu -M pc,max-ram-below-4g=2G -m 4G     -> 2048M low, 
>>> 2048M high
>>> -     *    qemu -M pc,max-ram-below-4g=4G -m 3968M  -> 3968M low 
>>> (=4G-128M)
>>> +     * There is no RAM split for the isapc machine
>>>        */
>>>       if (xen_enabled()) {
>>>           xen_hvm_init_pc(pcms, &ram_memory);
>>>       } else {
>>>           ram_memory = machine->ram;
>>> -        if (!pcms->max_ram_below_4g) {
>>> -            pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */
>>> -        }
>>> -        lowmem = pcms->max_ram_below_4g;
>>> -        if (machine->ram_size >= pcms->max_ram_below_4g) {
>>> -            if (pcmc->gigabyte_align) {
>>> -                if (lowmem > 0xc0000000) {
>>> -                    lowmem = 0xc0000000;
>>> -                }
>>> -                if (lowmem & (1 * GiB - 1)) {
>>> -                    warn_report("Large machine and max_ram_below_4g "
>>> -                                "(%" PRIu64 ") not a multiple of 1G; "
>>> -                                "possible bad performance.",
>>> -                                pcms->max_ram_below_4g);
>>> -                }
>>> -            }
>>> -        }
>>> -        if (machine->ram_size >= lowmem) {
>>> -            x86ms->above_4g_mem_size = machine->ram_size - lowmem;
>>> -            x86ms->below_4g_mem_size = lowmem;
>>> -        } else {
>>> -            x86ms->above_4g_mem_size = 0;
>>> -            x86ms->below_4g_mem_size = machine->ram_size;
>>> -        }
>>> +        pcms->max_ram_below_4g = 4 * GiB;
>>> +        x86ms->above_4g_mem_size = 0;
>>> +        x86ms->below_4g_mem_size = machine->ram_size;
>>
>> I think we need to sanity check the machine->ram_size is not bigger 
>> than 4G, and error out if it exceeds.
> 
> Amazingly there is currently no limit for the isapc machine, but I shall 
> add it in for v7.

With the PCI hole removed it appears that TCG and KVM have a different 
idea as to the maximum allowable amount of RAM available:

Fails with KVM:
./build/qemu-system-x86_64 -accel kvm -M isapc -m 4079M
qemu-system-x86_64: kvm_set_user_memory_region: 
KVM_SET_USER_MEMORY_REGION failed, slot=0, start=0x0, size=0xfef00000: 
File exists
kvm_set_phys_mem: error registering slot: File exists

Works with TCG:
./build/qemu-system-x86_64 -accel tcg -M isapc -m 4096M

Given that the original limit above is 3.5G I think it's best to revert 
back to using 3.5G instead of 4G so that both TCG and KVM behave the 
same, whilst also allowing a bit of wiggle room if required.


ATB,

Mark.

Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

[Mark Cave-Ayland]

On 27/08/2025 12:50, Xiaoyao Li wrote:

> On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>
>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst 
>>>> it is
>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>> sense to allow modern 64-bit CPUs to be used.
>>>>
>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>>> handle the case where if a user inadvertently uses -cpu max then the 
>>>> "best"
>>>> 32-bit CPU is used (in this case the pentium3).
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>   1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index c03324281b..5720b6b556 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, 
>>>> int value, Error **errp)
>>>>   #ifdef CONFIG_ISAPC
>>>>   static void pc_init_isa(MachineState *machine)
>>>>   {
>>>> +    /*
>>>> +     * There is a small chance that someone unintentionally passes 
>>>> "- cpu max"
>>>> +     * for the isapc machine, which will provide a much more modern 
>>>> 32-bit
>>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>>> cpu type has
>>>> +     * been specified, choose the "best" 32-bit cpu possible which 
>>>> we consider
>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>>> that the
>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>> +     */
>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>>> pentium3");
>>>> +    }
>>>
>>> Do we need to handle the case of "-cpu host"?
>>
>> I don't believe so. I wasn't originally planning to support "-cpu max" 
>> for isapc, however Daniel mentioned that it could possibly be 
>> generated from libvirt so it makes sense to add the above check to 
>> warn in this case and then continue.
> 
> "host" cpu type is the child of "max", so "-cpu host" will pass in the 
> is_cpu_type_supported(), the same as "max".
> 
> While we are changing "max" to "pentium3", I think it needs to do the 
> same for "host". Otherwise, "-cpu host" won't get any warning and expose 
> the native features to the VMs that are mostly not supposed to exist for 
> isapc, e.g., the LM CUPID bit.

That makes sense given that for compatibility we would want to hide more 
modern features from the guest, particularly if running legacy OSs.

I'll make this change and likely post a v7 soon as there are quite a lot 
of changes here already, and wait for any further input from Jiri or Daniel.


ATB,

Mark.