[PATCH v2 00/16] hw/i386/pc: Remove deprecated 2.6 and 2.7 PC machines

[PATCH v2 00/16] hw/i386/pc: Remove deprecated 2.6 and 2.7 PC machines

[Philippe Mathieu-Daudé]

(repost since v1 was incomplete)

The versioned 'pc' and 'q35' machines up to 2.12 been marked
as deprecated two releases ago, and are older than 6 years,
so according to our support policy we can remove them.

This series only includes the 2.6 and 2.7 machines removal,
as it is a big enough number of LoC removed. Rest will
follow.

Based-on: <20250429140825.25964-1-philmd@linaro.org>

Philippe Mathieu-Daudé (16):
  hw/i386/pc: Remove deprecated pc-q35-2.6 and pc-i440fx-2.6 machines
  hw/i386/pc: Remove PCMachineClass::legacy_cpu_hotplug field
  hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field
  hw/i386/pc: Remove pc_compat_2_6[] array
  hw/intc/apic: Remove APICCommonState::legacy_instance_id field
  hw/core/machine: Remove hw_compat_2_6[] array
  hw/virtio/virtio-mmio: Remove
    VirtIOMMIOProxy::format_transport_address field
  hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines
  hw/i386/pc: Remove pc_compat_2_7[] array
  hw/audio/pcspk: Remove PCSpkState::migrate field
  hw/core/machine: Remove hw_compat_2_7[] array
  hw/i386/intel_iommu: Remove IntelIOMMUState::buggy_eim field
  hw/intc/ioapic: Remove IOAPICCommonState::version field
  hw/virtio/virtio-pci: Remove VirtIOPCIProxy::ignore_backend_features
    field
  hw/char/virtio-serial: Do not expose the 'emergency-write' property
  hw/virtio/virtio-pci: Remove VIRTIO_PCI_FLAG_PAGE_PER_VQ definition

 hw/intc/ioapic_internal.h       |   3 +-
 include/hw/acpi/cpu_hotplug.h   |   3 -
 include/hw/boards.h             |   6 -
 include/hw/i386/apic_internal.h |   1 -
 include/hw/i386/intel_iommu.h   |   1 -
 include/hw/i386/pc.h            |   9 --
 include/hw/i386/x86.h           |   2 -
 include/hw/virtio/virtio-mmio.h |   1 -
 include/hw/virtio/virtio-pci.h  |   2 -
 hw/acpi/cpu_hotplug.c           | 230 --------------------------------
 hw/audio/pcspk.c                |  10 --
 hw/char/virtio-serial-bus.c     |  10 +-
 hw/core/machine.c               |  17 ---
 hw/display/virtio-vga.c         |  10 --
 hw/i386/acpi-build.c            |   4 +-
 hw/i386/intel_iommu.c           |   5 +-
 hw/i386/microvm.c               |   3 -
 hw/i386/multiboot.c             |   7 +-
 hw/i386/pc.c                    |  18 ---
 hw/i386/pc_piix.c               |  23 ----
 hw/i386/pc_q35.c                |  24 ----
 hw/i386/x86-common.c            |   3 +-
 hw/i386/x86.c                   |   2 -
 hw/intc/apic_common.c           |   5 -
 hw/intc/ioapic.c                |  18 +--
 hw/intc/ioapic_common.c         |   2 +-
 hw/virtio/virtio-mmio.c         |  15 ---
 hw/virtio/virtio-pci.c          |  12 +-
 28 files changed, 19 insertions(+), 427 deletions(-)

-- 
2.47.1

[PATCH v2 01/16] hw/i386/pc: Remove deprecated pc-q35-2.6 and pc-i440fx-2.6 machines

[Philippe Mathieu-Daudé]

These machines has been supported for a period of more than 6 years.
According to our versioned machine support policy (see commit
ce80c4fa6ff "docs: document special exception for machine type
deprecation & removal") they can now be removed.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 14 --------------
 hw/i386/pc_q35.c  | 14 --------------
 2 files changed, 28 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7a62bb06500..98a118fd4a0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -764,20 +764,6 @@ static void pc_i440fx_machine_2_7_options(MachineClass *m)
 
 DEFINE_I440FX_MACHINE(2, 7);
 
-static void pc_i440fx_machine_2_6_options(MachineClass *m)
-{
-    X86MachineClass *x86mc = X86_MACHINE_CLASS(m);
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
-
-    pc_i440fx_machine_2_7_options(m);
-    pcmc->legacy_cpu_hotplug = true;
-    x86mc->fwcfg_dma_enabled = false;
-    compat_props_add(m->compat_props, hw_compat_2_6, hw_compat_2_6_len);
-    compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
-}
-
-DEFINE_I440FX_MACHINE(2, 6);
-
 #ifdef CONFIG_ISAPC
 static void isapc_machine_options(MachineClass *m)
 {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 33211b1876f..b7ffb5f1216 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -658,17 +658,3 @@ static void pc_q35_machine_2_7_options(MachineClass *m)
 }
 
 DEFINE_Q35_MACHINE(2, 7);
-
-static void pc_q35_machine_2_6_options(MachineClass *m)
-{
-    X86MachineClass *x86mc = X86_MACHINE_CLASS(m);
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
-
-    pc_q35_machine_2_7_options(m);
-    pcmc->legacy_cpu_hotplug = true;
-    x86mc->fwcfg_dma_enabled = false;
-    compat_props_add(m->compat_props, hw_compat_2_6, hw_compat_2_6_len);
-    compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
-}
-
-DEFINE_Q35_MACHINE(2, 6);
-- 
2.47.1

Re: [PATCH v2 01/16] hw/i386/pc: Remove deprecated pc-q35-2.6 and pc-i440fx-2.6 machines

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> These machines has been supported for a period of more than 6 years.
> According to our versioned machine support policy (see commit
> ce80c4fa6ff "docs: document special exception for machine type
> deprecation & removal") they can now be removed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 14 --------------
>   hw/i386/pc_q35.c  | 14 --------------
>   2 files changed, 28 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7a62bb06500..98a118fd4a0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -764,20 +764,6 @@ static void pc_i440fx_machine_2_7_options(MachineClass *m)
>   
>   DEFINE_I440FX_MACHINE(2, 7);
>   
> -static void pc_i440fx_machine_2_6_options(MachineClass *m)
> -{
> -    X86MachineClass *x86mc = X86_MACHINE_CLASS(m);
> -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> -
> -    pc_i440fx_machine_2_7_options(m);
> -    pcmc->legacy_cpu_hotplug = true;
> -    x86mc->fwcfg_dma_enabled = false;
> -    compat_props_add(m->compat_props, hw_compat_2_6, hw_compat_2_6_len);
> -    compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
> -}
> -
> -DEFINE_I440FX_MACHINE(2, 6);
> -
>   #ifdef CONFIG_ISAPC
>   static void isapc_machine_options(MachineClass *m)
>   {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 33211b1876f..b7ffb5f1216 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -658,17 +658,3 @@ static void pc_q35_machine_2_7_options(MachineClass *m)
>   }
>   
>   DEFINE_Q35_MACHINE(2, 7);
> -
> -static void pc_q35_machine_2_6_options(MachineClass *m)
> -{
> -    X86MachineClass *x86mc = X86_MACHINE_CLASS(m);
> -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> -
> -    pc_q35_machine_2_7_options(m);
> -    pcmc->legacy_cpu_hotplug = true;
> -    x86mc->fwcfg_dma_enabled = false;
> -    compat_props_add(m->compat_props, hw_compat_2_6, hw_compat_2_6_len);
> -    compat_props_add(m->compat_props, pc_compat_2_6, pc_compat_2_6_len);
> -}
> -
> -DEFINE_Q35_MACHINE(2, 6);

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


ATB,

Mark.

Re: [PATCH v2 01/16] hw/i386/pc: Remove deprecated pc-q35-2.6 and pc-i440fx-2.6 machines

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> These machines has been supported for a period of more than 6 years.
> According to our versioned machine support policy (see commit
> ce80c4fa6ff "docs: document special exception for machine type
> deprecation & removal") they can now be removed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 14 --------------
>   hw/i386/pc_q35.c  | 14 --------------
>   2 files changed, 28 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 02/16] hw/i386/pc: Remove PCMachineClass::legacy_cpu_hotplug field

[Philippe Mathieu-Daudé]

The PCMachineClass::legacy_cpu_hotplug boolean was only used
by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
removed. Remove it and simplify build_dsdt(), removing
build_legacy_cpu_hotplug_aml() altogether.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/cpu_hotplug.h |   3 -
 include/hw/i386/pc.h          |   3 -
 hw/acpi/cpu_hotplug.c         | 230 ----------------------------------
 hw/i386/acpi-build.c          |   4 +-
 4 files changed, 1 insertion(+), 239 deletions(-)

diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 3b932abbbbe..aeee630cf05 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -34,7 +34,4 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
 void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
                                 CPUHotplugState *cpuhp_state,
                                 uint16_t io_port);
-
-void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
-                                  uint16_t io_base);
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 79b72c54dd3..a3de3e9560d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -110,9 +110,6 @@ struct PCMachineClass {
     bool enforce_amd_1tb_hole;
     bool isa_bios_alias;
 
-    /* generate legacy CPU hotplug AML */
-    bool legacy_cpu_hotplug;
-
     /* use PVH to load kernels that support this feature */
     bool pvh_enabled;
 
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index aa0e1e3efa5..fe439705bda 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -116,233 +116,3 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
     memory_region_del_subregion(parent, &gpe_cpu->io);
     cpu_hotplug_hw_init(parent, gpe_cpu->device, cpuhp_state, io_port);
 }
-
-void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
-                                  uint16_t io_base)
-{
-    Aml *dev;
-    Aml *crs;
-    Aml *pkg;
-    Aml *field;
-    Aml *method;
-    Aml *if_ctx;
-    Aml *else_ctx;
-    int i, apic_idx;
-    Aml *sb_scope = aml_scope("_SB");
-    uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
-    Aml *cpu_id = aml_arg(1);
-    Aml *apic_id = aml_arg(0);
-    Aml *cpu_on = aml_local(0);
-    Aml *madt = aml_local(1);
-    Aml *cpus_map = aml_name(CPU_ON_BITMAP);
-    Aml *zero = aml_int(0);
-    Aml *one = aml_int(1);
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
-    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
-    X86MachineState *x86ms = X86_MACHINE(machine);
-
-    /*
-     * _MAT method - creates an madt apic buffer
-     * apic_id = Arg0 = Local APIC ID
-     * cpu_id  = Arg1 = Processor ID
-     * cpu_on = Local0 = CPON flag for this cpu
-     * madt = Local1 = Buffer (in madt apic form) to return
-     */
-    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
-    aml_append(method,
-        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
-    aml_append(method,
-        aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
-    /* Update the processor id, lapic id, and enable/disable status */
-    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
-    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
-    aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
-    aml_append(method, aml_return(madt));
-    aml_append(sb_scope, method);
-
-    /*
-     * _STA method - return ON status of cpu
-     * apic_id = Arg0 = Local APIC ID
-     * cpu_on = Local0 = CPON flag for this cpu
-     */
-    method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
-    aml_append(method,
-        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
-    if_ctx = aml_if(cpu_on);
-    {
-        aml_append(if_ctx, aml_return(aml_int(0xF)));
-    }
-    aml_append(method, if_ctx);
-    else_ctx = aml_else();
-    {
-        aml_append(else_ctx, aml_return(zero));
-    }
-    aml_append(method, else_ctx);
-    aml_append(sb_scope, method);
-
-    method = aml_method(CPU_EJECT_METHOD, 2, AML_NOTSERIALIZED);
-    aml_append(method, aml_sleep(200));
-    aml_append(sb_scope, method);
-
-    method = aml_method(CPU_SCAN_METHOD, 0, AML_NOTSERIALIZED);
-    {
-        Aml *while_ctx, *if_ctx2, *else_ctx2;
-        Aml *bus_check_evt = aml_int(1);
-        Aml *remove_evt = aml_int(3);
-        Aml *status_map = aml_local(5); /* Local5 = active cpu bitmap */
-        Aml *byte = aml_local(2); /* Local2 = last read byte from bitmap */
-        Aml *idx = aml_local(0); /* Processor ID / APIC ID iterator */
-        Aml *is_cpu_on = aml_local(1); /* Local1 = CPON flag for cpu */
-        Aml *status = aml_local(3); /* Local3 = active state for cpu */
-
-        aml_append(method, aml_store(aml_name(CPU_STATUS_MAP), status_map));
-        aml_append(method, aml_store(zero, byte));
-        aml_append(method, aml_store(zero, idx));
-
-        /* While (idx < SizeOf(CPON)) */
-        while_ctx = aml_while(aml_lless(idx, aml_sizeof(cpus_map)));
-        aml_append(while_ctx,
-            aml_store(aml_derefof(aml_index(cpus_map, idx)), is_cpu_on));
-
-        if_ctx = aml_if(aml_and(idx, aml_int(0x07), NULL));
-        {
-            /* Shift down previously read bitmap byte */
-            aml_append(if_ctx, aml_shiftright(byte, one, byte));
-        }
-        aml_append(while_ctx, if_ctx);
-
-        else_ctx = aml_else();
-        {
-            /* Read next byte from cpu bitmap */
-            aml_append(else_ctx, aml_store(aml_derefof(aml_index(status_map,
-                       aml_shiftright(idx, aml_int(3), NULL))), byte));
-        }
-        aml_append(while_ctx, else_ctx);
-
-        aml_append(while_ctx, aml_store(aml_and(byte, one, NULL), status));
-        if_ctx = aml_if(aml_lnot(aml_equal(is_cpu_on, status)));
-        {
-            /* State change - update CPON with new state */
-            aml_append(if_ctx, aml_store(status, aml_index(cpus_map, idx)));
-            if_ctx2 = aml_if(aml_equal(status, one));
-            {
-                aml_append(if_ctx2,
-                    aml_call2(AML_NOTIFY_METHOD, idx, bus_check_evt));
-            }
-            aml_append(if_ctx, if_ctx2);
-            else_ctx2 = aml_else();
-            {
-                aml_append(else_ctx2,
-                    aml_call2(AML_NOTIFY_METHOD, idx, remove_evt));
-            }
-        }
-        aml_append(if_ctx, else_ctx2);
-        aml_append(while_ctx, if_ctx);
-
-        aml_append(while_ctx, aml_increment(idx)); /* go to next cpu */
-        aml_append(method, while_ctx);
-    }
-    aml_append(sb_scope, method);
-
-    /* The current AML generator can cover the APIC ID range [0..255],
-     * inclusive, for VCPU hotplug. */
-    QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
-    if (x86ms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
-        error_report("max_cpus is too large. APIC ID of last CPU is %u",
-                     x86ms->apic_id_limit - 1);
-        exit(1);
-    }
-
-    /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
-    dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
-    aml_append(dev,
-        aml_name_decl("_UID", aml_string("CPU Hotplug resources"))
-    );
-    /* device present, functioning, decoding, not shown in UI */
-    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
-    crs = aml_resource_template();
-    aml_append(crs,
-        aml_io(AML_DECODE16, io_base, io_base, 1, ACPI_GPE_PROC_LEN)
-    );
-    aml_append(dev, aml_name_decl("_CRS", crs));
-    aml_append(sb_scope, dev);
-    /* declare CPU hotplug MMIO region and PRS field to access it */
-    aml_append(sb_scope, aml_operation_region(
-        "PRST", AML_SYSTEM_IO, aml_int(io_base), ACPI_GPE_PROC_LEN));
-    field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("PRS", 256));
-    aml_append(sb_scope, field);
-
-    /* build Processor object for each processor */
-    for (i = 0; i < apic_ids->len; i++) {
-        int cpu_apic_id = apic_ids->cpus[i].arch_id;
-
-        assert(cpu_apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
-
-        dev = aml_processor(i, 0, 0, "CP%.02X", cpu_apic_id);
-
-        method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
-        aml_append(method,
-            aml_return(aml_call2(CPU_MAT_METHOD,
-                                 aml_int(cpu_apic_id), aml_int(i))
-        ));
-        aml_append(dev, method);
-
-        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-        aml_append(method,
-            aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(cpu_apic_id))));
-        aml_append(dev, method);
-
-        method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
-        aml_append(method,
-            aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(cpu_apic_id),
-                aml_arg(0)))
-        );
-        aml_append(dev, method);
-
-        aml_append(sb_scope, dev);
-    }
-
-    /* build this code:
-     *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
-     */
-    /* Arg0 = APIC ID */
-    method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
-    for (i = 0; i < apic_ids->len; i++) {
-        int cpu_apic_id = apic_ids->cpus[i].arch_id;
-
-        if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(cpu_apic_id)));
-        aml_append(if_ctx,
-            aml_notify(aml_name("CP%.02X", cpu_apic_id), aml_arg(1))
-        );
-        aml_append(method, if_ctx);
-    }
-    aml_append(sb_scope, method);
-
-    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
-     *
-     * Note: The ability to create variable-sized packages was first
-     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
-     * ith up to 255 elements. Windows guests up to win2k8 fail when
-     * VarPackageOp is used.
-     */
-    pkg = x86ms->apic_id_limit <= 255 ? aml_package(x86ms->apic_id_limit) :
-                                        aml_varpackage(x86ms->apic_id_limit);
-
-    for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
-        int cpu_apic_id = apic_ids->cpus[i].arch_id;
-
-        for (; apic_idx < cpu_apic_id; apic_idx++) {
-            aml_append(pkg, aml_int(0));
-        }
-        aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
-        apic_idx = cpu_apic_id + 1;
-    }
-    aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
-    aml_append(ctx, sb_scope);
-
-    method = aml_method("\\_GPE._E02", 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_call0("\\_SB." CPU_SCAN_METHOD));
-    aml_append(ctx, method);
-}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3fffa4a3328..625889783ec 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1465,9 +1465,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     }
     aml_append(dsdt, scope);
 
