> On Nov 26, 2019, at 8:39 AM, Marc-André Lureau <marcandre.lur...@gmail.com> > wrote: > > Hi
Heya, thanks for the review. > > On Mon, Nov 25, 2019 at 7:37 PM Felipe Franciosi <fel...@nutanix.com> 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 <fel...@nutanix.com> > > > Some comments below, otherwise: > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> > >> --- >> hw/acpi/ich9.c | 93 +++------------------------------------ >> hw/isa/lpc_ich9.c | 14 +----- >> hw/misc/edu.c | 12 +---- >> hw/pci-host/q35.c | 14 ++---- >> hw/ppc/spapr.c | 17 ++------ >> hw/vfio/pci-quirks.c | 18 ++------ >> memory.c | 15 +------ >> target/arm/cpu.c | 21 +-------- >> target/i386/sev.c | 102 +++---------------------------------------- >> 9 files changed, 29 insertions(+), 277 deletions(-) >> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c >> index 94dc5147ce..aa3c7a59ab 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,12 @@ 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, false, errp); >> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED, >> + &pm->disable_s4, false, errp); >> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL, >> + &pm->s4_val, false, errp); > > Sometime object properties are registered with error_abort, sometime > with errp, sometime with NULL. > > Here you changed the argument. Not a big deal, but I think you should > leave it as is for now. And we can address the error handling > inconsisteny another day. You are right. Let me go over that once more and send a v2. I don't believe in changing too many things at once and improvements or not, it should be done separately (if anything to allow an easier revert later on). :) > >> 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 232c7916f3..9abe07247e 100644 >> --- a/hw/isa/lpc_ich9.c >> +++ b/hw/isa/lpc_ich9.c >> @@ -627,15 +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); >> - uint8_t value = lpc->sci_gsi; >> - >> - visit_type_uint8(v, name, &value, errp); >> -} >> - >> static void ich9_lpc_initfn(Object *obj) >> { >> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj); >> @@ -643,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, true, NULL); >> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD, >> &acpi_enable_cmd, true, NULL); >> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD, >> diff --git a/hw/misc/edu.c b/hw/misc/edu.c >> index d5e2bdbb57..200503ecfd 100644 >> --- a/hw/misc/edu.c >> +++ b/hw/misc/edu.c >> @@ -396,21 +396,13 @@ 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, false, 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..61fbe04fe9 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, true, 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..1b9400526f 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,9 @@ 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, false, &error_abort); >> + >> 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..34335e071e 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,8 @@ 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, true, NULL); > > nit: (void *) is enough, no? Actually, I think this is completely wrong. I asked AW on IRC (he was away and I didn't wait; oops), but I'll Cc him here and Alexey as well (who wrote the code). Maybe I'm missing something, but it looks like this is passing the absolute value of cap->tgt as a pointer. Shouldn't it just be &cap->tg? I don't understand the casting to (void *). Later, in vfio_pci_nvlink2_get_*, it does: uint64_t val = (uintptr_t)opaque; visit_type_unitXX(..., &val, ...); It looks to me like that only gets the local variable and doesn't return anything in practice. But again, I'm not familiar with this at all so I may be saying non-sense. If this is confirmed to be wrong, I can fix this in an extra patch in this series. Thoughts welcome. F. > >> trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt, >> nv2reg->size); >> free_exit: >> @@ -2301,9 +2292,8 @@ 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, true, >> NULL); > > same > >> trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, >> captgt->tgt, >> atsdreg->size); >> >> diff --git a/memory.c b/memory.c >> index 06484c2bff..0a34ee3c86 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, true, &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..aa7278e540 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,8 @@ 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, false, >> &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..23d7ab8b72 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,11 @@ 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, false, >> NULL); >> + object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, >> NULL); >> + object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, >> NULL); >> + object_property_add_uint32_ptr(obj, "reduced-phys-bits", >> + &sev->reduced_phys_bits, false, NULL); >> } >> >> /* sev guest info */ >> -- >> 2.20.1 >> > > > -- > Marc-André Lureau