[PATCH 0/9] Simplify initialization of PC machines

[PATCH 0/9] Simplify initialization of PC machines

[Bernhard Beschow]

The series aims to simplify the initialization process of all PC-based machines.

It consists of streamlining redundant code, as well as consolidating the setup
of system flash and generation of smbios data which are currently fairly
distributed.

These changes are expected to make the code easier to understand and maintain.

Best regards,
Bernhard

Bernhard Beschow (9):
  hw/i386/x86: Let ioapic_init_gsi() take parent as pointer
  hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc
    machines
  hw/i386/x86: Turn apic_xrupt_override into class attribute
  hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()
  hw/i386/pc: Defer smbios_set_defaults() to machine_done
  hw/i386/pc: Confine system flash handling to pc_sysfw
  hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it
  hw/i386/pc: Populate RTC attribute directly
  hw/i386/pc_{piix,q35}: Eliminate local pci_bus/pci_host variables

 hw/i386/fw_cfg.h      |  3 ++-
 include/hw/i386/pc.h  |  5 ----
 include/hw/i386/x86.h |  5 ++--
 hw/i386/acpi-common.c |  3 ++-
 hw/i386/fw_cfg.c      | 12 +++++++++-
 hw/i386/microvm.c     |  2 +-
 hw/i386/pc.c          | 25 +++++---------------
 hw/i386/pc_piix.c     | 55 ++++++++++++++-----------------------------
 hw/i386/pc_q35.c      | 38 ++++++++++--------------------
 hw/i386/pc_sysfw.c    | 17 ++++---------
 hw/i386/x86.c         |  7 +++---
 11 files changed, 62 insertions(+), 110 deletions(-)

-- 
2.43.0

[PATCH 1/9] hw/i386/x86: Let ioapic_init_gsi() take parent as pointer

[Bernhard Beschow]

Rather than taking a QOM name which has to be resolved, let's pass the parent
directly as pointer. This simplifies the code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h | 2 +-
 hw/i386/microvm.c     | 2 +-
 hw/i386/pc_piix.c     | 7 +++----
 hw/i386/pc_q35.c      | 4 ++--
 hw/i386/x86.c         | 7 +++----
 5 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index da19ae1546..6fe29fbb87 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -138,7 +138,7 @@ typedef struct GSIState {
 
 qemu_irq x86_allocate_cpu_irq(void);
 void gsi_handler(void *opaque, int n, int level);
-void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
+void ioapic_init_gsi(GSIState *gsi_state, Object *parent);
 DeviceState *ioapic_init_secondary(GSIState *gsi_state);
 
 /* pc_sysfw.c */
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index ca55aecc3b..61a772dfe6 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -175,7 +175,7 @@ static void microvm_devices_init(MicrovmMachineState *mms)
                           &error_abort);
     isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
 
-    ioapic_init_gsi(gsi_state, "machine");
+    ioapic_init_gsi(gsi_state, OBJECT(mms));
     if (ioapics > 1) {
         x86ms->ioapic2 = ioapic_init_secondary(gsi_state);
     }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 70d12bb1b5..5ed3d69181 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -109,6 +109,7 @@ static void pc_init1(MachineState *machine,
     X86MachineState *x86ms = X86_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
+    Object *phb = NULL;
     PCIBus *pci_bus = NULL;
     ISABus *isa_bus;
     Object *piix4_pm = NULL;
@@ -192,8 +193,6 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pcmc->pci_enabled) {
-        Object *phb;
-
         pci_memory = g_new(MemoryRegion, 1);
         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
         rom_memory = pci_memory;
@@ -320,8 +319,8 @@ static void pc_init1(MachineState *machine,
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
     }
 
-    if (pcmc->pci_enabled) {
-        ioapic_init_gsi(gsi_state, "i440fx");
+    if (phb) {
+        ioapic_init_gsi(gsi_state, phb);
     }
 
     if (tcg_enabled()) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..53da8b552d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -286,8 +286,8 @@ static void pc_q35_init(MachineState *machine)
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
     }
 
-    if (pcmc->pci_enabled) {
-        ioapic_init_gsi(gsi_state, "q35");
+    if (phb) {
+        ioapic_init_gsi(gsi_state, OBJECT(phb));
     }
 
     if (tcg_enabled()) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 2b6291ad8d..3b33d09a53 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -636,20 +636,19 @@ void gsi_handler(void *opaque, int n, int level)
     }
 }
 
