* [PATCH v3 1/4] qom/object: enable setter for uint types
2019-11-29 17:46 [PATCH v3 0/4] Improve default object property_add uint helpers Felipe Franciosi
@ 2019-11-29 17:46 ` Felipe Franciosi
2019-11-30 7:50 ` Marc-André Lureau
2019-11-29 17:46 ` [PATCH v3 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Felipe Franciosi @ 2019-11-29 17:46 UTC (permalink / raw)
To: Marc-Andre Lureau, Philippe Mathieu-Daude, Stefan Hajnoczi,
Eduardo Habkost, Markus Armbruster, Alexey Kardashevskiy
Cc: qemu-devel, Felipe Franciosi
Traditionally, the uint-specific property helpers only offer getters.
When adding object (or class) uint types, one must therefore use the
generic property helper if a setter is needed (and probably duplicate
some code writing their own getters/setters).
This enhances the uint-specific property helper APIs by adding a
bitwise-or'd 'flags' field and modifying all clients of that API to set
this paramater to OBJ_PROP_FLAG_READ. This maintains the current
behaviour whilst allowing others to also set OBJ_PROP_FLAG_WRITE (or use
the more convenient OBJ_PROP_FLAG_READWRITE) in the future (which will
automatically install a setter). Other flags may be added later.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
hw/acpi/ich9.c | 4 +-
hw/acpi/pcihp.c | 7 +-
hw/acpi/piix4.c | 12 +--
hw/isa/lpc_ich9.c | 4 +-
hw/ppc/spapr_drc.c | 3 +-
include/qom/object.h | 44 +++++++--
qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++-----
ui/console.c | 4 +-
8 files changed, 246 insertions(+), 48 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2034dd749e..742fb78226 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
pm->s4_val = 2;
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
- &pm->pm_io_base, errp);
+ &pm->pm_io_base, OBJ_PROP_FLAG_READ, errp);
object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
ich9_pm_get_gpe0_blk,
NULL, NULL, pm, NULL);
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
- &gpe0_len, errp);
+ &gpe0_len, OBJ_PROP_FLAG_READ, errp);
object_property_add_bool(obj, "memory-hotplug-support",
ich9_pm_get_memory_hotplug_support,
ich9_pm_set_memory_hotplug_support,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 8413348a33..4dcef372bf 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
*bus_bsel = (*bsel_alloc)++;
object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
- bus_bsel, &error_abort);
+ bus_bsel, OBJ_PROP_FLAG_READ,
+ &error_abort);
}
return bsel_alloc;
@@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
memory_region_add_subregion(address_space_io, s->io_base, &s->io);
object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
- &error_abort);
+ OBJ_PROP_FLAG_READ, &error_abort);
object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
- &error_abort);
+ OBJ_PROP_FLAG_READ, &error_abort);
}
const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93aec2dd2c..fe05a3ce46 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
static const uint16_t sci_int = 9;
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
- &acpi_enable_cmd, NULL);
+ &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
- &acpi_disable_cmd, NULL);
+ &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
- &gpe0_blk, NULL);
+ &gpe0_blk, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
- &gpe0_blk_len, NULL);
+ &gpe0_blk_len, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
- &sci_int, NULL);
+ &sci_int, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
- &s->io_base, NULL);
+ &s->io_base, OBJ_PROP_FLAG_READ, NULL);
}
static void piix4_pm_realize(PCIDevice *dev, Error **errp)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..99517c3946 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
ich9_lpc_get_sci_int,
NULL, NULL, NULL, NULL);
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
- &acpi_enable_cmd, NULL);
+ &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
- &acpi_disable_cmd, NULL);
+ &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 62f1a42592..bbd4bf35c7 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -553,7 +553,8 @@ static void spapr_dr_connector_instance_init(Object *obj)
SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
+ object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_READ,
+ NULL);
object_property_add(obj, "index", "uint32", prop_get_index,
NULL, NULL, NULL, NULL);
object_property_add(obj, "fdt", "struct", prop_get_fdt,
diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c77f..d2bfd76741 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1579,65 +1579,93 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
void (*get)(Object *, struct tm *, Error **),
Error **errp);
+typedef enum {
+ /* Automatically add a getter to the property */
+ OBJ_PROP_FLAG_READ = 1 << 0,
+ /* Automatically add a setter to the property */
+ OBJ_PROP_FLAG_WRITE = 1 << 1,
+ /* Automatically add a getter and a setter to the property */
+ OBJ_PROP_FLAG_READWRITE = (OBJ_PROP_FLAG_READ | OBJ_PROP_FLAG_WRITE),
+} ObjectPropertyFlags;
+
/**
* object_property_add_uint8_ptr:
* @obj: the object to add a property to
* @name: the name of the property
* @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
* @errp: if an error occurs, a pointer to an area to store the error
*
* Add an integer property in memory. This function will add a
* property of type 'uint8'.
*/
void object_property_add_uint8_ptr(Object *obj, const char *name,
- const uint8_t *v, Error **errp);
+ const uint8_t *v, ObjectPropertyFlags flags,
+ Error **errp);
void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
- const uint8_t *v, Error **errp);
+ const uint8_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp);
/**
* object_property_add_uint16_ptr:
* @obj: the object to add a property to
* @name: the name of the property
* @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
* @errp: if an error occurs, a pointer to an area to store the error
*
* Add an integer property in memory. This function will add a
* property of type 'uint16'.
*/
void object_property_add_uint16_ptr(Object *obj, const char *name,
- const uint16_t *v, Error **errp);
+ const uint16_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp);
void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
- const uint16_t *v, Error **errp);
+ const uint16_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp);
/**
* object_property_add_uint32_ptr:
* @obj: the object to add a property to
* @name: the name of the property
* @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
* @errp: if an error occurs, a pointer to an area to store the error
*
* Add an integer property in memory. This function will add a
* property of type 'uint32'.
*/
void object_property_add_uint32_ptr(Object *obj, const char *name,
- const uint32_t *v, Error **errp);
+ const uint32_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp);
void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
- const uint32_t *v, Error **errp);
+ const uint32_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp);
/**
* object_property_add_uint64_ptr:
* @obj: the object to add a property to
* @name: the name of the property
* @v: pointer to value
+ * @flags: bitwise-or'd ObjectPropertyFlags
* @errp: if an error occurs, a pointer to an area to store the error
*
* Add an integer property in memory. This function will add a
* property of type 'uint64'.
*/
void object_property_add_uint64_ptr(Object *obj, const char *name,
- const uint64_t *v, Error **Errp);
+ const uint64_t *v,
+ ObjectPropertyFlags flags,
+ Error **Errp);
void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
- const uint64_t *v, Error **Errp);
+ const uint64_t *v,
+ ObjectPropertyFlags flags,
+ Error **Errp);
/**
* object_property_add_alias:
diff --git a/qom/object.c b/qom/object.c
index d51b57fba1..77c2682296 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
visit_type_uint8(v, name, &value, errp);
}
+static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint8_t *field = opaque;
+ uint8_t value;
+ Error *local_err = NULL;
+
+ visit_type_uint8(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ *field = value;
+}
+
static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
visit_type_uint16(v, name, &value, errp);
}
+static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint16_t *field = opaque;
+ uint16_t value;
+ Error *local_err = NULL;
+
+ visit_type_uint16(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ *field = value;
+}
+
static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
visit_type_uint32(v, name, &value, errp);
}
+static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint32_t *field = opaque;
+ uint32_t value;
+ Error *local_err = NULL;
+
+ visit_type_uint32(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ *field = value;
+}
+
static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
visit_type_uint64(v, name, &value, errp);
}
+static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ uint64_t *field = opaque;
+ uint64_t value;
+ Error *local_err = NULL;
+
+ visit_type_uint64(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ *field = value;
+}
+
void object_property_add_uint8_ptr(Object *obj, const char *name,
- const uint8_t *v, Error **errp)
+ const uint8_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_property_add(obj, name, "uint8", property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint8_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint8_ptr;
+ }
+
+ object_property_add(obj, name, "uint8",
+ getter, setter, NULL, (void *)v, errp);
}
void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
- const uint8_t *v, Error **errp)
+ const uint8_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint8_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint8_ptr;
+ }
+
+ object_class_property_add(klass, name, "uint8",
+ getter, setter, NULL, (void *)v, errp);
}
void object_property_add_uint16_ptr(Object *obj, const char *name,
- const uint16_t *v, Error **errp)
+ const uint16_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_property_add(obj, name, "uint16", property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint16_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint16_ptr;
+ }
+
+ object_property_add(obj, name, "uint16",
+ getter, setter, NULL, (void *)v, errp);
}
void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
- const uint16_t *v, Error **errp)
+ const uint16_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint16_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint16_ptr;
+ }
+
+ object_class_property_add(klass, name, "uint16",
+ getter, setter, NULL, (void *)v, errp);
}
void object_property_add_uint32_ptr(Object *obj, const char *name,
- const uint32_t *v, Error **errp)
+ const uint32_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_property_add(obj, name, "uint32", property_get_uint32_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint32_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint32_ptr;
+ }
+
+ object_property_add(obj, name, "uint32",
+ getter, setter, NULL, (void *)v, errp);
}
void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
- const uint32_t *v, Error **errp)
+ const uint32_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint32_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint32_ptr;
+ }
+
+ object_class_property_add(klass, name, "uint32",
+ getter, setter, NULL, (void *)v, errp);
}
void object_property_add_uint64_ptr(Object *obj, const char *name,
- const uint64_t *v, Error **errp)
+ const uint64_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_property_add(obj, name, "uint64", property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint64_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint64_ptr;
+ }
+
+ object_property_add(obj, name, "uint64",
+ getter, setter, NULL, (void *)v, errp);
}
void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
- const uint64_t *v, Error **errp)
+ const uint64_t *v,
+ ObjectPropertyFlags flags,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
+ ObjectPropertyAccessor *getter = NULL;
+ ObjectPropertyAccessor *setter = NULL;
+
+ if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
+ getter = property_get_uint64_ptr;
+ }
+
+ if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
+ setter = property_set_uint64_ptr;
+ }
+
+ object_class_property_add(klass, name, "uint64",
+ getter, setter, NULL, (void *)v, errp);
}
typedef struct {
diff --git a/ui/console.c b/ui/console.c
index 82d1ddac9c..bcbe65e696 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
object_property_allow_set_link,
OBJ_PROP_LINK_STRONG,
&error_abort);
- object_property_add_uint32_ptr(obj, "head",
- &s->head, &error_abort);
+ object_property_add_uint32_ptr(obj, "head", &s->head,
+ OBJ_PROP_FLAG_READ, &error_abort);
if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
(console_type == GRAPHIC_CONSOLE))) {
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/4] qom/object: enable setter for uint types
2019-11-29 17:46 ` [PATCH v3 1/4] qom/object: enable setter for uint types Felipe Franciosi
@ 2019-11-30 7:50 ` Marc-André Lureau
2019-12-02 15:06 ` Felipe Franciosi
0 siblings, 1 reply; 16+ messages in thread
From: Marc-André Lureau @ 2019-11-30 7:50 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Stefan Hajnoczi, Philippe Mathieu-Daude
Hi
On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Traditionally, the uint-specific property helpers only offer getters.
> When adding object (or class) uint types, one must therefore use the
> generic property helper if a setter is needed (and probably duplicate
> some code writing their own getters/setters).
>
> This enhances the uint-specific property helper APIs by adding a
> bitwise-or'd 'flags' field and modifying all clients of that API to set
> this paramater to OBJ_PROP_FLAG_READ. This maintains the current
> behaviour whilst allowing others to also set OBJ_PROP_FLAG_WRITE (or use
> the more convenient OBJ_PROP_FLAG_READWRITE) in the future (which will
> automatically install a setter). Other flags may be added later.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
> hw/acpi/ich9.c | 4 +-
> hw/acpi/pcihp.c | 7 +-
> hw/acpi/piix4.c | 12 +--
> hw/isa/lpc_ich9.c | 4 +-
> hw/ppc/spapr_drc.c | 3 +-
> include/qom/object.h | 44 +++++++--
> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++-----
> ui/console.c | 4 +-
> 8 files changed, 246 insertions(+), 48 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2034dd749e..742fb78226 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> pm->s4_val = 2;
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> - &pm->pm_io_base, errp);
> + &pm->pm_io_base, OBJ_PROP_FLAG_READ, errp);
> object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
> ich9_pm_get_gpe0_blk,
> NULL, NULL, pm, NULL);
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> - &gpe0_len, errp);
> + &gpe0_len, OBJ_PROP_FLAG_READ, errp);
> object_property_add_bool(obj, "memory-hotplug-support",
> ich9_pm_get_memory_hotplug_support,
> ich9_pm_set_memory_hotplug_support,
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 8413348a33..4dcef372bf 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>
> *bus_bsel = (*bsel_alloc)++;
> object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> - bus_bsel, &error_abort);
> + bus_bsel, OBJ_PROP_FLAG_READ,
> + &error_abort);
> }
>
> return bsel_alloc;
> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> memory_region_add_subregion(address_space_io, s->io_base, &s->io);
>
> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
> - &error_abort);
> + OBJ_PROP_FLAG_READ, &error_abort);
> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> - &error_abort);
> + OBJ_PROP_FLAG_READ, &error_abort);
> }
>
> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 93aec2dd2c..fe05a3ce46 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
> static const uint16_t sci_int = 9;
>
> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> - &acpi_enable_cmd, NULL);
> + &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> - &gpe0_blk, NULL);
> + &gpe0_blk, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> - &gpe0_blk_len, NULL);
> + &gpe0_blk_len, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> - &sci_int, NULL);
> + &sci_int, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> - &s->io_base, NULL);
> + &s->io_base, OBJ_PROP_FLAG_READ, NULL);
> }
>
> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 17c292e306..99517c3946 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> ich9_lpc_get_sci_int,
> NULL, NULL, NULL, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> - &acpi_enable_cmd, NULL);
> + &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
>
> ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
> }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 62f1a42592..bbd4bf35c7 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -553,7 +553,8 @@ static void spapr_dr_connector_instance_init(Object *obj)
> SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> - object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> + object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_READ,
> + NULL);
> object_property_add(obj, "index", "uint32", prop_get_index,
> NULL, NULL, NULL, NULL);
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 128d00c77f..d2bfd76741 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1579,65 +1579,93 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
> void (*get)(Object *, struct tm *, Error **),
> Error **errp);
>
> +typedef enum {
> + /* Automatically add a getter to the property */
> + OBJ_PROP_FLAG_READ = 1 << 0,
> + /* Automatically add a setter to the property */
> + OBJ_PROP_FLAG_WRITE = 1 << 1,
> + /* Automatically add a getter and a setter to the property */
> + OBJ_PROP_FLAG_READWRITE = (OBJ_PROP_FLAG_READ | OBJ_PROP_FLAG_WRITE),
> +} ObjectPropertyFlags;
> +
> /**
> * object_property_add_uint8_ptr:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
> * @errp: if an error occurs, a pointer to an area to store the error
> *
> * Add an integer property in memory. This function will add a
> * property of type 'uint8'.
> */
> void object_property_add_uint8_ptr(Object *obj, const char *name,
> - const uint8_t *v, Error **errp);
> + const uint8_t *v, ObjectPropertyFlags flags,
> + Error **errp);
> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> - const uint8_t *v, Error **errp);
> + const uint8_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp);
>
> /**
> * object_property_add_uint16_ptr:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
> * @errp: if an error occurs, a pointer to an area to store the error
> *
> * Add an integer property in memory. This function will add a
> * property of type 'uint16'.
> */
> void object_property_add_uint16_ptr(Object *obj, const char *name,
> - const uint16_t *v, Error **errp);
> + const uint16_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp);
> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> - const uint16_t *v, Error **errp);
> + const uint16_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp);
>
> /**
> * object_property_add_uint32_ptr:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
> * @errp: if an error occurs, a pointer to an area to store the error
> *
> * Add an integer property in memory. This function will add a
> * property of type 'uint32'.
> */
> void object_property_add_uint32_ptr(Object *obj, const char *name,
> - const uint32_t *v, Error **errp);
> + const uint32_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp);
> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> - const uint32_t *v, Error **errp);
> + const uint32_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp);
>
> /**
> * object_property_add_uint64_ptr:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @flags: bitwise-or'd ObjectPropertyFlags
> * @errp: if an error occurs, a pointer to an area to store the error
> *
> * Add an integer property in memory. This function will add a
> * property of type 'uint64'.
> */
> void object_property_add_uint64_ptr(Object *obj, const char *name,
> - const uint64_t *v, Error **Errp);
> + const uint64_t *v,
> + ObjectPropertyFlags flags,
> + Error **Errp);
> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> - const uint64_t *v, Error **Errp);
> + const uint64_t *v,
> + ObjectPropertyFlags flags,
> + Error **Errp);
>
> /**
> * object_property_add_alias:
> diff --git a/qom/object.c b/qom/object.c
> index d51b57fba1..77c2682296 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
> visit_type_uint8(v, name, &value, errp);
> }
>
> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + uint8_t *field = opaque;
> + uint8_t value;
> + Error *local_err = NULL;
> +
> + visit_type_uint8(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + *field = value;
> +}
> +
> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> visit_type_uint16(v, name, &value, errp);
> }
>
> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + uint16_t *field = opaque;
> + uint16_t value;
> + Error *local_err = NULL;
> +
> + visit_type_uint16(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + *field = value;
> +}
> +
> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> visit_type_uint32(v, name, &value, errp);
> }
>
> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + uint32_t *field = opaque;
> + uint32_t value;
> + Error *local_err = NULL;
> +
> + visit_type_uint32(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + *field = value;
> +}
> +
> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> visit_type_uint64(v, name, &value, errp);
> }
>
> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + uint64_t *field = opaque;
> + uint64_t value;
> + Error *local_err = NULL;
> +
> + visit_type_uint64(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + *field = value;
> +}
> +
> void object_property_add_uint8_ptr(Object *obj, const char *name,
> - const uint8_t *v, Error **errp)
> + const uint8_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint8_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint8_ptr;
> + }
> +
> + object_property_add(obj, name, "uint8",
> + getter, setter, NULL, (void *)v, errp);
Or just:
object_property_add(obj, name, "uint8",
flags & OBJ_PROP_FLAG_READ ? property_get_uint8_ptr : NULL,
flags & OBJ_PROP_FLAG_WRITE ? property_set_uint8_ptr : NULL,
NULL, v, errp);
> }
>
> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> - const uint8_t *v, Error **errp)
> + const uint8_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint8_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint8_ptr;
> + }
> +
> + object_class_property_add(klass, name, "uint8",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_property_add_uint16_ptr(Object *obj, const char *name,
> - const uint16_t *v, Error **errp)
> + const uint16_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint16_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint16_ptr;
> + }
> +
> + object_property_add(obj, name, "uint16",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> - const uint16_t *v, Error **errp)
> + const uint16_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint16_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint16_ptr;
> + }
> +
> + object_class_property_add(klass, name, "uint16",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_property_add_uint32_ptr(Object *obj, const char *name,
> - const uint32_t *v, Error **errp)
> + const uint32_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint32_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint32_ptr;
> + }
> +
> + object_property_add(obj, name, "uint32",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> - const uint32_t *v, Error **errp)
> + const uint32_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint32_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint32_ptr;
> + }
> +
> + object_class_property_add(klass, name, "uint32",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_property_add_uint64_ptr(Object *obj, const char *name,
> - const uint64_t *v, Error **errp)
> + const uint64_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint64_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint64_ptr;
> + }
> +
> + object_property_add(obj, name, "uint64",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> - const uint64_t *v, Error **errp)
> + const uint64_t *v,
> + ObjectPropertyFlags flags,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> - NULL, NULL, (void *)v, errp);
> + ObjectPropertyAccessor *getter = NULL;
> + ObjectPropertyAccessor *setter = NULL;
> +
> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
> + getter = property_get_uint64_ptr;
> + }
> +
> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
> + setter = property_set_uint64_ptr;
> + }
> +
> + object_class_property_add(klass, name, "uint64",
> + getter, setter, NULL, (void *)v, errp);
> }
>
> typedef struct {
> diff --git a/ui/console.c b/ui/console.c
> index 82d1ddac9c..bcbe65e696 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG,
> &error_abort);
> - object_property_add_uint32_ptr(obj, "head",
> - &s->head, &error_abort);
> + object_property_add_uint32_ptr(obj, "head", &s->head,
> + OBJ_PROP_FLAG_READ, &error_abort);
>
> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
> (console_type == GRAPHIC_CONSOLE))) {
> --
> 2.20.1
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/4] qom/object: enable setter for uint types
2019-11-30 7:50 ` Marc-André Lureau
@ 2019-12-02 15:06 ` Felipe Franciosi
0 siblings, 0 replies; 16+ messages in thread
From: Felipe Franciosi @ 2019-12-02 15:06 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Stefan Hajnoczi, Philippe Mathieu-Daude
Heya
> On Nov 30, 2019, at 7:50 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>>
>> Traditionally, the uint-specific property helpers only offer getters.
>> When adding object (or class) uint types, one must therefore use the
>> generic property helper if a setter is needed (and probably duplicate
>> some code writing their own getters/setters).
>>
>> This enhances the uint-specific property helper APIs by adding a
>> bitwise-or'd 'flags' field and modifying all clients of that API to set
>> this paramater to OBJ_PROP_FLAG_READ. This maintains the current
>> behaviour whilst allowing others to also set OBJ_PROP_FLAG_WRITE (or use
>> the more convenient OBJ_PROP_FLAG_READWRITE) in the future (which will
>> automatically install a setter). Other flags may be added later.
>>
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> hw/acpi/ich9.c | 4 +-
>> hw/acpi/pcihp.c | 7 +-
>> hw/acpi/piix4.c | 12 +--
>> hw/isa/lpc_ich9.c | 4 +-
>> hw/ppc/spapr_drc.c | 3 +-
>> include/qom/object.h | 44 +++++++--
>> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++-----
>> ui/console.c | 4 +-
>> 8 files changed, 246 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 2034dd749e..742fb78226 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -454,12 +454,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>> pm->s4_val = 2;
>>
>> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>> - &pm->pm_io_base, errp);
>> + &pm->pm_io_base, OBJ_PROP_FLAG_READ, errp);
>> object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>> ich9_pm_get_gpe0_blk,
>> NULL, NULL, pm, NULL);
>> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
>> - &gpe0_len, errp);
>> + &gpe0_len, OBJ_PROP_FLAG_READ, errp);
>> object_property_add_bool(obj, "memory-hotplug-support",
>> ich9_pm_get_memory_hotplug_support,
>> ich9_pm_set_memory_hotplug_support,
>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>> index 8413348a33..4dcef372bf 100644
>> --- a/hw/acpi/pcihp.c
>> +++ b/hw/acpi/pcihp.c
>> @@ -80,7 +80,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>>
>> *bus_bsel = (*bsel_alloc)++;
>> object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
>> - bus_bsel, &error_abort);
>> + bus_bsel, OBJ_PROP_FLAG_READ,
>> + &error_abort);
>> }
>>
>> return bsel_alloc;
>> @@ -373,9 +374,9 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>> memory_region_add_subregion(address_space_io, s->io_base, &s->io);
>>
>> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
>> - &error_abort);
>> + OBJ_PROP_FLAG_READ, &error_abort);
>> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
>> - &error_abort);
>> + OBJ_PROP_FLAG_READ, &error_abort);
>> }
>>
>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 93aec2dd2c..fe05a3ce46 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -443,17 +443,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
>> static const uint16_t sci_int = 9;
>>
>> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>> - &acpi_enable_cmd, NULL);
>> + &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> - &acpi_disable_cmd, NULL);
>> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>> - &gpe0_blk, NULL);
>> + &gpe0_blk, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>> - &gpe0_blk_len, NULL);
>> + &gpe0_blk_len, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>> - &sci_int, NULL);
>> + &sci_int, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>> - &s->io_base, NULL);
>> + &s->io_base, OBJ_PROP_FLAG_READ, NULL);
>> }
>>
>> static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 17c292e306..99517c3946 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -645,9 +645,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>> ich9_lpc_get_sci_int,
>> NULL, NULL, NULL, NULL);
>> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
>> - &acpi_enable_cmd, NULL);
>> + &acpi_enable_cmd, OBJ_PROP_FLAG_READ, NULL);
>> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> - &acpi_disable_cmd, NULL);
>> + &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
>>
>> ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
>> }
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 62f1a42592..bbd4bf35c7 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -553,7 +553,8 @@ static void spapr_dr_connector_instance_init(Object *obj)
>> SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
>> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>>
>> - object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
>> + object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_READ,
>> + NULL);
>> object_property_add(obj, "index", "uint32", prop_get_index,
>> NULL, NULL, NULL, NULL);
>> object_property_add(obj, "fdt", "struct", prop_get_fdt,
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 128d00c77f..d2bfd76741 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1579,65 +1579,93 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>> void (*get)(Object *, struct tm *, Error **),
>> Error **errp);
>>
>> +typedef enum {
>> + /* Automatically add a getter to the property */
>> + OBJ_PROP_FLAG_READ = 1 << 0,
>> + /* Automatically add a setter to the property */
>> + OBJ_PROP_FLAG_WRITE = 1 << 1,
>> + /* Automatically add a getter and a setter to the property */
>> + OBJ_PROP_FLAG_READWRITE = (OBJ_PROP_FLAG_READ | OBJ_PROP_FLAG_WRITE),
>> +} ObjectPropertyFlags;
>> +
>> /**
>> * object_property_add_uint8_ptr:
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>> * @errp: if an error occurs, a pointer to an area to store the error
>> *
>> * Add an integer property in memory. This function will add a
>> * property of type 'uint8'.
>> */
>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>> - const uint8_t *v, Error **errp);
>> + const uint8_t *v, ObjectPropertyFlags flags,
>> + Error **errp);
>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>> - const uint8_t *v, Error **errp);
>> + const uint8_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp);
>>
>> /**
>> * object_property_add_uint16_ptr:
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>> * @errp: if an error occurs, a pointer to an area to store the error
>> *
>> * Add an integer property in memory. This function will add a
>> * property of type 'uint16'.
>> */
>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>> - const uint16_t *v, Error **errp);
>> + const uint16_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp);
>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> - const uint16_t *v, Error **errp);
>> + const uint16_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp);
>>
>> /**
>> * object_property_add_uint32_ptr:
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>> * @errp: if an error occurs, a pointer to an area to store the error
>> *
>> * Add an integer property in memory. This function will add a
>> * property of type 'uint32'.
>> */
>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>> - const uint32_t *v, Error **errp);
>> + const uint32_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp);
>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>> - const uint32_t *v, Error **errp);
>> + const uint32_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp);
>>
>> /**
>> * object_property_add_uint64_ptr:
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @flags: bitwise-or'd ObjectPropertyFlags
>> * @errp: if an error occurs, a pointer to an area to store the error
>> *
>> * Add an integer property in memory. This function will add a
>> * property of type 'uint64'.
>> */
>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>> - const uint64_t *v, Error **Errp);
>> + const uint64_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **Errp);
>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>> - const uint64_t *v, Error **Errp);
>> + const uint64_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **Errp);
>>
>> /**
>> * object_property_add_alias:
>> diff --git a/qom/object.c b/qom/object.c
>> index d51b57fba1..77c2682296 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -2326,6 +2326,22 @@ static void property_get_uint8_ptr(Object *obj, Visitor *v, const char *name,
>> visit_type_uint8(v, name, &value, errp);
>> }
>>
>> +static void property_set_uint8_ptr(Object *obj, Visitor *v, const char *name,
>> + void *opaque, Error **errp)
>> +{
>> + uint8_t *field = opaque;
>> + uint8_t value;
>> + Error *local_err = NULL;
>> +
>> + visit_type_uint8(v, name, &value, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + *field = value;
>> +}
>> +
>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2333,6 +2349,22 @@ static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>> visit_type_uint16(v, name, &value, errp);
>> }
>>
>> +static void property_set_uint16_ptr(Object *obj, Visitor *v, const char *name,
>> + void *opaque, Error **errp)
>> +{
>> + uint16_t *field = opaque;
>> + uint16_t value;
>> + Error *local_err = NULL;
>> +
>> + visit_type_uint16(v, name, &value, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + *field = value;
>> +}
>> +
>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2340,6 +2372,22 @@ static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>> visit_type_uint32(v, name, &value, errp);
>> }
>>
>> +static void property_set_uint32_ptr(Object *obj, Visitor *v, const char *name,
>> + void *opaque, Error **errp)
>> +{
>> + uint32_t *field = opaque;
>> + uint32_t value;
>> + Error *local_err = NULL;
>> +
>> + visit_type_uint32(v, name, &value, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + *field = value;
>> +}
>> +
>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2347,60 +2395,180 @@ static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>> visit_type_uint64(v, name, &value, errp);
>> }
>>
>> +static void property_set_uint64_ptr(Object *obj, Visitor *v, const char *name,
>> + void *opaque, Error **errp)
>> +{
>> + uint64_t *field = opaque;
>> + uint64_t value;
>> + Error *local_err = NULL;
>> +
>> + visit_type_uint64(v, name, &value, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + *field = value;
>> +}
>> +
>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>> - const uint8_t *v, Error **errp)
>> + const uint8_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint8_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint8_ptr;
>> + }
>> +
>> + object_property_add(obj, name, "uint8",
>> + getter, setter, NULL, (void *)v, errp);
>
> Or just:
>
> object_property_add(obj, name, "uint8",
> flags & OBJ_PROP_FLAG_READ ? property_get_uint8_ptr : NULL,
> flags & OBJ_PROP_FLAG_WRITE ? property_set_uint8_ptr : NULL,
> NULL, v, errp);
I had that in an earlier patchset and changed it. We briefly discussed
the benefits of actually having "flags" (instead of the original
"readonly"), which allows for more elaborate (yet generic) uint type
getters/setters (like ones that don't accept zero). The expanded model
will let us grow into that cleanly.
F.
>
>> }
>>
>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>> - const uint8_t *v, Error **errp)
>> + const uint8_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint8_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint8_ptr;
>> + }
>> +
>> + object_class_property_add(klass, name, "uint8",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_property_add_uint16_ptr(Object *obj, const char *name,
>> - const uint16_t *v, Error **errp)
>> + const uint16_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint16_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint16_ptr;
>> + }
>> +
>> + object_property_add(obj, name, "uint16",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> - const uint16_t *v, Error **errp)
>> + const uint16_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint16_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint16_ptr;
>> + }
>> +
>> + object_class_property_add(klass, name, "uint16",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_property_add_uint32_ptr(Object *obj, const char *name,
>> - const uint32_t *v, Error **errp)
>> + const uint32_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint32_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint32_ptr;
>> + }
>> +
>> + object_property_add(obj, name, "uint32",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>> - const uint32_t *v, Error **errp)
>> + const uint32_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint32_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint32_ptr;
>> + }
>> +
>> + object_class_property_add(klass, name, "uint32",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_property_add_uint64_ptr(Object *obj, const char *name,
>> - const uint64_t *v, Error **errp)
>> + const uint64_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint64_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint64_ptr;
>> + }
>> +
>> + object_property_add(obj, name, "uint64",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>> - const uint64_t *v, Error **errp)
>> + const uint64_t *v,
>> + ObjectPropertyFlags flags,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + ObjectPropertyAccessor *getter = NULL;
>> + ObjectPropertyAccessor *setter = NULL;
>> +
>> + if ((flags & OBJ_PROP_FLAG_READ) == OBJ_PROP_FLAG_READ) {
>> + getter = property_get_uint64_ptr;
>> + }
>> +
>> + if ((flags & OBJ_PROP_FLAG_WRITE) == OBJ_PROP_FLAG_WRITE) {
>> + setter = property_set_uint64_ptr;
>> + }
>> +
>> + object_class_property_add(klass, name, "uint64",
>> + getter, setter, NULL, (void *)v, errp);
>> }
>>
>> typedef struct {
>> diff --git a/ui/console.c b/ui/console.c
>> index 82d1ddac9c..bcbe65e696 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -1296,8 +1296,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
>> object_property_allow_set_link,
>> OBJ_PROP_LINK_STRONG,
>> &error_abort);
>> - object_property_add_uint32_ptr(obj, "head",
>> - &s->head, &error_abort);
>> + object_property_add_uint32_ptr(obj, "head", &s->head,
>> + OBJ_PROP_FLAG_READ, &error_abort);
>>
>> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>> (console_type == GRAPHIC_CONSOLE))) {
>> --
>> 2.20.1
>>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/4] ich9: fix getter type for sci_int property
2019-11-29 17:46 [PATCH v3 0/4] Improve default object property_add uint helpers Felipe Franciosi
2019-11-29 17:46 ` [PATCH v3 1/4] qom/object: enable setter for uint types Felipe Franciosi
@ 2019-11-29 17:46 ` Felipe Franciosi
2019-11-30 7:51 ` Marc-André Lureau
2019-11-29 17:46 ` [PATCH v3 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Felipe Franciosi @ 2019-11-29 17:46 UTC (permalink / raw)
To: Marc-Andre Lureau, Philippe Mathieu-Daude, Stefan Hajnoczi,
Eduardo Habkost, Markus Armbruster, Alexey Kardashevskiy
Cc: qemu-devel, Felipe Franciosi
When QOM APIs were added to ich9 in 6f1426ab, the getter for sci_int was
written using uint32_t. However, the object property is uint8_t. This
fixes the getter for correctness.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
hw/isa/lpc_ich9.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 99517c3946..9f4e00dddd 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -631,9 +631,7 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
- uint32_t value = lpc->sci_gsi;
-
- visit_type_uint32(v, name, &value, errp);
+ visit_type_uint8(v, name, &lpc->sci_gsi, errp);
}
static void ich9_lpc_add_properties(ICH9LPCState *lpc)
@@ -641,7 +639,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
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(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint32",
+ object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
ich9_lpc_get_sci_int,
NULL, NULL, NULL, NULL);
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/4] ich9: fix getter type for sci_int property
2019-11-29 17:46 ` [PATCH v3 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
@ 2019-11-30 7:51 ` Marc-André Lureau
0 siblings, 0 replies; 16+ messages in thread
From: Marc-André Lureau @ 2019-11-30 7:51 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Stefan Hajnoczi, Philippe Mathieu-Daude
On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> When QOM APIs were added to ich9 in 6f1426ab, the getter for sci_int was
> written using uint32_t. However, the object property is uint8_t. This
> fixes the getter for correctness.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> hw/isa/lpc_ich9.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 99517c3946..9f4e00dddd 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -631,9 +631,7 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> - uint32_t value = lpc->sci_gsi;
> -
> - visit_type_uint32(v, name, &value, errp);
> + visit_type_uint8(v, name, &lpc->sci_gsi, errp);
> }
>
> static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> @@ -641,7 +639,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> 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(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint32",
> + object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
> ich9_lpc_get_sci_int,
> NULL, NULL, NULL, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> --
> 2.20.1
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 3/4] ich9: Simplify ich9_lpc_initfn
2019-11-29 17:46 [PATCH v3 0/4] Improve default object property_add uint helpers Felipe Franciosi
2019-11-29 17:46 ` [PATCH v3 1/4] qom/object: enable setter for uint types Felipe Franciosi
2019-11-29 17:46 ` [PATCH v3 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
@ 2019-11-29 17:46 ` Felipe Franciosi
2019-11-30 7:52 ` Marc-André Lureau
2019-11-29 17:46 ` [PATCH v3 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
2019-12-02 15:00 ` [PATCH v3 0/4] Improve default object property_add " Stefan Hajnoczi
4 siblings, 1 reply; 16+ messages in thread
From: Felipe Franciosi @ 2019-11-29 17:46 UTC (permalink / raw)
To: Marc-Andre Lureau, Philippe Mathieu-Daude, Stefan Hajnoczi,
Eduardo Habkost, Markus Armbruster, Alexey Kardashevskiy
Cc: qemu-devel, Felipe Franciosi
Currently, ich9_lpc_initfn simply serves as a caller to
ich9_lpc_add_properties. This simplifies the code a bit by eliminating
ich9_lpc_add_properties altogether and executing its logic in the parent
object initialiser function.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
hw/isa/lpc_ich9.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 9f4e00dddd..c40bb3c420 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -634,12 +634,14 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
visit_type_uint8(v, name, &lpc->sci_gsi, errp);
}
-static void ich9_lpc_add_properties(ICH9LPCState *lpc)
+static void ich9_lpc_initfn(Object *obj)
{
+ ICH9LPCState *lpc = ICH9_LPC_DEVICE(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(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
+ object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
ich9_lpc_get_sci_int,
NULL, NULL, NULL, NULL);
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
@@ -647,14 +649,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
&acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
- ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
-}
-
-static void ich9_lpc_initfn(Object *obj)
-{
- ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
-
- ich9_lpc_add_properties(lpc);
+ ich9_pm_add_properties(obj, &lpc->pm, NULL);
}
static void ich9_lpc_realize(PCIDevice *d, Error **errp)
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/4] ich9: Simplify ich9_lpc_initfn
2019-11-29 17:46 ` [PATCH v3 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
@ 2019-11-30 7:52 ` Marc-André Lureau
0 siblings, 0 replies; 16+ messages in thread
From: Marc-André Lureau @ 2019-11-30 7:52 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Stefan Hajnoczi, Philippe Mathieu-Daude
On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Currently, ich9_lpc_initfn simply serves as a caller to
> ich9_lpc_add_properties. This simplifies the code a bit by eliminating
> ich9_lpc_add_properties altogether and executing its logic in the parent
> object initialiser function.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/isa/lpc_ich9.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 9f4e00dddd..c40bb3c420 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -634,12 +634,14 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
> visit_type_uint8(v, name, &lpc->sci_gsi, errp);
> }
>
> -static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> +static void ich9_lpc_initfn(Object *obj)
> {
> + ICH9LPCState *lpc = ICH9_LPC_DEVICE(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(OBJECT(lpc), ACPI_PM_PROP_SCI_INT, "uint8",
> + object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
> ich9_lpc_get_sci_int,
> NULL, NULL, NULL, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> @@ -647,14 +649,7 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> &acpi_disable_cmd, OBJ_PROP_FLAG_READ, NULL);
>
> - ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
> -}
> -
> -static void ich9_lpc_initfn(Object *obj)
> -{
> - ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> -
> - ich9_lpc_add_properties(lpc);
> + ich9_pm_add_properties(obj, &lpc->pm, NULL);
> }
>
> static void ich9_lpc_realize(PCIDevice *d, Error **errp)
> --
> 2.20.1
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-11-29 17:46 [PATCH v3 0/4] Improve default object property_add uint helpers Felipe Franciosi
` (2 preceding siblings ...)
2019-11-29 17:46 ` [PATCH v3 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
@ 2019-11-29 17:46 ` Felipe Franciosi
2019-11-30 7:55 ` Marc-André Lureau
2019-12-02 6:31 ` Alexey Kardashevskiy
2019-12-02 15:00 ` [PATCH v3 0/4] Improve default object property_add " Stefan Hajnoczi
4 siblings, 2 replies; 16+ messages in thread
From: Felipe Franciosi @ 2019-11-29 17:46 UTC (permalink / raw)
To: Marc-Andre Lureau, Philippe Mathieu-Daude, Stefan Hajnoczi,
Eduardo Habkost, Markus Armbruster, Alexey Kardashevskiy
Cc: qemu-devel, Felipe Franciosi
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 <felipe@nutanix.com>
---
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);
+
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,
+ 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 */
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-11-29 17:46 ` [PATCH v3 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
@ 2019-11-30 7:55 ` Marc-André Lureau
2019-12-02 6:31 ` Alexey Kardashevskiy
1 sibling, 0 replies; 16+ messages in thread
From: Marc-André Lureau @ 2019-11-30 7:55 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Stefan Hajnoczi, Philippe Mathieu-Daude
On Fri, Nov 29, 2019 at 9:46 PM Felipe Franciosi <felipe@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 <felipe@nutanix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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);
> +
> 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,
> + 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 */
> --
> 2.20.1
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-11-29 17:46 ` [PATCH v3 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
2019-11-30 7:55 ` Marc-André Lureau
@ 2019-12-02 6:31 ` Alexey Kardashevskiy
2019-12-10 12:04 ` Felipe Franciosi
1 sibling, 1 reply; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-12-02 6:31 UTC (permalink / raw)
To: Felipe Franciosi, Marc-Andre Lureau, Philippe Mathieu-Daude,
Stefan Hajnoczi, Eduardo Habkost, Markus Armbruster
Cc: qemu-devel
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 <felipe@nutanix.com>
> ---
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-12-02 6:31 ` Alexey Kardashevskiy
@ 2019-12-10 12:04 ` Felipe Franciosi
2019-12-12 1:05 ` Alexey Kardashevskiy
2019-12-17 22:19 ` Felipe Franciosi
0 siblings, 2 replies; 16+ messages in thread
From: Felipe Franciosi @ 2019-12-10 12:04 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Eduardo Habkost, qemu-devel, Markus Armbruster,
Marc-Andre Lureau, Stefan Hajnoczi, Philippe Mathieu-Daude
Hi
> On Dec 2, 2019, at 6:31 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> 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 <felipe@nutanix.com>
>> ---
>> 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...
Ok.
>
>
>> +
>> 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.
Maybe I just don't understand where this value comes from. It sounds
like you know what you are talking about :) so I'll send a v4 and
leave this method untouched, unless you have a chance to test this and
tell me that it still works.
>
> btw what is this patchset based on? I tried applying it on top of 3 week
> old and today upstream and it failed. Thanks,
I'm not sure why there were problems. I just rebased my branch on top
of latest master (without issues). I pushed it to Github so you can
have a look. Let me know if you want to try it out or if I should just
send a v4 straight away dropping the changes immediately above.
https://github.com/franciozzy/qemu
Thanks!
F.
>
>
>
>
>
>> + 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-12-10 12:04 ` Felipe Franciosi
@ 2019-12-12 1:05 ` Alexey Kardashevskiy
2019-12-17 22:19 ` Felipe Franciosi
1 sibling, 0 replies; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-12-12 1:05 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, qemu-devel, Markus Armbruster,
Marc-Andre Lureau, Stefan Hajnoczi, Philippe Mathieu-Daude
On 10/12/2019 23:04, Felipe Franciosi wrote:
> Hi
>
>> On Dec 2, 2019, at 6:31 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> 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 <felipe@nutanix.com>
>>> ---
>>> 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...
>
> Ok.
>
>>
>>
>>> +
>>> 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.
>
> Maybe I just don't understand where this value comes from. It sounds
> like you know what you are talking about :) so I'll send a v4 and
> leave this method untouched, unless you have a chance to test this and
> tell me that it still works.
This does not work, crashes as:
Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
0x0000000100a20cd8 in property_get_uint64_ptr (obj=0x102557c10,
v=0x101550dd0, name=0x100c87a48 "nvlink2-tgt", opaque=0x40000000000,
errp=0x7fffffffe8e8) at /home/aik/p/qemu/qom/object.c:2394
2394 uint64_t value = *(uint64_t *)opaque;
Again, object_property_add_uint64_ptr normally takes pointers and
cap->tgt is not a pointer, see the removed vfio_pci_nvlink2_get_tgt()
above - opaque is converted to tgt with (uintptr_t), not (uintptr_t*).
>>
>> btw what is this patchset based on? I tried applying it on top of 3 week
>> old and today upstream and it failed. Thanks,
>
> I'm not sure why there were problems. I just rebased my branch on top
> of latest master (without issues). I pushed it to Github so you can
> have a look. Let me know if you want to try it out or if I should just
> send a v4 straight away dropping the changes immediately above.
>
> https://github.com/franciozzy/qemu
The franciozzy/autosetters branch with this on top -
https://github.com/aik/qemu/commit/94c33bb7debf
- works fine. Thanks,
>
> Thanks!
> F.
>
>>
>>
>>
>>
>>
>>> + 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
>
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-12-10 12:04 ` Felipe Franciosi
2019-12-12 1:05 ` Alexey Kardashevskiy
@ 2019-12-17 22:19 ` Felipe Franciosi
2019-12-18 0:19 ` Alexey Kardashevskiy
1 sibling, 1 reply; 16+ messages in thread
From: Felipe Franciosi @ 2019-12-17 22:19 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Eduardo Habkost, qemu-devel, Markus Armbruster,
Marc-Andre Lureau, Stefan Hajnoczi, Philippe Mathieu-Daude
Hi Alexey,
I don't know how, but somehow I didn't receive your reply:
https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02127.html
(I was about to follow up, then I decided to look at the archives to
make sure your response didn't get lost in my client somehow...)
Still not sure of what happened, lol, let's move on. :)
I'm top-posting as I couldn't pull your response in for a proper reply.
You said:
> The franciozzy/autosetters branch with this on top -
> https://github.com/aik/qemu/commit/94c33bb7debf
> - works fine. Thanks,
Your patch basically reverts a part of my commit and then makes the
change Marc-Andre recommended (by dropping the (void *) cast).
Is it ok for me to just drop that part of my patch and send the v4?
You can follow-up on the cast change afterwards.
Thanks,
F.
> On Dec 10, 2019, at 1:04 PM, Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Hi
>
>> On Dec 2, 2019, at 6:31 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> 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 <felipe@nutanix.com>
>>> ---
>>> 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...
>
> Ok.
>
>>
>>
>>> +
>>> 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.
>
> Maybe I just don't understand where this value comes from. It sounds
> like you know what you are talking about :) so I'll send a v4 and
> leave this method untouched, unless you have a chance to test this and
> tell me that it still works.
>
>>
>> btw what is this patchset based on? I tried applying it on top of 3 week
>> old and today upstream and it failed. Thanks,
>
> I'm not sure why there were problems. I just rebased my branch on top
> of latest master (without issues). I pushed it to Github so you can
> have a look. Let me know if you want to try it out or if I should just
> send a v4 straight away dropping the changes immediately above.
>
> https://github.com/franciozzy/qemu
>
> Thanks!
> F.
>
>>
>>
>>
>>
>>
>>> + 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers
2019-12-17 22:19 ` Felipe Franciosi
@ 2019-12-18 0:19 ` Alexey Kardashevskiy
0 siblings, 0 replies; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-12-18 0:19 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, qemu-devel, Markus Armbruster,
Marc-Andre Lureau, Stefan Hajnoczi, Philippe Mathieu-Daude
On 18/12/2019 09:19, Felipe Franciosi wrote:
> Hi Alexey,
>
> I don't know how, but somehow I didn't receive your reply:
> https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02127.html
>
> (I was about to follow up, then I decided to look at the archives to
> make sure your response didn't get lost in my client somehow...)
>
> Still not sure of what happened, lol, let's move on. :)
dunno, I spent time configuring "spf" on ozlabs.ru and gmail to make my
mails go through all possible checks :)
>
> I'm top-posting as I couldn't pull your response in for a proper reply.
>
> You said:
>> The franciozzy/autosetters branch with this on top -
>> https://github.com/aik/qemu/commit/94c33bb7debf
>> - works fine. Thanks,
>
> Your patch basically reverts a part of my commit and then makes the
> change Marc-Andre recommended (by dropping the (void *) cast).
>
> Is it ok for me to just drop that part of my patch and send the v4?
Yes. Thanks,
> You can follow-up on the cast change afterwards.
>
> Thanks,
> F.
>
>
>> On Dec 10, 2019, at 1:04 PM, Felipe Franciosi <felipe@nutanix.com> wrote:
>>
>> Hi
>>
>>> On Dec 2, 2019, at 6:31 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>
>>>
>>> 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 <felipe@nutanix.com>
>>>> ---
>>>> 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...
>>
>> Ok.
>>
>>>
>>>
>>>> +
>>>> 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.
>>
>> Maybe I just don't understand where this value comes from. It sounds
>> like you know what you are talking about :) so I'll send a v4 and
>> leave this method untouched, unless you have a chance to test this and
>> tell me that it still works.
>>
>>>
>>> btw what is this patchset based on? I tried applying it on top of 3 week
>>> old and today upstream and it failed. Thanks,
>>
>> I'm not sure why there were problems. I just rebased my branch on top
>> of latest master (without issues). I pushed it to Github so you can
>> have a look. Let me know if you want to try it out or if I should just
>> send a v4 straight away dropping the changes immediately above.
>>
>> https://github.com/franciozzy/qemu
>>
>> Thanks!
>> F.
>>
>>>
>>>
>>>
>>>
>>>
>>>> + 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
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] Improve default object property_add uint helpers
2019-11-29 17:46 [PATCH v3 0/4] Improve default object property_add uint helpers Felipe Franciosi
` (3 preceding siblings ...)
2019-11-29 17:46 ` [PATCH v3 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
@ 2019-12-02 15:00 ` Stefan Hajnoczi
4 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2019-12-02 15:00 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Eduardo Habkost, Alexey Kardashevskiy, qemu-devel,
Markus Armbruster, Marc-Andre Lureau, Philippe Mathieu-Daude
[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]
On Fri, Nov 29, 2019 at 05:46:47PM +0000, Felipe Franciosi wrote:
> This improves the family of object_property_add_uintXX_ptr helpers by enabling
> a default getter/setter only when desired. To prevent an API behavioural change
> (from clients that already used these helpers and did not want a setter), we
> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1
> enhances the API and modify current users.
>
> While modifying the clients of the API, a couple of improvement opportunities
> were observed in ich9. These were added in separate patches (2 and 3).
>
> Patch 3 cleans up a lot of existing code by moving various objects to the
> enhanced API. Previously, those objects had their own getters/setters that only
> updated the values without further checks. Some of them actually lacked a check
> for setting overflows, which could have resulted in undesired values being set.
> The new default setters include a check for that, not updating the values in
> case of errors (and propagating them). If they did not provide an error
> pointer, then that behaviour was maintained.
>
>
> Felipe Franciosi (4):
> qom/object: enable setter for uint types
> ich9: fix getter type for sci_int property
> ich9: Simplify ich9_lpc_initfn
> qom/object: Use common get/set uint helpers
>
> hw/acpi/ich9.c | 99 ++------------------
> hw/acpi/pcihp.c | 7 +-
> hw/acpi/piix4.c | 12 +--
> hw/isa/lpc_ich9.c | 27 ++----
> hw/misc/edu.c | 13 +--
> hw/pci-host/q35.c | 14 +--
> hw/ppc/spapr.c | 18 +---
> hw/ppc/spapr_drc.c | 3 +-
> hw/vfio/pci-quirks.c | 20 ++--
> include/qom/object.h | 44 +++++++--
> memory.c | 15 +--
> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++-----
> target/arm/cpu.c | 22 +----
> target/i386/sev.c | 106 ++-------------------
> ui/console.c | 4 +-
> 15 files changed, 288 insertions(+), 332 deletions(-)
>
> --
> 2.20.1
>
> Changelog:
> v1->v2:
> - Update sci_int directly instead of using stack variable
> - Defining an enhanced ObjectPropertyFlags instead of just 'readonly'
> - Erroring out directly (instead of using gotos) on default setters
> - Retaining lack of errp passing when it wasn't there
> v2->v3:
> - Rename flags _RD to _READ and _WR to _WRITE
> - Add a convenience _READWRITE flag
> - Drop the usage of UL in the bit flag definitions
Modulo the review comments that have already been raised:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread