> On Nov 27, 2019, at 11:58 PM, Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
>
>
>
> On 26/11/2019 20:39, Felipe Franciosi wrote:
>>
>>
>>> 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?
>
>
> Passing &cap->tgt requres @cap to stay in memory until the user of that
> property reads it which is not the case here - the property is set when
> the device is "realized" and used every time the pseries machine is
> reset. This is rather highjacking QOM and properties to pass a 64bit
> value from VFIO to PPC64/pseries without sharing any C structures.
>
>
>> I don't understand the casting to (void *).
>
> object_property_add() takes void*, and cap->tgt is not exactly an
> address so it is not a pointer, it is an abbreviated host hardware
> address which acts here more like a handle/cookie as in fact it is only
> 56bit but whatever :)
>
>
>
>> 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.
>
>
> This visit_type_unitXX() thing puts @val to the visitor object which is
> then read by object_property_get(). I am having hardtime tracing this
> code, below is the example of a read (hopefully TB won't break
> formatting much). Thanks,
>
>
> (gdb) bt
> #0 0x0000000100b902e0 in qobject_output_add_obj (qov=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", value=0x103b8f000) at
> /home/aik/p/qemu/qapi/qobject-output-visitor.c:83
> #1 0x0000000100b908c0 in qobject_output_type_uint64 (v=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
> at /home/aik/p/qemu/qapi/qobject-output-visitor.c:158
> #2 0x0000000100b8be94 in visit_type_uint64 (v=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
> at /home/aik/p/qemu/qapi/qapi-visit-core.c:207
> #3 0x00000001004678f4 in vfio_pci_nvlink2_get_tgt (obj=0x102a84910,
> v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", opaque=0x40000000000,
> errp=0x7fffffffe888) at /home/aik/p/qemu/hw/vfio/pci-quirks.c:2195
> #4 0x0000000100a45720 in object_property_get (obj=0x102a84910,
> v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", errp=0x7fffffffe888) at
> /home/aik/p/qemu/qom/object.c:1257
> #5 0x0000000100a49fe8 in object_property_get_qobject (obj=0x102a84910,
> name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
> /home/aik/p/qemu/qom/qom-qobject.c:38
> #6 0x0000000100a460c8 in object_property_get_uint (obj=0x102a84910,
> name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
> /home/aik/p/qemu/qom/object.c:1407
> #7 0x00000001004eca98 in spapr_phb_pci_collect_nvgpu (bus=0x101d817f0,
> pdev=0x102a84910, opaque=0x101dbfaa0) at
> /home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:139
> #8 0x000000010087795c in pci_for_each_device_under_bus
> (bus=0x101d817f0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
> opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1638
> #9 0x00000001008779fc in pci_for_each_device (bus=0x101d817f0,
> bus_num=0x0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
> opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1650
> #10 0x00000001004ecdd0 in spapr_phb_nvgpu_setup (sphb=0x101d34f20,
> errp=0x7fffffffeb68) at /home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:187
> #11 0x00000001004cb8f8 in spapr_phb_reset (qdev=0x101d34f20) at
> /home/aik/p/qemu/hw/ppc/spapr_pci.c:2049
> #12 0x0000000100718858 in device_reset (dev=0x101d34f20) at
> /home/aik/p/qemu/hw/core/qdev.c:1112
> #13 0x00000001007159f0 in qdev_reset_one (dev=0x101d34f20, opaque=0x0)
> at /home/aik/p/qemu/hw/core/qdev.c:277
> #14 0x0000000100716d18 in qdev_walk_children (dev=0x101d34f20,
> pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
> post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
> /home/aik/p/qemu/hw/core/qdev.c:593
> #15 0x000000010071d1ac in qbus_walk_children (bus=0x1016df320,
> pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
> post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
> /home/aik/p/qemu/hw/core/bus.c:53
> #16 0x0000000100715bf4 in qbus_reset_all (bus=0x1016df320) at
> /home/aik/p/qemu/hw/core/qdev.c:303
> #17 0x0000000100715c4c in qbus_reset_all_fn (opaque=0x1016df320) at
> /home/aik/p/qemu/hw/core/qdev.c:309
> #18 0x000000010071df6c in qemu_devices_reset () at
> /home/aik/p/qemu/hw/core/reset.c:69
> #19 0x00000001004a3554 in spapr_machine_reset (machine=0x1016bec00) at
> /home/aik/p/qemu/hw/ppc/spapr.c:1684
> #20 0x0000000100688488 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE)
> at /home/aik/p/qemu/vl.c:1550
> #21 0x0000000100692b3c in main (argc=0x2e, argv=0x7ffffffff568,
> envp=0x7ffffffff6e0) at /home/aik/p/qemu/vl.c:4419
>
>
>>
>> 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
>>
>
> --
> Alexey
Thanks for the analysis. If you are happy with the usage, I'll send a
v2 of my series shortly which doesn't really touch this code.
Cheers,
F.