-void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
+void ioapic_init_gsi(GSIState *gsi_state, Object *parent)
 {
     DeviceState *dev;
     SysBusDevice *d;
     unsigned int i;
 
-    assert(parent_name);
+    assert(parent);
     if (kvm_ioapic_in_kernel()) {
         dev = qdev_new(TYPE_KVM_IOAPIC);
     } else {
         dev = qdev_new(TYPE_IOAPIC);
     }
-    object_property_add_child(object_resolve_path(parent_name, NULL),
-                              "ioapic", OBJECT(dev));
+    object_property_add_child(parent, "ioapic", OBJECT(dev));
     d = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(d, &error_fatal);
     sysbus_mmio_map(d, 0, IO_APIC_DEFAULT_ADDRESS);
-- 
2.43.0

[PATCH 2/9] hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc machines

[Bernhard Beschow]

Both invocations are the same and either one is always executed. Avoid this
redundancy.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5ed3d69181..adb3b5ed43 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -340,11 +340,8 @@ static void pc_init1(MachineState *machine,
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
-    if (pcmc->pci_enabled) {
-        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
-    }
 #ifdef CONFIG_IDE_ISA
-    else {
+    if (!pcmc->pci_enabled) {
         DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
         int i;
 
@@ -362,10 +359,11 @@ static void pc_init1(MachineState *machine,
             busname[4] = '0' + i;
             idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
         }
-        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
     }
 #endif
 
+    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+
     if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
 
-- 
2.43.0

Re: [PATCH 2/9] hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc machines

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> Both invocations are the same and either one is always executed. Avoid this
> redundancy.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_piix.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)

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

[PATCH 3/9] hw/i386/x86: Turn apic_xrupt_override into class attribute

[Bernhard Beschow]

The attribute isn't user-changeable and only true for pc-based machines. Turn it
into a class attribute which allows for inlining pc_guest_info_init() into
pc_machine_initfn().

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/x86.h | 3 ++-
 hw/i386/acpi-common.c | 3 ++-
 hw/i386/pc.c          | 5 ++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 6fe29fbb87..4dc30dcb4d 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -34,6 +34,8 @@ struct X86MachineClass {
     bool save_tsc_khz;
     /* use DMA capable linuxboot option rom */
     bool fwcfg_dma_enabled;
+    /* CPU and apic information: */
+    bool apic_xrupt_override;
 };
 
 struct X86MachineState {
@@ -57,7 +59,6 @@ struct X86MachineState {
     uint64_t above_4g_mem_start;
 
     /* CPU and apic information: */
-    bool apic_xrupt_override;
     unsigned pci_irq_mask;
     unsigned apic_id_limit;
     uint16_t boot_cpus;
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
index 43dc23f7e0..cea4b3d71c 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -100,6 +100,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
     int i;
     bool x2apic_mode = false;
     MachineClass *mc = MACHINE_GET_CLASS(x86ms);
+    X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
     const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
     AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = oem_id,
                         .oem_table_id = oem_table_id };
@@ -122,7 +123,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
                      IO_APIC_SECONDARY_ADDRESS, IO_APIC_SECONDARY_IRQBASE);
     }
 
-    if (x86ms->apic_xrupt_override) {
+    if (x86mc->apic_xrupt_override) {
         build_xrupt_override(table_data, 0, 2,
             0 /* Flags: Conforms to the specifications of the bus */);
     }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 196827531a..12facf73b1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -708,9 +708,6 @@ void pc_machine_done(Notifier *notifier, void *data)
 
 void pc_guest_info_init(PCMachineState *pcms)
 {
-    X86MachineState *x86ms = X86_MACHINE(pcms);
-
-    x86ms->apic_xrupt_override = true;
     pcms->machine_done.notify = pc_machine_done;
     qemu_add_machine_init_done_notifier(&pcms->machine_done);
 }
@@ -1807,6 +1804,7 @@ static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp)
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
     PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
@@ -1826,6 +1824,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->pvh_enabled = true;
     pcmc->kvmclock_create_always = true;
     pcmc->resizable_acpi_blob = true;
+    x86mc->apic_xrupt_override = true;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = pc_get_hotplug_handler;
     mc->hotplug_allowed = pc_hotplug_allowed;
-- 
2.43.0

Re: [PATCH 3/9] hw/i386/x86: Turn apic_xrupt_override into class attribute

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> The attribute isn't user-changeable and only true for pc-based machines. Turn it
> into a class attribute which allows for inlining pc_guest_info_init() into
> pc_machine_initfn().
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/x86.h | 3 ++-
>   hw/i386/acpi-common.c | 3 ++-
>   hw/i386/pc.c          | 5 ++---
>   3 files changed, 6 insertions(+), 5 deletions(-)

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

[PATCH 4/9] hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()

[Bernhard Beschow]

Resolves redundant code in the piix and q35 machines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h | 2 --
 hw/i386/pc.c         | 9 +++------
 hw/i386/pc_piix.c    | 2 --
 hw/i386/pc_q35.c     | 2 --
 4 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ec0e5efcb2..287f1ab553 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -151,8 +151,6 @@ extern int fd_bootchk;
 
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-void pc_guest_info_init(PCMachineState *pcms);
-
 #define PCI_HOST_PROP_RAM_MEM          "ram-mem"
 #define PCI_HOST_PROP_PCI_MEM          "pci-mem"
 #define PCI_HOST_PROP_SYSTEM_MEM       "system-mem"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 12facf73b1..45738d8548 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -706,12 +706,6 @@ void pc_machine_done(Notifier *notifier, void *data)
     }
 }
 