-    if (pcmc->legacy_cpu_hotplug) {
-        build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
-    } else {
+    {
         CPUHotplugFeatures opts = {
             .acpi_1_compatible = true, .has_legacy_cphp = true,
             .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
-- 
2.47.1

Re: [PATCH v2 02/16] hw/i386/pc: Remove PCMachineClass::legacy_cpu_hotplug field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The PCMachineClass::legacy_cpu_hotplug boolean was only used
> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
> removed. Remove it and simplify build_dsdt(), removing
> build_legacy_cpu_hotplug_aml() altogether.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/acpi/cpu_hotplug.h |   3 -
>   include/hw/i386/pc.h          |   3 -
>   hw/acpi/cpu_hotplug.c         | 230 ----------------------------------
>   hw/i386/acpi-build.c          |   4 +-
>   4 files changed, 1 insertion(+), 239 deletions(-)
> 
> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> index 3b932abbbbe..aeee630cf05 100644
> --- a/include/hw/acpi/cpu_hotplug.h
> +++ b/include/hw/acpi/cpu_hotplug.h
> @@ -34,7 +34,4 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>   void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>                                   CPUHotplugState *cpuhp_state,
>                                   uint16_t io_port);
> -
> -void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> -                                  uint16_t io_base);
>   #endif
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 79b72c54dd3..a3de3e9560d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -110,9 +110,6 @@ struct PCMachineClass {
>       bool enforce_amd_1tb_hole;
>       bool isa_bios_alias;
>   
> -    /* generate legacy CPU hotplug AML */
> -    bool legacy_cpu_hotplug;
> -
>       /* use PVH to load kernels that support this feature */
>       bool pvh_enabled;
>   
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index aa0e1e3efa5..fe439705bda 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -116,233 +116,3 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>       memory_region_del_subregion(parent, &gpe_cpu->io);
>       cpu_hotplug_hw_init(parent, gpe_cpu->device, cpuhp_state, io_port);
>   }
> -
> -void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> -                                  uint16_t io_base)
> -{
> -    Aml *dev;
> -    Aml *crs;
> -    Aml *pkg;
> -    Aml *field;
> -    Aml *method;
> -    Aml *if_ctx;
> -    Aml *else_ctx;
> -    int i, apic_idx;
> -    Aml *sb_scope = aml_scope("_SB");
> -    uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
> -    Aml *cpu_id = aml_arg(1);
> -    Aml *apic_id = aml_arg(0);
> -    Aml *cpu_on = aml_local(0);
> -    Aml *madt = aml_local(1);
> -    Aml *cpus_map = aml_name(CPU_ON_BITMAP);
> -    Aml *zero = aml_int(0);
> -    Aml *one = aml_int(1);
> -    MachineClass *mc = MACHINE_GET_CLASS(machine);
> -    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> -    X86MachineState *x86ms = X86_MACHINE(machine);
> -
> -    /*
> -     * _MAT method - creates an madt apic buffer
> -     * apic_id = Arg0 = Local APIC ID
> -     * cpu_id  = Arg1 = Processor ID
> -     * cpu_on = Local0 = CPON flag for this cpu
> -     * madt = Local1 = Buffer (in madt apic form) to return
> -     */
> -    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
> -    aml_append(method,
> -        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
> -    aml_append(method,
> -        aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
> -    /* Update the processor id, lapic id, and enable/disable status */
> -    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
> -    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
> -    aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
> -    aml_append(method, aml_return(madt));
> -    aml_append(sb_scope, method);
> -
> -    /*
> -     * _STA method - return ON status of cpu
> -     * apic_id = Arg0 = Local APIC ID
> -     * cpu_on = Local0 = CPON flag for this cpu
> -     */
> -    method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
> -    aml_append(method,
> -        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
> -    if_ctx = aml_if(cpu_on);
> -    {
> -        aml_append(if_ctx, aml_return(aml_int(0xF)));
> -    }
> -    aml_append(method, if_ctx);
> -    else_ctx = aml_else();
> -    {
> -        aml_append(else_ctx, aml_return(zero));
> -    }
> -    aml_append(method, else_ctx);
> -    aml_append(sb_scope, method);
> -
> -    method = aml_method(CPU_EJECT_METHOD, 2, AML_NOTSERIALIZED);
> -    aml_append(method, aml_sleep(200));
> -    aml_append(sb_scope, method);
> -
> -    method = aml_method(CPU_SCAN_METHOD, 0, AML_NOTSERIALIZED);
> -    {
> -        Aml *while_ctx, *if_ctx2, *else_ctx2;
> -        Aml *bus_check_evt = aml_int(1);
> -        Aml *remove_evt = aml_int(3);
> -        Aml *status_map = aml_local(5); /* Local5 = active cpu bitmap */
> -        Aml *byte = aml_local(2); /* Local2 = last read byte from bitmap */
> -        Aml *idx = aml_local(0); /* Processor ID / APIC ID iterator */
> -        Aml *is_cpu_on = aml_local(1); /* Local1 = CPON flag for cpu */
> -        Aml *status = aml_local(3); /* Local3 = active state for cpu */
> -
> -        aml_append(method, aml_store(aml_name(CPU_STATUS_MAP), status_map));
> -        aml_append(method, aml_store(zero, byte));
> -        aml_append(method, aml_store(zero, idx));
> -
> -        /* While (idx < SizeOf(CPON)) */
> -        while_ctx = aml_while(aml_lless(idx, aml_sizeof(cpus_map)));
> -        aml_append(while_ctx,
> -            aml_store(aml_derefof(aml_index(cpus_map, idx)), is_cpu_on));
> -
> -        if_ctx = aml_if(aml_and(idx, aml_int(0x07), NULL));
> -        {
> -            /* Shift down previously read bitmap byte */
> -            aml_append(if_ctx, aml_shiftright(byte, one, byte));
> -        }
> -        aml_append(while_ctx, if_ctx);
> -
> -        else_ctx = aml_else();
> -        {
> -            /* Read next byte from cpu bitmap */
> -            aml_append(else_ctx, aml_store(aml_derefof(aml_index(status_map,
> -                       aml_shiftright(idx, aml_int(3), NULL))), byte));
> -        }
> -        aml_append(while_ctx, else_ctx);
> -
> -        aml_append(while_ctx, aml_store(aml_and(byte, one, NULL), status));
> -        if_ctx = aml_if(aml_lnot(aml_equal(is_cpu_on, status)));
> -        {
> -            /* State change - update CPON with new state */
> -            aml_append(if_ctx, aml_store(status, aml_index(cpus_map, idx)));
> -            if_ctx2 = aml_if(aml_equal(status, one));
> -            {
> -                aml_append(if_ctx2,
> -                    aml_call2(AML_NOTIFY_METHOD, idx, bus_check_evt));
> -            }
> -            aml_append(if_ctx, if_ctx2);
> -            else_ctx2 = aml_else();
> -            {
> -                aml_append(else_ctx2,
> -                    aml_call2(AML_NOTIFY_METHOD, idx, remove_evt));
> -            }
> -        }
> -        aml_append(if_ctx, else_ctx2);
> -        aml_append(while_ctx, if_ctx);
> -
> -        aml_append(while_ctx, aml_increment(idx)); /* go to next cpu */
> -        aml_append(method, while_ctx);
> -    }
> -    aml_append(sb_scope, method);
> -
> -    /* The current AML generator can cover the APIC ID range [0..255],
> -     * inclusive, for VCPU hotplug. */
> -    QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> -    if (x86ms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> -        error_report("max_cpus is too large. APIC ID of last CPU is %u",
> -                     x86ms->apic_id_limit - 1);
> -        exit(1);
> -    }
> -
> -    /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> -    dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
> -    aml_append(dev,
> -        aml_name_decl("_UID", aml_string("CPU Hotplug resources"))
> -    );
> -    /* device present, functioning, decoding, not shown in UI */
> -    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> -    crs = aml_resource_template();
> -    aml_append(crs,
> -        aml_io(AML_DECODE16, io_base, io_base, 1, ACPI_GPE_PROC_LEN)
> -    );
> -    aml_append(dev, aml_name_decl("_CRS", crs));
> -    aml_append(sb_scope, dev);
> -    /* declare CPU hotplug MMIO region and PRS field to access it */
> -    aml_append(sb_scope, aml_operation_region(
> -        "PRST", AML_SYSTEM_IO, aml_int(io_base), ACPI_GPE_PROC_LEN));
> -    field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
> -    aml_append(field, aml_named_field("PRS", 256));
> -    aml_append(sb_scope, field);
> -
> -    /* build Processor object for each processor */
> -    for (i = 0; i < apic_ids->len; i++) {
> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
> -
> -        assert(cpu_apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> -
> -        dev = aml_processor(i, 0, 0, "CP%.02X", cpu_apic_id);
> -
> -        method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
> -        aml_append(method,
> -            aml_return(aml_call2(CPU_MAT_METHOD,
> -                                 aml_int(cpu_apic_id), aml_int(i))
> -        ));
> -        aml_append(dev, method);
> -
> -        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -        aml_append(method,
> -            aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(cpu_apic_id))));
> -        aml_append(dev, method);
> -
> -        method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> -        aml_append(method,
> -            aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(cpu_apic_id),
> -                aml_arg(0)))
> -        );
> -        aml_append(dev, method);
> -
> -        aml_append(sb_scope, dev);
> -    }
> -
> -    /* build this code:
> -     *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
> -     */
> -    /* Arg0 = APIC ID */
> -    method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
> -    for (i = 0; i < apic_ids->len; i++) {
> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
> -
> -        if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(cpu_apic_id)));
> -        aml_append(if_ctx,
> -            aml_notify(aml_name("CP%.02X", cpu_apic_id), aml_arg(1))
> -        );
> -        aml_append(method, if_ctx);
> -    }
> -    aml_append(sb_scope, method);
> -
> -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
> -     *
> -     * Note: The ability to create variable-sized packages was first
> -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> -     * ith up to 255 elements. Windows guests up to win2k8 fail when
> -     * VarPackageOp is used.
> -     */
> -    pkg = x86ms->apic_id_limit <= 255 ? aml_package(x86ms->apic_id_limit) :
> -                                        aml_varpackage(x86ms->apic_id_limit);
> -
> -    for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
> -
> -        for (; apic_idx < cpu_apic_id; apic_idx++) {
> -            aml_append(pkg, aml_int(0));
> -        }
> -        aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
> -        apic_idx = cpu_apic_id + 1;
> -    }
> -    aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
> -    aml_append(ctx, sb_scope);
> -
> -    method = aml_method("\\_GPE._E02", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_call0("\\_SB." CPU_SCAN_METHOD));
> -    aml_append(ctx, method);
> -}
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 3fffa4a3328..625889783ec 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1465,9 +1465,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>       }
>       aml_append(dsdt, scope);
>   
> -    if (pcmc->legacy_cpu_hotplug) {
> -        build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> -    } else {
> +    {
>           CPUHotplugFeatures opts = {
>               .acpi_1_compatible = true, .has_legacy_cphp = true,
>               .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,

Do you happen to know exactly why the CPU hotplug ACPI entry was 
deprecated (it might be helpful to add this to the commit message). Anyhow:

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


ATB,

Mark.

Re: [PATCH v2 02/16] hw/i386/pc: Remove PCMachineClass::legacy_cpu_hotplug field

[Philippe Mathieu-Daudé]

On 2/5/25 10:57, Mark Cave-Ayland wrote:
> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
> 
>> The PCMachineClass::legacy_cpu_hotplug boolean was only used
>> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
>> removed. Remove it and simplify build_dsdt(), removing
>> build_legacy_cpu_hotplug_aml() altogether.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/acpi/cpu_hotplug.h |   3 -
>>   include/hw/i386/pc.h          |   3 -
>>   hw/acpi/cpu_hotplug.c         | 230 ----------------------------------
>>   hw/i386/acpi-build.c          |   4 +-
>>   4 files changed, 1 insertion(+), 239 deletions(-)
>>
>> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/ 
>> cpu_hotplug.h
>> index 3b932abbbbe..aeee630cf05 100644
>> --- a/include/hw/acpi/cpu_hotplug.h
>> +++ b/include/hw/acpi/cpu_hotplug.h
>> @@ -34,7 +34,4 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion 
>> *parent, Object *owner,
>>   void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>>                                   CPUHotplugState *cpuhp_state,
>>                                   uint16_t io_port);
>> -
>> -void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>> -                                  uint16_t io_base);
>>   #endif
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 79b72c54dd3..a3de3e9560d 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -110,9 +110,6 @@ struct PCMachineClass {
>>       bool enforce_amd_1tb_hole;
>>       bool isa_bios_alias;
>> -    /* generate legacy CPU hotplug AML */
>> -    bool legacy_cpu_hotplug;
>> -
>>       /* use PVH to load kernels that support this feature */
>>       bool pvh_enabled;
>> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
>> index aa0e1e3efa5..fe439705bda 100644
>> --- a/hw/acpi/cpu_hotplug.c
>> +++ b/hw/acpi/cpu_hotplug.c
>> @@ -116,233 +116,3 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug 
>> *gpe_cpu,
>>       memory_region_del_subregion(parent, &gpe_cpu->io);
>>       cpu_hotplug_hw_init(parent, gpe_cpu->device, cpuhp_state, io_port);
>>   }
>> -
>> -void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>> -                                  uint16_t io_base)
>> -{
>> -    Aml *dev;
>> -    Aml *crs;
>> -    Aml *pkg;
>> -    Aml *field;
>> -    Aml *method;
>> -    Aml *if_ctx;
>> -    Aml *else_ctx;
>> -    int i, apic_idx;
>> -    Aml *sb_scope = aml_scope("_SB");
>> -    uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
>> -    Aml *cpu_id = aml_arg(1);
>> -    Aml *apic_id = aml_arg(0);
>> -    Aml *cpu_on = aml_local(0);
>> -    Aml *madt = aml_local(1);
>> -    Aml *cpus_map = aml_name(CPU_ON_BITMAP);
>> -    Aml *zero = aml_int(0);
>> -    Aml *one = aml_int(1);
>> -    MachineClass *mc = MACHINE_GET_CLASS(machine);
>> -    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
>> -    X86MachineState *x86ms = X86_MACHINE(machine);
>> -
>> -    /*
>> -     * _MAT method - creates an madt apic buffer
>> -     * apic_id = Arg0 = Local APIC ID
>> -     * cpu_id  = Arg1 = Processor ID
>> -     * cpu_on = Local0 = CPON flag for this cpu
>> -     * madt = Local1 = Buffer (in madt apic form) to return
>> -     */
>> -    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
>> -    aml_append(method,
>> -        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
>> -    aml_append(method,
>> -        aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
>> -    /* Update the processor id, lapic id, and enable/disable status */
>> -    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
>> -    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
>> -    aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
>> -    aml_append(method, aml_return(madt));
>> -    aml_append(sb_scope, method);
>> -
>> -    /*
>> -     * _STA method - return ON status of cpu
>> -     * apic_id = Arg0 = Local APIC ID
>> -     * cpu_on = Local0 = CPON flag for this cpu
>> -     */
>> -    method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
>> -    aml_append(method,
>> -        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
>> -    if_ctx = aml_if(cpu_on);
>> -    {
>> -        aml_append(if_ctx, aml_return(aml_int(0xF)));
>> -    }
>> -    aml_append(method, if_ctx);
>> -    else_ctx = aml_else();
>> -    {
>> -        aml_append(else_ctx, aml_return(zero));
>> -    }
>> -    aml_append(method, else_ctx);
>> -    aml_append(sb_scope, method);
>> -
>> -    method = aml_method(CPU_EJECT_METHOD, 2, AML_NOTSERIALIZED);
>> -    aml_append(method, aml_sleep(200));
>> -    aml_append(sb_scope, method);
>> -
>> -    method = aml_method(CPU_SCAN_METHOD, 0, AML_NOTSERIALIZED);
>> -    {
>> -        Aml *while_ctx, *if_ctx2, *else_ctx2;
>> -        Aml *bus_check_evt = aml_int(1);
>> -        Aml *remove_evt = aml_int(3);
>> -        Aml *status_map = aml_local(5); /* Local5 = active cpu bitmap */
>> -        Aml *byte = aml_local(2); /* Local2 = last read byte from 
>> bitmap */
>> -        Aml *idx = aml_local(0); /* Processor ID / APIC ID iterator */
>> -        Aml *is_cpu_on = aml_local(1); /* Local1 = CPON flag for cpu */
>> -        Aml *status = aml_local(3); /* Local3 = active state for cpu */
>> -
>> -        aml_append(method, aml_store(aml_name(CPU_STATUS_MAP), 
>> status_map));
>> -        aml_append(method, aml_store(zero, byte));
>> -        aml_append(method, aml_store(zero, idx));
>> -
>> -        /* While (idx < SizeOf(CPON)) */
>> -        while_ctx = aml_while(aml_lless(idx, aml_sizeof(cpus_map)));
>> -        aml_append(while_ctx,
>> -            aml_store(aml_derefof(aml_index(cpus_map, idx)), 
>> is_cpu_on));
>> -
>> -        if_ctx = aml_if(aml_and(idx, aml_int(0x07), NULL));
>> -        {
>> -            /* Shift down previously read bitmap byte */
>> -            aml_append(if_ctx, aml_shiftright(byte, one, byte));
>> -        }
>> -        aml_append(while_ctx, if_ctx);
>> -
>> -        else_ctx = aml_else();
>> -        {
>> -            /* Read next byte from cpu bitmap */
>> -            aml_append(else_ctx, 
>> aml_store(aml_derefof(aml_index(status_map,
>> -                       aml_shiftright(idx, aml_int(3), NULL))), byte));
>> -        }
>> -        aml_append(while_ctx, else_ctx);
>> -
>> -        aml_append(while_ctx, aml_store(aml_and(byte, one, NULL), 
>> status));
>> -        if_ctx = aml_if(aml_lnot(aml_equal(is_cpu_on, status)));
>> -        {
>> -            /* State change - update CPON with new state */
>> -            aml_append(if_ctx, aml_store(status, aml_index(cpus_map, 
>> idx)));
>> -            if_ctx2 = aml_if(aml_equal(status, one));
>> -            {
>> -                aml_append(if_ctx2,
>> -                    aml_call2(AML_NOTIFY_METHOD, idx, bus_check_evt));
>> -            }
>> -            aml_append(if_ctx, if_ctx2);
>> -            else_ctx2 = aml_else();
>> -            {
>> -                aml_append(else_ctx2,
>> -                    aml_call2(AML_NOTIFY_METHOD, idx, remove_evt));
>> -            }
>> -        }
>> -        aml_append(if_ctx, else_ctx2);
>> -        aml_append(while_ctx, if_ctx);
>> -
>> -        aml_append(while_ctx, aml_increment(idx)); /* go to next cpu */
>> -        aml_append(method, while_ctx);
>> -    }
>> -    aml_append(sb_scope, method);
>> -
>> -    /* The current AML generator can cover the APIC ID range [0..255],
>> -     * inclusive, for VCPU hotplug. */
>> -    QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
>> -    if (x86ms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
>> -        error_report("max_cpus is too large. APIC ID of last CPU is %u",
>> -                     x86ms->apic_id_limit - 1);
>> -        exit(1);
>> -    }
>> -
>> -    /* create PCI0.PRES device and its _CRS to reserve CPU hotplug 
>> MMIO */
>> -    dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
>> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
>> -    aml_append(dev,
>> -        aml_name_decl("_UID", aml_string("CPU Hotplug resources"))
>> -    );
>> -    /* device present, functioning, decoding, not shown in UI */
>> -    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>> -    crs = aml_resource_template();
>> -    aml_append(crs,
>> -        aml_io(AML_DECODE16, io_base, io_base, 1, ACPI_GPE_PROC_LEN)
>> -    );
>> -    aml_append(dev, aml_name_decl("_CRS", crs));
>> -    aml_append(sb_scope, dev);
>> -    /* declare CPU hotplug MMIO region and PRS field to access it */
>> -    aml_append(sb_scope, aml_operation_region(
>> -        "PRST", AML_SYSTEM_IO, aml_int(io_base), ACPI_GPE_PROC_LEN));
>> -    field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
>> -    aml_append(field, aml_named_field("PRS", 256));
>> -    aml_append(sb_scope, field);
>> -
>> -    /* build Processor object for each processor */
>> -    for (i = 0; i < apic_ids->len; i++) {
>> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
>> -
>> -        assert(cpu_apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>> -
>> -        dev = aml_processor(i, 0, 0, "CP%.02X", cpu_apic_id);
>> -
>> -        method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
>> -        aml_append(method,
>> -            aml_return(aml_call2(CPU_MAT_METHOD,
>> -                                 aml_int(cpu_apic_id), aml_int(i))
>> -        ));
>> -        aml_append(dev, method);
>> -
>> -        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>> -        aml_append(method,
>> -            aml_return(aml_call1(CPU_STATUS_METHOD, 
>> aml_int(cpu_apic_id))));
>> -        aml_append(dev, method);
>> -
>> -        method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
>> -        aml_append(method,
>> -            aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(cpu_apic_id),
>> -                aml_arg(0)))
>> -        );
>> -        aml_append(dev, method);
>> -
>> -        aml_append(sb_scope, dev);
>> -    }
>> -
>> -    /* build this code:
>> -     *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, 
>> Arg1)} ...}
>> -     */
>> -    /* Arg0 = APIC ID */
>> -    method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
>> -    for (i = 0; i < apic_ids->len; i++) {
>> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
>> -
>> -        if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(cpu_apic_id)));
>> -        aml_append(if_ctx,
>> -            aml_notify(aml_name("CP%.02X", cpu_apic_id), aml_arg(1))
>> -        );
>> -        aml_append(method, if_ctx);
>> -    }
>> -    aml_append(sb_scope, method);
>> -
>> -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
>> -     *
>> -     * Note: The ability to create variable-sized packages was first
>> -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
>> -     * ith up to 255 elements. Windows guests up to win2k8 fail when
>> -     * VarPackageOp is used.
>> -     */
>> -    pkg = x86ms->apic_id_limit <= 255 ? aml_package(x86ms- 
>> >apic_id_limit) :
>> -                                        aml_varpackage(x86ms- 
>> >apic_id_limit);
>> -
>> -    for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
>> -        int cpu_apic_id = apic_ids->cpus[i].arch_id;
>> -
>> -        for (; apic_idx < cpu_apic_id; apic_idx++) {
>> -            aml_append(pkg, aml_int(0));
>> -        }
>> -        aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
>> -        apic_idx = cpu_apic_id + 1;
>> -    }
>> -    aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
>> -    aml_append(ctx, sb_scope);
>> -
>> -    method = aml_method("\\_GPE._E02", 0, AML_NOTSERIALIZED);
>> -    aml_append(method, aml_call0("\\_SB." CPU_SCAN_METHOD));
>> -    aml_append(ctx, method);
>> -}
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 3fffa4a3328..625889783ec 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1465,9 +1465,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>       }
>>       aml_append(dsdt, scope);
>> -    if (pcmc->legacy_cpu_hotplug) {
>> -        build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>> -    } else {
>> +    {
>>           CPUHotplugFeatures opts = {
>>               .acpi_1_compatible = true, .has_legacy_cphp = true,
>>               .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : 
>> NULL,
> 
> Do you happen to know exactly why the CPU hotplug ACPI entry was 
> deprecated (it might be helpful to add this to the commit message).

I'll mention:

commit 679dd1a957df418453efdd3ed2914dba5cd73773
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Wed Jun 15 11:25:23 2016 +0200

     pc: use new CPU hotplug interface since 2.7 machine type

     For compatibility reasons PC/Q35 will start with legacy
     CPU hotplug interface by default but with new CPU hotplug
     AML code since 2.7 machine type. That way legacy firmware
     that doesn't use QEMU generated ACPI tables will be
     able to continue using legacy CPU hotplug interface.

     While new machine type, with firmware supporting QEMU
     provided ACPI tables, will generate new CPU hotplug AML,
     which will switch to new CPU hotplug interface when
     guest OS executes its _INI method on ACPI tables
     loading.


> Anyhow:
> 
> Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>

Thanks!

Re: [PATCH v2 02/16] hw/i386/pc: Remove PCMachineClass::legacy_cpu_hotplug field

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The PCMachineClass::legacy_cpu_hotplug boolean was only used
> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
> removed. Remove it and simplify build_dsdt(), removing
> build_legacy_cpu_hotplug_aml() altogether.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/acpi/cpu_hotplug.h |   3 -
>   include/hw/i386/pc.h          |   3 -
>   hw/acpi/cpu_hotplug.c         | 230 ----------------------------------
>   hw/i386/acpi-build.c          |   4 +-
>   4 files changed, 1 insertion(+), 239 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Philippe Mathieu-Daudé]

The X86MachineClass::fwcfg_dma_enabled boolean was only used
by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
removed. Remove it and simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/x86.h | 2 --
 hw/i386/microvm.c     | 3 ---
 hw/i386/multiboot.c   | 7 +------
 hw/i386/x86-common.c  | 3 +--
 hw/i386/x86.c         | 2 --
 5 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index fc460b82f82..29d37af11e6 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -29,8 +29,6 @@
 struct X86MachineClass {
     MachineClass parent;
 
-    /* use DMA capable linuxboot option rom */
-    bool fwcfg_dma_enabled;
     /* CPU and apic information: */
     bool apic_xrupt_override;
 };
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index e0daf0d4fc3..b1262fb1523 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -637,7 +637,6 @@ GlobalProperty microvm_properties[] = {
 
 static void microvm_class_init(ObjectClass *oc, const void *data)
 {
-    X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
     MicrovmMachineClass *mmc = MICROVM_MACHINE_CLASS(oc);
     MachineClass *mc = MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
@@ -671,8 +670,6 @@ static void microvm_class_init(ObjectClass *oc, const void *data)
     hc->unplug_request = microvm_device_unplug_request_cb;
     hc->unplug = microvm_device_unplug_cb;
 
-    x86mc->fwcfg_dma_enabled = true;
-
     object_class_property_add(oc, MICROVM_MACHINE_RTC, "OnOffAuto",
                               microvm_machine_get_rtc,
                               microvm_machine_set_rtc,
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 6e6b96bc345..bfa7e8f1e83 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
                    int kernel_file_size,
                    uint8_t *header)
 {
-    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
     int i, is_multiboot = 0;
     uint32_t flags = 0;
     uint32_t mh_entry_addr;
@@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
     fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
                      sizeof(bootinfo));
 
-    if (multiboot_dma_enabled) {
-        option_rom[nb_option_roms].name = "multiboot_dma.bin";
-    } else {
-        option_rom[nb_option_roms].name = "multiboot.bin";
-    }
+    option_rom[nb_option_roms].name = "multiboot_dma.bin";
     option_rom[nb_option_roms].bootindex = 0;
     nb_option_roms++;
 
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index 1b0671c5239..27254a0e9f1 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -634,7 +634,6 @@ void x86_load_linux(X86MachineState *x86ms,
                     int acpi_data_size,
                     bool pvh_enabled)
 {
-    bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
     uint16_t protocol;
     int setup_size, kernel_size, cmdline_size;
     int dtb_size, setup_data_offset;
@@ -993,7 +992,7 @@ void x86_load_linux(X86MachineState *x86ms,
 
     option_rom[nb_option_roms].bootindex = 0;
     option_rom[nb_option_roms].name = "linuxboot.bin";
-    if (linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
+    if (fw_cfg_dma_enabled(fw_cfg)) {
         option_rom[nb_option_roms].name = "linuxboot_dma.bin";
     }
     nb_option_roms++;
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f80533df1c5..dbf104d60af 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -375,14 +375,12 @@ static void x86_machine_initfn(Object *obj)
 static void x86_machine_class_init(ObjectClass *oc, const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
-    X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
 
     mc->cpu_index_to_instance_props = x86_cpu_index_to_props;
     mc->get_default_cpu_node_id = x86_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
     mc->kvm_type = x86_kvm_type;
-    x86mc->fwcfg_dma_enabled = true;
     nc->nmi_monitor_handler = x86_nmi;
 
     object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
-- 
2.47.1

Re: [PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The X86MachineClass::fwcfg_dma_enabled boolean was only used
> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
> removed. Remove it and simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/x86.h | 2 --
>   hw/i386/microvm.c     | 3 ---
>   hw/i386/multiboot.c   | 7 +------
>   hw/i386/x86-common.c  | 3 +--
>   hw/i386/x86.c         | 2 --
>   5 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index fc460b82f82..29d37af11e6 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -29,8 +29,6 @@
>   struct X86MachineClass {
>       MachineClass parent;
>   
> -    /* use DMA capable linuxboot option rom */
> -    bool fwcfg_dma_enabled;
>       /* CPU and apic information: */
>       bool apic_xrupt_override;
>   };
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index e0daf0d4fc3..b1262fb1523 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -637,7 +637,6 @@ GlobalProperty microvm_properties[] = {
>   
>   static void microvm_class_init(ObjectClass *oc, const void *data)
>   {
> -    X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
>       MicrovmMachineClass *mmc = MICROVM_MACHINE_CLASS(oc);
>       MachineClass *mc = MACHINE_CLASS(oc);
>       HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> @@ -671,8 +670,6 @@ static void microvm_class_init(ObjectClass *oc, const void *data)
>       hc->unplug_request = microvm_device_unplug_request_cb;
>       hc->unplug = microvm_device_unplug_cb;
>   
> -    x86mc->fwcfg_dma_enabled = true;
> -
>       object_class_property_add(oc, MICROVM_MACHINE_RTC, "OnOffAuto",
>                                 microvm_machine_get_rtc,
>                                 microvm_machine_set_rtc,
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 6e6b96bc345..bfa7e8f1e83 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
>                      int kernel_file_size,
>                      uint8_t *header)
>   {
> -    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
>       int i, is_multiboot = 0;
>       uint32_t flags = 0;
>       uint32_t mh_entry_addr;
> @@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
>       fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
>                        sizeof(bootinfo));
>   
> -    if (multiboot_dma_enabled) {
> -        option_rom[nb_option_roms].name = "multiboot_dma.bin";
> -    } else {
> -        option_rom[nb_option_roms].name = "multiboot.bin";
> -    }
> +    option_rom[nb_option_roms].name = "multiboot_dma.bin";

Question: now that all machines support DMA-capable fw_cfg, does that 
mean that the non-DMA options roms above can also be removed?

>       option_rom[nb_option_roms].bootindex = 0;
>       nb_option_roms++;
>   
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index 1b0671c5239..27254a0e9f1 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -634,7 +634,6 @@ void x86_load_linux(X86MachineState *x86ms,
>                       int acpi_data_size,
>                       bool pvh_enabled)
>   {
> -    bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
>       uint16_t protocol;
>       int setup_size, kernel_size, cmdline_size;
>       int dtb_size, setup_data_offset;
> @@ -993,7 +992,7 @@ void x86_load_linux(X86MachineState *x86ms,
>   
>       option_rom[nb_option_roms].bootindex = 0;
>       option_rom[nb_option_roms].name = "linuxboot.bin";
> -    if (linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
> +    if (fw_cfg_dma_enabled(fw_cfg)) {
>           option_rom[nb_option_roms].name = "linuxboot_dma.bin";
>       }

And same question here too.

>       nb_option_roms++;
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index f80533df1c5..dbf104d60af 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -375,14 +375,12 @@ static void x86_machine_initfn(Object *obj)
>   static void x86_machine_class_init(ObjectClass *oc, const void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> -    X86MachineClass *x86mc = X86_MACHINE_CLASS(oc);
>       NMIClass *nc = NMI_CLASS(oc);
>   
>       mc->cpu_index_to_instance_props = x86_cpu_index_to_props;
>       mc->get_default_cpu_node_id = x86_get_default_cpu_node_id;
>       mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
>       mc->kvm_type = x86_kvm_type;
> -    x86mc->fwcfg_dma_enabled = true;
>       nc->nmi_monitor_handler = x86_nmi;
>   
>       object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",


ATB,

Mark.

Re: [PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Philippe Mathieu-Daudé]

On 2/5/25 11:08, Mark Cave-Ayland wrote:
> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
> 
>> The X86MachineClass::fwcfg_dma_enabled boolean was only used
>> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
>> removed. Remove it and simplify.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/i386/x86.h | 2 --
>>   hw/i386/microvm.c     | 3 ---
>>   hw/i386/multiboot.c   | 7 +------
>>   hw/i386/x86-common.c  | 3 +--
>>   hw/i386/x86.c         | 2 --
>>   5 files changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>> index fc460b82f82..29d37af11e6 100644
>> --- a/include/hw/i386/x86.h
>> +++ b/include/hw/i386/x86.h
>> @@ -29,8 +29,6 @@
>>   struct X86MachineClass {
>>       MachineClass parent;
>> -    /* use DMA capable linuxboot option rom */
>> -    bool fwcfg_dma_enabled;
>>       /* CPU and apic information: */
>>       bool apic_xrupt_override;
>>   };


>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index 6e6b96bc345..bfa7e8f1e83 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
>>                      int kernel_file_size,
>>                      uint8_t *header)
>>   {
>> -    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)- 
>> >fwcfg_dma_enabled;
>>       int i, is_multiboot = 0;
>>       uint32_t flags = 0;
>>       uint32_t mh_entry_addr;
>> @@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
>>       fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
>>                        sizeof(bootinfo));
>> -    if (multiboot_dma_enabled) {
>> -        option_rom[nb_option_roms].name = "multiboot_dma.bin";
>> -    } else {
>> -        option_rom[nb_option_roms].name = "multiboot.bin";
>> -    }
>> +    option_rom[nb_option_roms].name = "multiboot_dma.bin";
> 
> Question: now that all machines support DMA-capable fw_cfg, does that 
> mean that the non-DMA options roms above can also be removed?

All x86 machines, but there are still 2 not supporting it: HPPA and
MIPS Loongson-3:

hw/hppa/machine.c:204:    fw_cfg = fw_cfg_init_mem(addr, addr + 4);

hw/mips/loongson3_virt.c:289:    fw_cfg = fw_cfg_init_mem_wide(cfg_addr, 
cfg_addr + 8, 8, 0, NULL);

Re: [PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Thomas Huth]

On 02/05/2025 12.45, Philippe Mathieu-Daudé wrote:
> On 2/5/25 11:08, Mark Cave-Ayland wrote:
>> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
>>
>>> The X86MachineClass::fwcfg_dma_enabled boolean was only used
>>> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
>>> removed. Remove it and simplify.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/i386/x86.h | 2 --
>>>   hw/i386/microvm.c     | 3 ---
>>>   hw/i386/multiboot.c   | 7 +------
>>>   hw/i386/x86-common.c  | 3 +--
>>>   hw/i386/x86.c         | 2 --
>>>   5 files changed, 2 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>>> index fc460b82f82..29d37af11e6 100644
>>> --- a/include/hw/i386/x86.h
>>> +++ b/include/hw/i386/x86.h
>>> @@ -29,8 +29,6 @@
>>>   struct X86MachineClass {
>>>       MachineClass parent;
>>> -    /* use DMA capable linuxboot option rom */
>>> -    bool fwcfg_dma_enabled;
>>>       /* CPU and apic information: */
>>>       bool apic_xrupt_override;
>>>   };
> 
> 
>>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>>> index 6e6b96bc345..bfa7e8f1e83 100644
>>> --- a/hw/i386/multiboot.c
>>> +++ b/hw/i386/multiboot.c
>>> @@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
>>>                      int kernel_file_size,
>>>                      uint8_t *header)
>>>   {
>>> -    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)- 
>>> >fwcfg_dma_enabled;
>>>       int i, is_multiboot = 0;
>>>       uint32_t flags = 0;
>>>       uint32_t mh_entry_addr;
>>> @@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
>>>       fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
>>>                        sizeof(bootinfo));
>>> -    if (multiboot_dma_enabled) {
>>> -        option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>> -    } else {
>>> -        option_rom[nb_option_roms].name = "multiboot.bin";
>>> -    }
>>> +    option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>
>> Question: now that all machines support DMA-capable fw_cfg, does that mean 
>> that the non-DMA options roms above can also be removed?
> 
> All x86 machines, but there are still 2 not supporting it: HPPA and
> MIPS Loongson-3:
> 
> hw/hppa/machine.c:204:    fw_cfg = fw_cfg_init_mem(addr, addr + 4);
> 
> hw/mips/loongson3_virt.c:289:    fw_cfg = fw_cfg_init_mem_wide(cfg_addr, 
> cfg_addr + 8, 8, 0, NULL);
> 

But these don't use "multiboot.bin", do they? So I think you could remove 
pc-bios/multiboot.bin now from the repo?

Same question for "linuxboot.bin" : All users in hw/i386 seem to enable DMA, 
so fw_cfg_dma_enabled() should always return true here? If so, I think the 
normal "linuxboot.bin" could go away, too?

  Thomas

Re: [PATCH v2 03/16] hw/i386/x86: Remove X86MachineClass::fwcfg_dma_enabled field

[Philippe Mathieu-Daudé]

On 5/5/25 11:06, Thomas Huth wrote:
> On 02/05/2025 12.45, Philippe Mathieu-Daudé wrote:
>> On 2/5/25 11:08, Mark Cave-Ayland wrote:
>>> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
>>>
>>>> The X86MachineClass::fwcfg_dma_enabled boolean was only used
>>>> by the pc-q35-2.6 and pc-i440fx-2.6 machines, which got
>>>> removed. Remove it and simplify.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   include/hw/i386/x86.h | 2 --
>>>>   hw/i386/microvm.c     | 3 ---
>>>>   hw/i386/multiboot.c   | 7 +------
>>>>   hw/i386/x86-common.c  | 3 +--
>>>>   hw/i386/x86.c         | 2 --
>>>>   5 files changed, 2 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>>>> index fc460b82f82..29d37af11e6 100644
>>>> --- a/include/hw/i386/x86.h
>>>> +++ b/include/hw/i386/x86.h
>>>> @@ -29,8 +29,6 @@
>>>>   struct X86MachineClass {
>>>>       MachineClass parent;
>>>> -    /* use DMA capable linuxboot option rom */
>>>> -    bool fwcfg_dma_enabled;
>>>>       /* CPU and apic information: */
>>>>       bool apic_xrupt_override;
>>>>   };
>>
>>
>>>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>>>> index 6e6b96bc345..bfa7e8f1e83 100644
>>>> --- a/hw/i386/multiboot.c
>>>> +++ b/hw/i386/multiboot.c
>>>> @@ -153,7 +153,6 @@ int load_multiboot(X86MachineState *x86ms,
>>>>                      int kernel_file_size,
>>>>                      uint8_t *header)
>>>>   {
>>>> -    bool multiboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)- 
>>>> >fwcfg_dma_enabled;
>>>>       int i, is_multiboot = 0;
>>>>       uint32_t flags = 0;
>>>>       uint32_t mh_entry_addr;
>>>> @@ -402,11 +401,7 @@ int load_multiboot(X86MachineState *x86ms,
>>>>       fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
>>>>                        sizeof(bootinfo));
>>>> -    if (multiboot_dma_enabled) {
>>>> -        option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>>> -    } else {
>>>> -        option_rom[nb_option_roms].name = "multiboot.bin";
>>>> -    }
>>>> +    option_rom[nb_option_roms].name = "multiboot_dma.bin";
>>>
>>> Question: now that all machines support DMA-capable fw_cfg, does that 
>>> mean that the non-DMA options roms above can also be removed?
>>
>> All x86 machines, but there are still 2 not supporting it: HPPA and
>> MIPS Loongson-3:
>>
>> hw/hppa/machine.c:204:    fw_cfg = fw_cfg_init_mem(addr, addr + 4);
>>
>> hw/mips/loongson3_virt.c:289:    fw_cfg = 
>> fw_cfg_init_mem_wide(cfg_addr, cfg_addr + 8, 8, 0, NULL);
>>
> 
> But these don't use "multiboot.bin", do they? So I think you could 
> remove pc-bios/multiboot.bin now from the repo?
> 
> Same question for "linuxboot.bin" : All users in hw/i386 seem to enable 
> DMA, so fw_cfg_dma_enabled() should always return true here? If so, I 
> think the normal "linuxboot.bin" could go away, too?

You are right!

[PATCH v2 04/16] hw/i386/pc: Remove pc_compat_2_6[] array

[Philippe Mathieu-Daudé]

The pc_compat_2_6[] array was only used by the pc-q35-2.6
and pc-i440fx-2.6 machines, which got removed. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h | 3 ---
 hw/i386/pc.c         | 8 --------
 2 files changed, 11 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a3de3e9560d..4fb2033bc54 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -292,9 +292,6 @@ extern const size_t pc_compat_2_8_len;
 extern GlobalProperty pc_compat_2_7[];
 extern const size_t pc_compat_2_7_len;
 
-extern GlobalProperty pc_compat_2_6[];
-extern const size_t pc_compat_2_6_len;
-
 #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
     static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
                                                  const void *data) \
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 49632b69d29..7573b880905 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -251,14 +251,6 @@ GlobalProperty pc_compat_2_7[] = {
 };
 const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
 
-GlobalProperty pc_compat_2_6[] = {
-    { TYPE_X86_CPU, "cpuid-0xb", "off" },
-    { "vmxnet3", "romfile", "" },
-    { TYPE_X86_CPU, "fill-mtrr-mask", "off" },
-    { "apic-common", "legacy-instance-id", "on", }
-};
-const size_t pc_compat_2_6_len = G_N_ELEMENTS(pc_compat_2_6);
-
 /*
  * @PC_FW_DATA:
  * Size of the chunk of memory at the top of RAM for the BIOS ACPI tables
-- 
2.47.1

Re: [PATCH v2 04/16] hw/i386/pc: Remove pc_compat_2_6[] array

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The pc_compat_2_6[] array was only used by the pc-q35-2.6
> and pc-i440fx-2.6 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h | 3 ---
>   hw/i386/pc.c         | 8 --------
>   2 files changed, 11 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a3de3e9560d..4fb2033bc54 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -292,9 +292,6 @@ extern const size_t pc_compat_2_8_len;
>   extern GlobalProperty pc_compat_2_7[];
>   extern const size_t pc_compat_2_7_len;
>   
> -extern GlobalProperty pc_compat_2_6[];
> -extern const size_t pc_compat_2_6_len;
> -
>   #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
>       static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
>                                                    const void *data) \
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 49632b69d29..7573b880905 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -251,14 +251,6 @@ GlobalProperty pc_compat_2_7[] = {
>   };
>   const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
>   
> -GlobalProperty pc_compat_2_6[] = {
> -    { TYPE_X86_CPU, "cpuid-0xb", "off" },
> -    { "vmxnet3", "romfile", "" },
> -    { TYPE_X86_CPU, "fill-mtrr-mask", "off" },
> -    { "apic-common", "legacy-instance-id", "on", }
> -};
> -const size_t pc_compat_2_6_len = G_N_ELEMENTS(pc_compat_2_6);
> -
>   /*
>    * @PC_FW_DATA:
>    * Size of the chunk of memory at the top of RAM for the BIOS ACPI tables

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


ATB,

Mark.

Re: [PATCH v2 04/16] hw/i386/pc: Remove pc_compat_2_6[] array

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The pc_compat_2_6[] array was only used by the pc-q35-2.6
> and pc-i440fx-2.6 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h | 3 ---
>   hw/i386/pc.c         | 8 --------
>   2 files changed, 11 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 04/16] hw/i386/pc: Remove pc_compat_2_6[] array

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The pc_compat_2_6[] array was only used by the pc-q35-2.6
> and pc-i440fx-2.6 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h | 3 ---
>   hw/i386/pc.c         | 8 --------
>   2 files changed, 11 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a3de3e9560d..4fb2033bc54 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -292,9 +292,6 @@ extern const size_t pc_compat_2_8_len;
>   extern GlobalProperty pc_compat_2_7[];
>   extern const size_t pc_compat_2_7_len;
>   
> -extern GlobalProperty pc_compat_2_6[];
> -extern const size_t pc_compat_2_6_len;
> -
>   #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
>       static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
>                                                    const void *data) \
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 49632b69d29..7573b880905 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -251,14 +251,6 @@ GlobalProperty pc_compat_2_7[] = {
>   };
>   const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
>   
> -GlobalProperty pc_compat_2_6[] = {
> -    { TYPE_X86_CPU, "cpuid-0xb", "off" },
> -    { "vmxnet3", "romfile", "" },
> -    { TYPE_X86_CPU, "fill-mtrr-mask", "off" },
> -    { "apic-common", "legacy-instance-id", "on", }
> -};

So you took care of apic-common in the next patch, but what about 
fill-mtrr-mask and cpuid-0xb ? I think these could be cleaned up, too?

  Thomas

Re: [PATCH v2 04/16] hw/i386/pc: Remove pc_compat_2_6[] array

[Philippe Mathieu-Daudé]

On 5/5/25 11:19, Thomas Huth wrote:
> On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
>> The pc_compat_2_6[] array was only used by the pc-q35-2.6
>> and pc-i440fx-2.6 machines, which got removed. Remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/i386/pc.h | 3 ---
>>   hw/i386/pc.c         | 8 --------
>>   2 files changed, 11 deletions(-)
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index a3de3e9560d..4fb2033bc54 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -292,9 +292,6 @@ extern const size_t pc_compat_2_8_len;
>>   extern GlobalProperty pc_compat_2_7[];
>>   extern const size_t pc_compat_2_7_len;
>> -extern GlobalProperty pc_compat_2_6[];
>> -extern const size_t pc_compat_2_6_len;
>> -
>>   #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
>>       static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
>>                                                    const void *data) \
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 49632b69d29..7573b880905 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -251,14 +251,6 @@ GlobalProperty pc_compat_2_7[] = {
>>   };
>>   const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
>> -GlobalProperty pc_compat_2_6[] = {
>> -    { TYPE_X86_CPU, "cpuid-0xb", "off" },
>> -    { "vmxnet3", "romfile", "" },
>> -    { TYPE_X86_CPU, "fill-mtrr-mask", "off" },
>> -    { "apic-common", "legacy-instance-id", "on", }
>> -};
> 
> So you took care of apic-common in the next patch, but what about fill- 
> mtrr-mask and cpuid-0xb ? I think these could be cleaned up, too?

I'm not sure about CPU features, I'll defer the corresponding cleanups
to someone more familiar with the architecture and its real world uses.

[PATCH v2 05/16] hw/intc/apic: Remove APICCommonState::legacy_instance_id field

[Philippe Mathieu-Daudé]

The APICCommonState::legacy_instance_id boolean was only set
in the pc_compat_2_6[] array, via the 'legacy-instance-id=on'
property. We removed all machines using that array, lets remove
that property, simplifying apic_common_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/apic_internal.h | 1 -
 hw/intc/apic_common.c           | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 429278da618..db6a9101530 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -188,7 +188,6 @@ struct APICCommonState {
     uint32_t vapic_control;
     DeviceState *vapic;
     hwaddr vapic_paddr; /* note: persistence via kvmvapic */
-    bool legacy_instance_id;
     uint32_t extended_log_dest;
 };
 
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 37a7a7019d3..1d259b97e63 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -294,9 +294,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
         info->enable_tpr_reporting(s, true);
     }
 
-    if (s->legacy_instance_id) {
-        instance_id = VMSTATE_INSTANCE_ID_ANY;
-    }
     vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
                                    s, -1, 0, NULL);
 
@@ -412,8 +409,6 @@ static const Property apic_properties_common[] = {
     DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
     DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
                     true),
-    DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
-                     false),
 };
 
 static void apic_common_get_id(Object *obj, Visitor *v, const char *name,
-- 
2.47.1

Re: [PATCH v2 05/16] hw/intc/apic: Remove APICCommonState::legacy_instance_id field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The APICCommonState::legacy_instance_id boolean was only set
> in the pc_compat_2_6[] array, via the 'legacy-instance-id=on'
> property. We removed all machines using that array, lets remove
> that property, simplifying apic_common_realize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/apic_internal.h | 1 -
>   hw/intc/apic_common.c           | 5 -----
>   2 files changed, 6 deletions(-)
> 
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 429278da618..db6a9101530 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -188,7 +188,6 @@ struct APICCommonState {
>       uint32_t vapic_control;
>       DeviceState *vapic;
>       hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> -    bool legacy_instance_id;
>       uint32_t extended_log_dest;
>   };
>   
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 37a7a7019d3..1d259b97e63 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -294,9 +294,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>           info->enable_tpr_reporting(s, true);
>       }
>   
> -    if (s->legacy_instance_id) {
> -        instance_id = VMSTATE_INSTANCE_ID_ANY;
> -    }
>       vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
>                                      s, -1, 0, NULL);

With the legacy_instance_id removed, is it now also possible to register 
vmstate_apic_common directly via dc->vmsd instead?

> @@ -412,8 +409,6 @@ static const Property apic_properties_common[] = {
>       DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
>       DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
>                       true),
> -    DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
> -                     false),
>   };
>   
>   static void apic_common_get_id(Object *obj, Visitor *v, const char *name,

Anyhow:

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


ATB,

Mark.

Re: [PATCH v2 05/16] hw/intc/apic: Remove APICCommonState::legacy_instance_id field

[Philippe Mathieu-Daudé]

Hi Mark,

On 2/5/25 11:14, Mark Cave-Ayland wrote:
> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
> 
>> The APICCommonState::legacy_instance_id boolean was only set
>> in the pc_compat_2_6[] array, via the 'legacy-instance-id=on'
>> property. We removed all machines using that array, lets remove
>> that property, simplifying apic_common_realize().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/i386/apic_internal.h | 1 -
>>   hw/intc/apic_common.c           | 5 -----
>>   2 files changed, 6 deletions(-)
>>
>> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/ 
>> apic_internal.h
>> index 429278da618..db6a9101530 100644
>> --- a/include/hw/i386/apic_internal.h
>> +++ b/include/hw/i386/apic_internal.h
>> @@ -188,7 +188,6 @@ struct APICCommonState {
>>       uint32_t vapic_control;
>>       DeviceState *vapic;
>>       hwaddr vapic_paddr; /* note: persistence via kvmvapic */
>> -    bool legacy_instance_id;
>>       uint32_t extended_log_dest;
>>   };
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index 37a7a7019d3..1d259b97e63 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -294,9 +294,6 @@ static void apic_common_realize(DeviceState *dev, 
>> Error **errp)
>>           info->enable_tpr_reporting(s, true);
>>       }
>> -    if (s->legacy_instance_id) {
>> -        instance_id = VMSTATE_INSTANCE_ID_ANY;
>> -    }
>>       vmstate_register_with_alias_id(NULL, instance_id, 
>> &vmstate_apic_common,
>>                                      s, -1, 0, NULL);
> 
> With the legacy_instance_id removed, is it now also possible to register 
> vmstate_apic_common directly via dc->vmsd instead?

I don't think so because it still uses initial_apic_id, but I'm not sure:

     uint32_t instance_id = s->initial_apic_id;

Re: [PATCH v2 05/16] hw/intc/apic: Remove APICCommonState::legacy_instance_id field

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The APICCommonState::legacy_instance_id boolean was only set
> in the pc_compat_2_6[] array, via the 'legacy-instance-id=on'
> property. We removed all machines using that array, lets remove
> that property, simplifying apic_common_realize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/apic_internal.h | 1 -
>   hw/intc/apic_common.c           | 5 -----
>   2 files changed, 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 06/16] hw/core/machine: Remove hw_compat_2_6[] array

[Philippe Mathieu-Daudé]

The hw_compat_2_6[] array was only used by the pc-q35-2.6 and
pc-i440fx-2.6 machines, which got removed. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/boards.h | 3 ---
 hw/core/machine.c   | 8 --------
 2 files changed, 11 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5f1a0fb7e28..a881db8e7d6 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -841,7 +841,4 @@ extern const size_t hw_compat_2_8_len;
 extern GlobalProperty hw_compat_2_7[];
 extern const size_t hw_compat_2_7_len;
 
-extern GlobalProperty hw_compat_2_6[];
-extern const size_t hw_compat_2_6_len;
-
 #endif
diff --git a/hw/core/machine.c b/hw/core/machine.c
index e7001bf92cd..ce98820f277 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -275,14 +275,6 @@ GlobalProperty hw_compat_2_7[] = {
 };
 const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
 
-GlobalProperty hw_compat_2_6[] = {
-    { "virtio-mmio", "format_transport_address", "off" },
-    /* Optional because not all virtio-pci devices support legacy mode */
-    { "virtio-pci", "disable-modern", "on",  .optional = true },
-    { "virtio-pci", "disable-legacy", "off", .optional = true },
-};
-const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
-
 MachineState *current_machine;
 
 static char *machine_get_kernel(Object *obj, Error **errp)
-- 
2.47.1

Re: [PATCH v2 06/16] hw/core/machine: Remove hw_compat_2_6[] array

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The hw_compat_2_6[] array was only used by the pc-q35-2.6 and
> pc-i440fx-2.6 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/boards.h | 3 ---
>   hw/core/machine.c   | 8 --------
>   2 files changed, 11 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5f1a0fb7e28..a881db8e7d6 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -841,7 +841,4 @@ extern const size_t hw_compat_2_8_len;
>   extern GlobalProperty hw_compat_2_7[];
>   extern const size_t hw_compat_2_7_len;
>   
> -extern GlobalProperty hw_compat_2_6[];
> -extern const size_t hw_compat_2_6_len;
> -
>   #endif
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e7001bf92cd..ce98820f277 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -275,14 +275,6 @@ GlobalProperty hw_compat_2_7[] = {
>   };
>   const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
>   
> -GlobalProperty hw_compat_2_6[] = {
> -    { "virtio-mmio", "format_transport_address", "off" },
> -    /* Optional because not all virtio-pci devices support legacy mode */
> -    { "virtio-pci", "disable-modern", "on",  .optional = true },
> -    { "virtio-pci", "disable-legacy", "off", .optional = true },
> -};
> -const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
> -
>   MachineState *current_machine;
>   
>   static char *machine_get_kernel(Object *obj, Error **errp)

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


ATB,

Mark.

Re: [PATCH v2 06/16] hw/core/machine: Remove hw_compat_2_6[] array

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The hw_compat_2_6[] array was only used by the pc-q35-2.6 and
> pc-i440fx-2.6 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/boards.h | 3 ---
>   hw/core/machine.c   | 8 --------
>   2 files changed, 11 deletions(-)

It's still used in the arm virt machine which has not been removed yet?

  Thomas

Re: [PATCH v2 06/16] hw/core/machine: Remove hw_compat_2_6[] array

[Philippe Mathieu-Daudé]

On 5/5/25 11:13, Thomas Huth wrote:
> On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
>> The hw_compat_2_6[] array was only used by the pc-q35-2.6 and
>> pc-i440fx-2.6 machines, which got removed. Remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/boards.h | 3 ---
>>   hw/core/machine.c   | 8 --------
>>   2 files changed, 11 deletions(-)
> 
> It's still used in the arm virt machine which has not been removed yet?

Sorry, I forgot to mention in the cover this series consider
ARM virt-2.6 up to virt-2.12 machines removed because the
series doing that is already queued in target-arm.next:
https://lore.kernel.org/qemu-devel/20250116145944.38028-1-philmd@linaro.org/

Re: [PATCH v2 06/16] hw/core/machine: Remove hw_compat_2_6[] array

[Philippe Mathieu-Daudé]

On 5/5/25 11:58, Philippe Mathieu-Daudé wrote:
> On 5/5/25 11:13, Thomas Huth wrote:
>> On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
>>> The hw_compat_2_6[] array was only used by the pc-q35-2.6 and
>>> pc-i440fx-2.6 machines, which got removed. Remove it.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/boards.h | 3 ---
>>>   hw/core/machine.c   | 8 --------
>>>   2 files changed, 11 deletions(-)
>>
>> It's still used in the arm virt machine which has not been removed yet?
> 
> Sorry, I forgot to mention in the cover this series consider
> ARM virt-2.6 up to virt-2.12 machines removed because the
> series doing that is already queued in target-arm.next:
> https://lore.kernel.org/qemu-devel/20250116145944.38028-1- 
> philmd@linaro.org/

The ARM patches has been merged as commits 1731ec33c38 ("hw/arm/virt:
Remove deprecated virt-2.6 machine") and 390becbf7fa ("hw/arm/virt:
Remove deprecated virt-2.7 machine").

[PATCH v2 07/16] hw/virtio/virtio-mmio: Remove VirtIOMMIOProxy::format_transport_address field

[Philippe Mathieu-Daudé]

The VirtIOMMIOProxy::format_transport_address boolean was only set
in the hw_compat_2_6[] array, via the 'format_transport_address=off'
property. We removed all machines using that array, lets remove
that property, simplifying virtio_mmio_bus_get_dev_path().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/virtio/virtio-mmio.h |  1 -
 hw/virtio/virtio-mmio.c         | 15 ---------------
 2 files changed, 16 deletions(-)

diff --git a/include/hw/virtio/virtio-mmio.h b/include/hw/virtio/virtio-mmio.h
index aa492620228..8b19ec2291a 100644
--- a/include/hw/virtio/virtio-mmio.h
+++ b/include/hw/virtio/virtio-mmio.h
@@ -66,7 +66,6 @@ struct VirtIOMMIOProxy {
     uint32_t guest_page_shift;
     /* virtio-bus */
     VirtioBusState bus;
-    bool format_transport_address;
     /* Fields only used for non-legacy (v2) devices */
     uint32_t guest_features[2];
     VirtIOMMIOQueue vqs[VIRTIO_QUEUE_MAX];
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 532c67107ba..b7ee115b990 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -752,8 +752,6 @@ static void virtio_mmio_pre_plugged(DeviceState *d, Error **errp)
 /* virtio-mmio device */
 
 static const Property virtio_mmio_properties[] = {
-    DEFINE_PROP_BOOL("format_transport_address", VirtIOMMIOProxy,
-                     format_transport_address, true),
     DEFINE_PROP_BOOL("force-legacy", VirtIOMMIOProxy, legacy, true),
     DEFINE_PROP_BIT("ioeventfd", VirtIOMMIOProxy, flags,
                     VIRTIO_IOMMIO_FLAG_USE_IOEVENTFD_BIT, true),
@@ -815,19 +813,6 @@ static char *virtio_mmio_bus_get_dev_path(DeviceState *dev)
     virtio_mmio_proxy = VIRTIO_MMIO(virtio_mmio_bus->parent);
     proxy_path = qdev_get_dev_path(DEVICE(virtio_mmio_proxy));
 
-    /*
-     * If @format_transport_address is false, then we just perform the same as
-     * virtio_bus_get_dev_path(): we delegate the address formatting for the
-     * device on the virtio-mmio bus to the bus that the virtio-mmio proxy
-     * (i.e., the device that implements the virtio-mmio bus) resides on. In
-     * this case the base address of the virtio-mmio transport will be
-     * invisible.
-     */
-    if (!virtio_mmio_proxy->format_transport_address) {
-        return proxy_path;
-    }
-
-    /* Otherwise, we append the base address of the transport. */
     section = memory_region_find(&virtio_mmio_proxy->iomem, 0, 0x200);
     assert(section.mr);
 
-- 
2.47.1

Re: [PATCH v2 07/16] hw/virtio/virtio-mmio: Remove VirtIOMMIOProxy::format_transport_address field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The VirtIOMMIOProxy::format_transport_address boolean was only set
> in the hw_compat_2_6[] array, via the 'format_transport_address=off'
> property. We removed all machines using that array, lets remove
> that property, simplifying virtio_mmio_bus_get_dev_path().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/virtio/virtio-mmio.h |  1 -
>   hw/virtio/virtio-mmio.c         | 15 ---------------
>   2 files changed, 16 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-mmio.h b/include/hw/virtio/virtio-mmio.h
> index aa492620228..8b19ec2291a 100644
> --- a/include/hw/virtio/virtio-mmio.h
> +++ b/include/hw/virtio/virtio-mmio.h
> @@ -66,7 +66,6 @@ struct VirtIOMMIOProxy {
>       uint32_t guest_page_shift;
>       /* virtio-bus */
>       VirtioBusState bus;
> -    bool format_transport_address;
>       /* Fields only used for non-legacy (v2) devices */
>       uint32_t guest_features[2];
>       VirtIOMMIOQueue vqs[VIRTIO_QUEUE_MAX];
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 532c67107ba..b7ee115b990 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -752,8 +752,6 @@ static void virtio_mmio_pre_plugged(DeviceState *d, Error **errp)
>   /* virtio-mmio device */
>   
>   static const Property virtio_mmio_properties[] = {
> -    DEFINE_PROP_BOOL("format_transport_address", VirtIOMMIOProxy,
> -                     format_transport_address, true),
>       DEFINE_PROP_BOOL("force-legacy", VirtIOMMIOProxy, legacy, true),
>       DEFINE_PROP_BIT("ioeventfd", VirtIOMMIOProxy, flags,
>                       VIRTIO_IOMMIO_FLAG_USE_IOEVENTFD_BIT, true),
> @@ -815,19 +813,6 @@ static char *virtio_mmio_bus_get_dev_path(DeviceState *dev)
>       virtio_mmio_proxy = VIRTIO_MMIO(virtio_mmio_bus->parent);
>       proxy_path = qdev_get_dev_path(DEVICE(virtio_mmio_proxy));
>   
> -    /*
> -     * If @format_transport_address is false, then we just perform the same as
> -     * virtio_bus_get_dev_path(): we delegate the address formatting for the
> -     * device on the virtio-mmio bus to the bus that the virtio-mmio proxy
> -     * (i.e., the device that implements the virtio-mmio bus) resides on. In
> -     * this case the base address of the virtio-mmio transport will be
> -     * invisible.
> -     */
> -    if (!virtio_mmio_proxy->format_transport_address) {
> -        return proxy_path;
> -    }
> -
> -    /* Otherwise, we append the base address of the transport. */
>       section = memory_region_find(&virtio_mmio_proxy->iomem, 0, 0x200);
>       assert(section.mr);

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


ATB,

Mark.

[PATCH v2 08/16] hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines

[Philippe Mathieu-Daudé]

These machines has been supported for a period of more than 6 years.
According to our versioned machine support policy (see commit
ce80c4fa6ff "docs: document special exception for machine type
deprecation & removal") they can now be removed.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c |  9 ---------
 hw/i386/pc_q35.c  | 10 ----------
 2 files changed, 19 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 98a118fd4a0..98bd8d0e67b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -755,15 +755,6 @@ static void pc_i440fx_machine_2_8_options(MachineClass *m)
 
 DEFINE_I440FX_MACHINE(2, 8);
 
-static void pc_i440fx_machine_2_7_options(MachineClass *m)
-{
-    pc_i440fx_machine_2_8_options(m);
-    compat_props_add(m->compat_props, hw_compat_2_7, hw_compat_2_7_len);
-    compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
-}
-
-DEFINE_I440FX_MACHINE(2, 7);
-
 #ifdef CONFIG_ISAPC
 static void isapc_machine_options(MachineClass *m)
 {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b7ffb5f1216..a1f46cd8f03 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -648,13 +648,3 @@ static void pc_q35_machine_2_8_options(MachineClass *m)
 }
 
 DEFINE_Q35_MACHINE(2, 8);
-
-static void pc_q35_machine_2_7_options(MachineClass *m)
-{
-    pc_q35_machine_2_8_options(m);
-    m->max_cpus = 255;
-    compat_props_add(m->compat_props, hw_compat_2_7, hw_compat_2_7_len);
-    compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
-}
-
-DEFINE_Q35_MACHINE(2, 7);
-- 
2.47.1

Re: [PATCH v2 08/16] hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> These machines has been supported for a period of more than 6 years.
> According to our versioned machine support policy (see commit
> ce80c4fa6ff "docs: document special exception for machine type
> deprecation & removal") they can now be removed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c |  9 ---------
>   hw/i386/pc_q35.c  | 10 ----------
>   2 files changed, 19 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 98a118fd4a0..98bd8d0e67b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -755,15 +755,6 @@ static void pc_i440fx_machine_2_8_options(MachineClass *m)
>   
>   DEFINE_I440FX_MACHINE(2, 8);
>   
> -static void pc_i440fx_machine_2_7_options(MachineClass *m)
> -{
> -    pc_i440fx_machine_2_8_options(m);
> -    compat_props_add(m->compat_props, hw_compat_2_7, hw_compat_2_7_len);
> -    compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
> -}
> -
> -DEFINE_I440FX_MACHINE(2, 7);
> -
>   #ifdef CONFIG_ISAPC
>   static void isapc_machine_options(MachineClass *m)
>   {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b7ffb5f1216..a1f46cd8f03 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -648,13 +648,3 @@ static void pc_q35_machine_2_8_options(MachineClass *m)
>   }
>   
>   DEFINE_Q35_MACHINE(2, 8);
> -
> -static void pc_q35_machine_2_7_options(MachineClass *m)
> -{
> -    pc_q35_machine_2_8_options(m);
> -    m->max_cpus = 255;
> -    compat_props_add(m->compat_props, hw_compat_2_7, hw_compat_2_7_len);
> -    compat_props_add(m->compat_props, pc_compat_2_7, pc_compat_2_7_len);
> -}
> -
> -DEFINE_Q35_MACHINE(2, 7);

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


ATB,

Mark.

Re: [PATCH v2 08/16] hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> These machines has been supported for a period of more than 6 years.
> According to our versioned machine support policy (see commit
> ce80c4fa6ff "docs: document special exception for machine type
> deprecation & removal") they can now be removed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c |  9 ---------
>   hw/i386/pc_q35.c  | 10 ----------
>   2 files changed, 19 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 08/16] hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> These machines has been supported for a period of more than 6 years.
> According to our versioned machine support policy (see commit
> ce80c4fa6ff "docs: document special exception for machine type
> deprecation & removal") they can now be removed.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c |  9 ---------
>   hw/i386/pc_q35.c  | 10 ----------
>   2 files changed, 19 deletions(-)

Please also remove the related code from tests/qtest/test-x86-cpuid-compat.c!

  Thanks,
   Thomas

Re: [PATCH v2 08/16] hw/i386/pc: Remove deprecated pc-q35-2.7 and pc-i440fx-2.7 machines

[Philippe Mathieu-Daudé]

On 5/5/25 12:35, Thomas Huth wrote:
> On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
>> These machines has been supported for a period of more than 6 years.
>> According to our versioned machine support policy (see commit
>> ce80c4fa6ff "docs: document special exception for machine type
>> deprecation & removal") they can now be removed.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c |  9 ---------
>>   hw/i386/pc_q35.c  | 10 ----------
>>   2 files changed, 19 deletions(-)
> 
> Please also remove the related code from tests/qtest/test-x86-cpuid- 
> compat.c!

Considered in v3 ;)
https://lore.kernel.org/qemu-devel/97eadaef-7c1b-4561-af9a-bd887e00d19c@linaro.org/

[PATCH v2 09/16] hw/i386/pc: Remove pc_compat_2_7[] array

[Philippe Mathieu-Daudé]

The pc_compat_2_7[] array was only used by the pc-q35-2.7
and pc-i440fx-2.7 machines, which got removed. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h |  3 ---
 hw/i386/pc.c         | 10 ----------
 2 files changed, 13 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 4fb2033bc54..319ec82f709 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -289,9 +289,6 @@ extern const size_t pc_compat_2_9_len;
 extern GlobalProperty pc_compat_2_8[];
 extern const size_t pc_compat_2_8_len;
 
-extern GlobalProperty pc_compat_2_7[];
-extern const size_t pc_compat_2_7_len;
-
 #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
     static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
                                                  const void *data) \
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7573b880905..ee7095c89a8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -241,16 +241,6 @@ GlobalProperty pc_compat_2_8[] = {
 };
 const size_t pc_compat_2_8_len = G_N_ELEMENTS(pc_compat_2_8);
 
-GlobalProperty pc_compat_2_7[] = {
-    { TYPE_X86_CPU, "l3-cache", "off" },
-    { TYPE_X86_CPU, "full-cpuid-auto-level", "off" },
-    { "Opteron_G3" "-" TYPE_X86_CPU, "family", "15" },
-    { "Opteron_G3" "-" TYPE_X86_CPU, "model", "6" },
-    { "Opteron_G3" "-" TYPE_X86_CPU, "stepping", "1" },
-    { "isa-pcspk", "migrate", "off" },
-};
-const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
-
 /*
  * @PC_FW_DATA:
  * Size of the chunk of memory at the top of RAM for the BIOS ACPI tables
-- 
2.47.1

Re: [PATCH v2 09/16] hw/i386/pc: Remove pc_compat_2_7[] array

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The pc_compat_2_7[] array was only used by the pc-q35-2.7
> and pc-i440fx-2.7 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h |  3 ---
>   hw/i386/pc.c         | 10 ----------
>   2 files changed, 13 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4fb2033bc54..319ec82f709 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -289,9 +289,6 @@ extern const size_t pc_compat_2_9_len;
>   extern GlobalProperty pc_compat_2_8[];
>   extern const size_t pc_compat_2_8_len;
>   
> -extern GlobalProperty pc_compat_2_7[];
> -extern const size_t pc_compat_2_7_len;
> -
>   #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
>       static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
>                                                    const void *data) \
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7573b880905..ee7095c89a8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -241,16 +241,6 @@ GlobalProperty pc_compat_2_8[] = {
>   };
>   const size_t pc_compat_2_8_len = G_N_ELEMENTS(pc_compat_2_8);
>   
> -GlobalProperty pc_compat_2_7[] = {
> -    { TYPE_X86_CPU, "l3-cache", "off" },
> -    { TYPE_X86_CPU, "full-cpuid-auto-level", "off" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "family", "15" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "model", "6" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "stepping", "1" },
> -    { "isa-pcspk", "migrate", "off" },
> -};
> -const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);
> -
>   /*
>    * @PC_FW_DATA:
>    * Size of the chunk of memory at the top of RAM for the BIOS ACPI tables

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


ATB,

Mark.

Re: [PATCH v2 09/16] hw/i386/pc: Remove pc_compat_2_7[] array

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The pc_compat_2_7[] array was only used by the pc-q35-2.7
> and pc-i440fx-2.7 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h |  3 ---
>   hw/i386/pc.c         | 10 ----------
>   2 files changed, 13 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4fb2033bc54..319ec82f709 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -289,9 +289,6 @@ extern const size_t pc_compat_2_9_len;
>   extern GlobalProperty pc_compat_2_8[];
>   extern const size_t pc_compat_2_8_len;
>   
> -extern GlobalProperty pc_compat_2_7[];
> -extern const size_t pc_compat_2_7_len;
> -
>   #define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
>       static void pc_machine_##suffix##_class_init(ObjectClass *oc, \
>                                                    const void *data) \
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7573b880905..ee7095c89a8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -241,16 +241,6 @@ GlobalProperty pc_compat_2_8[] = {
>   };
>   const size_t pc_compat_2_8_len = G_N_ELEMENTS(pc_compat_2_8);
>   
> -GlobalProperty pc_compat_2_7[] = {
> -    { TYPE_X86_CPU, "l3-cache", "off" },
> -    { TYPE_X86_CPU, "full-cpuid-auto-level", "off" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "family", "15" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "model", "6" },
> -    { "Opteron_G3" "-" TYPE_X86_CPU, "stepping", "1" },
> -    { "isa-pcspk", "migrate", "off" },
> -};
> -const size_t pc_compat_2_7_len = G_N_ELEMENTS(pc_compat_2_7);

Missing clean-up patches for l3-cache and full-cpuid-auto-level?

  Thomas

[PATCH v2 10/16] hw/audio/pcspk: Remove PCSpkState::migrate field

[Philippe Mathieu-Daudé]

The PCSpkState::migrate boolean was only set in the
pc_compat_2_7[] array, via the 'migrate=off' property.
We removed all machines using that array, lets remove
that property, simplifying vmstate_spk[].

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/audio/pcspk.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index a419161b5b1..0e83ba0bf73 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -56,7 +56,6 @@ struct PCSpkState {
     unsigned int play_pos;
     uint8_t data_on;
     uint8_t dummy_refresh_clock;
-    bool migrate;
 };
 
 static const char *s_spk = "pcspk";
@@ -196,18 +195,10 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp)
     pcspk_state = s;
 }
 
-static bool migrate_needed(void *opaque)
-{
-    PCSpkState *s = opaque;
-
-    return s->migrate;
-}
-
 static const VMStateDescription vmstate_spk = {
     .name = "pcspk",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = migrate_needed,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT8(data_on, PCSpkState),
         VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
@@ -218,7 +209,6 @@ static const VMStateDescription vmstate_spk = {
 static const Property pcspk_properties[] = {
     DEFINE_AUDIO_PROPERTIES(PCSpkState, card),
     DEFINE_PROP_UINT32("iobase", PCSpkState, iobase,  0x61),
-    DEFINE_PROP_BOOL("migrate", PCSpkState, migrate,  true),
 };
 
 static void pcspk_class_initfn(ObjectClass *klass, const void *data)
-- 
2.47.1

Re: [PATCH v2 10/16] hw/audio/pcspk: Remove PCSpkState::migrate field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The PCSpkState::migrate boolean was only set in the
> pc_compat_2_7[] array, via the 'migrate=off' property.
> We removed all machines using that array, lets remove
> that property, simplifying vmstate_spk[].
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/audio/pcspk.c | 10 ----------
>   1 file changed, 10 deletions(-)
> 
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index a419161b5b1..0e83ba0bf73 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -56,7 +56,6 @@ struct PCSpkState {
>       unsigned int play_pos;
>       uint8_t data_on;
>       uint8_t dummy_refresh_clock;
> -    bool migrate;
>   };
>   
>   static const char *s_spk = "pcspk";
> @@ -196,18 +195,10 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp)
>       pcspk_state = s;
>   }
>   
> -static bool migrate_needed(void *opaque)
> -{
> -    PCSpkState *s = opaque;
> -
> -    return s->migrate;
> -}
> -
>   static const VMStateDescription vmstate_spk = {
>       .name = "pcspk",
>       .version_id = 1,
>       .minimum_version_id = 1,
> -    .needed = migrate_needed,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT8(data_on, PCSpkState),
>           VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
> @@ -218,7 +209,6 @@ static const VMStateDescription vmstate_spk = {
>   static const Property pcspk_properties[] = {
>       DEFINE_AUDIO_PROPERTIES(PCSpkState, card),
>       DEFINE_PROP_UINT32("iobase", PCSpkState, iobase,  0x61),
> -    DEFINE_PROP_BOOL("migrate", PCSpkState, migrate,  true),
>   };
>   
>   static void pcspk_class_initfn(ObjectClass *klass, const void *data)

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


ATB,

Mark.

Re: [PATCH v2 10/16] hw/audio/pcspk: Remove PCSpkState::migrate field

[Thomas Huth]

On 01/05/2025 20.36, Philippe Mathieu-Daudé wrote:
> The PCSpkState::migrate boolean was only set in the
> pc_compat_2_7[] array, via the 'migrate=off' property.
> We removed all machines using that array, lets remove
> that property, simplifying vmstate_spk[].
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/audio/pcspk.c | 10 ----------
>   1 file changed, 10 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 11/16] hw/core/machine: Remove hw_compat_2_7[] array

[Philippe Mathieu-Daudé]

The hw_compat_2_7[] array was only used by the pc-q35-2.7 and
pc-i440fx-2.7 machines, which got removed. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/boards.h | 3 ---
 hw/core/machine.c   | 9 ---------
 2 files changed, 12 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index a881db8e7d6..77707c4376a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -838,7 +838,4 @@ extern const size_t hw_compat_2_9_len;
 extern GlobalProperty hw_compat_2_8[];
 extern const size_t hw_compat_2_8_len;
 
-extern GlobalProperty hw_compat_2_7[];
-extern const size_t hw_compat_2_7_len;
-
 #endif
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ce98820f277..bde19a2ff67 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -266,15 +266,6 @@ GlobalProperty hw_compat_2_8[] = {
 };
 const size_t hw_compat_2_8_len = G_N_ELEMENTS(hw_compat_2_8);
 
-GlobalProperty hw_compat_2_7[] = {
-    { "virtio-pci", "page-per-vq", "on" },
-    { "virtio-serial-device", "emergency-write", "off" },
-    { "ioapic", "version", "0x11" },
-    { "intel-iommu", "x-buggy-eim", "true" },
-    { "virtio-pci", "x-ignore-backend-features", "on" },
-};
-const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
-
 MachineState *current_machine;
 
 static char *machine_get_kernel(Object *obj, Error **errp)
-- 
2.47.1

Re: [PATCH v2 11/16] hw/core/machine: Remove hw_compat_2_7[] array

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The hw_compat_2_7[] array was only used by the pc-q35-2.7 and
> pc-i440fx-2.7 machines, which got removed. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/boards.h | 3 ---
>   hw/core/machine.c   | 9 ---------
>   2 files changed, 12 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a881db8e7d6..77707c4376a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -838,7 +838,4 @@ extern const size_t hw_compat_2_9_len;
>   extern GlobalProperty hw_compat_2_8[];
>   extern const size_t hw_compat_2_8_len;
>   
> -extern GlobalProperty hw_compat_2_7[];
> -extern const size_t hw_compat_2_7_len;
> -
>   #endif
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ce98820f277..bde19a2ff67 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -266,15 +266,6 @@ GlobalProperty hw_compat_2_8[] = {
>   };
>   const size_t hw_compat_2_8_len = G_N_ELEMENTS(hw_compat_2_8);
>   
> -GlobalProperty hw_compat_2_7[] = {
> -    { "virtio-pci", "page-per-vq", "on" },
> -    { "virtio-serial-device", "emergency-write", "off" },
> -    { "ioapic", "version", "0x11" },
> -    { "intel-iommu", "x-buggy-eim", "true" },
> -    { "virtio-pci", "x-ignore-backend-features", "on" },
> -};
> -const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
> -
>   MachineState *current_machine;
>   
>   static char *machine_get_kernel(Object *obj, Error **errp)

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


ATB,

Mark.

[PATCH v2 12/16] hw/i386/intel_iommu: Remove IntelIOMMUState::buggy_eim field

[Philippe Mathieu-Daudé]

The IntelIOMMUState::buggy_eim boolean was only set in
the hw_compat_2_7[] array, via the 'x-buggy-eim=true'
property. We removed all machines using that array, lets
remove that property, simplifying vtd_decide_config().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/intel_iommu.h | 1 -
 hw/i386/intel_iommu.c         | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index e95477e8554..29304329d05 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -303,7 +303,6 @@ struct IntelIOMMUState {
     uint32_t intr_size;             /* Number of IR table entries */
     bool intr_eime;                 /* Extended interrupt mode enabled */
     OnOffAuto intr_eim;             /* Toggle for EIM cabability */
-    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
     bool dma_drain;                 /* Whether DMA r/w draining enabled */
     bool dma_translation;           /* Whether DMA translation supported */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5f8ed1243d1..c980cecb4ee 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3823,7 +3823,6 @@ static const Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
     DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
                             ON_OFF_AUTO_AUTO),
-    DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
     DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
                       VTD_HOST_ADDRESS_WIDTH),
     DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
@@ -4731,11 +4730,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
     }
 
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
-        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
+        s->intr_eim = kvm_irqchip_in_kernel()
                       && x86_iommu_ir_supported(x86_iommu) ?
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
-    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
+    if (s->intr_eim == ON_OFF_AUTO_ON) {
         if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
             error_setg(errp, "eim=on requires support on the KVM side"
                              "(X2APIC_API, first shipped in v4.7)");
-- 
2.47.1

Re: [PATCH v2 12/16] hw/i386/intel_iommu: Remove IntelIOMMUState::buggy_eim field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The IntelIOMMUState::buggy_eim boolean was only set in
> the hw_compat_2_7[] array, via the 'x-buggy-eim=true'
> property. We removed all machines using that array, lets
> remove that property, simplifying vtd_decide_config().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/intel_iommu.h | 1 -
>   hw/i386/intel_iommu.c         | 5 ++---
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index e95477e8554..29304329d05 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -303,7 +303,6 @@ struct IntelIOMMUState {
>       uint32_t intr_size;             /* Number of IR table entries */
>       bool intr_eime;                 /* Extended interrupt mode enabled */
>       OnOffAuto intr_eim;             /* Toggle for EIM cabability */
> -    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
>       uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
>       bool dma_drain;                 /* Whether DMA r/w draining enabled */
>       bool dma_translation;           /* Whether DMA translation supported */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 5f8ed1243d1..c980cecb4ee 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3823,7 +3823,6 @@ static const Property vtd_properties[] = {
>       DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>       DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
>                               ON_OFF_AUTO_AUTO),
> -    DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
>       DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
>                         VTD_HOST_ADDRESS_WIDTH),
>       DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> @@ -4731,11 +4730,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>       }
>   
>       if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
> +        s->intr_eim = kvm_irqchip_in_kernel()
>                         && x86_iommu_ir_supported(x86_iommu) ?
>                                                 ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>       }
> -    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
>           if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
>               error_setg(errp, "eim=on requires support on the KVM side"
>                                "(X2APIC_API, first shipped in v4.7)");

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


ATB,

Mark.

[PATCH v2 13/16] hw/intc/ioapic: Remove IOAPICCommonState::version field

[Philippe Mathieu-Daudé]

The IOAPICCommonState::version integer was only set
in the hw_compat_2_7[] array, via the 'version=0x11'
property. We removed all machines using that array,
lets remove that property, simplify by only using the
default version (defined as IOAPIC_VER_DEF).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/ioapic_internal.h |  3 +--
 hw/intc/ioapic.c          | 18 ++----------------
 hw/intc/ioapic_common.c   |  2 +-
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h
index 51205767f44..330ce195222 100644
--- a/hw/intc/ioapic_internal.h
+++ b/hw/intc/ioapic_internal.h
@@ -82,7 +82,7 @@
 #define IOAPIC_ID_MASK                  0xf
 
 #define IOAPIC_VER_ENTRIES_SHIFT        16
-
+#define IOAPIC_VER_DEF                  0x20
 
 #define TYPE_IOAPIC_COMMON "ioapic-common"
 OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, IOAPIC_COMMON)
@@ -104,7 +104,6 @@ struct IOAPICCommonState {
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
     Notifier machine_done;
-    uint8_t version;
     uint64_t irq_count[IOAPIC_NUM_PINS];
     int irq_level[IOAPIC_NUM_PINS];
     int irq_eoi[IOAPIC_NUM_PINS];
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 133bef852d1..5cc97767d9d 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
             val = s->id << IOAPIC_ID_SHIFT;
             break;
         case IOAPIC_REG_VER:
-            val = s->version |
+            val = IOAPIC_VER_DEF |
                 ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
             break;
         default:
@@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         }
         break;
     case IOAPIC_EOI:
-        /* Explicit EOI is only supported for IOAPIC version 0x20 */
-        if (size != 4 || s->version != 0x20) {
+        if (size != 4) {
             break;
         }
         ioapic_eoi_broadcast(val);
@@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier *notifier, void *data)
 #endif
 }
 
-#define IOAPIC_VER_DEF 0x20
-
 static void ioapic_realize(DeviceState *dev, Error **errp)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
 
-    if (s->version != 0x11 && s->version != 0x20) {
-        error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 "
-                   "(default: 0x%x).", IOAPIC_VER_DEF);
-        return;
-    }
-
     memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
                           "ioapic", 0x1000);
 
