Marc-André Lureau <marcandre.lur...@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.lur...@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.