-void pc_guest_info_init(PCMachineState *pcms)
-{
-    pcms->machine_done.notify = pc_machine_done;
-    qemu_add_machine_init_done_notifier(&pcms->machine_done);
-}
-
 /* setup pci memory address space mapping into system address space */
 void pc_pci_as_mapping_init(MemoryRegion *system_memory,
                             MemoryRegion *pci_address_space)
@@ -1751,6 +1745,9 @@ static void pc_machine_initfn(Object *obj)
     object_property_add_alias(OBJECT(pcms), "pcspk-audiodev",
                               OBJECT(pcms->pcspk), "audiodev");
     cxl_machine_init(obj, &pcms->cxl_devices_state);
+
+    pcms->machine_done.notify = pc_machine_done;
+    qemu_add_machine_init_done_notifier(&pcms->machine_done);
 }
 
 int pc_machine_kvm_type(MachineState *machine, const char *kvm_type)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index adb3b5ed43..4ca2dc08e7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -226,8 +226,6 @@ static void pc_init1(MachineState *machine,
                                                &error_abort);
     }
 
-    pc_guest_info_init(pcms);
-
     if (pcmc->smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
         /* These values are guest ABI, do not change */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 53da8b552d..fe0c2849fd 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -199,8 +199,6 @@ static void pc_q35_init(MachineState *machine)
         rom_memory = system_memory;
     }
 
-    pc_guest_info_init(pcms);
-
     if (pcmc->smbios_defaults) {
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", mc->desc,
-- 
2.43.0

Re: [PATCH 4/9] hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> Resolves redundant code in the piix and q35 machines.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/pc.h | 2 --
>   hw/i386/pc.c         | 9 +++------
>   hw/i386/pc_piix.c    | 2 --
>   hw/i386/pc_q35.c     | 2 --
>   4 files changed, 3 insertions(+), 12 deletions(-)

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

[PATCH 5/9] hw/i386/pc: Defer smbios_set_defaults() to machine_done

[Bernhard Beschow]

Handling most of smbios data generation in the machine_done notifier is similar
to how the ARM virt machine handles it which also calls smbios_set_defaults()
there. The result is that all pc machines are freed from explicitly worrying
about smbios setup.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/fw_cfg.h     |  3 ++-
 include/hw/i386/pc.h |  1 -
 hw/i386/fw_cfg.c     | 12 +++++++++++-
 hw/i386/pc.c         |  2 +-
 hw/i386/pc_piix.c    | 10 ----------
 hw/i386/pc_q35.c     |  9 ---------
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
index 86ca7c1c0c..1e1de6b4a3 100644
--- a/hw/i386/fw_cfg.h
+++ b/hw/i386/fw_cfg.h
@@ -10,6 +10,7 @@
 #define HW_I386_FW_CFG_H
 
 #include "hw/boards.h"
+#include "hw/i386/pc.h"
 #include "hw/nvram/fw_cfg.h"
 
 #define FW_CFG_IO_BASE     0x510
@@ -22,7 +23,7 @@
 FWCfgState *fw_cfg_arch_create(MachineState *ms,
                                uint16_t boot_cpus,
                                uint16_t apic_id_limit);
-void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg);
+void fw_cfg_build_smbios(PCMachineState *ms, FWCfgState *fw_cfg);
 void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg);
 void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 287f1ab553..0e6c41e908 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -12,7 +12,6 @@
 #include "hw/hotplug.h"
 #include "qom/object.h"
 #include "hw/i386/sgx-epc.h"
-#include "hw/firmware/smbios.h"
 #include "hw/cxl/cxl.h"
 
 #define HPET_INTCAP "hpet-intcap"
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 7362daa45a..98a478c276 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -48,15 +48,25 @@ const char *fw_cfg_arch_key_name(uint16_t key)
     return NULL;
 }
 
-void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
+void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg)
 {
 #ifdef CONFIG_SMBIOS
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
     struct smbios_phys_mem_area *mem_array;
     unsigned i, array_count;
+    MachineState *ms = MACHINE(pcms);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(pcms);
     X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
 
+    if (pcmc->smbios_defaults) {
+        /* These values are guest ABI, do not change */
+        smbios_set_defaults("QEMU", mc->desc, mc->name,
+                            pcmc->smbios_legacy_mode, pcmc->smbios_uuid_encoded,
+                            pcms->smbios_entry_point_type);
+    }
+
     /* tell smbios about cpuid version and features */
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 45738d8548..369c21fb99 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -699,7 +699,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (x86ms->fw_cfg) {
-        fw_cfg_build_smbios(MACHINE(pcms), x86ms->fw_cfg);
+        fw_cfg_build_smbios(pcms, x86ms->fw_cfg);
         fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4ca2dc08e7..5addaae978 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -36,7 +36,6 @@
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/southbridge/piix.h"
 #include "hw/display/ramfb.h"
-#include "hw/firmware/smbios.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
@@ -226,15 +225,6 @@ static void pc_init1(MachineState *machine,
                                                &error_abort);
     }
 
-    if (pcmc->smbios_defaults) {
-        MachineClass *mc = MACHINE_GET_CLASS(machine);
-        /* These values are guest ABI, do not change */
-        smbios_set_defaults("QEMU", mc->desc,
-                            mc->name, pcmc->smbios_legacy_mode,
-                            pcmc->smbios_uuid_encoded,
-                            pcms->smbios_entry_point_type);
-    }
-
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory, rom_memory, hole64_size);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index fe0c2849fd..5184abda92 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,7 +45,6 @@
 #include "hw/i386/amd_iommu.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/display/ramfb.h"
