Re: [Qemu-devel] [PATCH 13/17] qdev: use appropriate getter/setters type

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Based on underlying property type, use the appropriate getters/setters.

How did you find the ones that need changing?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/i386/acpi-build.c      | 12 ++++++------
>  hw/pci-host/gpex.c        |  2 +-
>  hw/pci-host/q35.c         |  2 +-
>  hw/pci-host/xilinx-pcie.c |  2 +-
>  target/i386/cpu.c         |  2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 767da5d78e..1707fae9bf 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -149,21 +149,21 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      /* Fill in optional s3/s4 related properties */
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>      if (o) {
> -        pm->s3_disabled = qnum_get_int(qobject_to_qnum(o), &error_abort);
> +        pm->s3_disabled = qnum_get_uint(qobject_to_qnum(o), &error_abort);
>      } else {
>          pm->s3_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
>      if (o) {
> -        pm->s4_disabled = qnum_get_int(qobject_to_qnum(o), &error_abort);
> +        pm->s4_disabled = qnum_get_uint(qobject_to_qnum(o), &error_abort);
>      } else {
>          pm->s4_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
>      if (o) {
> -        pm->s4_val = qnum_get_int(qobject_to_qnum(o), &error_abort);
> +        pm->s4_val = qnum_get_uint(qobject_to_qnum(o), &error_abort);
>      } else {
>          pm->s4_val = false;
>      }

The getter and setter of properties ACPI_PM_PROP_S3_DISABLED,
ACPI_PM_PROP_S4_DISABLED and ACPI_PM_PROP_S4_VAL use visit_type_uint8().

object_property_get_qobject() uses this getter below the hood with a
QObject output visitor, to get the value into a QObject.  Since the
getter uses visit_type_uint8(), the QObject is a QNum with QNUM_U64.

The appropriate way to extract the QNum's value is qnum_get_uint().

Okay.

> @@ -499,7 +499,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
> -        int64_t bsel_val = qnum_get_int(qobject_to_qnum(bsel), &error_abort);
> +        uint64_t bsel_val = qnum_get_uint(qobject_to_qnum(bsel), &error_abort);
>  
>          aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val)));
>          notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);

Similar argument, only slightly more tricky, as the getter dereferences
a pointer.

> @@ -609,7 +609,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>  
>      /* If bus supports hotplug select it and notify about local events */
>      if (bsel) {
> -        int64_t bsel_val = qnum_get_int(qobject_to_qnum(bsel), &error_abort);
> +        uint64_t bsel_val = qnum_get_uint(qobject_to_qnum(bsel), &error_abort);
>          aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
>          aml_append(method,
>              aml_call2("DVNT", aml_name("PCIU"), aml_int(1) /* Device Check */)

Likewise (@bsel is the same as above).

> @@ -2590,7 +2590,7 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>  
>      o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
>      assert(o);
> -    mcfg->mcfg_size = qnum_get_int(qobject_to_qnum(o), &error_abort);
> +    mcfg->mcfg_size = qnum_get_uint(qobject_to_qnum(o), &error_abort);
>      qobject_decref(o);
>      return true;
>  }

Similar argument.

I suspect the hunk I challenged in PATCH 09 actually belongs here.

> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 66055ee5cc..8d3a64008c 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -94,7 +94,7 @@ static void gpex_host_initfn(Object *obj)
>  
>      object_initialize(root, sizeof(*root), TYPE_GPEX_ROOT_DEVICE);
>      object_property_add_child(obj, "gpex_root", OBJECT(root), NULL);
> -    qdev_prop_set_uint32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
> +    qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(root), "multifunction", false);
>  }
>  

"addr" is a property of TYPE_PCI_DEVICE, defined with
DEFINE_PROP_PCI_DEVFN().  It's int32_t, even though only 8 bits are
used.  Okay.

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 5438be8253..3cbe8cfcea 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -173,7 +173,7 @@ static void q35_host_initfn(Object *obj)
>  
>      object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
>      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
> -    qdev_prop_set_uint32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
> +    qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
>  
>      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "uint32",

Likewise.

> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 8b71e2d950..461ed41151 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -150,7 +150,7 @@ static void xilinx_pcie_host_init(Object *obj)
>  
>      object_initialize(root, sizeof(*root), TYPE_XILINX_PCIE_ROOT);
>      object_property_add_child(obj, "root", OBJECT(root), NULL);
> -    qdev_prop_set_uint32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
> +    qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
>      qdev_prop_set_bit(DEVICE(root), "multifunction", false);
>  }
>  

Likewise.

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1e8a5b55c0..5bb8131bb8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3191,7 +3191,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>                                OBJECT(cpu->apic_state), &error_abort);
>      object_unref(OBJECT(cpu->apic_state));
>  
> -    qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
> +    qdev_prop_set_int32(cpu->apic_state, "id", cpu->apic_id);
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(cpu->apic_state);
>      apic->cpu = cpu;

This one's a bit of a mess.

The getter and setter of TYPE_APIC_COMMON property "id" are
apic_common_get_id() and apic_common_set_id().

apic_common_get_id() reads either APICCommonState member uint32_t
initial_apic_id or uint8_t id into an int64_t local variable.  It then
passes this variable to visit_type_int().

apic_common_set_id() uses visit_type_int() to read the value into a
local variable, which it then assigns both to initial_apic_id and id.

While the state backing the property is two unsigned members, 8 and 32
bits wide, the actual visitor is 64 bits signed.

cpu->apic_id is uint32_t.

qdev_prop_set_uint32() isn't really wrong, because any uint32_t value is
also a valid int64_t value.

qdev_prop_set_int32() is implicitly converts cpu->apic_id from uint32_t
to int32_t.  Perhaps that's even okay, but I don't care, I want this
mess cleaned up :)

Two possible ways to clean up:

1. Use qdev_prop_set_int().  Then the members get converted to
int64_t and back.  Looks safe to me.

2. Change getter and setter to use visit_type_uint32().  Then
everything's uint32_t, except for @id.

I prefer 2.

This getter and setter business drives me nuts.  Them mixing up
signedness and width doesn't help.