Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers

[Alexey Kardashevskiy]

On 30/11/2019 04:46, Felipe Franciosi wrote:
> Several objects implemented their own uint property getters and setters,
> despite them being straightforward (without any checks/validations on
> the values themselves) and identical across objects. This makes use of
> an enhanced API for object_property_add_uintXX_ptr() which offers
> default setters.
> 
> Some of these setters used to update the value even if the type visit
> failed (eg. because the value being set overflowed over the given type).
> The new setter introduces a check for these errors, not updating the
> value if an error occurred. The error is propagated.
> 
> Signed-off-by: Felipe Franciosi <address@hidden>
> ---
>  hw/acpi/ich9.c       |  95 ++++----------------------------------
>  hw/isa/lpc_ich9.c    |  12 +----
>  hw/misc/edu.c        |  13 ++----
>  hw/pci-host/q35.c    |  14 ++----
>  hw/ppc/spapr.c       |  18 ++------
>  hw/vfio/pci-quirks.c |  20 +++-----
>  memory.c             |  15 +-----
>  target/arm/cpu.c     |  22 ++-------
>  target/i386/sev.c    | 106 ++++---------------------------------------
>  9 files changed, 40 insertions(+), 275 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 742fb78226..d9305be891 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, 
> bool value,
>      s->pm.cpu_hotplug_legacy = value;
>  }
>  
> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    uint8_t value = pm->disable_s3;
> -
> -    visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    Error *local_err = NULL;
> -    uint8_t value;
> -
> -    visit_type_uint8(v, name, &value, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    pm->disable_s3 = value;
> -out:
> -    error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    uint8_t value = pm->disable_s4;
> -
> -    visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    Error *local_err = NULL;
> -    uint8_t value;
> -
> -    visit_type_uint8(v, name, &value, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    pm->disable_s4 = value;
> -out:
> -    error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    uint8_t value = pm->s4_val;
> -
> -    visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> -{
> -    ICH9LPCPMRegs *pm = opaque;
> -    Error *local_err = NULL;
> -    uint8_t value;
> -
> -    visit_type_uint8(v, name, &value, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    pm->s4_val = value;
> -out:
> -    error_propagate(errp, local_err);
> -}
> -
>  static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
>  {
>      ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> @@ -468,18 +393,14 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
> *pm, Error **errp)
>                               ich9_pm_get_cpu_hotplug_legacy,
>                               ich9_pm_set_cpu_hotplug_legacy,
>                               NULL);
> -    object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> -                        ich9_pm_get_disable_s3,
> -                        ich9_pm_set_disable_s3,
> -                        NULL, pm, NULL);
> -    object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
> -                        ich9_pm_get_disable_s4,
> -                        ich9_pm_set_disable_s4,
> -                        NULL, pm, NULL);
> -    object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
> -                        ich9_pm_get_s4_val,
> -                        ich9_pm_set_s4_val,
> -                        NULL, pm, NULL);
> +    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
> +                                  &pm->disable_s3, OBJ_PROP_FLAG_READWRITE,
> +                                  NULL);
> +    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
> +                                  &pm->disable_s4, OBJ_PROP_FLAG_READWRITE,
> +                                  NULL);
> +    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
> +                                  &pm->s4_val, OBJ_PROP_FLAG_READWRITE, 
> NULL);
>      object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
>                               ich9_pm_get_enable_tco,
>                               ich9_pm_set_enable_tco,
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index c40bb3c420..b99a613954 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -627,13 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN
>  };
>  
> -static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
> -                                 void *opaque, Error **errp)
> -{
> -    ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> -    visit_type_uint8(v, name, &lpc->sci_gsi, errp);
> -}
> -
>  static void ich9_lpc_initfn(Object *obj)
>  {
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> @@ -641,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
>      static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>      static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>  
> -    object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
> -                        ich9_lpc_get_sci_int,
> -                        NULL, NULL, NULL, NULL);
> +    object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
> +                                  &lpc->sci_gsi, OBJ_PROP_FLAG_READ, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>                                    &acpi_enable_cmd, OBJ_PROP_FLAG_READ, 
> NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index d5e2bdbb57..ff10f5b794 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -396,21 +396,14 @@ static void pci_edu_uninit(PCIDevice *pdev)
>      msi_uninit(pdev);
>  }
>  
> -static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
> -                           void *opaque, Error **errp)
> -{
> -    uint64_t *val = opaque;
> -
> -    visit_type_uint64(v, name, val, errp);
> -}
> -
>  static void edu_instance_init(Object *obj)
>  {
>      EduState *edu = EDU(obj);
>  
>      edu->dma_mask = (1UL << 28) - 1;
> -    object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
> -                    edu_obj_uint64, NULL, &edu->dma_mask, NULL);
> +    object_property_add_uint64_ptr(obj, "dma_mask",
> +                                   &edu->dma_mask, OBJ_PROP_FLAG_READWRITE,
> +                                   NULL);
>  }
>  
>  static void edu_class_init(ObjectClass *class, void *data)
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 158d270b9f..f384ab95c6 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, 
> Visitor *v,
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> -static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char 
> *name,
> -                                    void *opaque, Error **errp)
> -{
> -    PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
> -
> -    visit_type_uint64(v, name, &e->size, errp);
> -}
> -
>  /*
>   * NOTE: setting defaults for the mch.* fields in this table
>   * doesn't work, because mch is a separate QOM object that is
> @@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
>  {
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>      PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> +    PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
>  
>      memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
>                            "pci-conf-idx", 4);
> @@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
>                          q35_host_get_pci_hole64_end,
>                          NULL, NULL, NULL, NULL);
>  
> -    object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
> -                        q35_host_get_mmcfg_size,
> -                        NULL, NULL, NULL, NULL);
> +    object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
> +                                   &pehb->size, OBJ_PROP_FLAG_READ, NULL);
>  
>      object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
>                               (Object **) &s->mch.ram_memory,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..668f045023 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const 
> char *value, Error **errp)
>      }
>  }
>  
> -static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
> -static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
>  static char *spapr_get_ic_mode(Object *obj, Error **errp)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -3336,8 +3324,10 @@ static void spapr_instance_init(Object *obj)
>      object_property_set_description(obj, "resize-hpt",
>                                      "Resizing of the Hash Page Table 
> (enabled, disabled, required)",
>                                      NULL);
> -    object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
> -                        spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
> +    object_property_add_uint32_ptr(obj, "vsmt",
> +                                   &spapr->vsmt, OBJ_PROP_FLAG_READWRITE,
> +                                   &error_abort);


Ths looks alright but...


> +
>      object_property_set_description(obj, "vsmt",
>                                      "Virtual SMT: KVM behaves as if this 
> were"
>                                      " the host's SMT mode", &error_abort);
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 136f3a9ad6..d769c99bde 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error 
> **errp)
>      return 0;
>  }
>  
> -static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
> -                                     const char *name,
> -                                     void *opaque, Error **errp)
> -{
> -    uint64_t tgt = (uintptr_t) opaque;
> -    visit_type_uint64(v, name, &tgt, errp);
> -}
> -
>  static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
>                                                   const char *name,
>                                                   void *opaque, Error **errp)
> @@ -2240,9 +2232,9 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, 
> Error **errp)
>                                 nv2reg->size, p);
>      QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>  
> -    object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
> -                        vfio_pci_nvlink2_get_tgt, NULL, NULL,
> -                        (void *) (uintptr_t) cap->tgt, NULL);
> +    object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
> +                                   (void *)(uintptr_t)cap->tgt,


... this does not seem equalent. The getter will assume cap->tgt is an
userspace address which it is not. &cap->tgt would be QOM-correct but
won't work because of the lifetime of @cap but this is another story.

btw what is this patchset based on? I tried applying it on top of 3 week
old and today upstream and it failed. Thanks,





> +                                   OBJ_PROP_FLAG_READ, NULL);
>      trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
>                                            nv2reg->size);
>  free_exit:
> @@ -2301,9 +2293,9 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error 
> **errp)
>          QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>      }
>  
> -    object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
> -                        vfio_pci_nvlink2_get_tgt, NULL, NULL,
> -                        (void *) (uintptr_t) captgt->tgt, NULL);
> +    object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
> +                                   (void *)(uintptr_t)captgt->tgt,
> +                                   OBJ_PROP_FLAG_READ, NULL);
>      trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, 
> captgt->tgt,
>                                                atsdreg->size);
>  
> diff --git a/memory.c b/memory.c
> index 06484c2bff..7dac2aa059 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
>      memory_region_do_init(mr, owner, name, size);
>  }
>  
> -static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    MemoryRegion *mr = MEMORY_REGION(obj);
> -    uint64_t value = mr->addr;
> -
> -    visit_type_uint64(v, name, &value, errp);
> -}
> -
>  static void memory_region_get_container(Object *obj, Visitor *v,
>                                          const char *name, void *opaque,
>                                          Error **errp)
> @@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
>                               NULL, NULL, &error_abort);
>      op->resolve = memory_region_resolve_container;
>  
> -    object_property_add(OBJECT(mr), "addr", "uint64",
> -                        memory_region_get_addr,
> -                        NULL, /* memory_region_set_addr */
> -                        NULL, NULL, &error_abort);
> +    object_property_add_uint64_ptr(OBJECT(mr), "addr",
> +                                   &mr->addr, OBJ_PROP_FLAG_READ, 
> &error_abort);
>      object_property_add(OBJECT(mr), "priority", "uint32",
>                          memory_region_get_priority,
>                          NULL, /* memory_region_set_priority */
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 7a4ac9339b..bbe25a73c4 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error 
> **errp)
>      cpu->has_pmu = value;
>  }
>  
> -static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> -{
> -    ARMCPU *cpu = ARM_CPU(obj);
> -
> -    visit_type_uint32(v, name, &cpu->init_svtor, errp);
> -}
> -
> -static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> -{
> -    ARMCPU *cpu = ARM_CPU(obj);
> -
> -    visit_type_uint32(v, name, &cpu->init_svtor, errp);
> -}
> -
>  void arm_cpu_post_init(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -1165,9 +1149,9 @@ void arm_cpu_post_init(Object *obj)
>           * a simple DEFINE_PROP_UINT32 for this because we want to permit
>           * the property to be set after realize.
>           */
> -        object_property_add(obj, "init-svtor", "uint32",
> -                            arm_get_init_svtor, arm_set_init_svtor,
> -                            NULL, NULL, &error_abort);
> +        object_property_add_uint32_ptr(obj, "init-svtor",
> +                                       &cpu->init_svtor,
> +                                       OBJ_PROP_FLAG_READWRITE, 
> &error_abort);
>      }
>  
>      qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 024bb24e51..846018a12d 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
>              "guest owners session parameters (encoded with base64)", NULL);
>  }
>  
> -static void
> -qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
> -                      void *opaque, Error **errp)
> -{
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -    uint32_t value;
> -
> -    visit_type_uint32(v, name, &value, errp);
> -    sev->handle = value;
> -}
> -
> -static void
> -qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
> -                      void *opaque, Error **errp)
> -{
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -    uint32_t value;
> -
> -    visit_type_uint32(v, name, &value, errp);
> -    sev->policy = value;
> -}
> -
> -static void
> -qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
> -                       void *opaque, Error **errp)
> -{
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -    uint32_t value;
> -
> -    visit_type_uint32(v, name, &value, errp);
> -    sev->cbitpos = value;
> -}
> -
> -static void
> -qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -    uint32_t value;
> -
> -    visit_type_uint32(v, name, &value, errp);
> -    sev->reduced_phys_bits = value;
> -}
> -
> -static void
> -qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
> -                      void *opaque, Error **errp)
> -{
> -    uint32_t value;
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> -    value = sev->policy;
> -    visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
> -                      void *opaque, Error **errp)
> -{
> -    uint32_t value;
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> -    value = sev->handle;
> -    visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
> -                       void *opaque, Error **errp)
> -{
> -    uint32_t value;
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> -    value = sev->cbitpos;
> -    visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
> -                                   void *opaque, Error **errp)
> -{
> -    uint32_t value;
> -    QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> -    value = sev->reduced_phys_bits;
> -    visit_type_uint32(v, name, &value, errp);
> -}
> -
>  static void
>  qsev_guest_init(Object *obj)
>  {
> @@ -361,15 +273,15 @@ qsev_guest_init(Object *obj)
>  
>      sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>      sev->policy = DEFAULT_GUEST_POLICY;
> -    object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
> -                        qsev_guest_set_policy, NULL, NULL, NULL);
> -    object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
> -                        qsev_guest_set_handle, NULL, NULL, NULL);
> -    object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
> -                        qsev_guest_set_cbitpos, NULL, NULL, NULL);
> -    object_property_add(obj, "reduced-phys-bits", "uint32",
> -                        qsev_guest_get_reduced_phys_bits,
> -                        qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
> +    object_property_add_uint32_ptr(obj, "policy", &sev->policy,
> +                                   OBJ_PROP_FLAG_READWRITE, NULL);
> +    object_property_add_uint32_ptr(obj, "handle", &sev->handle,
> +                                   OBJ_PROP_FLAG_READWRITE, NULL);
> +    object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos,
> +                                   OBJ_PROP_FLAG_READWRITE, NULL);
> +    object_property_add_uint32_ptr(obj, "reduced-phys-bits",
> +                                   &sev->reduced_phys_bits,
> +                                   OBJ_PROP_FLAG_READWRITE, NULL);
>  }
>  
>  /* sev guest info */
> 

-- 
Alexey