Re: [PATCH v3 00/53] Make qdev static property API usable by any QOM type

[Eduardo Habkost]

Paolo, do you have any objections to adding the field property
API in this series to QOM, considering the recent discussions
about long term plans for QOM properties?

Note that I still want to perform most of the changes in this
series, even if the code is kept inside `hw/core`.  Moving the
code to `qom` is just one extra step, but not essential.


On Thu, Nov 12, 2020 at 04:42:57PM -0500, Eduardo Habkost wrote:
> Based-on: 20201111183823.283752-1-ehabkost@redhat.com
> Git branch: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-make-generic
> 
> This series refactor the qdev property code so the static
> property system from qdev becomes a common QOM API that can be
> used by any QOM type.
> 
> As an example, at the end of the series we use the new API at:
> * check-qom-proplist unit test, to replace the hand-written
>   getter and setter for the "sv" property
> * target/i386/sev.c, to replace the instance properties
>   registered using object_property_add_uint32_ptr()
> 
> I still want to make object_class_add_field_properties() the
> recommended interface for registering QOM properties,
> but this will be done in another series.
> 
> Changes v2 -> v3
> ----------------
> 
> * Patches were reordered
> 
> * New qom/qom.h header file
>   (See "qom: Add new qom.h header")
> 
> * Fixed memory leak in the array property refactor from v2
>   (See "qdev: Get rid of ArrayElementProperty struct")
> 
> * Made object_property_add_field() copy the Property struct
>   passed as argument, to make the array property memory leak
>   easier to fix
>   (See "qdev: Make object_property_add_field() copy the Property struct")
> 
> * Both object_class_add_field_properties() and
>   object_class_property_add_field() functions are available,
>   but only object_class_property_add_field() are made public
>   by this series.  object_class_add_field_properties() is
>   used to implement device_class_set_props().
>   (See "qom: object_class_property_add_field() function" &
>   "qdev: Separate generic and device-specific property registration")
> 
> * Now object_class_property_add_field() will get a copy of the
>   Property struct, to avoid tricks involving static variables
>   in the FIELD_PROP macro.
>   (See "qom: object_class_property_add_field() function")
> 
> * check-qom-proplist won't use object_property_add_field(),
>   as it is an internal API and I don't want to make it a bad
>   example to be followed.
>   (See "tests: Use field property at check-qom-proplist test case")
> 
> * Property.qdev_prop_name is now Property.name_template, because
>   it might be used outside qdev in case we make
>   object_class_add_field_properties() public.
>   (See "qdev: Rename Property.name to Property.name_template")
> 
> * New unit test for array property
>   (See "tests: Add unit test for qdev array properties")
> 
> * Include sev patch for using class properties instead of
>   object_property_add_uint32_ptr()
>   (See "sev: Use class properties")
> 
> * FIELD_PROP macro is now inside field-property.h
>   (See "qom: FIELD_PROP macro")
> 
> * DEFINE_PROP_* macros are now defined using PROP_*, not
>   the other way around
>   (See "qom: PROP_* macros")
> 
> Changes v1 -> v2
> ----------------
> 
> * Rename functions and source files to call the new feature
>   "field property" instead of "static property"
> 
> * Change the API signature from an array-based interface similar
>   to device_class_set_props() to a object_property_add()-like
>   interface.
> 
>   This means instead of doing this:
> 
>     object_class_property_add_static_props(oc, my_array_of_props);
> 
>   properties are registered like this:
> 
>     object_class_property_add_field(oc, "property-name"
>                                     PROP_XXX(MyState, my_field),
>                                     prop_allow_set_always);
> 
>   where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL,
>   etc.
> 
> * Make Property.name optional.  Rename it to qdev_prop_name,
>   and restrict its usage to qdev property tracking code.
> 
> * Make allow_set callback mandatory, to avoid ambiguity
> 
> * Big cleanup of array property code.  We don't need a custom
>   release function for array elements anymore, because we don't
>   need to save the property name in the Property struct anymore.
> 
> * Moved UUID property to qdev-properties-system, because
>   it still has dependencies on qdev code
> 
> Eduardo Habkost (53):
>   cs4231: Get rid of empty property array
>   cpu: Move cpu_common_props to hw/core/cpu.c
>   qdev: Move property code to qdev-properties.[ch]
>   qdev: Check dev->realized at set_size()
>   sparc: Check dev->realized at sparc_set_nwindows()
>   qdev: Don't use dev->id on set_size32() error message
>   qdev: Make PropertyInfo.print method get Object* argument
>   qdev: Make bit_prop_set() get Object* argument
>   qdev: Make qdev_get_prop_ptr() get Object* arg
>   qdev: Make qdev_find_global_prop() get Object* argument
>   qdev: Make check_prop_still_unset() get Object* argument
>   qdev: Make error_set_from_qdev_prop_error() get Object* argument
>   qdev: Make qdev_propinfo_get_uint16() static
>   qdev: Move UUID property to qdev-properties-system.c
>   qdev: Move softmmu properties to qdev-properties-system.h
>   qdev: Reuse DEFINE_PROP in all DEFINE_PROP_* macros
>   sparc: Use DEFINE_PROP for nwindows property
>   qdev: Get just property name at error_set_from_qdev_prop_error()
>   qdev: Avoid using prop->name unnecessarily
>   qdev: Add name parameter to qdev_class_add_property()
>   qdev: Add name argument to PropertyInfo.create method
>   qdev: Wrap getters and setters in separate helpers
>   qdev: Move dev->realized check to qdev_property_set()
>   qdev: Make PropertyInfo.create return ObjectProperty*
>   qdev: Make qdev_class_add_property() more flexible
>   qdev: Separate generic and device-specific property registration
>   qdev: Rename qdev_propinfo_* to field_prop_*
>   qdev: Move qdev_prop_tpm declaration to tpm_prop.h
>   qdev: Rename qdev_prop_* to prop_info_*
>   qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr()
>   qdev: Avoid unnecessary DeviceState* variable at set_prop_arraylen()
>   tests: Add unit test for qdev array properties
>   qom: Add allow_set callback to ObjectProperty
>   qdev: Make qdev_prop_allow_set() a ObjectProperty.allow_set callback
>   qdev: Rename Property.name to Property.name_template
>   qdev: Don't set .name_template for array elements
>   qdev: Remove ArrayElementProperty.propname field
>   qdev: Remove ArrayElementProperty.release field
>   qdev: Get rid of ArrayElementProperty struct
>   qdev: Rename array_element_release() to
>     static_prop_release_dynamic_prop()
>   qdev: Make object_property_add_field() copy the Property struct
>   qdev: Reuse object_property_add_field() when adding array elements
>   qdev: Move static_prop_release_dynamic_prop() closer to its usage
>   qom: Add new qom.h header
>   qdev: Move core field property code to QOM
>   qdev: Move base property types to qom/property-types.c
>   qom: Include static property API reference in documentation
>   qom: object_class_property_add_field() function
>   qom: FIELD_PROP macro
>   qom: Delete DEFINE_PROP_*SIGNED_NODEFAULT macro
>   qom: PROP_* macros
>   tests: Use field property at check-qom-proplist test case
>   sev: Use class properties
> 
>  docs/devel/qom.rst                    |  17 +-
>  audio/audio.h                         |   1 +
>  hw/core/qdev-prop-internal.h          |  30 -
>  hw/tpm/tpm_prop.h                     |   2 +
>  include/hw/block/block.h              |   1 +
>  include/hw/core/cpu.h                 |   1 -
>  include/hw/qdev-core.h                |  37 --
>  include/hw/qdev-properties-system.h   |  77 +++
>  include/hw/qdev-properties.h          | 244 +-------
>  include/net/net.h                     |   1 +
>  include/qom/field-property-internal.h |  80 +++
>  include/qom/field-property.h          | 128 ++++
>  include/qom/object.h                  |  38 ++
>  include/qom/property-types.h          | 176 ++++++
>  include/qom/qom.h                     |  12 +
>  backends/tpm/tpm_util.c               |  16 +-
>  cpu.c                                 |  15 -
>  hw/acpi/vmgenid.c                     |   1 +
>  hw/arm/pxa2xx.c                       |   1 +
>  hw/arm/strongarm.c                    |   1 +
>  hw/audio/cs4231.c                     |   5 -
>  hw/block/fdc.c                        |   1 +
>  hw/block/m25p80.c                     |   1 +
>  hw/block/nand.c                       |   1 +
>  hw/block/onenand.c                    |   1 +
>  hw/block/pflash_cfi01.c               |   1 +
>  hw/block/pflash_cfi02.c               |   1 +
>  hw/block/vhost-user-blk.c             |   1 +
>  hw/block/xen-block.c                  |  11 +-
>  hw/char/avr_usart.c                   |   1 +
>  hw/char/bcm2835_aux.c                 |   1 +
>  hw/char/cadence_uart.c                |   1 +
>  hw/char/cmsdk-apb-uart.c              |   1 +
>  hw/char/debugcon.c                    |   1 +
>  hw/char/digic-uart.c                  |   1 +
>  hw/char/escc.c                        |   1 +
>  hw/char/etraxfs_ser.c                 |   1 +
>  hw/char/exynos4210_uart.c             |   1 +
>  hw/char/grlib_apbuart.c               |   1 +
>  hw/char/ibex_uart.c                   |   1 +
>  hw/char/imx_serial.c                  |   1 +
>  hw/char/ipoctal232.c                  |   1 +
>  hw/char/lm32_juart.c                  |   1 +
>  hw/char/lm32_uart.c                   |   1 +
>  hw/char/mcf_uart.c                    |   1 +
>  hw/char/milkymist-uart.c              |   1 +
>  hw/char/nrf51_uart.c                  |   1 +
>  hw/char/parallel.c                    |   1 +
>  hw/char/pl011.c                       |   1 +
>  hw/char/renesas_sci.c                 |   1 +
>  hw/char/sclpconsole-lm.c              |   1 +
>  hw/char/sclpconsole.c                 |   1 +
>  hw/char/serial-pci-multi.c            |   1 +
>  hw/char/serial.c                      |   1 +
>  hw/char/spapr_vty.c                   |   1 +
>  hw/char/stm32f2xx_usart.c             |   1 +
>  hw/char/terminal3270.c                |   1 +
>  hw/char/virtio-console.c              |   1 +
>  hw/char/xilinx_uartlite.c             |   1 +
>  hw/core/cpu.c                         |  15 +
>  hw/core/qdev-properties-system.c      | 258 ++++-----
>  hw/core/qdev-properties.c             | 806 +++-----------------------
>  hw/core/qdev.c                        | 120 ----
>  hw/hyperv/vmbus.c                     |   1 +
>  hw/i386/kvm/i8254.c                   |   1 +
>  hw/ide/qdev.c                         |   1 +
>  hw/intc/arm_gicv3_common.c            |   2 +-
>  hw/intc/rx_icu.c                      |   4 +-
>  hw/ipmi/ipmi_bmc_extern.c             |   1 +
>  hw/ipmi/ipmi_bmc_sim.c                |   1 +
>  hw/misc/allwinner-sid.c               |   1 +
>  hw/misc/arm_sysctl.c                  |   4 +-
>  hw/misc/ivshmem.c                     |   1 +
>  hw/misc/mac_via.c                     |   1 +
>  hw/misc/sifive_u_otp.c                |   1 +
>  hw/net/e1000e.c                       |   6 +-
>  hw/net/rocker/rocker.c                |   1 +
>  hw/nvram/eeprom_at24c.c               |   1 +
>  hw/nvram/spapr_nvram.c                |   1 +
>  hw/pci-bridge/gen_pcie_root_port.c    |   1 +
>  hw/pci/pci.c                          |   1 +
>  hw/ppc/pnv_pnor.c                     |   1 +
>  hw/rdma/vmw/pvrdma_main.c             |   1 +
>  hw/rtc/mc146818rtc.c                  |   1 +
>  hw/s390x/css.c                        |  13 +-
>  hw/s390x/s390-pci-bus.c               |  10 +-
>  hw/scsi/scsi-disk.c                   |   1 +
>  hw/scsi/scsi-generic.c                |   1 +
>  hw/scsi/vhost-user-scsi.c             |   1 +
>  hw/sd/sd.c                            |   1 +
>  hw/usb/ccid-card-passthru.c           |   1 +
>  hw/usb/dev-serial.c                   |   1 +
>  hw/usb/redirect.c                     |   1 +
>  hw/vfio/pci-quirks.c                  |  11 +-
>  hw/vfio/pci.c                         |   7 +-
>  hw/virtio/vhost-user-fs.c             |   1 +
>  hw/virtio/vhost-user-vsock.c          |   1 +
>  hw/virtio/virtio-iommu-pci.c          |   1 +
>  hw/xen/xen_pt.c                       |   1 +
>  migration/migration.c                 |   1 +
>  qom/field-property.c                  | 151 +++++
>  qom/object.c                          |  16 +
>  qom/property-types.c                  | 546 +++++++++++++++++
>  softmmu/qdev-monitor.c                |   9 +-
>  target/arm/cpu.c                      |   6 +-
>  target/i386/sev.c                     |  25 +-
>  target/sparc/cpu.c                    |   5 +-
>  tests/check-qom-proplist.c            |  39 +-
>  tests/test-qdev-global-props.c        |  61 ++
>  qom/meson.build                       |   2 +
>  110 files changed, 1659 insertions(+), 1418 deletions(-)
>  delete mode 100644 hw/core/qdev-prop-internal.h
>  create mode 100644 include/hw/qdev-properties-system.h
>  create mode 100644 include/qom/field-property-internal.h
>  create mode 100644 include/qom/field-property.h
>  create mode 100644 include/qom/property-types.h
>  create mode 100644 include/qom/qom.h
>  create mode 100644 qom/field-property.c
>  create mode 100644 qom/property-types.c
> 
> -- 
> 2.28.0
> 

-- 
Eduardo