[PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

[PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

[Philippe Mathieu-Daudé]

Bernhard warned for QOM class abuse here [*]:

> A realize method is supposed to modify a single instance only
> while we're modifying the behavior of whole classes here, i.e.
> will affect every instance of these classes. This goes against
> QOM design principles and will therefore be confusing for people
> who are familiar with QOM in particular and OOP in general.

This series fixes the cases I found while auditing.

[*] https://lore.kernel.org/qemu-devel/0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com/

Philippe Mathieu-Daudé (3):
  hw/mips/jazz: Fix modifying QOM class internal state from instance
  hw/ppc/e500plat: Fix modifying QOM class internal state from instance
  hw/i2c/pmbus_device: Fix modifying QOM class internals from instance

 hw/i2c/pmbus_device.c | 17 ++++++++++-------
 hw/mips/jazz.c        | 41 +++++++++++++++++++++++------------------
 hw/ppc/e500plat.c     | 25 +++++++++++--------------
 3 files changed, 44 insertions(+), 39 deletions(-)

-- 
2.38.1

[PATCH 1/3] hw/mips/jazz: Fix modifying QOM class internal state from instance

[Philippe Mathieu-Daudé]

QOM object instance should not modify its class state (because
all other objects instanciated from this class get affected).

Instead of modifying the MIPSCPUClass 'no_data_aborts' field
in the instance machine_init() handler, set it in the machine
class_init handler. Since 2 machines require this, share the
common code in a new machine_class_ignore_data_abort() helper.

Inspired-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/jazz.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index ca4426a92c..de2e827bf8 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -128,7 +128,6 @@ static void mips_jazz_init(MachineState *machine,
     int bios_size, n, big_endian;
     Clock *cpuclk;
     MIPSCPU *cpu;
-    MIPSCPUClass *mcc;
     CPUMIPSState *env;
     qemu_irq *i8259;
     rc4030_dma *dmas;
@@ -177,23 +176,6 @@ static void mips_jazz_init(MachineState *machine,
     env = &cpu->env;
     qemu_register_reset(main_cpu_reset, cpu);
 
-    /*
-     * Chipset returns 0 in invalid reads and do not raise data exceptions.
-     * However, we can't simply add a global memory region to catch
-     * everything, as this would make all accesses including instruction
-     * accesses be ignored and not raise exceptions.
-     *
-     * NOTE: this behaviour of raising exceptions for bad instruction
-     * fetches but not bad data accesses was added in commit 54e755588cf1e9
-     * to restore behaviour broken by c658b94f6e8c206, but it is not clear
-     * whether the real hardware behaves this way. It is possible that
-     * real hardware ignores bad instruction fetches as well -- if so then
-     * we could replace this hijacking of CPU methods with a simple global
-     * memory region that catches all memory accesses, as we do on Malta.
-     */
-    mcc = MIPS_CPU_GET_CLASS(cpu);
-    mcc->no_data_aborts = true;
-
     /* allocate RAM */
     memory_region_add_subregion(address_space, 0, machine->ram);
 
@@ -414,6 +396,27 @@ void mips_pica61_init(MachineState *machine)
     mips_jazz_init(machine, JAZZ_PICA61);
 }
 
+static void machine_class_ignore_data_abort(MachineClass *mc)
+{
+    MIPSCPUClass *mcc = MIPS_CPU_CLASS(mc);
+
+    /*
+     * Chipset returns 0 in invalid reads and do not raise data exceptions.
+     * However, we can't simply add a global memory region to catch
+     * everything, as this would make all accesses including instruction
+     * accesses be ignored and not raise exceptions.
+     *
+     * NOTE: this behaviour of raising exceptions for bad instruction
+     * fetches but not bad data accesses was added in commit 54e755588cf1e9
+     * to restore behaviour broken by c658b94f6e8c206, but it is not clear
+     * whether the real hardware behaves this way. It is possible that
+     * real hardware ignores bad instruction fetches as well -- if so then
+     * we could replace this hijacking of CPU methods with a simple global
+     * memory region that catches all memory accesses, as we do on Malta.
+     */
+    mcc->no_data_aborts = true;
+}
+
 static void mips_magnum_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -423,6 +426,7 @@ static void mips_magnum_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("R4000");
     mc->default_ram_id = "mips_jazz.ram";
+    machine_class_ignore_data_abort(mc);
 }
 
 static const TypeInfo mips_magnum_type = {
@@ -440,6 +444,7 @@ static void mips_pica61_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("R4000");
     mc->default_ram_id = "mips_jazz.ram";
+    machine_class_ignore_data_abort(mc);
 }
 
 static const TypeInfo mips_pica61_type = {
-- 
2.38.1

Re: [PATCH 1/3] hw/mips/jazz: Fix modifying QOM class internal state from instance

[Richard Henderson]

On 5/22/23 23:44, Philippe Mathieu-Daudé wrote:
> QOM object instance should not modify its class state (because
> all other objects instanciated from this class get affected).
> 
> Instead of modifying the MIPSCPUClass 'no_data_aborts' field
> in the instance machine_init() handler, set it in the machine
> class_init handler. Since 2 machines require this, share the
> common code in a new machine_class_ignore_data_abort() helper.
> 
> Inspired-by: Bernhard Beschow<shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/mips/jazz.c | 41 +++++++++++++++++++++++------------------
>   1 file changed, 23 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/3] hw/ppc/e500plat: Fix modifying QOM class internal state from instance

[Philippe Mathieu-Daudé]

QOM object instance should not modify its class state (because
all other objects instanciated from this class get affected).

Instead of modifying the PPCE500MachineClass 'mpic_version' field
in the instance machine_init() handler, set it in the machine
class init handler (e500plat_machine_class_init).

Inspired-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/e500plat.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 3032bd3f6d..c3b0ed01cf 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -30,18 +30,6 @@ static void e500plat_fixup_devtree(void *fdt)
                      sizeof(compatible));
 }
 
