Re: [PATCH-for-9.0 v2 5/8] hw: Prefer qdev_prop_set_bit over object_property_set_bool for QDev

[Peter Maydell]

On Thu, 23 Nov 2023 at 14:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The QOM API is lower level than the QDev one. When an instance is
> QDev and setting the property can not fail (using &error_abort),
> prefer qdev_prop_set_bit() over object_property_set_bool().
>
> Mechanical transformation using the following coccinelle patch:
>
>   @@
>   expression o, p, v;
>   @@
>   -            object_property_set_bool(OBJECT(o), p, v, &error_abort)
>   +            qdev_prop_set_bit(DEVICE(o), p, v)
>   @@@@
>   -            object_property_set_bool(o, p, v, &error_abort)
>   +            qdev_prop_set_bit(DEVICE(o), p, v)
>
> manually adding the missing "hw/qdev-properties.h" header.
>
> In hw/arm/armsse.c we use the available 'cpudev' instead of 'cpuobj'.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> @@ -1287,8 +1288,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, 
> struct arm_boot_info *info)
>               * CPU.
>               */
>              if (cs != first_cpu) {
> -                object_property_set_bool(cpuobj, "start-powered-off", true,
> -                                         &error_abort);
> +                qdev_prop_set_bit(DEVICE(cpuobj), "start-powered-off", true);
>              }
>          }
>      }

This makes this code look a bit weird. Currently we have a loop
which has an "Object *cpuobj" which it uses to set properties,
in both cases using the object_property_* APIs. With this change,
we do half the job using a QOM API and the other half using
a qdev API. It would be good to follow up by converting the
other property-set so we can have a local Device * instead of
the Object *.


> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index eace854335..6733652120 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -263,20 +263,13 @@ static void pc_init1(MachineState *machine,
>          size_t i;
>
>          pci_dev = pci_new_multifunction(-1, pcms->south_bridge);
> -        object_property_set_bool(OBJECT(pci_dev), "has-usb",
> -                                 machine_usb(machine), &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
> -                                 x86_machine_is_acpi_enabled(x86ms),
> -                                 &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-pic", false,
> -                                 &error_abort);
> -        object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
> -                                 &error_abort);
> -        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
> -        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
> -                                 x86_machine_is_smm_enabled(x86ms),
> -                                 &error_abort);
>          dev = DEVICE(pci_dev);
> +        qdev_prop_set_bit(dev, "has-usb", machine_usb(machine));
> +        qdev_prop_set_bit(dev, "has-acpi", 
> x86_machine_is_acpi_enabled(x86ms));
> +        qdev_prop_set_bit(dev, "has-pic", false);
> +        qdev_prop_set_bit(dev, "has-pit", false);
> +        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);

This line also can just use "dev".

> +        qdev_prop_set_bit(dev, "smm-enabled", 
> x86_machine_is_smm_enabled(x86ms));
>          for (i = 0; i < ISA_NUM_IRQS; i++) {
>              qdev_connect_gpio_out_named(dev, "isa-irqs", i, x86ms->gsi[i]);
>          }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM