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

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 30 Aug 2025 at 16:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>
> PCI is always enabled on the pc-i440fx machine so hardcode the relevant logic
> in pc_init1(). Add an assert() to ensure that this is always the case at
> runtime as already done in pc_q35_init().
>
> 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>
> Link: https://lore.kernel.org/r/20250828111057.468712-16-mark.caveayland@nutanix.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Hi; Coverity points out (CID 1620557) that this change means
that an if() check later on in the function is no longer needed:

> @@ -195,38 +200,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;
> +    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);
> +    phb = OBJECT(qdev_new(TYPE_I440FX_PCI_HOST_BRIDGE));

phb is now guaranteed not to be NULL (when before it might have
been NULL), so this bit of code near the bottom of pc_init1():

    if (phb) {
        ioapic_init_gsi(gsi_state, phb);
    }

can be simplified to unconditionally call ioapic_init_gsi().

Would you mind sending a followup patch to clean that up?

thanks
-- PMM