-#include "hw/firmware/smbios.h"
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/intc/ioapic.h"
@@ -199,14 +198,6 @@ static void pc_q35_init(MachineState *machine)
         rom_memory = system_memory;
     }
 
-    if (pcmc->smbios_defaults) {
-        /* These values are guest ABI, do not change */
-        smbios_set_defaults("QEMU", mc->desc,
-                            mc->name, pcmc->smbios_legacy_mode,
-                            pcmc->smbios_uuid_encoded,
-                            pcms->smbios_entry_point_type);
-    }
-
     /* create pci host bus */
     phb = OBJECT(qdev_new(TYPE_Q35_HOST_DEVICE));
 
-- 
2.43.0

Re: [PATCH 5/9] hw/i386/pc: Defer smbios_set_defaults() to machine_done

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> Handling most of smbios data generation in the machine_done notifier is similar
> to how the ARM virt machine handles it which also calls smbios_set_defaults()
> there. The result is that all pc machines are freed from explicitly worrying
> about smbios setup.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/fw_cfg.h     |  3 ++-
>   include/hw/i386/pc.h |  1 -
>   hw/i386/fw_cfg.c     | 12 +++++++++++-
>   hw/i386/pc.c         |  2 +-
>   hw/i386/pc_piix.c    | 10 ----------
>   hw/i386/pc_q35.c     |  9 ---------
>   6 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> index 86ca7c1c0c..1e1de6b4a3 100644
> --- a/hw/i386/fw_cfg.h
> +++ b/hw/i386/fw_cfg.h
> @@ -10,6 +10,7 @@
>   #define HW_I386_FW_CFG_H


> @@ -22,7 +23,7 @@
>   FWCfgState *fw_cfg_arch_create(MachineState *ms,
>                                  uint16_t boot_cpus,
>                                  uint16_t apic_id_limit);
> -void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg);
> +void fw_cfg_build_smbios(PCMachineState *ms, FWCfgState *fw_cfg);

s/ms/pcms/

Otherwise, nice cleanup:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 7362daa45a..98a478c276 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -48,15 +48,25 @@ const char *fw_cfg_arch_key_name(uint16_t key)
>       return NULL;
>   }
>   
> -void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
> +void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg)

[PATCH 6/9] hw/i386/pc: Confine system flash handling to pc_sysfw

[Bernhard Beschow]

Rather than distributing PC system flash handling across three files, let's
confine it to one. Now, pc_system_firmware_init() creates, configures and cleans
up the system flash which makes the code easier to understand. It also avoids
the extra call to pc_system_flash_cleanup_unused() in the Xen case.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h | 2 --
 hw/i386/pc.c         | 1 -
 hw/i386/pc_piix.c    | 1 -
 hw/i386/pc_sysfw.c   | 6 ++++--
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 0e6c41e908..77228822f5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -191,8 +191,6 @@ void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 #define TYPE_PORT92 "port92"
 
 /* pc_sysfw.c */
-void pc_system_flash_create(PCMachineState *pcms);
-void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
                                int *data_len);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 369c21fb99..4e73cd8a3b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1740,7 +1740,6 @@ static void pc_machine_initfn(Object *obj)
 #endif
     pcms->default_bus_bypass_iommu = false;
 
-    pc_system_flash_create(pcms);
     pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
     object_property_add_alias(OBJECT(pcms), "pcspk-audiodev",
                               OBJECT(pcms->pcspk), "audiodev");
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5addaae978..5c928ac71b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -232,7 +232,6 @@ static void pc_init1(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);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c8d9e71b88..b4c3833352 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -91,7 +91,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
     return PFLASH_CFI01(dev);
 }
 
-void pc_system_flash_create(PCMachineState *pcms)
+static void pc_system_flash_create(PCMachineState *pcms)
 {
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
 
@@ -103,7 +103,7 @@ void pc_system_flash_create(PCMachineState *pcms)
     }
 }
 