-static void e500plat_init(MachineState *machine)
-{
-    PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
-    /* Older KVM versions don't support EPR which breaks guests when we announce
-       MPIC variants that support EPR. Revert to an older one for those */
-    if (kvm_enabled() && !kvmppc_has_cap_epr()) {
-        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
-    }
-
-    ppce500_init(machine);
-}
-
 static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                             DeviceState *dev, Error **errp)
 {
@@ -81,7 +69,6 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     pmc->pci_first_slot = 0x1;
     pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
     pmc->fixup_devtree = e500plat_fixup_devtree;
-    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
     pmc->has_mpc8xxx_gpio = true;
     pmc->has_esdhc = true;
     pmc->platform_bus_base = 0xf00000000ULL;
@@ -94,8 +81,18 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     pmc->pci_mmio_bus_base = 0xE0000000ULL;
     pmc->spin_base = 0xFEF000000ULL;
 
+    if (kvm_enabled() && !kvmppc_has_cap_epr()) {
+        /*
+         * Older KVM versions don't support EPR which breaks guests when
+         * we announce MPIC variants that support EPR. Revert to an older
+         * one for those.
+         */
+        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
+    } else {
+        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
+    }
+
     mc->desc = "generic paravirt e500 platform";
-    mc->init = e500plat_init;
     mc->max_cpus = 32;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
     mc->default_ram_id = "mpc8544ds.ram";
-- 
2.38.1

Re: [PATCH 2/3] hw/ppc/e500plat: Fix modifying QOM class internal state from instance

[Alexander Graf]

Hi Philippe,

On 23.05.23 08:44, Philippe Mathieu-Daudé wrote:
> QOM object instance should not modify its class state (because
> all other objects instanciated from this class get affected).
>
> Instead of modifying the PPCE500MachineClass 'mpic_version' field
> in the instance machine_init() handler, set it in the machine
> class init handler (e500plat_machine_class_init).
>
> Inspired-by: Bernhard Beschow <shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/e500plat.c | 25 +++++++++++--------------
>   1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 3032bd3f6d..c3b0ed01cf 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -30,18 +30,6 @@ static void e500plat_fixup_devtree(void *fdt)
>                        sizeof(compatible));
>   }
>   
> -static void e500plat_init(MachineState *machine)
> -{
> -    PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
> -    /* Older KVM versions don't support EPR which breaks guests when we announce
> -       MPIC variants that support EPR. Revert to an older one for those */
> -    if (kvm_enabled() && !kvmppc_has_cap_epr()) {
> -        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
> -    }
> -
> -    ppce500_init(machine);


Won't this drop the call to ppce500_init(machine)?

> -}
> -
>   static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                               DeviceState *dev, Error **errp)
>   {
> @@ -81,7 +69,6 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>       pmc->pci_first_slot = 0x1;
>       pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
>       pmc->fixup_devtree = e500plat_fixup_devtree;
> -    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
>       pmc->has_mpc8xxx_gpio = true;
>       pmc->has_esdhc = true;
>       pmc->platform_bus_base = 0xf00000000ULL;
> @@ -94,8 +81,18 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>       pmc->pci_mmio_bus_base = 0xE0000000ULL;
>       pmc->spin_base = 0xFEF000000ULL;
>   
> +    if (kvm_enabled() && !kvmppc_has_cap_epr()) {
> +        /*
> +         * Older KVM versions don't support EPR which breaks guests when
> +         * we announce MPIC variants that support EPR. Revert to an older
> +         * one for those.
> +         */
> +        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
> +    } else {
> +        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
> +    }
> +
>       mc->desc = "generic paravirt e500 platform";
> -    mc->init = e500plat_init;


I suppose best would be to just put it in here instead of e500plat_init?


Alex


>       mc->max_cpus = 32;
>       mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>       mc->default_ram_id = "mpc8544ds.ram";

Re: [PATCH 2/3] hw/ppc/e500plat: Fix modifying QOM class internal state from instance

[Richard Henderson]

On 5/22/23 23:44, Philippe Mathieu-Daudé wrote:
> QOM object instance should not modify its class state (because
> all other objects instanciated from this class get affected).
> 
> Instead of modifying the PPCE500MachineClass 'mpic_version' field
> in the instance machine_init() handler, set it in the machine
> class init handler (e500plat_machine_class_init).
> 
> Inspired-by: Bernhard Beschow<shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/ppc/e500plat.c | 25 +++++++++++--------------
>   1 file changed, 11 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/3] hw/i2c/pmbus_device: Fix modifying QOM class internals from instance