@@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev)
     timer_free(s->delayed_ioapic_service_timer);
 }
 
-static const Property ioapic_properties[] = {
-    DEFINE_PROP_UINT8("version", IOAPICCommonState, version, IOAPIC_VER_DEF),
-};
-
 static void ioapic_class_init(ObjectClass *klass, const void *data)
 {
     IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
@@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, const void *data)
      */
     k->post_load = ioapic_update_kvm_routes;
     device_class_set_legacy_reset(dc, ioapic_reset_common);
-    device_class_set_props(dc, ioapic_properties);
 }
 
 static const TypeInfo ioapic_info = {
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index fce3486e519..8b3e2ba9384 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, IOAPICCommonState *s)
     int i;
 
     g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x sel=0x%02x",
-                           s->version, s->id, s->ioregsel);
+                           IOAPIC_VER_DEF, s->id, s->ioregsel);
     if (s->ioregsel) {
         g_string_append_printf(buf, " (redir[%u])\n",
                                (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1);
-- 
2.47.1

Re: [PATCH v2 13/16] hw/intc/ioapic: Remove IOAPICCommonState::version field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The IOAPICCommonState::version integer was only set
> in the hw_compat_2_7[] array, via the 'version=0x11'
> property. We removed all machines using that array,
> lets remove that property, simplify by only using the
> default version (defined as IOAPIC_VER_DEF).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/intc/ioapic_internal.h |  3 +--
>   hw/intc/ioapic.c          | 18 ++----------------
>   hw/intc/ioapic_common.c   |  2 +-
>   3 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h
> index 51205767f44..330ce195222 100644
> --- a/hw/intc/ioapic_internal.h
> +++ b/hw/intc/ioapic_internal.h
> @@ -82,7 +82,7 @@
>   #define IOAPIC_ID_MASK                  0xf
>   
>   #define IOAPIC_VER_ENTRIES_SHIFT        16
> -
> +#define IOAPIC_VER_DEF                  0x20
>   
>   #define TYPE_IOAPIC_COMMON "ioapic-common"
>   OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, IOAPIC_COMMON)
> @@ -104,7 +104,6 @@ struct IOAPICCommonState {
>       uint32_t irr;
>       uint64_t ioredtbl[IOAPIC_NUM_PINS];
>       Notifier machine_done;
> -    uint8_t version;
>       uint64_t irq_count[IOAPIC_NUM_PINS];
>       int irq_level[IOAPIC_NUM_PINS];
>       int irq_eoi[IOAPIC_NUM_PINS];
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 133bef852d1..5cc97767d9d 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
>               val = s->id << IOAPIC_ID_SHIFT;
>               break;
>           case IOAPIC_REG_VER:
> -            val = s->version |
> +            val = IOAPIC_VER_DEF |
>                   ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
>               break;
>           default:
> @@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>           }
>           break;
>       case IOAPIC_EOI:
> -        /* Explicit EOI is only supported for IOAPIC version 0x20 */
> -        if (size != 4 || s->version != 0x20) {
> +        if (size != 4) {
>               break;
>           }
>           ioapic_eoi_broadcast(val);
> @@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier *notifier, void *data)
>   #endif
>   }
>   
> -#define IOAPIC_VER_DEF 0x20
> -
>   static void ioapic_realize(DeviceState *dev, Error **errp)
>   {
>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>   
> -    if (s->version != 0x11 && s->version != 0x20) {
> -        error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 "
> -                   "(default: 0x%x).", IOAPIC_VER_DEF);
> -        return;
> -    }
> -
>       memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
>                             "ioapic", 0x1000);
>   
> @@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev)
>       timer_free(s->delayed_ioapic_service_timer);
>   }
>   
> -static const Property ioapic_properties[] = {
> -    DEFINE_PROP_UINT8("version", IOAPICCommonState, version, IOAPIC_VER_DEF),
> -};
> -
>   static void ioapic_class_init(ObjectClass *klass, const void *data)
>   {
>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
> @@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, const void *data)
>        */
>       k->post_load = ioapic_update_kvm_routes;
>       device_class_set_legacy_reset(dc, ioapic_reset_common);
> -    device_class_set_props(dc, ioapic_properties);
>   }
>   
>   static const TypeInfo ioapic_info = {
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index fce3486e519..8b3e2ba9384 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, IOAPICCommonState *s)
>       int i;
>   
>       g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x sel=0x%02x",
> -                           s->version, s->id, s->ioregsel);
> +                           IOAPIC_VER_DEF, s->id, s->ioregsel);
>       if (s->ioregsel) {
>           g_string_append_printf(buf, " (redir[%u])\n",
>                                  (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1);

Mildly curious that other than the reported version the version field 
doesn't appear to control anything else - was the original bug that QEMU 
implemented a 0x20 IOAPIC but incorrectly reported it as a 0x11 IOAPIC?

Anyhow the diff itself looks good so:

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


ATB,

Mark.

Re: [PATCH v2 13/16] hw/intc/ioapic: Remove IOAPICCommonState::version field

[Philippe Mathieu-Daudé]

On 2/5/25 11:31, Mark Cave-Ayland wrote:
> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
> 
>> The IOAPICCommonState::version integer was only set
>> in the hw_compat_2_7[] array, via the 'version=0x11'
>> property. We removed all machines using that array,
>> lets remove that property, simplify by only using the
>> default version (defined as IOAPIC_VER_DEF).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/intc/ioapic_internal.h |  3 +--
>>   hw/intc/ioapic.c          | 18 ++----------------
>>   hw/intc/ioapic_common.c   |  2 +-
>>   3 files changed, 4 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h
>> index 51205767f44..330ce195222 100644
>> --- a/hw/intc/ioapic_internal.h
>> +++ b/hw/intc/ioapic_internal.h
>> @@ -82,7 +82,7 @@
>>   #define IOAPIC_ID_MASK                  0xf
>>   #define IOAPIC_VER_ENTRIES_SHIFT        16
>> -
>> +#define IOAPIC_VER_DEF                  0x20
>>   #define TYPE_IOAPIC_COMMON "ioapic-common"
>>   OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, 
>> IOAPIC_COMMON)
>> @@ -104,7 +104,6 @@ struct IOAPICCommonState {
>>       uint32_t irr;
>>       uint64_t ioredtbl[IOAPIC_NUM_PINS];
>>       Notifier machine_done;
>> -    uint8_t version;
>>       uint64_t irq_count[IOAPIC_NUM_PINS];
>>       int irq_level[IOAPIC_NUM_PINS];
>>       int irq_eoi[IOAPIC_NUM_PINS];
>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>> index 133bef852d1..5cc97767d9d 100644
>> --- a/hw/intc/ioapic.c
>> +++ b/hw/intc/ioapic.c
>> @@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, 
>> unsigned int size)
>>               val = s->id << IOAPIC_ID_SHIFT;
>>               break;
>>           case IOAPIC_REG_VER:
>> -            val = s->version |
>> +            val = IOAPIC_VER_DEF |
>>                   ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
>>               break;
>>           default:
>> @@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, 
>> uint64_t val,
>>           }
>>           break;
>>       case IOAPIC_EOI:
>> -        /* Explicit EOI is only supported for IOAPIC version 0x20 */
>> -        if (size != 4 || s->version != 0x20) {
>> +        if (size != 4) {
>>               break;
>>           }
>>           ioapic_eoi_broadcast(val);
>> @@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier 
>> *notifier, void *data)
>>   #endif
>>   }
>> -#define IOAPIC_VER_DEF 0x20
>> -
>>   static void ioapic_realize(DeviceState *dev, Error **errp)
>>   {
>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>> -    if (s->version != 0x11 && s->version != 0x20) {
>> -        error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 "
>> -                   "(default: 0x%x).", IOAPIC_VER_DEF);
>> -        return;
>> -    }
>> -
>>       memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
>>                             "ioapic", 0x1000);
>> @@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev)
>>       timer_free(s->delayed_ioapic_service_timer);
>>   }
>> -static const Property ioapic_properties[] = {
>> -    DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 
>> IOAPIC_VER_DEF),
>> -};
>> -
>>   static void ioapic_class_init(ObjectClass *klass, const void *data)
>>   {
>>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>> @@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, 
>> const void *data)
>>        */
>>       k->post_load = ioapic_update_kvm_routes;
>>       device_class_set_legacy_reset(dc, ioapic_reset_common);
>> -    device_class_set_props(dc, ioapic_properties);
>>   }
>>   static const TypeInfo ioapic_info = {
>> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
>> index fce3486e519..8b3e2ba9384 100644
>> --- a/hw/intc/ioapic_common.c
>> +++ b/hw/intc/ioapic_common.c
>> @@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, 
>> IOAPICCommonState *s)
>>       int i;
>>       g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x 
>> sel=0x%02x",
>> -                           s->version, s->id, s->ioregsel);
>> +                           IOAPIC_VER_DEF, s->id, s->ioregsel);
>>       if (s->ioregsel) {
>>           g_string_append_printf(buf, " (redir[%u])\n",
>>                                  (s->ioregsel - 
>> IOAPIC_REG_REDTBL_BASE) >> 1);
> 
> Mildly curious that other than the reported version the version field 
> doesn't appear to control anything else - was the original bug that QEMU 
> implemented a 0x20 IOAPIC but incorrectly reported it as a 0x11 IOAPIC?

I'll mention:

commit 20fd4b7b6d9282fe0cb83601f1821f31bd257458
Author: Peter Xu <peterx@redhat.com>
Date:   Mon Aug 1 21:59:19 2016 +0800

     x86: ioapic: add support for explicit EOI

     Some old Linux kernels (upstream before v4.0), or any released RHEL
     kernels has problem in sending APIC EOI when IR is enabled.
     Meanwhile, many of them only support explicit EOI for IOAPIC, which
     is only introduced in IOAPIC version 0x20. This patch provide a way
     to boost QEMU IOAPIC to version 0x20, in order for QEMU to correctly
     receive EOI messages.

     Without boosting IOAPIC version to 0x20, kernels before commit
     d32932d ("x86/irq: Convert IOAPIC to use hierarchical irqdomain
     interfaces") will have trouble enabling both IR and level-triggered
     interrupt devices (like e1000).

     To upgrade IOAPIC to version 0x20, we need to specify:

       -global ioapic.version=0x20

     To be compatible with old systems, 0x11 will still be the default
     IOAPIC version. Here 0x11 and 0x20 are the only versions to be
     supported.

     One thing to mention: this patch only applies to emulated IOAPIC. It
     does not affect kernel IOAPIC behavior.

(see also:)

commit 048a2e8869cb7e26013e40d860c9ebdf8e28c2ac
Author: Peter Xu <peterx@redhat.com>
Date:   Fri Sep 23 13:33:15 2016 +0800

     x86: ioapic: boost default version to 0x20

     It's 2.8 now, and maybe it's time to switch IOAPIC default version to
     0x20.

     Signed-off-by: Peter Xu <peterx@redhat.com>
     Message-Id: <1474608795-23058-1-git-send-email-peterx@redhat.com>
     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/include/hw/compat.h b/include/hw/compat.h
index a1d66944924..46412b229a7 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -8,0 +9,4 @@
+    },{\
+        .driver   = "ioapic",\
+        .property = "version",\
+        .value    = "0x11",\
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 31791b09860..fd9208fde08 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -419 +419 @@ static Property ioapic_properties[] = {
-    DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 0x11),
+    DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 0x20),

> 
> Anyhow the diff itself looks good so:
> 
> Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>

Thanks!

[PATCH v2 14/16] hw/virtio/virtio-pci: Remove VirtIOPCIProxy::ignore_backend_features field

[Philippe Mathieu-Daudé]

The VirtIOPCIProxy::ignore_backend_features boolean was only set
in the hw_compat_2_7[] array, via the 'x-ignore-backend-features=on'
property. We removed all machines using that array, lets remove
that property, simplify by only using the default version.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/virtio/virtio-pci.h | 1 -
 hw/virtio/virtio-pci.c         | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index f962c9116c1..9838e8650a6 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -149,7 +149,6 @@ struct VirtIOPCIProxy {
     int config_cap;
     uint32_t flags;
     bool disable_modern;
-    bool ignore_backend_features;
     OnOffAuto disable_legacy;
     /* Transitional device id */
     uint16_t trans_devid;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 8d68e56641a..7c965771907 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1965,8 +1965,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
      * Virtio capabilities present without
      * VIRTIO_F_VERSION_1 confuses guests
      */
-    if (!proxy->ignore_backend_features &&
-            !virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
+    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
         virtio_pci_disable_modern(proxy);
 
         if (!legacy) {
@@ -2351,8 +2350,6 @@ static const Property virtio_pci_properties[] = {
                     VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, false),
     DEFINE_PROP_BIT("page-per-vq", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT, false),
-    DEFINE_PROP_BOOL("x-ignore-backend-features", VirtIOPCIProxy,
-                     ignore_backend_features, false),
     DEFINE_PROP_BIT("ats", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_ATS_BIT, false),
     DEFINE_PROP_BIT("x-ats-page-aligned", VirtIOPCIProxy, flags,
-- 
2.47.1

Re: [PATCH v2 14/16] hw/virtio/virtio-pci: Remove VirtIOPCIProxy::ignore_backend_features field

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The VirtIOPCIProxy::ignore_backend_features boolean was only set
> in the hw_compat_2_7[] array, via the 'x-ignore-backend-features=on'
> property. We removed all machines using that array, lets remove
> that property, simplify by only using the default version.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/virtio/virtio-pci.h | 1 -
>   hw/virtio/virtio-pci.c         | 5 +----
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index f962c9116c1..9838e8650a6 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -149,7 +149,6 @@ struct VirtIOPCIProxy {
>       int config_cap;
>       uint32_t flags;
>       bool disable_modern;
> -    bool ignore_backend_features;
>       OnOffAuto disable_legacy;
>       /* Transitional device id */
>       uint16_t trans_devid;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8d68e56641a..7c965771907 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1965,8 +1965,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>        * Virtio capabilities present without
>        * VIRTIO_F_VERSION_1 confuses guests
>        */
> -    if (!proxy->ignore_backend_features &&
> -            !virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
> +    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
>           virtio_pci_disable_modern(proxy);
>   
>           if (!legacy) {
> @@ -2351,8 +2350,6 @@ static const Property virtio_pci_properties[] = {
>                       VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, false),
>       DEFINE_PROP_BIT("page-per-vq", VirtIOPCIProxy, flags,
>                       VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT, false),
> -    DEFINE_PROP_BOOL("x-ignore-backend-features", VirtIOPCIProxy,
> -                     ignore_backend_features, false),
>       DEFINE_PROP_BIT("ats", VirtIOPCIProxy, flags,
>                       VIRTIO_PCI_FLAG_ATS_BIT, false),
>       DEFINE_PROP_BIT("x-ats-page-aligned", VirtIOPCIProxy, flags,

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


ATB,

Mark.

[PATCH v2 15/16] hw/char/virtio-serial: Do not expose the 'emergency-write' property

[Philippe Mathieu-Daudé]

The VIRTIO_CONSOLE_F_EMERG_WRITE feature bit was only set
in the hw_compat_2_7[] array, via the 'emergency-write=off'
property. We removed all machines using that array, lets remove
that property. All instances have this feature bit set and
it can not be disabled.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/virtio-serial-bus.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index eb79f5258b6..31bd1b7535d 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1019,6 +1019,13 @@ static void virtser_port_device_unrealize(DeviceState *dev)
     }
 }
 
+static void virtio_serial_instance_init(Object *obj)
+{
+    VirtIOSerial *vser = VIRTIO_SERIAL(obj);
+
+    vser->host_features |= BIT_ULL(VIRTIO_CONSOLE_F_EMERG_WRITE);
+}
+
 static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1155,8 +1162,6 @@ static const VMStateDescription vmstate_virtio_console = {
 static const Property virtio_serial_properties[] = {
     DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
                                                   31),
-    DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
-                      VIRTIO_CONSOLE_F_EMERG_WRITE, true),
 };
 
 static void virtio_serial_class_init(ObjectClass *klass, const void *data)
@@ -1186,6 +1191,7 @@ static void virtio_serial_class_init(ObjectClass *klass, const void *data)
 static const TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_SERIAL,
     .parent = TYPE_VIRTIO_DEVICE,
+    .instance_init = virtio_serial_instance_init,
     .instance_size = sizeof(VirtIOSerial),
     .class_init = virtio_serial_class_init,
     .interfaces = (const InterfaceInfo[]) {
-- 
2.47.1

Re: [PATCH v2 15/16] hw/char/virtio-serial: Do not expose the 'emergency-write' property

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> The VIRTIO_CONSOLE_F_EMERG_WRITE feature bit was only set
> in the hw_compat_2_7[] array, via the 'emergency-write=off'
> property. We removed all machines using that array, lets remove
> that property. All instances have this feature bit set and
> it can not be disabled.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/char/virtio-serial-bus.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index eb79f5258b6..31bd1b7535d 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1019,6 +1019,13 @@ static void virtser_port_device_unrealize(DeviceState *dev)
>       }
>   }
>   
> +static void virtio_serial_instance_init(Object *obj)
> +{
> +    VirtIOSerial *vser = VIRTIO_SERIAL(obj);
> +
> +    vser->host_features |= BIT_ULL(VIRTIO_CONSOLE_F_EMERG_WRITE);
> +}
> +
>   static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
>   {
>       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -1155,8 +1162,6 @@ static const VMStateDescription vmstate_virtio_console = {
>   static const Property virtio_serial_properties[] = {
>       DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
>                                                     31),
> -    DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
> -                      VIRTIO_CONSOLE_F_EMERG_WRITE, true),
>   };
>   
>   static void virtio_serial_class_init(ObjectClass *klass, const void *data)
> @@ -1186,6 +1191,7 @@ static void virtio_serial_class_init(ObjectClass *klass, const void *data)
>   static const TypeInfo virtio_device_info = {
>       .name = TYPE_VIRTIO_SERIAL,
>       .parent = TYPE_VIRTIO_DEVICE,
> +    .instance_init = virtio_serial_instance_init,
>       .instance_size = sizeof(VirtIOSerial),
>       .class_init = virtio_serial_class_init,
>       .interfaces = (const InterfaceInfo[]) {

You could also consider removing vser->host_features completely, since 
it is always ORd into the host_features list in get_features(), plus all 
remaining mentions are of the form:

if (virtio_has_feature(vser->host_features,
         VIRTIO_CONSOLE_F_EMERG_WRITE)) { ... }


ATB,

Mark.

Re: [PATCH v2 15/16] hw/char/virtio-serial: Do not expose the 'emergency-write' property

[Philippe Mathieu-Daudé]

On 2/5/25 11:41, Mark Cave-Ayland wrote:
> On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:
> 
>> The VIRTIO_CONSOLE_F_EMERG_WRITE feature bit was only set
>> in the hw_compat_2_7[] array, via the 'emergency-write=off'
>> property. We removed all machines using that array, lets remove
>> that property. All instances have this feature bit set and
>> it can not be disabled.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/char/virtio-serial-bus.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index eb79f5258b6..31bd1b7535d 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -1019,6 +1019,13 @@ static void 
>> virtser_port_device_unrealize(DeviceState *dev)
>>       }
>>   }
>> +static void virtio_serial_instance_init(Object *obj)
>> +{
>> +    VirtIOSerial *vser = VIRTIO_SERIAL(obj);
>> +
>> +    vser->host_features |= BIT_ULL(VIRTIO_CONSOLE_F_EMERG_WRITE);
>> +}
>> +
>>   static void virtio_serial_device_realize(DeviceState *dev, Error 
>> **errp)
>>   {
>>       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> @@ -1155,8 +1162,6 @@ static const VMStateDescription 
>> vmstate_virtio_console = {
>>   static const Property virtio_serial_properties[] = {
>>       DEFINE_PROP_UINT32("max_ports", VirtIOSerial, 
>> serial.max_virtserial_ports,
>>                                                     31),
>> -    DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
>> -                      VIRTIO_CONSOLE_F_EMERG_WRITE, true),
>>   };
>>   static void virtio_serial_class_init(ObjectClass *klass, const void 
>> *data)
>> @@ -1186,6 +1191,7 @@ static void virtio_serial_class_init(ObjectClass 
>> *klass, const void *data)
>>   static const TypeInfo virtio_device_info = {
>>       .name = TYPE_VIRTIO_SERIAL,
>>       .parent = TYPE_VIRTIO_DEVICE,
>> +    .instance_init = virtio_serial_instance_init,
>>       .instance_size = sizeof(VirtIOSerial),
>>       .class_init = virtio_serial_class_init,
>>       .interfaces = (const InterfaceInfo[]) {
> 
> You could also consider removing vser->host_features completely, since 
> it is always ORd into the host_features list in get_features(), plus all 
> remaining mentions are of the form:
> 
> if (virtio_has_feature(vser->host_features,
>          VIRTIO_CONSOLE_F_EMERG_WRITE)) { ... }

Good remark, thanks!

[PATCH v2 16/16] hw/virtio/virtio-pci: Remove VIRTIO_PCI_FLAG_PAGE_PER_VQ definition

[Philippe Mathieu-Daudé]

VIRTIO_PCI_FLAG_PAGE_PER_VQ was only used by the hw_compat_2_7[]
array, via the 'page-per-vq=on' property. We removed all
machines using that array, lets remove all the code around
VIRTIO_PCI_FLAG_PAGE_PER_VQ (see commit 9a4c0e220d8 for similar
VIRTIO_PCI_FLAG_* enum removal).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/virtio/virtio-pci.h |  1 -
 hw/display/virtio-vga.c        | 10 ----------
 hw/virtio/virtio-pci.c         |  7 +------
 3 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 9838e8650a6..8abc5f8f20d 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -33,7 +33,6 @@ enum {
     VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT,
     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT,
     VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT,
-    VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT,
     VIRTIO_PCI_FLAG_ATS_BIT,
     VIRTIO_PCI_FLAG_INIT_DEVERR_BIT,
     VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT,
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 40e60f70fcd..83d01f089b5 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -141,16 +141,6 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
                                VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
     }
 
-    if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
-        /*
-         * with page-per-vq=off there is no padding space we can use
-         * for the stdvga registers.  Make the common and isr regions
-         * smaller then.
-         */
-        vpci_dev->common.size /= 2;
-        vpci_dev->isr.size /= 2;
-    }
-
     offset = memory_region_size(&vpci_dev->modern_bar);
     offset -= vpci_dev->notify.size;
     vpci_dev->notify.offset = offset;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7c965771907..4e0d4bda6ed 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -314,12 +314,9 @@ static bool virtio_pci_ioeventfd_enabled(DeviceState *d)
     return (proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD) != 0;
 }
 
-#define QEMU_VIRTIO_PCI_QUEUE_MEM_MULT 0x1000
-
 static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy *proxy)
 {
-    return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ?
-        QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4;
+    return 4;
 }
 
 static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
@@ -2348,8 +2345,6 @@ static const Property virtio_pci_properties[] = {
                     VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
     DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, false),
-    DEFINE_PROP_BIT("page-per-vq", VirtIOPCIProxy, flags,
-                    VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT, false),
     DEFINE_PROP_BIT("ats", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_ATS_BIT, false),
     DEFINE_PROP_BIT("x-ats-page-aligned", VirtIOPCIProxy, flags,
-- 
2.47.1

Re: [PATCH v2 16/16] hw/virtio/virtio-pci: Remove VIRTIO_PCI_FLAG_PAGE_PER_VQ definition

[Mark Cave-Ayland]

On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote:

> VIRTIO_PCI_FLAG_PAGE_PER_VQ was only used by the hw_compat_2_7[]
> array, via the 'page-per-vq=on' property. We removed all
> machines using that array, lets remove all the code around
> VIRTIO_PCI_FLAG_PAGE_PER_VQ (see commit 9a4c0e220d8 for similar
> VIRTIO_PCI_FLAG_* enum removal).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/virtio/virtio-pci.h |  1 -
>   hw/display/virtio-vga.c        | 10 ----------
>   hw/virtio/virtio-pci.c         |  7 +------
>   3 files changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 9838e8650a6..8abc5f8f20d 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -33,7 +33,6 @@ enum {
>       VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT,
>       VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT,
>       VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT,
> -    VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT,
>       VIRTIO_PCI_FLAG_ATS_BIT,
>       VIRTIO_PCI_FLAG_INIT_DEVERR_BIT,
>       VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT,
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 40e60f70fcd..83d01f089b5 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -141,16 +141,6 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>                                  VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
>       }
>   
> -    if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
> -        /*
> -         * with page-per-vq=off there is no padding space we can use
> -         * for the stdvga registers.  Make the common and isr regions
> -         * smaller then.
> -         */
> -        vpci_dev->common.size /= 2;
> -        vpci_dev->isr.size /= 2;
> -    }
> -
>       offset = memory_region_size(&vpci_dev->modern_bar);
>       offset -= vpci_dev->notify.size;
>       vpci_dev->notify.offset = offset;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 7c965771907..4e0d4bda6ed 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -314,12 +314,9 @@ static bool virtio_pci_ioeventfd_enabled(DeviceState *d)
>       return (proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD) != 0;
>   }
>   
> -#define QEMU_VIRTIO_PCI_QUEUE_MEM_MULT 0x1000
> -
>   static inline int virtio_pci_queue_mem_mult(struct VirtIOPCIProxy *proxy)
>   {
> -    return (proxy->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ) ?
> -        QEMU_VIRTIO_PCI_QUEUE_MEM_MULT : 4;
> +    return 4;
>   }
>   
>   static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
> @@ -2348,8 +2345,6 @@ static const Property virtio_pci_properties[] = {
>                       VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
>       DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
>                       VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, false),
> -    DEFINE_PROP_BIT("page-per-vq", VirtIOPCIProxy, flags,
> -                    VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT, false),
>       DEFINE_PROP_BIT("ats", VirtIOPCIProxy, flags,
>                       VIRTIO_PCI_FLAG_ATS_BIT, false),
>       DEFINE_PROP_BIT("x-ats-page-aligned", VirtIOPCIProxy, flags,

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


ATB,

Mark.