-void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 {
     char *prop_name;
     int i;
@@ -212,6 +212,8 @@ void pc_system_firmware_init(PCMachineState *pcms,
         return;
     }
 
+    pc_system_flash_create(pcms);
+
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         pflash_cfi01_legacy_drive(pcms->flash[i],
-- 
2.43.0

Re: [PATCH 6/9] hw/i386/pc: Confine system flash handling to pc_sysfw

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> Rather than distributing PC system flash handling across three files, let's
> confine it to one. Now, pc_system_firmware_init() creates, configures and cleans
> up the system flash which makes the code easier to understand. It also avoids
> the extra call to pc_system_flash_cleanup_unused() in the Xen case.

Maybe add " because pc_memory_init() is not called by Xen."

> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/pc.h | 2 --
>   hw/i386/pc.c         | 1 -
>   hw/i386/pc_piix.c    | 1 -
>   hw/i386/pc_sysfw.c   | 6 ++++--
>   4 files changed, 4 insertions(+), 6 deletions(-)

Nice!

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

[PATCH 7/9] hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it

[Bernhard Beschow]

pc_system_flash_create() checked for pcmc->pci_enabled which is redundant since
its caller already checked it. The method can be turned into just two lines, so
inline and remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_sysfw.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b4c3833352..2dcaa116ad 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -91,18 +91,6 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
     return PFLASH_CFI01(dev);
 }
 
-static void pc_system_flash_create(PCMachineState *pcms)
-{
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-
-    if (pcmc->pci_enabled) {
-        pcms->flash[0] = pc_pflash_create(pcms, "system.flash0",
-                                          "pflash0");
-        pcms->flash[1] = pc_pflash_create(pcms, "system.flash1",
-                                          "pflash1");
-    }
-}
-
 static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 {
     char *prop_name;
@@ -212,7 +200,8 @@ void pc_system_firmware_init(PCMachineState *pcms,
         return;
     }
 
-    pc_system_flash_create(pcms);
+    pcms->flash[0] = pc_pflash_create(pcms, "system.flash0", "pflash0");
+    pcms->flash[1] = pc_pflash_create(pcms, "system.flash1", "pflash1");
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
-- 
2.43.0

Re: [PATCH 7/9] hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> pc_system_flash_create() checked for pcmc->pci_enabled which is redundant since
> its caller already checked it. The method can be turned into just two lines, so
> inline and remove it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_sysfw.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)

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

[PATCH 8/9] hw/i386/pc: Populate RTC attribute directly

[Bernhard Beschow]

Both the piix and the q35 machines introduce an rtc_state variable and defer the
initialization of the X86MachineState::rtc attribute to pc_cmos_init(). Resolve
this complication which makes pc_cmos_init() do what it says on the tin.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c      |  8 --------
 hw/i386/pc_piix.c | 15 +++++++--------
 hw/i386/pc_q35.c  |  7 +++----
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4e73cd8a3b..ca74c6fbae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -617,14 +617,6 @@ void pc_cmos_init(PCMachineState *pcms,
     mc146818rtc_set_cmos_data(s, 0x5c, val >> 8);
     mc146818rtc_set_cmos_data(s, 0x5d, val >> 16);
 
-    object_property_add_link(OBJECT(pcms), "rtc_state",
-                             TYPE_ISA_DEVICE,
-                             (Object **)&x86ms->rtc,
-                             object_property_allow_set_link,
-                             OBJ_PROP_LINK_STRONG);
-    object_property_set_link(OBJECT(pcms), "rtc_state", OBJECT(s),
-                             &error_abort);
-
     set_boot_dev(s, MACHINE(pcms)->boot_config.order, &error_fatal);
 
     val = 0;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5c928ac71b..adb7926b2e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -115,7 +115,6 @@ static void pc_init1(MachineState *machine,
     qemu_irq smi_irq;
     GSIState *gsi_state;
     BusState *idebus[MAX_IDE_BUS];
-    ISADevice *rtc_state;
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory = NULL;
     MemoryRegion *rom_memory = system_memory;
@@ -280,8 +279,8 @@ static void pc_init1(MachineState *machine,
         }
 
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
-        rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
-                                                             "rtc"));
+        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));
@@ -292,9 +291,9 @@ static void pc_init1(MachineState *machine,
                               &error_abort);
         isa_bus_register_input_irqs(isa_bus, x86ms->gsi);
 
-        rtc_state = isa_new(TYPE_MC146818_RTC);
-        qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
-        isa_realize_and_unref(rtc_state, 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(isa_bus, 0);
         pcms->hpet_enabled = false;
@@ -322,7 +321,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, true,
                          0x4);
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
@@ -349,7 +348,7 @@ static void pc_init1(MachineState *machine,
     }
 #endif
 
-    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+    pc_cmos_init(pcms, idebus[0], idebus[1], x86ms->rtc);
 
     if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 5184abda92..d313ba5509 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -126,7 +126,6 @@ static void pc_q35_init(MachineState *machine)
     PCIDevice *lpc;
     DeviceState *lpc_dev;
     BusState *idebus[MAX_SATA_PORTS];
-    ISADevice *rtc_state;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
@@ -245,7 +244,7 @@ static void pc_q35_init(MachineState *machine)
     }
     pci_realize_and_unref(lpc, host_bus, &error_fatal);
 
-    rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
+    x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
@@ -289,7 +288,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, !mc->no_floppy,
                          0xff0104);
 
     if (pcms->sata_enabled) {
@@ -325,7 +324,7 @@ static void pc_q35_init(MachineState *machine)
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
     }
 
-    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+    pc_cmos_init(pcms, idebus[0], idebus[1], x86ms->rtc);
 
     /* the rest devices to which pci devfn is automatically assigned */
     pc_vga_init(isa_bus, host_bus);