[Philippe Mathieu-Daudé]

QOM object instance should not modify its class state (because
all other objects instanciated from this class get affected).

Instead of modifying the PMBusDeviceClass 'device_num_pages' field
the first time a instance is initialized (in pmbus_pages_alloc),
introduce a new pmbus_pages_num() helper which returns the page
number from the class without modifying the class state.

The code logic become slighly simplified.

Inspired-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i2c/pmbus_device.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 44fe4eddbb..8bc9d5108a 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -190,15 +190,18 @@ static void pmbus_quick_cmd(SMBusDevice *smd, uint8_t read)
     }
 }
 
+static uint8_t pmbus_pages_num(PMBusDevice *pmdev)
+{
+    const PMBusDeviceClass *k = PMBUS_DEVICE_GET_CLASS(pmdev);
+
+    /* some PMBus devices don't use the PAGE command, so they get 1 page */
+    return k->device_num_pages ? : 1;
+}
+
 static void pmbus_pages_alloc(PMBusDevice *pmdev)
 {
-    /* some PMBus devices don't use the PAGE command, so they get 1 page */
-    PMBusDeviceClass *k = PMBUS_DEVICE_GET_CLASS(pmdev);
-    if (k->device_num_pages == 0) {
-        k->device_num_pages = 1;
-    }
-    pmdev->num_pages = k->device_num_pages;
-    pmdev->pages = g_new0(PMBusPage, k->device_num_pages);
+    pmdev->num_pages = pmbus_pages_num(pmdev);
+    pmdev->pages = g_new0(PMBusPage, pmdev->num_pages);
 }
 
 void pmbus_check_limits(PMBusDevice *pmdev)
-- 
2.38.1

Re: [PATCH 3/3] hw/i2c/pmbus_device: Fix modifying QOM class internals from instance

[Richard Henderson]

On 5/22/23 23:44, Philippe Mathieu-Daudé wrote:
> QOM object instance should not modify its class state (because
> all other objects instanciated from this class get affected).
> 
> Instead of modifying the PMBusDeviceClass 'device_num_pages' field
> the first time a instance is initialized (in pmbus_pages_alloc),
> introduce a new pmbus_pages_num() helper which returns the page
> number from the class without modifying the class state.
> 
> The code logic become slighly simplified.
> 
> Inspired-by: Bernhard Beschow<shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i2c/pmbus_device.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/3] hw: Fix abuse of QOM class internals modified by their instances

[Philippe Mathieu-Daudé]

(posted too quickly)

On 23/5/23 08:44, Philippe Mathieu-Daudé wrote:
> Bernhard warned for QOM class abuse here [*]:
> 
>> A realize method is supposed to modify a single instance only
>> while we're modifying the behavior of whole classes here, i.e.
>> will affect every instance of these classes. This goes against
>> QOM design principles and will therefore be confusing for people
>> who are familiar with QOM in particular and OOP in general.
> 
> This series fixes the cases I found while auditing.

Audited but not yet fixed:

- accel/xen
   xen_init() sets 'default_ram_id'
   -> need to figure migration compatibility (because using shared
      pc_machine_class_init and DEFINE_PC_MACHINE), will be posted
      separately.

- hw/core/bus
   qbus_init_internal() increments 'automatic_ids'
   -> need more thought.

- hw/core/
   machine_parse_smp_config() sets 'smp_props.has_clusters'
   -> dubious

- hw/remote/
   remote_object_init() and probe_pci_info() set 'vendor_id', 'nr_devs'
   -> need more thought.

- qom/object/
   object_dynamic_cast_assert() populates object_cast_cache[]
   -> QOM internal, likely OK.

- softmmu/vl
   configure_blockdev() overwrites 'machine_class->block_default_type'
   -> dubious

- gdbstub
   ppc_gdb_gen_spr_xml() sets 'gdb_num_sprs' and 'gdb_spr_xml'
   -> dubious, likely fixable (maybe like patch #3 of this series)

- tests/unit/
   smp_parse_test() set 'smp_props.prefer_sockets'
   -> dubious, probably tolerable.

> [*] https://lore.kernel.org/qemu-devel/0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com/