-- 
2.43.0

[PATCH 9/9] hw/i386/pc_{piix, q35}: Eliminate local pci_bus/pci_host variables

[Bernhard Beschow]

There is no advantage in having these local variables which 1/ needlessly have
different identifiers in both machines and 2/ which are redundant to pcms->bus
which is almost as short.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 14 ++++++--------
 hw/i386/pc_q35.c  | 16 +++++++---------
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index adb7926b2e..a9f0e255ad 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -109,7 +109,6 @@ static void pc_init1(MachineState *machine,
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     Object *phb = NULL;
-    PCIBus *pci_bus = NULL;
     ISABus *isa_bus;
     Object *piix4_pm = NULL;
     qemu_irq smi_irq;
@@ -213,11 +212,10 @@ static void pc_init1(MachineState *machine,
                                 &error_fatal);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
 
-        pci_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
-        pci_bus_map_irqs(pci_bus,
+        pcms->bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pci.0"));
+        pci_bus_map_irqs(pcms->bus,
                          xen_enabled() ? xen_pci_slot_get_pirq
                                        : pc_pci_slot_get_pirq);
-        pcms->bus = pci_bus;
 
         hole64_size = object_property_get_uint(phb,
                                                PCI_HOST_PROP_PCI_HOLE64_SIZE,
@@ -262,7 +260,7 @@ static void pc_init1(MachineState *machine,
         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, pci_bus, &error_fatal);
+        pci_realize_and_unref(pci_dev, pcms->bus, &error_fatal);
 
         if (xen_enabled()) {
             pci_device_set_intx_routing_notifier(
@@ -274,7 +272,7 @@ static void pc_init1(MachineState *machine,
              * connected to the IOAPIC directly.
              * These additional routes can be discovered through ACPI.
              */
-            pci_bus_irqs(pci_bus, xen_intx_set_irq, pci_dev,
+            pci_bus_irqs(pcms->bus, xen_intx_set_irq, pci_dev,
                          XEN_IOAPIC_NUM_PIRQS);
         }
 
@@ -313,7 +311,7 @@ static void pc_init1(MachineState *machine,
         x86_register_ferr_irq(x86ms->gsi[13]);
     }
 
-    pc_vga_init(isa_bus, pcmc->pci_enabled ? pci_bus : NULL);
+    pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->bus : NULL);
 
     assert(pcms->vmport != ON_OFF_AUTO__MAX);
     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
@@ -324,7 +322,7 @@ static void pc_init1(MachineState *machine,
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, true,
                          0x4);
 
-    pc_nic_init(pcmc, isa_bus, pci_bus);
+    pc_nic_init(pcmc, isa_bus, pcms->bus);
 
 #ifdef CONFIG_IDE_ISA
     if (!pcmc->pci_enabled) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d313ba5509..0eef9e6ca1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -122,7 +122,6 @@ static void pc_q35_init(MachineState *machine)
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
-    PCIBus *host_bus;
     PCIDevice *lpc;
     DeviceState *lpc_dev;
     BusState *idebus[MAX_SATA_PORTS];
@@ -227,8 +226,7 @@ static void pc_q35_init(MachineState *machine)
     sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), &error_fatal);
 
     /* pci */
-    host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
-    pcms->bus = host_bus;
+    pcms->bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
 
     /* irq lines */
     gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
@@ -242,7 +240,7 @@ static void pc_q35_init(MachineState *machine)
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
-    pci_realize_and_unref(lpc, host_bus, &error_fatal);
+    pci_realize_and_unref(lpc, pcms->bus, &error_fatal);
 
     x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
 
@@ -293,7 +291,7 @@ static void pc_q35_init(MachineState *machine)
 
     if (pcms->sata_enabled) {
         /* ahci and SATA device, for q35 1 ahci controller is built-in */
-        ahci = pci_create_simple_multifunction(host_bus,
+        ahci = pci_create_simple_multifunction(pcms->bus,
                                                PCI_DEVFN(ICH9_SATA1_DEV,
                                                          ICH9_SATA1_FUNC),
                                                "ich9-ahci");
@@ -308,14 +306,14 @@ static void pc_q35_init(MachineState *machine)
 
     if (machine_usb(machine)) {
         /* Should we create 6 UHCI according to ich9 spec? */
-        ehci_create_ich9_with_companions(host_bus, 0x1d);
+        ehci_create_ich9_with_companions(pcms->bus, 0x1d);
     }
 
     if (pcms->smbus_enabled) {
         PCIDevice *smb;
 
         /* TODO: Populate SPD eeprom data.  */
-        smb = pci_create_simple_multifunction(host_bus,
+        smb = pci_create_simple_multifunction(pcms->bus,
                                               PCI_DEVFN(ICH9_SMB_DEV,
                                                         ICH9_SMB_FUNC),
                                               TYPE_ICH9_SMB_DEVICE);
@@ -327,8 +325,8 @@ static void pc_q35_init(MachineState *machine)
     pc_cmos_init(pcms, idebus[0], idebus[1], x86ms->rtc);
 
     /* the rest devices to which pci devfn is automatically assigned */
-    pc_vga_init(isa_bus, host_bus);
-    pc_nic_init(pcmc, isa_bus, host_bus);
+    pc_vga_init(isa_bus, pcms->bus);
+    pc_nic_init(pcmc, isa_bus, pcms->bus);
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
-- 
2.43.0

Re: [PATCH 9/9] hw/i386/pc_{piix, q35}: Eliminate local pci_bus/pci_host variables

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> There is no advantage in having these local variables which 1/ needlessly have
> different identifiers in both machines and 2/ which are redundant to pcms->bus
> which is almost as short.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_piix.c | 14 ++++++--------
>   hw/i386/pc_q35.c  | 16 +++++++---------
>   2 files changed, 13 insertions(+), 17 deletions(-)

IMO it is a design mistake to have these fields in PCMachineState:

  27 typedef struct PCMachineState {

  36     /* Pointers to devices and objects: */
  37     PCIBus *bus;
  38     I2CBus *smbus;
  39     PFlashCFI01 *flash[2];
  40     ISADevice *pcspk;
  41     DeviceState *iommu;
  42     BusState *idebus[MAX_IDE_BUS];

Anyhow, back to your patch, please rename 'bus' -> 'pcibus'. Maybe
in a preliminary patch?

Re: [PATCH 9/9] hw/i386/pc_{piix, q35}: Eliminate local pci_bus/pci_host variables

[Bernhard Beschow]

Am 21. Februar 2024 15:50:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 8/2/24 23:03, Bernhard Beschow wrote:
>> There is no advantage in having these local variables which 1/ needlessly have
>> different identifiers in both machines and 2/ which are redundant to pcms->bus
>> which is almost as short.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/i386/pc_piix.c | 14 ++++++--------
>>   hw/i386/pc_q35.c  | 16 +++++++---------
>>   2 files changed, 13 insertions(+), 17 deletions(-)
>
>IMO it is a design mistake to have these fields in PCMachineState:
>
> 27 typedef struct PCMachineState {
>
> 36     /* Pointers to devices and objects: */
> 37     PCIBus *bus;
> 38     I2CBus *smbus;
> 39     PFlashCFI01 *flash[2];
> 40     ISADevice *pcspk;
> 41     DeviceState *iommu;
> 42     BusState *idebus[MAX_IDE_BUS];

Any ideas on how to resolve these? `flash` might be easy now. What about the buses in particular?

Best regards,
Bernhard

>
>Anyhow, back to your patch, please rename 'bus' -> 'pcibus'. Maybe
>in a preliminary patch?

Re: [PATCH 0/9] Simplify initialization of PC machines

[Bernhard Beschow]

Am 8. Februar 2024 22:03:40 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>The series aims to simplify the initialization process of all PC-based machines.
>
>
>
>It consists of streamlining redundant code, as well as consolidating the setup
>
>of system flash and generation of smbios data which are currently fairly
>
>distributed.
>
>
>
>These changes are expected to make the code easier to understand and maintain.
>
>
>
>Best regards,
>
>Bernhard
>
>
>
>Bernhard Beschow (9):
>
>  hw/i386/x86: Let ioapic_init_gsi() take parent as pointer
>
>  hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc
>
>    machines
>
>  hw/i386/x86: Turn apic_xrupt_override into class attribute
>
>  hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()
>
>  hw/i386/pc: Defer smbios_set_defaults() to machine_done
>
>  hw/i386/pc: Confine system flash handling to pc_sysfw
>
>  hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it
>
>  hw/i386/pc: Populate RTC attribute directly
>
>  hw/i386/pc_{piix,q35}: Eliminate local pci_bus/pci_host variables
>

Ping. Only patch 4 reviewed so far.

>
>
> hw/i386/fw_cfg.h      |  3 ++-
>
> include/hw/i386/pc.h  |  5 ----
>
> include/hw/i386/x86.h |  5 ++--
>
> hw/i386/acpi-common.c |  3 ++-
>
> hw/i386/fw_cfg.c      | 12 +++++++++-
>
> hw/i386/microvm.c     |  2 +-
>
> hw/i386/pc.c          | 25 +++++---------------
>
> hw/i386/pc_piix.c     | 55 ++++++++++++++-----------------------------
>
> hw/i386/pc_q35.c      | 38 ++++++++++--------------------
>
> hw/i386/pc_sysfw.c    | 17 ++++---------
>
> hw/i386/x86.c         |  7 +++---
>
> 11 files changed, 62 insertions(+), 110 deletions(-)
>
>
>
>-- >
>2.43.0
>
>
>

Re: [PATCH 0/9] Simplify initialization of PC machines

[Philippe Mathieu-Daudé]

On 8/2/24 23:03, Bernhard Beschow wrote:
> The series aims to simplify the initialization process of all PC-based machines.
> 
> It consists of streamlining redundant code, as well as consolidating the setup
> of system flash and generation of smbios data which are currently fairly
> distributed.
> 
> These changes are expected to make the code easier to understand and maintain.
> 
> Best regards,
> Bernhard
> 
> Bernhard Beschow (9):
>    hw/i386/x86: Let ioapic_init_gsi() take parent as pointer
>    hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc
>      machines
>    hw/i386/x86: Turn apic_xrupt_override into class attribute
>    hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()
>    hw/i386/pc: Defer smbios_set_defaults() to machine_done
>    hw/i386/pc: Confine system flash handling to pc_sysfw
>    hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it
>    hw/i386/pc: Populate RTC attribute directly
>    hw/i386/pc_{piix,q35}: Eliminate local pci_bus/pci_host variables

Patches 2-7 queued so far.

Re: [PATCH 0/9] Simplify initialization of PC machines

[Michael S. Tsirkin]

On Thu, Feb 08, 2024 at 11:03:40PM +0100, Bernhard Beschow wrote:
> The series aims to simplify the initialization process of all PC-based machines.
> 
> It consists of streamlining redundant code, as well as consolidating the setup
> of system flash and generation of smbios data which are currently fairly
> distributed.
> 
> These changes are expected to make the code easier to understand and maintain.
> 
> Best regards,
> Bernhard


This looks good to me overall.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I see Philippe started queueing these, fine by me.

> Bernhard Beschow (9):
>   hw/i386/x86: Let ioapic_init_gsi() take parent as pointer
>   hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc
>     machines
>   hw/i386/x86: Turn apic_xrupt_override into class attribute
>   hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()
>   hw/i386/pc: Defer smbios_set_defaults() to machine_done
>   hw/i386/pc: Confine system flash handling to pc_sysfw
>   hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it
>   hw/i386/pc: Populate RTC attribute directly
>   hw/i386/pc_{piix,q35}: Eliminate local pci_bus/pci_host variables
> 
>  hw/i386/fw_cfg.h      |  3 ++-
>  include/hw/i386/pc.h  |  5 ----
>  include/hw/i386/x86.h |  5 ++--
>  hw/i386/acpi-common.c |  3 ++-
>  hw/i386/fw_cfg.c      | 12 +++++++++-
>  hw/i386/microvm.c     |  2 +-
>  hw/i386/pc.c          | 25 +++++---------------
>  hw/i386/pc_piix.c     | 55 ++++++++++++++-----------------------------
>  hw/i386/pc_q35.c      | 38 ++++++++++--------------------
>  hw/i386/pc_sysfw.c    | 17 ++++---------
>  hw/i386/x86.c         |  7 +++---
>  11 files changed, 62 insertions(+), 110 deletions(-)
> 
> -- 
> 2.43.0
> 

Re: [PATCH 0/9] Simplify initialization of PC machines

[Bernhard Beschow]

Am 22. Februar 2024 15:25:01 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Thu, Feb 08, 2024 at 11:03:40PM +0100, Bernhard Beschow wrote:
>> The series aims to simplify the initialization process of all PC-based machines.
>> 
>> It consists of streamlining redundant code, as well as consolidating the setup
>> of system flash and generation of smbios data which are currently fairly
>> distributed.
>> 
>> These changes are expected to make the code easier to understand and maintain.
>> 
>> Best regards,
>> Bernhard
>
>
>This looks good to me overall.
>
>Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>I see Philippe started queueing these, fine by me.

Thanks so far to all involved! I've just sent v2: https://lore.kernel.org/qemu-devel/20240224135851.100361-1-shentey@gmail.com/

Best regars,
Bernhard

>
>> Bernhard Beschow (9):
>>   hw/i386/x86: Let ioapic_init_gsi() take parent as pointer
>>   hw/i386/pc_piix: Share pc_cmos_init() invocation between pc and isapc
>>     machines
>>   hw/i386/x86: Turn apic_xrupt_override into class attribute
>>   hw/i386/pc: Merge pc_guest_info_init() into pc_machine_initfn()
>>   hw/i386/pc: Defer smbios_set_defaults() to machine_done
>>   hw/i386/pc: Confine system flash handling to pc_sysfw
>>   hw/i386/pc_sysfw: Inline pc_system_flash_create() and remove it
>>   hw/i386/pc: Populate RTC attribute directly
>>   hw/i386/pc_{piix,q35}: Eliminate local pci_bus/pci_host variables
>> 
>>  hw/i386/fw_cfg.h      |  3 ++-
>>  include/hw/i386/pc.h  |  5 ----
>>  include/hw/i386/x86.h |  5 ++--
>>  hw/i386/acpi-common.c |  3 ++-
>>  hw/i386/fw_cfg.c      | 12 +++++++++-
>>  hw/i386/microvm.c     |  2 +-
>>  hw/i386/pc.c          | 25 +++++---------------
>>  hw/i386/pc_piix.c     | 55 ++++++++++++++-----------------------------
>>  hw/i386/pc_q35.c      | 38 ++++++++++--------------------
>>  hw/i386/pc_sysfw.c    | 17 ++++---------
>>  hw/i386/x86.c         |  7 +++---
>>  11 files changed, 62 insertions(+), 110 deletions(-)
>> 
>> -- 
>> 2.43.0
>> 
>