* [PATCH 0/4] Improve default object property_add uint helpers
@ 2019-11-25 15:36 Felipe Franciosi
2019-11-25 15:36 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-25 15:36 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
Cc: qemu-devel, Felipe Franciosi
This improves the family of object_property_add_uintXX_ptr helpers by enabling
a default setter when desired. To prevent an API behavioural change (from
clients that already used these helpers and did not want a setter), we add a
"readonly" parameter 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).
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 | 97 +++------------------------
hw/acpi/pcihp.c | 6 +-
hw/acpi/piix4.c | 12 ++--
hw/isa/lpc_ich9.c | 31 +++------
hw/misc/edu.c | 12 +---
hw/pci-host/q35.c | 14 +---
hw/ppc/spapr.c | 17 +----
hw/ppc/spapr_drc.c | 2 +-
hw/vfio/pci-quirks.c | 18 ++---
include/qom/object.h | 28 +++++---
memory.c | 15 +----
qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
target/arm/cpu.c | 21 +-----
target/i386/sev.c | 102 ++---------------------------
ui/console.c | 3 +-
15 files changed, 196 insertions(+), 334 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] qom/object: enable setter for uint types
2019-11-25 15:36 [PATCH 0/4] Improve default object property_add uint helpers Felipe Franciosi
@ 2019-11-25 15:36 ` Felipe Franciosi
2019-11-26 8:40 ` Marc-André Lureau
2019-11-25 15:36 ` [PATCH 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-25 15:36 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
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.
This enhances the uint-specific property helper APIs by adding a
'readonly' field and modifying all users of that API to set this
parameter to true. If 'readonly' is false, though, the helper will add
an automatic setter for the value.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
hw/acpi/ich9.c | 4 +-
hw/acpi/pcihp.c | 6 +-
hw/acpi/piix4.c | 12 ++--
hw/isa/lpc_ich9.c | 4 +-
hw/ppc/spapr_drc.c | 2 +-
include/qom/object.h | 28 +++++---
qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
ui/console.c | 3 +-
8 files changed, 163 insertions(+), 48 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2034dd749e..94dc5147ce 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, true, 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, true, 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..70bc1408e6 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -80,7 +80,7 @@ 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, true, &error_abort);
}
return bsel_alloc;
@@ -373,9 +373,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);
+ true, &error_abort);
object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
- &error_abort);
+ true, &error_abort);
}
const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93aec2dd2c..032ba11e62 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, true, NULL);
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
- &acpi_disable_cmd, NULL);
+ &acpi_disable_cmd, true, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
- &gpe0_blk, NULL);
+ &gpe0_blk, true, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
- &gpe0_blk_len, NULL);
+ &gpe0_blk_len, true, NULL);
object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
- &sci_int, NULL);
+ &sci_int, true, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
- &s->io_base, NULL);
+ &s->io_base, true, 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..5555ce3342 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, true, NULL);
object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
- &acpi_disable_cmd, NULL);
+ &acpi_disable_cmd, true, 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..76f389f56a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -553,7 +553,7 @@ 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, true, 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..8fc28ed0c9 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1584,60 +1584,72 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
* @obj: the object to add a property to
* @name: the name of the property
* @v: pointer to value
+ * @readonly: don't add setter for value
* @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, bool readonly,
+ Error **errp);
void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
- const uint8_t *v, Error **errp);
+ const uint8_t *v, bool readonly,
+ 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
+ * @readonly: don't add setter for value
* @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, bool readonly,
+ Error **errp);
void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
- const uint16_t *v, Error **errp);
+ const uint16_t *v, bool readonly,
+ 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
+ * @readonly: don't add setter for value
* @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, bool readonly,
+ Error **errp);
void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
- const uint32_t *v, Error **errp);
+ const uint32_t *v, bool readonly,
+ 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
+ * @readonly: don't add setter for value
* @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, bool readonly,
+ Error **Errp);
void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
- const uint64_t *v, Error **Errp);
+ const uint64_t *v, bool readonly,
+ Error **Errp);
/**
* object_property_add_alias:
diff --git a/qom/object.c b/qom/object.c
index d51b57fba1..775f702465 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2326,6 +2326,26 @@ 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) {
+ goto err;
+ }
+
+ *field = value;
+
+ return;
+
+err:
+ error_propagate(errp, local_err);
+}
+
static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2333,6 +2353,26 @@ 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) {
+ goto err;
+ }
+
+ *field = value;
+
+ return;
+
+err:
+ error_propagate(errp, local_err);
+}
+
static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2340,6 +2380,26 @@ 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) {
+ goto err;
+ }
+
+ *field = value;
+
+ return;
+
+err:
+ error_propagate(errp, local_err);
+}
+
static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -2347,60 +2407,104 @@ 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) {
+ goto err;
+ }
+
+ *field = value;
+
+ return;
+
+err:
+ error_propagate(errp, local_err);
+}
+
void object_property_add_uint8_ptr(Object *obj, const char *name,
- const uint8_t *v, Error **errp)
+ const uint8_t *v, bool readonly,
+ Error **errp)
{
- object_property_add(obj, name, "uint8", property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
+ object_property_add(obj, name, "uint8",
+ property_get_uint8_ptr,
+ readonly ? NULL : property_set_uint8_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
+ object_class_property_add(klass, name, "uint8",
+ property_get_uint8_ptr,
+ readonly ? NULL : property_set_uint8_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_property_add(obj, name, "uint16", property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
+ object_property_add(obj, name, "uint16",
+ property_get_uint16_ptr,
+ readonly ? NULL : property_set_uint16_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
+ object_class_property_add(klass, name, "uint16",
+ property_get_uint16_ptr,
+ readonly ? NULL : property_set_uint16_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_property_add(obj, name, "uint32", property_get_uint32_ptr,
- NULL, NULL, (void *)v, errp);
+ object_property_add(obj, name, "uint32",
+ property_get_uint32_ptr,
+ readonly ? NULL : property_set_uint32_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
- NULL, NULL, (void *)v, errp);
+ object_class_property_add(klass, name, "uint32",
+ property_get_uint32_ptr,
+ readonly ? NULL : property_set_uint32_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_property_add(obj, name, "uint64", property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
+ object_property_add(obj, name, "uint64",
+ property_get_uint64_ptr,
+ property_set_uint64_ptr,
+ 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, bool readonly,
+ Error **errp)
{
- object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
+ object_class_property_add(klass, name, "uint64",
+ property_get_uint64_ptr,
+ readonly ? NULL : property_set_uint64_ptr,
+ NULL, (void *)v, errp);
}
typedef struct {
diff --git a/ui/console.c b/ui/console.c
index 82d1ddac9c..3375c33d71 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1296,8 +1296,7 @@ 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, true, &error_abort);
if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
(console_type == GRAPHIC_CONSOLE))) {
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] ich9: fix getter type for sci_int property
2019-11-25 15:36 [PATCH 0/4] Improve default object property_add uint helpers Felipe Franciosi
2019-11-25 15:36 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
@ 2019-11-25 15:36 ` Felipe Franciosi
2019-11-25 16:43 ` Philippe Mathieu-Daudé
2019-11-26 8:40 ` Marc-André Lureau
2019-11-25 15:36 ` [PATCH 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
2019-11-25 15:36 ` [PATCH 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
3 siblings, 2 replies; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-25 15:36 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
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, 3 insertions(+), 3 deletions(-)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 5555ce3342..240979885d 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -631,9 +631,9 @@ 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;
+ uint8_t value = lpc->sci_gsi;
- visit_type_uint32(v, name, &value, errp);
+ visit_type_uint8(v, name, &value, errp);
}
static void ich9_lpc_add_properties(ICH9LPCState *lpc)
@@ -641,7 +641,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] 18+ messages in thread
* [PATCH 3/4] ich9: Simplify ich9_lpc_initfn
2019-11-25 15:36 [PATCH 0/4] Improve default object property_add uint helpers Felipe Franciosi
2019-11-25 15:36 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
2019-11-25 15:36 ` [PATCH 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
@ 2019-11-25 15:36 ` Felipe Franciosi
2019-11-26 8:39 ` Marc-André Lureau
2019-11-25 15:36 ` [PATCH 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
3 siblings, 1 reply; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-25 15:36 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
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 | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 240979885d..232c7916f3 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -636,27 +636,22 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
visit_type_uint8(v, name, &value, 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,
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
&acpi_enable_cmd, true, NULL);
- object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
&acpi_disable_cmd, true, 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] 18+ messages in thread
* [PATCH 4/4] qom/object: Use common get/set uint helpers
2019-11-25 15:36 [PATCH 0/4] Improve default object property_add uint helpers Felipe Franciosi
` (2 preceding siblings ...)
2019-11-25 15:36 ` [PATCH 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
@ 2019-11-25 15:36 ` Felipe Franciosi
2019-11-26 8:39 ` Marc-André Lureau
3 siblings, 1 reply; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-25 15:36 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
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 | 93 +++------------------------------------
hw/isa/lpc_ich9.c | 14 +-----
hw/misc/edu.c | 12 +----
hw/pci-host/q35.c | 14 ++----
hw/ppc/spapr.c | 17 ++------
hw/vfio/pci-quirks.c | 18 ++------
memory.c | 15 +------
target/arm/cpu.c | 21 +--------
target/i386/sev.c | 102 +++----------------------------------------
9 files changed, 29 insertions(+), 277 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 94dc5147ce..aa3c7a59ab 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
s->pm.cpu_hotplug_legacy = value;
}
-static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- uint8_t value = pm->disable_s3;
-
- visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- Error *local_err = NULL;
- uint8_t value;
-
- visit_type_uint8(v, name, &value, &local_err);
- if (local_err) {
- goto out;
- }
- pm->disable_s3 = value;
-out:
- error_propagate(errp, local_err);
-}
-
-static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- uint8_t value = pm->disable_s4;
-
- visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- Error *local_err = NULL;
- uint8_t value;
-
- visit_type_uint8(v, name, &value, &local_err);
- if (local_err) {
- goto out;
- }
- pm->disable_s4 = value;
-out:
- error_propagate(errp, local_err);
-}
-
-static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- uint8_t value = pm->s4_val;
-
- visit_type_uint8(v, name, &value, errp);
-}
-
-static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCPMRegs *pm = opaque;
- Error *local_err = NULL;
- uint8_t value;
-
- visit_type_uint8(v, name, &value, &local_err);
- if (local_err) {
- goto out;
- }
- pm->s4_val = value;
-out:
- error_propagate(errp, local_err);
-}
-
static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
{
ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
@@ -468,18 +393,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
ich9_pm_get_cpu_hotplug_legacy,
ich9_pm_set_cpu_hotplug_legacy,
NULL);
- object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
- ich9_pm_get_disable_s3,
- ich9_pm_set_disable_s3,
- NULL, pm, NULL);
- object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
- ich9_pm_get_disable_s4,
- ich9_pm_set_disable_s4,
- NULL, pm, NULL);
- object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
- ich9_pm_get_s4_val,
- ich9_pm_set_s4_val,
- NULL, pm, NULL);
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
+ &pm->disable_s3, false, errp);
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
+ &pm->disable_s4, false, errp);
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
+ &pm->s4_val, false, errp);
object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
ich9_pm_get_enable_tco,
ich9_pm_set_enable_tco,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 232c7916f3..9abe07247e 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -627,15 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
.endianness = DEVICE_LITTLE_ENDIAN
};
-static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
- uint8_t value = lpc->sci_gsi;
-
- visit_type_uint8(v, name, &value, errp);
-}
-
static void ich9_lpc_initfn(Object *obj)
{
ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
@@ -643,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
- object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
- ich9_lpc_get_sci_int,
- NULL, NULL, NULL, NULL);
+ object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
+ &lpc->sci_gsi, true, NULL);
object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
&acpi_enable_cmd, true, NULL);
object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index d5e2bdbb57..200503ecfd 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -396,21 +396,13 @@ static void pci_edu_uninit(PCIDevice *pdev)
msi_uninit(pdev);
}
-static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- uint64_t *val = opaque;
-
- visit_type_uint64(v, name, val, errp);
-}
-
static void edu_instance_init(Object *obj)
{
EduState *edu = EDU(obj);
edu->dma_mask = (1UL << 28) - 1;
- object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
- edu_obj_uint64, NULL, &edu->dma_mask, NULL);
+ object_property_add_uint64_ptr(obj, "dma_mask",
+ &edu->dma_mask, false, NULL);
}
static void edu_class_init(ObjectClass *class, void *data)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270b9f..61fbe04fe9 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
visit_type_uint64(v, name, &value, errp);
}
-static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
-
- visit_type_uint64(v, name, &e->size, errp);
-}
-
/*
* NOTE: setting defaults for the mch.* fields in this table
* doesn't work, because mch is a separate QOM object that is
@@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
{
Q35PCIHost *s = Q35_HOST_DEVICE(obj);
PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+ PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
"pci-conf-idx", 4);
@@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
q35_host_get_pci_hole64_end,
NULL, NULL, NULL, NULL);
- object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
- q35_host_get_mmcfg_size,
- NULL, NULL, NULL, NULL);
+ object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
+ &pehb->size, true, NULL);
object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
(Object **) &s->mch.ram_memory,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f6023c..1b9400526f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
}
}
-static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- visit_type_uint32(v, name, (uint32_t *)opaque, errp);
-}
-
-static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- visit_type_uint32(v, name, (uint32_t *)opaque, errp);
-}
-
static char *spapr_get_ic_mode(Object *obj, Error **errp)
{
SpaprMachineState *spapr = SPAPR_MACHINE(obj);
@@ -3336,8 +3324,9 @@ static void spapr_instance_init(Object *obj)
object_property_set_description(obj, "resize-hpt",
"Resizing of the Hash Page Table (enabled, disabled, required)",
NULL);
- object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
- spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
+ object_property_add_uint32_ptr(obj, "vsmt",
+ &spapr->vsmt, false, &error_abort);
+
object_property_set_description(obj, "vsmt",
"Virtual SMT: KVM behaves as if this were"
" the host's SMT mode", &error_abort);
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 136f3a9ad6..34335e071e 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
return 0;
}
-static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
- const char *name,
- void *opaque, Error **errp)
-{
- uint64_t tgt = (uintptr_t) opaque;
- visit_type_uint64(v, name, &tgt, errp);
-}
-
static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
const char *name,
void *opaque, Error **errp)
@@ -2240,9 +2232,8 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
nv2reg->size, p);
QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
- object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
- vfio_pci_nvlink2_get_tgt, NULL, NULL,
- (void *) (uintptr_t) cap->tgt, NULL);
+ object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
+ (void *)(uintptr_t)cap->tgt, true, NULL);
trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
nv2reg->size);
free_exit:
@@ -2301,9 +2292,8 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
}
- object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
- vfio_pci_nvlink2_get_tgt, NULL, NULL,
- (void *) (uintptr_t) captgt->tgt, NULL);
+ object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
+ (void *)(uintptr_t)captgt->tgt, true, NULL);
trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
atsdreg->size);
diff --git a/memory.c b/memory.c
index 06484c2bff..0a34ee3c86 100644
--- a/memory.c
+++ b/memory.c
@@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
memory_region_do_init(mr, owner, name, size);
}
-static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- MemoryRegion *mr = MEMORY_REGION(obj);
- uint64_t value = mr->addr;
-
- visit_type_uint64(v, name, &value, errp);
-}
-
static void memory_region_get_container(Object *obj, Visitor *v,
const char *name, void *opaque,
Error **errp)
@@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
NULL, NULL, &error_abort);
op->resolve = memory_region_resolve_container;
- object_property_add(OBJECT(mr), "addr", "uint64",
- memory_region_get_addr,
- NULL, /* memory_region_set_addr */
- NULL, NULL, &error_abort);
+ object_property_add_uint64_ptr(OBJECT(mr), "addr",
+ &mr->addr, true, &error_abort);
object_property_add(OBJECT(mr), "priority", "uint32",
memory_region_get_priority,
NULL, /* memory_region_set_priority */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a4ac9339b..aa7278e540 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
cpu->has_pmu = value;
}
-static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ARMCPU *cpu = ARM_CPU(obj);
-
- visit_type_uint32(v, name, &cpu->init_svtor, errp);
-}
-
-static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- ARMCPU *cpu = ARM_CPU(obj);
-
- visit_type_uint32(v, name, &cpu->init_svtor, errp);
-}
-
void arm_cpu_post_init(Object *obj)
{
ARMCPU *cpu = ARM_CPU(obj);
@@ -1165,9 +1149,8 @@ void arm_cpu_post_init(Object *obj)
* a simple DEFINE_PROP_UINT32 for this because we want to permit
* the property to be set after realize.
*/
- object_property_add(obj, "init-svtor", "uint32",
- arm_get_init_svtor, arm_set_init_svtor,
- NULL, NULL, &error_abort);
+ object_property_add_uint32_ptr(obj, "init-svtor",
+ &cpu->init_svtor, false, &error_abort);
}
qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 024bb24e51..23d7ab8b72 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
"guest owners session parameters (encoded with base64)", NULL);
}
-static void
-qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
- uint32_t value;
-
- visit_type_uint32(v, name, &value, errp);
- sev->handle = value;
-}
-
-static void
-qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
- uint32_t value;
-
- visit_type_uint32(v, name, &value, errp);
- sev->policy = value;
-}
-
-static void
-qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
- uint32_t value;
-
- visit_type_uint32(v, name, &value, errp);
- sev->cbitpos = value;
-}
-
-static void
-qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
- uint32_t value;
-
- visit_type_uint32(v, name, &value, errp);
- sev->reduced_phys_bits = value;
-}
-
-static void
-qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- uint32_t value;
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
- value = sev->policy;
- visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- uint32_t value;
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
- value = sev->handle;
- visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- uint32_t value;
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
- value = sev->cbitpos;
- visit_type_uint32(v, name, &value, errp);
-}
-
-static void
-qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- uint32_t value;
- QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
-
- value = sev->reduced_phys_bits;
- visit_type_uint32(v, name, &value, errp);
-}
-
static void
qsev_guest_init(Object *obj)
{
@@ -361,15 +273,11 @@ qsev_guest_init(Object *obj)
sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
sev->policy = DEFAULT_GUEST_POLICY;
- object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
- qsev_guest_set_policy, NULL, NULL, NULL);
- object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
- qsev_guest_set_handle, NULL, NULL, NULL);
- object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
- qsev_guest_set_cbitpos, NULL, NULL, NULL);
- object_property_add(obj, "reduced-phys-bits", "uint32",
- qsev_guest_get_reduced_phys_bits,
- qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
+ object_property_add_uint32_ptr(obj, "policy", &sev->policy, false, NULL);
+ object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, NULL);
+ object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, NULL);
+ object_property_add_uint32_ptr(obj, "reduced-phys-bits",
+ &sev->reduced_phys_bits, false, NULL);
}
/* sev guest info */
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] ich9: fix getter type for sci_int property
2019-11-25 15:36 ` [PATCH 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
@ 2019-11-25 16:43 ` Philippe Mathieu-Daudé
2019-11-26 9:55 ` Felipe Franciosi
2019-11-26 8:40 ` Marc-André Lureau
1 sibling, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-25 16:43 UTC (permalink / raw)
To: Felipe Franciosi, Eduardo Habkost, Markus Armbruster, Stefan Hajnoczi
Cc: qemu-devel
On 11/25/19 4:36 PM, Felipe Franciosi 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>
> ---
> hw/isa/lpc_ich9.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 5555ce3342..240979885d 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -631,9 +631,9 @@ 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;
> + uint8_t value = lpc->sci_gsi;
>
> - visit_type_uint32(v, name, &value, errp);
> + visit_type_uint8(v, name, &value, errp);
Maybe directly as:
visit_type_uint8(v, name, &lpc->sci_gsi, errp);
With/without stack variable:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> }
>
> static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> @@ -641,7 +641,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,
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qom/object: Use common get/set uint helpers
2019-11-25 15:36 ` [PATCH 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
@ 2019-11-26 8:39 ` Marc-André Lureau
2019-11-26 9:39 ` Felipe Franciosi
0 siblings, 1 reply; 18+ messages in thread
From: Marc-André Lureau @ 2019-11-26 8:39 UTC (permalink / raw)
To: Felipe Franciosi
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
Hi
On Mon, Nov 25, 2019 at 7:37 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>
Some comments below, otherwise:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/acpi/ich9.c | 93 +++------------------------------------
> hw/isa/lpc_ich9.c | 14 +-----
> hw/misc/edu.c | 12 +----
> hw/pci-host/q35.c | 14 ++----
> hw/ppc/spapr.c | 17 ++------
> hw/vfio/pci-quirks.c | 18 ++------
> memory.c | 15 +------
> target/arm/cpu.c | 21 +--------
> target/i386/sev.c | 102 +++----------------------------------------
> 9 files changed, 29 insertions(+), 277 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 94dc5147ce..aa3c7a59ab 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
> s->pm.cpu_hotplug_legacy = value;
> }
>
> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - uint8_t value = pm->disable_s3;
> -
> - visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - Error *local_err = NULL;
> - uint8_t value;
> -
> - visit_type_uint8(v, name, &value, &local_err);
> - if (local_err) {
> - goto out;
> - }
> - pm->disable_s3 = value;
> -out:
> - error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - uint8_t value = pm->disable_s4;
> -
> - visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - Error *local_err = NULL;
> - uint8_t value;
> -
> - visit_type_uint8(v, name, &value, &local_err);
> - if (local_err) {
> - goto out;
> - }
> - pm->disable_s4 = value;
> -out:
> - error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - uint8_t value = pm->s4_val;
> -
> - visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCPMRegs *pm = opaque;
> - Error *local_err = NULL;
> - uint8_t value;
> -
> - visit_type_uint8(v, name, &value, &local_err);
> - if (local_err) {
> - goto out;
> - }
> - pm->s4_val = value;
> -out:
> - error_propagate(errp, local_err);
> -}
> -
> static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
> {
> ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> @@ -468,18 +393,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> ich9_pm_get_cpu_hotplug_legacy,
> ich9_pm_set_cpu_hotplug_legacy,
> NULL);
> - object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> - ich9_pm_get_disable_s3,
> - ich9_pm_set_disable_s3,
> - NULL, pm, NULL);
> - object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
> - ich9_pm_get_disable_s4,
> - ich9_pm_set_disable_s4,
> - NULL, pm, NULL);
> - object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
> - ich9_pm_get_s4_val,
> - ich9_pm_set_s4_val,
> - NULL, pm, NULL);
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
> + &pm->disable_s3, false, errp);
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
> + &pm->disable_s4, false, errp);
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
> + &pm->s4_val, false, errp);
Sometime object properties are registered with error_abort, sometime
with errp, sometime with NULL.
Here you changed the argument. Not a big deal, but I think you should
leave it as is for now. And we can address the error handling
inconsisteny another day.
> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> ich9_pm_get_enable_tco,
> ich9_pm_set_enable_tco,
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 232c7916f3..9abe07247e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -627,15 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN
> };
>
> -static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> - uint8_t value = lpc->sci_gsi;
> -
> - visit_type_uint8(v, name, &value, errp);
> -}
> -
> static void ich9_lpc_initfn(Object *obj)
> {
> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
> @@ -643,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>
> - object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
> - ich9_lpc_get_sci_int,
> - NULL, NULL, NULL, NULL);
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
> + &lpc->sci_gsi, true, NULL);
> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
> &acpi_enable_cmd, true, NULL);
> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index d5e2bdbb57..200503ecfd 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -396,21 +396,13 @@ static void pci_edu_uninit(PCIDevice *pdev)
> msi_uninit(pdev);
> }
>
> -static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - uint64_t *val = opaque;
> -
> - visit_type_uint64(v, name, val, errp);
> -}
> -
> static void edu_instance_init(Object *obj)
> {
> EduState *edu = EDU(obj);
>
> edu->dma_mask = (1UL << 28) - 1;
> - object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
> - edu_obj_uint64, NULL, &edu->dma_mask, NULL);
> + object_property_add_uint64_ptr(obj, "dma_mask",
> + &edu->dma_mask, false, NULL);
> }
>
> static void edu_class_init(ObjectClass *class, void *data)
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 158d270b9f..61fbe04fe9 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
> visit_type_uint64(v, name, &value, errp);
> }
>
> -static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
> -
> - visit_type_uint64(v, name, &e->size, errp);
> -}
> -
> /*
> * NOTE: setting defaults for the mch.* fields in this table
> * doesn't work, because mch is a separate QOM object that is
> @@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
> {
> Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> + PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
>
> memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> "pci-conf-idx", 4);
> @@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
> q35_host_get_pci_hole64_end,
> NULL, NULL, NULL, NULL);
>
> - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
> - q35_host_get_mmcfg_size,
> - NULL, NULL, NULL, NULL);
> + object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
> + &pehb->size, true, NULL);
>
> object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
> (Object **) &s->mch.ram_memory,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..1b9400526f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
> }
> }
>
> -static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
> -static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> -}
> -
> static char *spapr_get_ic_mode(Object *obj, Error **errp)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -3336,8 +3324,9 @@ static void spapr_instance_init(Object *obj)
> object_property_set_description(obj, "resize-hpt",
> "Resizing of the Hash Page Table (enabled, disabled, required)",
> NULL);
> - object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
> - spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
> + object_property_add_uint32_ptr(obj, "vsmt",
> + &spapr->vsmt, false, &error_abort);
> +
> object_property_set_description(obj, "vsmt",
> "Virtual SMT: KVM behaves as if this were"
> " the host's SMT mode", &error_abort);
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 136f3a9ad6..34335e071e 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
> return 0;
> }
>
> -static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
> - const char *name,
> - void *opaque, Error **errp)
> -{
> - uint64_t tgt = (uintptr_t) opaque;
> - visit_type_uint64(v, name, &tgt, errp);
> -}
> -
> static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
> const char *name,
> void *opaque, Error **errp)
> @@ -2240,9 +2232,8 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
> nv2reg->size, p);
> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>
> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
> - (void *) (uintptr_t) cap->tgt, NULL);
> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
> + (void *)(uintptr_t)cap->tgt, true, NULL);
nit: (void *) is enough, no?
> trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
> nv2reg->size);
> free_exit:
> @@ -2301,9 +2292,8 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
> }
>
> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
> - (void *) (uintptr_t) captgt->tgt, NULL);
> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
> + (void *)(uintptr_t)captgt->tgt, true, NULL);
same
> trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
> atsdreg->size);
>
> diff --git a/memory.c b/memory.c
> index 06484c2bff..0a34ee3c86 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
> memory_region_do_init(mr, owner, name, size);
> }
>
> -static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - MemoryRegion *mr = MEMORY_REGION(obj);
> - uint64_t value = mr->addr;
> -
> - visit_type_uint64(v, name, &value, errp);
> -}
> -
> static void memory_region_get_container(Object *obj, Visitor *v,
> const char *name, void *opaque,
> Error **errp)
> @@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
> NULL, NULL, &error_abort);
> op->resolve = memory_region_resolve_container;
>
> - object_property_add(OBJECT(mr), "addr", "uint64",
> - memory_region_get_addr,
> - NULL, /* memory_region_set_addr */
> - NULL, NULL, &error_abort);
> + object_property_add_uint64_ptr(OBJECT(mr), "addr",
> + &mr->addr, true, &error_abort);
> object_property_add(OBJECT(mr), "priority", "uint32",
> memory_region_get_priority,
> NULL, /* memory_region_set_priority */
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 7a4ac9339b..aa7278e540 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
> cpu->has_pmu = value;
> }
>
> -static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ARMCPU *cpu = ARM_CPU(obj);
> -
> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
> -}
> -
> -static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - ARMCPU *cpu = ARM_CPU(obj);
> -
> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
> -}
> -
> void arm_cpu_post_init(Object *obj)
> {
> ARMCPU *cpu = ARM_CPU(obj);
> @@ -1165,9 +1149,8 @@ void arm_cpu_post_init(Object *obj)
> * a simple DEFINE_PROP_UINT32 for this because we want to permit
> * the property to be set after realize.
> */
> - object_property_add(obj, "init-svtor", "uint32",
> - arm_get_init_svtor, arm_set_init_svtor,
> - NULL, NULL, &error_abort);
> + object_property_add_uint32_ptr(obj, "init-svtor",
> + &cpu->init_svtor, false, &error_abort);
> }
>
> qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 024bb24e51..23d7ab8b72 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
> "guest owners session parameters (encoded with base64)", NULL);
> }
>
> -static void
> -qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> - uint32_t value;
> -
> - visit_type_uint32(v, name, &value, errp);
> - sev->handle = value;
> -}
> -
> -static void
> -qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> - uint32_t value;
> -
> - visit_type_uint32(v, name, &value, errp);
> - sev->policy = value;
> -}
> -
> -static void
> -qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> - uint32_t value;
> -
> - visit_type_uint32(v, name, &value, errp);
> - sev->cbitpos = value;
> -}
> -
> -static void
> -qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> - uint32_t value;
> -
> - visit_type_uint32(v, name, &value, errp);
> - sev->reduced_phys_bits = value;
> -}
> -
> -static void
> -qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - uint32_t value;
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> - value = sev->policy;
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - uint32_t value;
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> - value = sev->handle;
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - uint32_t value;
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> - value = sev->cbitpos;
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void
> -qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - uint32_t value;
> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
> -
> - value = sev->reduced_phys_bits;
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> static void
> qsev_guest_init(Object *obj)
> {
> @@ -361,15 +273,11 @@ qsev_guest_init(Object *obj)
>
> sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
> sev->policy = DEFAULT_GUEST_POLICY;
> - object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
> - qsev_guest_set_policy, NULL, NULL, NULL);
> - object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
> - qsev_guest_set_handle, NULL, NULL, NULL);
> - object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
> - qsev_guest_set_cbitpos, NULL, NULL, NULL);
> - object_property_add(obj, "reduced-phys-bits", "uint32",
> - qsev_guest_get_reduced_phys_bits,
> - qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
> + object_property_add_uint32_ptr(obj, "policy", &sev->policy, false, NULL);
> + object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, NULL);
> + object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, NULL);
> + object_property_add_uint32_ptr(obj, "reduced-phys-bits",
> + &sev->reduced_phys_bits, false, NULL);
> }
>
> /* sev guest info */
> --
> 2.20.1
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] ich9: Simplify ich9_lpc_initfn
2019-11-25 15:36 ` [PATCH 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
@ 2019-11-26 8:39 ` Marc-André Lureau
0 siblings, 0 replies; 18+ messages in thread
From: Marc-André Lureau @ 2019-11-26 8:39 UTC (permalink / raw)
To: Felipe Franciosi
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
On Mon, Nov 25, 2019 at 7:37 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>
yep, /me like simpler code,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/isa/lpc_ich9.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 240979885d..232c7916f3 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -636,27 +636,22 @@ static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
> visit_type_uint8(v, name, &value, 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,
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
> &acpi_enable_cmd, true, NULL);
> - object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
> &acpi_disable_cmd, true, 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] 18+ messages in thread
* Re: [PATCH 2/4] ich9: fix getter type for sci_int property
2019-11-25 15:36 ` [PATCH 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
2019-11-25 16:43 ` Philippe Mathieu-Daudé
@ 2019-11-26 8:40 ` Marc-André Lureau
1 sibling, 0 replies; 18+ messages in thread
From: Marc-André Lureau @ 2019-11-26 8:40 UTC (permalink / raw)
To: Felipe Franciosi
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
On Mon, Nov 25, 2019 at 7:37 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>
Good catch! (I have a few like that in a series pending. This one I didn't spot)
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
thanks
> ---
> hw/isa/lpc_ich9.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 5555ce3342..240979885d 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -631,9 +631,9 @@ 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;
> + uint8_t value = lpc->sci_gsi;
>
> - visit_type_uint32(v, name, &value, errp);
> + visit_type_uint8(v, name, &value, errp);
> }
>
> static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> @@ -641,7 +641,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] 18+ messages in thread
* Re: [PATCH 1/4] qom/object: enable setter for uint types
2019-11-25 15:36 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
@ 2019-11-26 8:40 ` Marc-André Lureau
2019-11-26 12:03 ` Felipe Franciosi
0 siblings, 1 reply; 18+ messages in thread
From: Marc-André Lureau @ 2019-11-26 8:40 UTC (permalink / raw)
To: Felipe Franciosi
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
Hi
On Mon, Nov 25, 2019 at 7:40 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.
>
> This enhances the uint-specific property helper APIs by adding a
> 'readonly' field and modifying all users of that API to set this
> parameter to true. If 'readonly' is false, though, the helper will add
> an automatic setter for the value.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
> hw/acpi/ich9.c | 4 +-
> hw/acpi/pcihp.c | 6 +-
> hw/acpi/piix4.c | 12 ++--
> hw/isa/lpc_ich9.c | 4 +-
> hw/ppc/spapr_drc.c | 2 +-
> include/qom/object.h | 28 +++++---
> qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
> ui/console.c | 3 +-
> 8 files changed, 163 insertions(+), 48 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2034dd749e..94dc5147ce 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, true, 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, true, 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..70bc1408e6 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -80,7 +80,7 @@ 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, true, &error_abort);
> }
>
> return bsel_alloc;
> @@ -373,9 +373,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);
> + true, &error_abort);
> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> - &error_abort);
> + true, &error_abort);
> }
>
> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 93aec2dd2c..032ba11e62 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, true, NULL);
> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, true, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> - &gpe0_blk, NULL);
> + &gpe0_blk, true, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> - &gpe0_blk_len, NULL);
> + &gpe0_blk_len, true, NULL);
> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> - &sci_int, NULL);
> + &sci_int, true, NULL);
> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> - &s->io_base, NULL);
> + &s->io_base, true, 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..5555ce3342 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, true, NULL);
> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - &acpi_disable_cmd, NULL);
> + &acpi_disable_cmd, true, 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..76f389f56a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -553,7 +553,7 @@ 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, true, 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..8fc28ed0c9 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1584,60 +1584,72 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @readonly: don't add setter for value
> * @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, bool readonly,
> + Error **errp);
> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> - const uint8_t *v, Error **errp);
> + const uint8_t *v, bool readonly,
> + Error **errp);
It would help readability and extensibility if we had an enum/bitflag:
OBJECT_PROP_WRITABLE, OBJECT_PROP_READABLE (& OBJECT_PROP_READWRITE
alias).
>
> /**
> * object_property_add_uint16_ptr:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @v: pointer to value
> + * @readonly: don't add setter for value
> * @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, bool readonly,
> + Error **errp);
> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> - const uint16_t *v, Error **errp);
> + const uint16_t *v, bool readonly,
> + 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
> + * @readonly: don't add setter for value
> * @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, bool readonly,
> + Error **errp);
> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> - const uint32_t *v, Error **errp);
> + const uint32_t *v, bool readonly,
> + 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
> + * @readonly: don't add setter for value
> * @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, bool readonly,
> + Error **Errp);
> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> - const uint64_t *v, Error **Errp);
> + const uint64_t *v, bool readonly,
> + Error **Errp);
>
> /**
> * object_property_add_alias:
> diff --git a/qom/object.c b/qom/object.c
> index d51b57fba1..775f702465 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2326,6 +2326,26 @@ 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) {
> + goto err;
> + }
> +
> + *field = value;
> +
> + return;
> +
> +err:
> + error_propagate(errp, local_err);
> +}
This is probably going to be refactored once the auto error series
land. But in the meantime, it would be nice to remove the goto label.
> +
> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2333,6 +2353,26 @@ 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) {
> + goto err;
> + }
> +
> + *field = value;
> +
> + return;
> +
> +err:
> + error_propagate(errp, local_err);
> +}
> +
same
> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2340,6 +2380,26 @@ 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) {
> + goto err;
> + }
> +
> + *field = value;
> +
> + return;
> +
> +err:
> + error_propagate(errp, local_err);
> +}
etc
> +
> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -2347,60 +2407,104 @@ 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) {
> + goto err;
> + }
> +
> + *field = value;
> +
> + return;
> +
> +err:
> + error_propagate(errp, local_err);
> +}
> +
> void object_property_add_uint8_ptr(Object *obj, const char *name,
> - const uint8_t *v, Error **errp)
> + const uint8_t *v, bool readonly,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_property_add(obj, name, "uint8",
> + property_get_uint8_ptr,
> + readonly ? NULL : property_set_uint8_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_class_property_add(klass, name, "uint8",
> + property_get_uint8_ptr,
> + readonly ? NULL : property_set_uint8_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_property_add(obj, name, "uint16",
> + property_get_uint16_ptr,
> + readonly ? NULL : property_set_uint16_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_class_property_add(klass, name, "uint16",
> + property_get_uint16_ptr,
> + readonly ? NULL : property_set_uint16_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_property_add(obj, name, "uint32",
> + property_get_uint32_ptr,
> + readonly ? NULL : property_set_uint32_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_class_property_add(klass, name, "uint32",
> + property_get_uint32_ptr,
> + readonly ? NULL : property_set_uint32_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_property_add(obj, name, "uint64",
> + property_get_uint64_ptr,
> + property_set_uint64_ptr,
> + 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, bool readonly,
> + Error **errp)
> {
> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> - NULL, NULL, (void *)v, errp);
> + object_class_property_add(klass, name, "uint64",
> + property_get_uint64_ptr,
> + readonly ? NULL : property_set_uint64_ptr,
> + NULL, (void *)v, errp);
> }
>
> typedef struct {
> diff --git a/ui/console.c b/ui/console.c
> index 82d1ddac9c..3375c33d71 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1296,8 +1296,7 @@ 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, true, &error_abort);
>
> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
> (console_type == GRAPHIC_CONSOLE))) {
> --
> 2.20.1
>
looks good otherwise
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qom/object: Use common get/set uint helpers
2019-11-26 8:39 ` Marc-André Lureau
@ 2019-11-26 9:39 ` Felipe Franciosi
2019-11-27 23:58 ` Alexey Kardashevskiy
0 siblings, 1 reply; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-26 9:39 UTC (permalink / raw)
To: Marc-André Lureau, Alex Williamson, Alexey Kardashevskiy
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
> On Nov 26, 2019, at 8:39 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
Heya, thanks for the review.
>
> On Mon, Nov 25, 2019 at 7:37 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>
>
>
> Some comments below, otherwise:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> ---
>> hw/acpi/ich9.c | 93 +++------------------------------------
>> hw/isa/lpc_ich9.c | 14 +-----
>> hw/misc/edu.c | 12 +----
>> hw/pci-host/q35.c | 14 ++----
>> hw/ppc/spapr.c | 17 ++------
>> hw/vfio/pci-quirks.c | 18 ++------
>> memory.c | 15 +------
>> target/arm/cpu.c | 21 +--------
>> target/i386/sev.c | 102 +++----------------------------------------
>> 9 files changed, 29 insertions(+), 277 deletions(-)
>>
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 94dc5147ce..aa3c7a59ab 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
>> s->pm.cpu_hotplug_legacy = value;
>> }
>>
>> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - uint8_t value = pm->disable_s3;
>> -
>> - visit_type_uint8(v, name, &value, errp);
>> -}
>> -
>> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - Error *local_err = NULL;
>> - uint8_t value;
>> -
>> - visit_type_uint8(v, name, &value, &local_err);
>> - if (local_err) {
>> - goto out;
>> - }
>> - pm->disable_s3 = value;
>> -out:
>> - error_propagate(errp, local_err);
>> -}
>> -
>> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - uint8_t value = pm->disable_s4;
>> -
>> - visit_type_uint8(v, name, &value, errp);
>> -}
>> -
>> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - Error *local_err = NULL;
>> - uint8_t value;
>> -
>> - visit_type_uint8(v, name, &value, &local_err);
>> - if (local_err) {
>> - goto out;
>> - }
>> - pm->disable_s4 = value;
>> -out:
>> - error_propagate(errp, local_err);
>> -}
>> -
>> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - uint8_t value = pm->s4_val;
>> -
>> - visit_type_uint8(v, name, &value, errp);
>> -}
>> -
>> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCPMRegs *pm = opaque;
>> - Error *local_err = NULL;
>> - uint8_t value;
>> -
>> - visit_type_uint8(v, name, &value, &local_err);
>> - if (local_err) {
>> - goto out;
>> - }
>> - pm->s4_val = value;
>> -out:
>> - error_propagate(errp, local_err);
>> -}
>> -
>> static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
>> {
>> ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>> @@ -468,18 +393,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>> ich9_pm_get_cpu_hotplug_legacy,
>> ich9_pm_set_cpu_hotplug_legacy,
>> NULL);
>> - object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
>> - ich9_pm_get_disable_s3,
>> - ich9_pm_set_disable_s3,
>> - NULL, pm, NULL);
>> - object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
>> - ich9_pm_get_disable_s4,
>> - ich9_pm_set_disable_s4,
>> - NULL, pm, NULL);
>> - object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
>> - ich9_pm_get_s4_val,
>> - ich9_pm_set_s4_val,
>> - NULL, pm, NULL);
>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
>> + &pm->disable_s3, false, errp);
>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
>> + &pm->disable_s4, false, errp);
>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
>> + &pm->s4_val, false, errp);
>
> Sometime object properties are registered with error_abort, sometime
> with errp, sometime with NULL.
>
> Here you changed the argument. Not a big deal, but I think you should
> leave it as is for now. And we can address the error handling
> inconsisteny another day.
You are right. Let me go over that once more and send a v2. I don't
believe in changing too many things at once and improvements or not,
it should be done separately (if anything to allow an easier revert
later on). :)
>
>> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
>> ich9_pm_get_enable_tco,
>> ich9_pm_set_enable_tco,
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 232c7916f3..9abe07247e 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -627,15 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
>> .endianness = DEVICE_LITTLE_ENDIAN
>> };
>>
>> -static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>> - uint8_t value = lpc->sci_gsi;
>> -
>> - visit_type_uint8(v, name, &value, errp);
>> -}
>> -
>> static void ich9_lpc_initfn(Object *obj)
>> {
>> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>> @@ -643,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>>
>> - object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
>> - ich9_lpc_get_sci_int,
>> - NULL, NULL, NULL, NULL);
>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
>> + &lpc->sci_gsi, true, NULL);
>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
>> &acpi_enable_cmd, true, NULL);
>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>> index d5e2bdbb57..200503ecfd 100644
>> --- a/hw/misc/edu.c
>> +++ b/hw/misc/edu.c
>> @@ -396,21 +396,13 @@ static void pci_edu_uninit(PCIDevice *pdev)
>> msi_uninit(pdev);
>> }
>>
>> -static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint64_t *val = opaque;
>> -
>> - visit_type_uint64(v, name, val, errp);
>> -}
>> -
>> static void edu_instance_init(Object *obj)
>> {
>> EduState *edu = EDU(obj);
>>
>> edu->dma_mask = (1UL << 28) - 1;
>> - object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
>> - edu_obj_uint64, NULL, &edu->dma_mask, NULL);
>> + object_property_add_uint64_ptr(obj, "dma_mask",
>> + &edu->dma_mask, false, NULL);
>> }
>>
>> static void edu_class_init(ObjectClass *class, void *data)
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 158d270b9f..61fbe04fe9 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>> visit_type_uint64(v, name, &value, errp);
>> }
>>
>> -static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
>> -
>> - visit_type_uint64(v, name, &e->size, errp);
>> -}
>> -
>> /*
>> * NOTE: setting defaults for the mch.* fields in this table
>> * doesn't work, because mch is a separate QOM object that is
>> @@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
>> {
>> Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>> PCIHostState *phb = PCI_HOST_BRIDGE(obj);
>> + PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
>>
>> memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
>> "pci-conf-idx", 4);
>> @@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
>> q35_host_get_pci_hole64_end,
>> NULL, NULL, NULL, NULL);
>>
>> - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
>> - q35_host_get_mmcfg_size,
>> - NULL, NULL, NULL, NULL);
>> + object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
>> + &pehb->size, true, NULL);
>>
>> object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
>> (Object **) &s->mch.ram_memory,
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e076f6023c..1b9400526f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
>> }
>> }
>>
>> -static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>> -}
>> -
>> -static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>> -}
>> -
>> static char *spapr_get_ic_mode(Object *obj, Error **errp)
>> {
>> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>> @@ -3336,8 +3324,9 @@ static void spapr_instance_init(Object *obj)
>> object_property_set_description(obj, "resize-hpt",
>> "Resizing of the Hash Page Table (enabled, disabled, required)",
>> NULL);
>> - object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
>> - spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
>> + object_property_add_uint32_ptr(obj, "vsmt",
>> + &spapr->vsmt, false, &error_abort);
>> +
>> object_property_set_description(obj, "vsmt",
>> "Virtual SMT: KVM behaves as if this were"
>> " the host's SMT mode", &error_abort);
>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>> index 136f3a9ad6..34335e071e 100644
>> --- a/hw/vfio/pci-quirks.c
>> +++ b/hw/vfio/pci-quirks.c
>> @@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
>> return 0;
>> }
>>
>> -static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
>> - const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint64_t tgt = (uintptr_t) opaque;
>> - visit_type_uint64(v, name, &tgt, errp);
>> -}
>> -
>> static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
>> const char *name,
>> void *opaque, Error **errp)
>> @@ -2240,9 +2232,8 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
>> nv2reg->size, p);
>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>>
>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>> - (void *) (uintptr_t) cap->tgt, NULL);
>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>> + (void *)(uintptr_t)cap->tgt, true, NULL);
>
> nit: (void *) is enough, no?
Actually, I think this is completely wrong. I asked AW on IRC (he was
away and I didn't wait; oops), but I'll Cc him here and Alexey as well
(who wrote the code).
Maybe I'm missing something, but it looks like this is passing the
absolute value of cap->tgt as a pointer. Shouldn't it just be
&cap->tg? I don't understand the casting to (void *). Later, in
vfio_pci_nvlink2_get_*, it does:
uint64_t val = (uintptr_t)opaque;
visit_type_unitXX(..., &val, ...);
It looks to me like that only gets the local variable and doesn't
return anything in practice. But again, I'm not familiar with this at
all so I may be saying non-sense.
If this is confirmed to be wrong, I can fix this in an extra patch in
this series. Thoughts welcome.
F.
>
>> trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
>> nv2reg->size);
>> free_exit:
>> @@ -2301,9 +2292,8 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>> }
>>
>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>> - (void *) (uintptr_t) captgt->tgt, NULL);
>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>> + (void *)(uintptr_t)captgt->tgt, true, NULL);
>
> same
>
>> trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
>> atsdreg->size);
>>
>> diff --git a/memory.c b/memory.c
>> index 06484c2bff..0a34ee3c86 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
>> memory_region_do_init(mr, owner, name, size);
>> }
>>
>> -static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - MemoryRegion *mr = MEMORY_REGION(obj);
>> - uint64_t value = mr->addr;
>> -
>> - visit_type_uint64(v, name, &value, errp);
>> -}
>> -
>> static void memory_region_get_container(Object *obj, Visitor *v,
>> const char *name, void *opaque,
>> Error **errp)
>> @@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
>> NULL, NULL, &error_abort);
>> op->resolve = memory_region_resolve_container;
>>
>> - object_property_add(OBJECT(mr), "addr", "uint64",
>> - memory_region_get_addr,
>> - NULL, /* memory_region_set_addr */
>> - NULL, NULL, &error_abort);
>> + object_property_add_uint64_ptr(OBJECT(mr), "addr",
>> + &mr->addr, true, &error_abort);
>> object_property_add(OBJECT(mr), "priority", "uint32",
>> memory_region_get_priority,
>> NULL, /* memory_region_set_priority */
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 7a4ac9339b..aa7278e540 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>> cpu->has_pmu = value;
>> }
>>
>> -static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ARMCPU *cpu = ARM_CPU(obj);
>> -
>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>> -}
>> -
>> -static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - ARMCPU *cpu = ARM_CPU(obj);
>> -
>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>> -}
>> -
>> void arm_cpu_post_init(Object *obj)
>> {
>> ARMCPU *cpu = ARM_CPU(obj);
>> @@ -1165,9 +1149,8 @@ void arm_cpu_post_init(Object *obj)
>> * a simple DEFINE_PROP_UINT32 for this because we want to permit
>> * the property to be set after realize.
>> */
>> - object_property_add(obj, "init-svtor", "uint32",
>> - arm_get_init_svtor, arm_set_init_svtor,
>> - NULL, NULL, &error_abort);
>> + object_property_add_uint32_ptr(obj, "init-svtor",
>> + &cpu->init_svtor, false, &error_abort);
>> }
>>
>> qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 024bb24e51..23d7ab8b72 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
>> "guest owners session parameters (encoded with base64)", NULL);
>> }
>>
>> -static void
>> -qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> - uint32_t value;
>> -
>> - visit_type_uint32(v, name, &value, errp);
>> - sev->handle = value;
>> -}
>> -
>> -static void
>> -qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> - uint32_t value;
>> -
>> - visit_type_uint32(v, name, &value, errp);
>> - sev->policy = value;
>> -}
>> -
>> -static void
>> -qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> - uint32_t value;
>> -
>> - visit_type_uint32(v, name, &value, errp);
>> - sev->cbitpos = value;
>> -}
>> -
>> -static void
>> -qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> - uint32_t value;
>> -
>> - visit_type_uint32(v, name, &value, errp);
>> - sev->reduced_phys_bits = value;
>> -}
>> -
>> -static void
>> -qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint32_t value;
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> -
>> - value = sev->policy;
>> - visit_type_uint32(v, name, &value, errp);
>> -}
>> -
>> -static void
>> -qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint32_t value;
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> -
>> - value = sev->handle;
>> - visit_type_uint32(v, name, &value, errp);
>> -}
>> -
>> -static void
>> -qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint32_t value;
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> -
>> - value = sev->cbitpos;
>> - visit_type_uint32(v, name, &value, errp);
>> -}
>> -
>> -static void
>> -qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>> - void *opaque, Error **errp)
>> -{
>> - uint32_t value;
>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>> -
>> - value = sev->reduced_phys_bits;
>> - visit_type_uint32(v, name, &value, errp);
>> -}
>> -
>> static void
>> qsev_guest_init(Object *obj)
>> {
>> @@ -361,15 +273,11 @@ qsev_guest_init(Object *obj)
>>
>> sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>> sev->policy = DEFAULT_GUEST_POLICY;
>> - object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
>> - qsev_guest_set_policy, NULL, NULL, NULL);
>> - object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
>> - qsev_guest_set_handle, NULL, NULL, NULL);
>> - object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
>> - qsev_guest_set_cbitpos, NULL, NULL, NULL);
>> - object_property_add(obj, "reduced-phys-bits", "uint32",
>> - qsev_guest_get_reduced_phys_bits,
>> - qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
>> + object_property_add_uint32_ptr(obj, "policy", &sev->policy, false, NULL);
>> + object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, NULL);
>> + object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, NULL);
>> + object_property_add_uint32_ptr(obj, "reduced-phys-bits",
>> + &sev->reduced_phys_bits, false, NULL);
>> }
>>
>> /* sev guest info */
>> --
>> 2.20.1
>>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] ich9: fix getter type for sci_int property
2019-11-25 16:43 ` Philippe Mathieu-Daudé
@ 2019-11-26 9:55 ` Felipe Franciosi
0 siblings, 0 replies; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-26 9:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Marc-André Lureau, Eduardo Habkost,
Stefan Hajnoczi, Markus Armbruster
Hi
> On Nov 25, 2019, at 4:43 PM, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 11/25/19 4:36 PM, Felipe Franciosi 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>
>> ---
>> hw/isa/lpc_ich9.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 5555ce3342..240979885d 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -631,9 +631,9 @@ 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;
>> + uint8_t value = lpc->sci_gsi;
>> - visit_type_uint32(v, name, &value, errp);
>> + visit_type_uint8(v, name, &value, errp);
>
> Maybe directly as:
>
> visit_type_uint8(v, name, &lpc->sci_gsi, errp);
>
Thanks for the suggestion. I'll improve it on a v2 which I'm sending
out anyway for other reasons.
For my own sake: why is the field called "sci_gsi", but
ACPI_PM_PROP_SCI_INT (and the getter) are called "sci_int"?
Thanks,
F.
> With/without stack variable:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>> }
>> static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>> @@ -641,7 +641,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,
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] qom/object: enable setter for uint types
2019-11-26 8:40 ` Marc-André Lureau
@ 2019-11-26 12:03 ` Felipe Franciosi
2019-11-26 12:18 ` Marc-André Lureau
0 siblings, 1 reply; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-26 12:03 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
Heya
> On Nov 26, 2019, at 8:40 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Nov 25, 2019 at 7:40 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.
>>
>> This enhances the uint-specific property helper APIs by adding a
>> 'readonly' field and modifying all users of that API to set this
>> parameter to true. If 'readonly' is false, though, the helper will add
>> an automatic setter for the value.
>>
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> hw/acpi/ich9.c | 4 +-
>> hw/acpi/pcihp.c | 6 +-
>> hw/acpi/piix4.c | 12 ++--
>> hw/isa/lpc_ich9.c | 4 +-
>> hw/ppc/spapr_drc.c | 2 +-
>> include/qom/object.h | 28 +++++---
>> qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
>> ui/console.c | 3 +-
>> 8 files changed, 163 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 2034dd749e..94dc5147ce 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, true, 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, true, 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..70bc1408e6 100644
>> --- a/hw/acpi/pcihp.c
>> +++ b/hw/acpi/pcihp.c
>> @@ -80,7 +80,7 @@ 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, true, &error_abort);
>> }
>>
>> return bsel_alloc;
>> @@ -373,9 +373,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);
>> + true, &error_abort);
>> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
>> - &error_abort);
>> + true, &error_abort);
>> }
>>
>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 93aec2dd2c..032ba11e62 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, true, NULL);
>> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> - &acpi_disable_cmd, NULL);
>> + &acpi_disable_cmd, true, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>> - &gpe0_blk, NULL);
>> + &gpe0_blk, true, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>> - &gpe0_blk_len, NULL);
>> + &gpe0_blk_len, true, NULL);
>> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>> - &sci_int, NULL);
>> + &sci_int, true, NULL);
>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>> - &s->io_base, NULL);
>> + &s->io_base, true, 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..5555ce3342 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, true, NULL);
>> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>> - &acpi_disable_cmd, NULL);
>> + &acpi_disable_cmd, true, 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..76f389f56a 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -553,7 +553,7 @@ 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, true, 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..8fc28ed0c9 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1584,60 +1584,72 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @readonly: don't add setter for value
>> * @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, bool readonly,
>> + Error **errp);
>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>> - const uint8_t *v, Error **errp);
>> + const uint8_t *v, bool readonly,
>> + Error **errp);
>
> It would help readability and extensibility if we had an enum/bitflag:
> OBJECT_PROP_WRITABLE, OBJECT_PROP_READABLE (& OBJECT_PROP_READWRITE
> alias).
I quite like your suggestion. When going through the various clients
of these, I found that many of them do not accept a zero value on the
setter. I didn't include them on this series, but if we had an
OBJ_PROP_REJECT_ZERO_VAL (or similar), we could take them in the
generic setters as well. Let me have a stab at that on the v2.
>
>>
>> /**
>> * object_property_add_uint16_ptr:
>> * @obj: the object to add a property to
>> * @name: the name of the property
>> * @v: pointer to value
>> + * @readonly: don't add setter for value
>> * @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, bool readonly,
>> + Error **errp);
>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> - const uint16_t *v, Error **errp);
>> + const uint16_t *v, bool readonly,
>> + 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
>> + * @readonly: don't add setter for value
>> * @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, bool readonly,
>> + Error **errp);
>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>> - const uint32_t *v, Error **errp);
>> + const uint32_t *v, bool readonly,
>> + 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
>> + * @readonly: don't add setter for value
>> * @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, bool readonly,
>> + Error **Errp);
>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>> - const uint64_t *v, Error **Errp);
>> + const uint64_t *v, bool readonly,
>> + Error **Errp);
>>
>> /**
>> * object_property_add_alias:
>> diff --git a/qom/object.c b/qom/object.c
>> index d51b57fba1..775f702465 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -2326,6 +2326,26 @@ 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) {
>> + goto err;
>> + }
>> +
>> + *field = value;
>> +
>> + return;
>> +
>> +err:
>> + error_propagate(errp, local_err);
>> +}
>
>
> This is probably going to be refactored once the auto error series
> land. But in the meantime, it would be nice to remove the goto label.
I have just mimicked what other code does. Do you prefer the
error_propagate + return within the if()? I think I do.
F.
>
>> +
>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2333,6 +2353,26 @@ 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) {
>> + goto err;
>> + }
>> +
>> + *field = value;
>> +
>> + return;
>> +
>> +err:
>> + error_propagate(errp, local_err);
>> +}
>> +
>
> same
>
>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2340,6 +2380,26 @@ 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) {
>> + goto err;
>> + }
>> +
>> + *field = value;
>> +
>> + return;
>> +
>> +err:
>> + error_propagate(errp, local_err);
>> +}
>
> etc
>
>> +
>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> {
>> @@ -2347,60 +2407,104 @@ 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) {
>> + goto err;
>> + }
>> +
>> + *field = value;
>> +
>> + return;
>> +
>> +err:
>> + error_propagate(errp, local_err);
>> +}
>> +
>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>> - const uint8_t *v, Error **errp)
>> + const uint8_t *v, bool readonly,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_property_add(obj, name, "uint8",
>> + property_get_uint8_ptr,
>> + readonly ? NULL : property_set_uint8_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_class_property_add(klass, name, "uint8",
>> + property_get_uint8_ptr,
>> + readonly ? NULL : property_set_uint8_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_property_add(obj, name, "uint16",
>> + property_get_uint16_ptr,
>> + readonly ? NULL : property_set_uint16_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_class_property_add(klass, name, "uint16",
>> + property_get_uint16_ptr,
>> + readonly ? NULL : property_set_uint16_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_property_add(obj, name, "uint32",
>> + property_get_uint32_ptr,
>> + readonly ? NULL : property_set_uint32_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_class_property_add(klass, name, "uint32",
>> + property_get_uint32_ptr,
>> + readonly ? NULL : property_set_uint32_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_property_add(obj, name, "uint64",
>> + property_get_uint64_ptr,
>> + property_set_uint64_ptr,
>> + 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, bool readonly,
>> + Error **errp)
>> {
>> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
>> - NULL, NULL, (void *)v, errp);
>> + object_class_property_add(klass, name, "uint64",
>> + property_get_uint64_ptr,
>> + readonly ? NULL : property_set_uint64_ptr,
>> + NULL, (void *)v, errp);
>> }
>>
>> typedef struct {
>> diff --git a/ui/console.c b/ui/console.c
>> index 82d1ddac9c..3375c33d71 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -1296,8 +1296,7 @@ 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, true, &error_abort);
>>
>> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>> (console_type == GRAPHIC_CONSOLE))) {
>> --
>> 2.20.1
>>
>
> looks good otherwise
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] qom/object: enable setter for uint types
2019-11-26 12:03 ` Felipe Franciosi
@ 2019-11-26 12:18 ` Marc-André Lureau
2019-11-26 13:41 ` Felipe Franciosi
0 siblings, 1 reply; 18+ messages in thread
From: Marc-André Lureau @ 2019-11-26 12:18 UTC (permalink / raw)
To: Felipe Franciosi
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
Hi
On Tue, Nov 26, 2019 at 4:03 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Heya
>
> > On Nov 26, 2019, at 8:40 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, Nov 25, 2019 at 7:40 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.
> >>
> >> This enhances the uint-specific property helper APIs by adding a
> >> 'readonly' field and modifying all users of that API to set this
> >> parameter to true. If 'readonly' is false, though, the helper will add
> >> an automatic setter for the value.
> >>
> >> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >> ---
> >> hw/acpi/ich9.c | 4 +-
> >> hw/acpi/pcihp.c | 6 +-
> >> hw/acpi/piix4.c | 12 ++--
> >> hw/isa/lpc_ich9.c | 4 +-
> >> hw/ppc/spapr_drc.c | 2 +-
> >> include/qom/object.h | 28 +++++---
> >> qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
> >> ui/console.c | 3 +-
> >> 8 files changed, 163 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >> index 2034dd749e..94dc5147ce 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, true, 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, true, 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..70bc1408e6 100644
> >> --- a/hw/acpi/pcihp.c
> >> +++ b/hw/acpi/pcihp.c
> >> @@ -80,7 +80,7 @@ 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, true, &error_abort);
> >> }
> >>
> >> return bsel_alloc;
> >> @@ -373,9 +373,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);
> >> + true, &error_abort);
> >> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
> >> - &error_abort);
> >> + true, &error_abort);
> >> }
> >>
> >> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> index 93aec2dd2c..032ba11e62 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, true, NULL);
> >> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >> - &acpi_disable_cmd, NULL);
> >> + &acpi_disable_cmd, true, NULL);
> >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> >> - &gpe0_blk, NULL);
> >> + &gpe0_blk, true, NULL);
> >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> >> - &gpe0_blk_len, NULL);
> >> + &gpe0_blk_len, true, NULL);
> >> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> >> - &sci_int, NULL);
> >> + &sci_int, true, NULL);
> >> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> >> - &s->io_base, NULL);
> >> + &s->io_base, true, 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..5555ce3342 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, true, NULL);
> >> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> >> - &acpi_disable_cmd, NULL);
> >> + &acpi_disable_cmd, true, 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..76f389f56a 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -553,7 +553,7 @@ 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, true, 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..8fc28ed0c9 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -1584,60 +1584,72 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
> >> * @obj: the object to add a property to
> >> * @name: the name of the property
> >> * @v: pointer to value
> >> + * @readonly: don't add setter for value
> >> * @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, bool readonly,
> >> + Error **errp);
> >> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> >> - const uint8_t *v, Error **errp);
> >> + const uint8_t *v, bool readonly,
> >> + Error **errp);
> >
> > It would help readability and extensibility if we had an enum/bitflag:
> > OBJECT_PROP_WRITABLE, OBJECT_PROP_READABLE (& OBJECT_PROP_READWRITE
> > alias).
>
> I quite like your suggestion. When going through the various clients
> of these, I found that many of them do not accept a zero value on the
> setter. I didn't include them on this series, but if we had an
> OBJ_PROP_REJECT_ZERO_VAL (or similar), we could take them in the
> generic setters as well. Let me have a stab at that on the v2.
Hmm, that's a bit more involved.
I would introduce min/max ranges instead. Let's not do that now.
>
> >
> >>
> >> /**
> >> * object_property_add_uint16_ptr:
> >> * @obj: the object to add a property to
> >> * @name: the name of the property
> >> * @v: pointer to value
> >> + * @readonly: don't add setter for value
> >> * @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, bool readonly,
> >> + Error **errp);
> >> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> >> - const uint16_t *v, Error **errp);
> >> + const uint16_t *v, bool readonly,
> >> + 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
> >> + * @readonly: don't add setter for value
> >> * @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, bool readonly,
> >> + Error **errp);
> >> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> >> - const uint32_t *v, Error **errp);
> >> + const uint32_t *v, bool readonly,
> >> + 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
> >> + * @readonly: don't add setter for value
> >> * @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, bool readonly,
> >> + Error **Errp);
> >> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> >> - const uint64_t *v, Error **Errp);
> >> + const uint64_t *v, bool readonly,
> >> + Error **Errp);
> >>
> >> /**
> >> * object_property_add_alias:
> >> diff --git a/qom/object.c b/qom/object.c
> >> index d51b57fba1..775f702465 100644
> >> --- a/qom/object.c
> >> +++ b/qom/object.c
> >> @@ -2326,6 +2326,26 @@ 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) {
> >> + goto err;
> >> + }
> >> +
> >> + *field = value;
> >> +
> >> + return;
> >> +
> >> +err:
> >> + error_propagate(errp, local_err);
> >> +}
> >
> >
> > This is probably going to be refactored once the auto error series
> > land. But in the meantime, it would be nice to remove the goto label.
>
> I have just mimicked what other code does. Do you prefer the
> error_propagate + return within the if()? I think I do.
Ok, I didn't look at existing code. Up to you
>
> F.
>
> >
> >> +
> >> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
> >> void *opaque, Error **errp)
> >> {
> >> @@ -2333,6 +2353,26 @@ 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) {
> >> + goto err;
> >> + }
> >> +
> >> + *field = value;
> >> +
> >> + return;
> >> +
> >> +err:
> >> + error_propagate(errp, local_err);
> >> +}
> >> +
> >
> > same
> >
> >> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
> >> void *opaque, Error **errp)
> >> {
> >> @@ -2340,6 +2380,26 @@ 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) {
> >> + goto err;
> >> + }
> >> +
> >> + *field = value;
> >> +
> >> + return;
> >> +
> >> +err:
> >> + error_propagate(errp, local_err);
> >> +}
> >
> > etc
> >
> >> +
> >> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
> >> void *opaque, Error **errp)
> >> {
> >> @@ -2347,60 +2407,104 @@ 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) {
> >> + goto err;
> >> + }
> >> +
> >> + *field = value;
> >> +
> >> + return;
> >> +
> >> +err:
> >> + error_propagate(errp, local_err);
> >> +}
> >> +
> >> void object_property_add_uint8_ptr(Object *obj, const char *name,
> >> - const uint8_t *v, Error **errp)
> >> + const uint8_t *v, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_property_add(obj, name, "uint8",
> >> + property_get_uint8_ptr,
> >> + readonly ? NULL : property_set_uint8_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_class_property_add(klass, name, "uint8",
> >> + property_get_uint8_ptr,
> >> + readonly ? NULL : property_set_uint8_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_property_add(obj, name, "uint16",
> >> + property_get_uint16_ptr,
> >> + readonly ? NULL : property_set_uint16_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_class_property_add(klass, name, "uint16",
> >> + property_get_uint16_ptr,
> >> + readonly ? NULL : property_set_uint16_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_property_add(obj, name, "uint32",
> >> + property_get_uint32_ptr,
> >> + readonly ? NULL : property_set_uint32_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_class_property_add(klass, name, "uint32",
> >> + property_get_uint32_ptr,
> >> + readonly ? NULL : property_set_uint32_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_property_add(obj, name, "uint64",
> >> + property_get_uint64_ptr,
> >> + property_set_uint64_ptr,
> >> + 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, bool readonly,
> >> + Error **errp)
> >> {
> >> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
> >> - NULL, NULL, (void *)v, errp);
> >> + object_class_property_add(klass, name, "uint64",
> >> + property_get_uint64_ptr,
> >> + readonly ? NULL : property_set_uint64_ptr,
> >> + NULL, (void *)v, errp);
> >> }
> >>
> >> typedef struct {
> >> diff --git a/ui/console.c b/ui/console.c
> >> index 82d1ddac9c..3375c33d71 100644
> >> --- a/ui/console.c
> >> +++ b/ui/console.c
> >> @@ -1296,8 +1296,7 @@ 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, true, &error_abort);
> >>
> >> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
> >> (console_type == GRAPHIC_CONSOLE))) {
> >> --
> >> 2.20.1
> >>
> >
> > looks good otherwise
> >
> >
> > --
> > Marc-André Lureau
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] qom/object: enable setter for uint types
2019-11-26 12:18 ` Marc-André Lureau
@ 2019-11-26 13:41 ` Felipe Franciosi
0 siblings, 0 replies; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-26 13:41 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
> On Nov 26, 2019, at 12:18 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Tue, Nov 26, 2019 at 4:03 PM Felipe Franciosi <felipe@nutanix.com> wrote:
>>
>> Heya
>>
>>> On Nov 26, 2019, at 8:40 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>>
>>> Hi
>>>
>>> On Mon, Nov 25, 2019 at 7:40 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.
>>>>
>>>> This enhances the uint-specific property helper APIs by adding a
>>>> 'readonly' field and modifying all users of that API to set this
>>>> parameter to true. If 'readonly' is false, though, the helper will add
>>>> an automatic setter for the value.
>>>>
>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>>> ---
>>>> hw/acpi/ich9.c | 4 +-
>>>> hw/acpi/pcihp.c | 6 +-
>>>> hw/acpi/piix4.c | 12 ++--
>>>> hw/isa/lpc_ich9.c | 4 +-
>>>> hw/ppc/spapr_drc.c | 2 +-
>>>> include/qom/object.h | 28 +++++---
>>>> qom/object.c | 152 ++++++++++++++++++++++++++++++++++++-------
>>>> ui/console.c | 3 +-
>>>> 8 files changed, 163 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>> index 2034dd749e..94dc5147ce 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, true, 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, true, 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..70bc1408e6 100644
>>>> --- a/hw/acpi/pcihp.c
>>>> +++ b/hw/acpi/pcihp.c
>>>> @@ -80,7 +80,7 @@ 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, true, &error_abort);
>>>> }
>>>>
>>>> return bsel_alloc;
>>>> @@ -373,9 +373,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);
>>>> + true, &error_abort);
>>>> object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_LEN_PROP, &s->io_len,
>>>> - &error_abort);
>>>> + true, &error_abort);
>>>> }
>>>>
>>>> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>> index 93aec2dd2c..032ba11e62 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, true, NULL);
>>>> object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>>> - &acpi_disable_cmd, NULL);
>>>> + &acpi_disable_cmd, true, NULL);
>>>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
>>>> - &gpe0_blk, NULL);
>>>> + &gpe0_blk, true, NULL);
>>>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
>>>> - &gpe0_blk_len, NULL);
>>>> + &gpe0_blk_len, true, NULL);
>>>> object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
>>>> - &sci_int, NULL);
>>>> + &sci_int, true, NULL);
>>>> object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
>>>> - &s->io_base, NULL);
>>>> + &s->io_base, true, 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..5555ce3342 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, true, NULL);
>>>> object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>>> - &acpi_disable_cmd, NULL);
>>>> + &acpi_disable_cmd, true, 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..76f389f56a 100644
>>>> --- a/hw/ppc/spapr_drc.c
>>>> +++ b/hw/ppc/spapr_drc.c
>>>> @@ -553,7 +553,7 @@ 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, true, 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..8fc28ed0c9 100644
>>>> --- a/include/qom/object.h
>>>> +++ b/include/qom/object.h
>>>> @@ -1584,60 +1584,72 @@ void object_class_property_add_tm(ObjectClass *klass, const char *name,
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @readonly: don't add setter for value
>>>> * @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, bool readonly,
>>>> + Error **errp);
>>>> void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
>>>> - const uint8_t *v, Error **errp);
>>>> + const uint8_t *v, bool readonly,
>>>> + Error **errp);
>>>
>>> It would help readability and extensibility if we had an enum/bitflag:
>>> OBJECT_PROP_WRITABLE, OBJECT_PROP_READABLE (& OBJECT_PROP_READWRITE
>>> alias).
>>
>> I quite like your suggestion. When going through the various clients
>> of these, I found that many of them do not accept a zero value on the
>> setter. I didn't include them on this series, but if we had an
>> OBJ_PROP_REJECT_ZERO_VAL (or similar), we could take them in the
>> generic setters as well. Let me have a stab at that on the v2.
>
> Hmm, that's a bit more involved.
>
> I would introduce min/max ranges instead. Let's not do that now.
Ok. I didn't see (m)any uses where a range was required. There were
quite a few particularly rejecting zeros, though. I like the
properties idea anyway, but I think it should be a bitmask so you can
leave the door open for these combinations. Thoughts?
>
>>
>>>
>>>>
>>>> /**
>>>> * object_property_add_uint16_ptr:
>>>> * @obj: the object to add a property to
>>>> * @name: the name of the property
>>>> * @v: pointer to value
>>>> + * @readonly: don't add setter for value
>>>> * @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, bool readonly,
>>>> + Error **errp);
>>>> void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>>>> - const uint16_t *v, Error **errp);
>>>> + const uint16_t *v, bool readonly,
>>>> + 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
>>>> + * @readonly: don't add setter for value
>>>> * @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, bool readonly,
>>>> + Error **errp);
>>>> void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
>>>> - const uint32_t *v, Error **errp);
>>>> + const uint32_t *v, bool readonly,
>>>> + 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
>>>> + * @readonly: don't add setter for value
>>>> * @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, bool readonly,
>>>> + Error **Errp);
>>>> void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
>>>> - const uint64_t *v, Error **Errp);
>>>> + const uint64_t *v, bool readonly,
>>>> + Error **Errp);
>>>>
>>>> /**
>>>> * object_property_add_alias:
>>>> diff --git a/qom/object.c b/qom/object.c
>>>> index d51b57fba1..775f702465 100644
>>>> --- a/qom/object.c
>>>> +++ b/qom/object.c
>>>> @@ -2326,6 +2326,26 @@ 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) {
>>>> + goto err;
>>>> + }
>>>> +
>>>> + *field = value;
>>>> +
>>>> + return;
>>>> +
>>>> +err:
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>
>>>
>>> This is probably going to be refactored once the auto error series
>>> land. But in the meantime, it would be nice to remove the goto label.
>>
>> I have just mimicked what other code does. Do you prefer the
>> error_propagate + return within the if()? I think I do.
>
> Ok, I didn't look at existing code. Up to you
Ok. In this case I may keep what I have in the patch to align with how
it's done everywhere else.
F.
>
>>
>> F.
>>
>>>
>>>> +
>>>> static void property_get_uint16_ptr(Object *obj, Visitor *v, const char *name,
>>>> void *opaque, Error **errp)
>>>> {
>>>> @@ -2333,6 +2353,26 @@ 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) {
>>>> + goto err;
>>>> + }
>>>> +
>>>> + *field = value;
>>>> +
>>>> + return;
>>>> +
>>>> +err:
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>
>>> same
>>>
>>>> static void property_get_uint32_ptr(Object *obj, Visitor *v, const char *name,
>>>> void *opaque, Error **errp)
>>>> {
>>>> @@ -2340,6 +2380,26 @@ 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) {
>>>> + goto err;
>>>> + }
>>>> +
>>>> + *field = value;
>>>> +
>>>> + return;
>>>> +
>>>> +err:
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>
>>> etc
>>>
>>>> +
>>>> static void property_get_uint64_ptr(Object *obj, Visitor *v, const char *name,
>>>> void *opaque, Error **errp)
>>>> {
>>>> @@ -2347,60 +2407,104 @@ 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) {
>>>> + goto err;
>>>> + }
>>>> +
>>>> + *field = value;
>>>> +
>>>> + return;
>>>> +
>>>> +err:
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>> void object_property_add_uint8_ptr(Object *obj, const char *name,
>>>> - const uint8_t *v, Error **errp)
>>>> + const uint8_t *v, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_property_add(obj, name, "uint8",
>>>> + property_get_uint8_ptr,
>>>> + readonly ? NULL : property_set_uint8_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_class_property_add(klass, name, "uint8", property_get_uint8_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_class_property_add(klass, name, "uint8",
>>>> + property_get_uint8_ptr,
>>>> + readonly ? NULL : property_set_uint8_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_property_add(obj, name, "uint16",
>>>> + property_get_uint16_ptr,
>>>> + readonly ? NULL : property_set_uint16_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_class_property_add(klass, name, "uint16", property_get_uint16_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_class_property_add(klass, name, "uint16",
>>>> + property_get_uint16_ptr,
>>>> + readonly ? NULL : property_set_uint16_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_property_add(obj, name, "uint32",
>>>> + property_get_uint32_ptr,
>>>> + readonly ? NULL : property_set_uint32_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_class_property_add(klass, name, "uint32", property_get_uint32_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_class_property_add(klass, name, "uint32",
>>>> + property_get_uint32_ptr,
>>>> + readonly ? NULL : property_set_uint32_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_property_add(obj, name, "uint64",
>>>> + property_get_uint64_ptr,
>>>> + property_set_uint64_ptr,
>>>> + 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, bool readonly,
>>>> + Error **errp)
>>>> {
>>>> - object_class_property_add(klass, name, "uint64", property_get_uint64_ptr,
>>>> - NULL, NULL, (void *)v, errp);
>>>> + object_class_property_add(klass, name, "uint64",
>>>> + property_get_uint64_ptr,
>>>> + readonly ? NULL : property_set_uint64_ptr,
>>>> + NULL, (void *)v, errp);
>>>> }
>>>>
>>>> typedef struct {
>>>> diff --git a/ui/console.c b/ui/console.c
>>>> index 82d1ddac9c..3375c33d71 100644
>>>> --- a/ui/console.c
>>>> +++ b/ui/console.c
>>>> @@ -1296,8 +1296,7 @@ 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, true, &error_abort);
>>>>
>>>> if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
>>>> (console_type == GRAPHIC_CONSOLE))) {
>>>> --
>>>> 2.20.1
>>>>
>>>
>>> looks good otherwise
>>>
>>>
>>> --
>>> Marc-André Lureau
>>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qom/object: Use common get/set uint helpers
2019-11-26 9:39 ` Felipe Franciosi
@ 2019-11-27 23:58 ` Alexey Kardashevskiy
2019-11-28 16:12 ` Felipe Franciosi
0 siblings, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2019-11-27 23:58 UTC (permalink / raw)
To: Felipe Franciosi, Marc-André Lureau, Alex Williamson
Cc: qemu-devel, Eduardo Habkost, Stefan Hajnoczi, Markus Armbruster
On 26/11/2019 20:39, Felipe Franciosi wrote:
>
>
>> On Nov 26, 2019, at 8:39 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>
> Heya, thanks for the review.
>
>>
>> On Mon, Nov 25, 2019 at 7:37 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>
>>
>>
>> Some comments below, otherwise:
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>> ---
>>> hw/acpi/ich9.c | 93 +++------------------------------------
>>> hw/isa/lpc_ich9.c | 14 +-----
>>> hw/misc/edu.c | 12 +----
>>> hw/pci-host/q35.c | 14 ++----
>>> hw/ppc/spapr.c | 17 ++------
>>> hw/vfio/pci-quirks.c | 18 ++------
>>> memory.c | 15 +------
>>> target/arm/cpu.c | 21 +--------
>>> target/i386/sev.c | 102 +++----------------------------------------
>>> 9 files changed, 29 insertions(+), 277 deletions(-)
>>>
>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>> index 94dc5147ce..aa3c7a59ab 100644
>>> --- a/hw/acpi/ich9.c
>>> +++ b/hw/acpi/ich9.c
>>> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
>>> s->pm.cpu_hotplug_legacy = value;
>>> }
>>>
>>> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - uint8_t value = pm->disable_s3;
>>> -
>>> - visit_type_uint8(v, name, &value, errp);
>>> -}
>>> -
>>> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - Error *local_err = NULL;
>>> - uint8_t value;
>>> -
>>> - visit_type_uint8(v, name, &value, &local_err);
>>> - if (local_err) {
>>> - goto out;
>>> - }
>>> - pm->disable_s3 = value;
>>> -out:
>>> - error_propagate(errp, local_err);
>>> -}
>>> -
>>> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - uint8_t value = pm->disable_s4;
>>> -
>>> - visit_type_uint8(v, name, &value, errp);
>>> -}
>>> -
>>> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - Error *local_err = NULL;
>>> - uint8_t value;
>>> -
>>> - visit_type_uint8(v, name, &value, &local_err);
>>> - if (local_err) {
>>> - goto out;
>>> - }
>>> - pm->disable_s4 = value;
>>> -out:
>>> - error_propagate(errp, local_err);
>>> -}
>>> -
>>> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - uint8_t value = pm->s4_val;
>>> -
>>> - visit_type_uint8(v, name, &value, errp);
>>> -}
>>> -
>>> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCPMRegs *pm = opaque;
>>> - Error *local_err = NULL;
>>> - uint8_t value;
>>> -
>>> - visit_type_uint8(v, name, &value, &local_err);
>>> - if (local_err) {
>>> - goto out;
>>> - }
>>> - pm->s4_val = value;
>>> -out:
>>> - error_propagate(errp, local_err);
>>> -}
>>> -
>>> static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
>>> {
>>> ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>>> @@ -468,18 +393,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>>> ich9_pm_get_cpu_hotplug_legacy,
>>> ich9_pm_set_cpu_hotplug_legacy,
>>> NULL);
>>> - object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
>>> - ich9_pm_get_disable_s3,
>>> - ich9_pm_set_disable_s3,
>>> - NULL, pm, NULL);
>>> - object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
>>> - ich9_pm_get_disable_s4,
>>> - ich9_pm_set_disable_s4,
>>> - NULL, pm, NULL);
>>> - object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
>>> - ich9_pm_get_s4_val,
>>> - ich9_pm_set_s4_val,
>>> - NULL, pm, NULL);
>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
>>> + &pm->disable_s3, false, errp);
>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
>>> + &pm->disable_s4, false, errp);
>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
>>> + &pm->s4_val, false, errp);
>>
>> Sometime object properties are registered with error_abort, sometime
>> with errp, sometime with NULL.
>>
>> Here you changed the argument. Not a big deal, but I think you should
>> leave it as is for now. And we can address the error handling
>> inconsisteny another day.
>
> You are right. Let me go over that once more and send a v2. I don't
> believe in changing too many things at once and improvements or not,
> it should be done separately (if anything to allow an easier revert
> later on). :)
>
>>
>>> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
>>> ich9_pm_get_enable_tco,
>>> ich9_pm_set_enable_tco,
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index 232c7916f3..9abe07247e 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -627,15 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
>>> .endianness = DEVICE_LITTLE_ENDIAN
>>> };
>>>
>>> -static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>>> - uint8_t value = lpc->sci_gsi;
>>> -
>>> - visit_type_uint8(v, name, &value, errp);
>>> -}
>>> -
>>> static void ich9_lpc_initfn(Object *obj)
>>> {
>>> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>>> @@ -643,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
>>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>>>
>>> - object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
>>> - ich9_lpc_get_sci_int,
>>> - NULL, NULL, NULL, NULL);
>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
>>> + &lpc->sci_gsi, true, NULL);
>>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
>>> &acpi_enable_cmd, true, NULL);
>>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>>> index d5e2bdbb57..200503ecfd 100644
>>> --- a/hw/misc/edu.c
>>> +++ b/hw/misc/edu.c
>>> @@ -396,21 +396,13 @@ static void pci_edu_uninit(PCIDevice *pdev)
>>> msi_uninit(pdev);
>>> }
>>>
>>> -static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint64_t *val = opaque;
>>> -
>>> - visit_type_uint64(v, name, val, errp);
>>> -}
>>> -
>>> static void edu_instance_init(Object *obj)
>>> {
>>> EduState *edu = EDU(obj);
>>>
>>> edu->dma_mask = (1UL << 28) - 1;
>>> - object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
>>> - edu_obj_uint64, NULL, &edu->dma_mask, NULL);
>>> + object_property_add_uint64_ptr(obj, "dma_mask",
>>> + &edu->dma_mask, false, NULL);
>>> }
>>>
>>> static void edu_class_init(ObjectClass *class, void *data)
>>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>>> index 158d270b9f..61fbe04fe9 100644
>>> --- a/hw/pci-host/q35.c
>>> +++ b/hw/pci-host/q35.c
>>> @@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>>> visit_type_uint64(v, name, &value, errp);
>>> }
>>>
>>> -static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
>>> -
>>> - visit_type_uint64(v, name, &e->size, errp);
>>> -}
>>> -
>>> /*
>>> * NOTE: setting defaults for the mch.* fields in this table
>>> * doesn't work, because mch is a separate QOM object that is
>>> @@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
>>> {
>>> Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>> PCIHostState *phb = PCI_HOST_BRIDGE(obj);
>>> + PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
>>>
>>> memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
>>> "pci-conf-idx", 4);
>>> @@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
>>> q35_host_get_pci_hole64_end,
>>> NULL, NULL, NULL, NULL);
>>>
>>> - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
>>> - q35_host_get_mmcfg_size,
>>> - NULL, NULL, NULL, NULL);
>>> + object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
>>> + &pehb->size, true, NULL);
>>>
>>> object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
>>> (Object **) &s->mch.ram_memory,
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index e076f6023c..1b9400526f 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
>>> }
>>> }
>>>
>>> -static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>>> -}
>>> -
>>> -static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>>> -}
>>> -
>>> static char *spapr_get_ic_mode(Object *obj, Error **errp)
>>> {
>>> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>>> @@ -3336,8 +3324,9 @@ static void spapr_instance_init(Object *obj)
>>> object_property_set_description(obj, "resize-hpt",
>>> "Resizing of the Hash Page Table (enabled, disabled, required)",
>>> NULL);
>>> - object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
>>> - spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
>>> + object_property_add_uint32_ptr(obj, "vsmt",
>>> + &spapr->vsmt, false, &error_abort);
>>> +
>>> object_property_set_description(obj, "vsmt",
>>> "Virtual SMT: KVM behaves as if this were"
>>> " the host's SMT mode", &error_abort);
>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>>> index 136f3a9ad6..34335e071e 100644
>>> --- a/hw/vfio/pci-quirks.c
>>> +++ b/hw/vfio/pci-quirks.c
>>> @@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
>>> return 0;
>>> }
>>>
>>> -static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
>>> - const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint64_t tgt = (uintptr_t) opaque;
>>> - visit_type_uint64(v, name, &tgt, errp);
>>> -}
>>> -
>>> static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
>>> const char *name,
>>> void *opaque, Error **errp)
>>> @@ -2240,9 +2232,8 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
>>> nv2reg->size, p);
>>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>>>
>>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>>> - (void *) (uintptr_t) cap->tgt, NULL);
>>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>>> + (void *)(uintptr_t)cap->tgt, true, NULL);
>>
>> nit: (void *) is enough, no?
>
> Actually, I think this is completely wrong. I asked AW on IRC (he was
> away and I didn't wait; oops), but I'll Cc him here and Alexey as well
> (who wrote the code).
>
> Maybe I'm missing something, but it looks like this is passing the
> absolute value of cap->tgt as a pointer. Shouldn't it just be
> &cap->tg?
Passing &cap->tgt requres @cap to stay in memory until the user of that
property reads it which is not the case here - the property is set when
the device is "realized" and used every time the pseries machine is
reset. This is rather highjacking QOM and properties to pass a 64bit
value from VFIO to PPC64/pseries without sharing any C structures.
> I don't understand the casting to (void *).
object_property_add() takes void*, and cap->tgt is not exactly an
address so it is not a pointer, it is an abbreviated host hardware
address which acts here more like a handle/cookie as in fact it is only
56bit but whatever :)
> Later, in
> vfio_pci_nvlink2_get_*, it does:
>
> uint64_t val = (uintptr_t)opaque;
> visit_type_unitXX(..., &val, ...);
>
> It looks to me like that only gets the local variable and doesn't
> return anything in practice. But again, I'm not familiar with this at
> all so I may be saying non-sense.
This visit_type_unitXX() thing puts @val to the visitor object which is
then read by object_property_get(). I am having hardtime tracing this
code, below is the example of a read (hopefully TB won't break
formatting much). Thanks,
(gdb) bt
#0 0x0000000100b902e0 in qobject_output_add_obj (qov=0x101dbfd70,
name=0x100dbdaf0 "nvlink2-tgt", value=0x103b8f000) at
/home/aik/p/qemu/qapi/qobject-output-visitor.c:83
#1 0x0000000100b908c0 in qobject_output_type_uint64 (v=0x101dbfd70,
name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
at /home/aik/p/qemu/qapi/qobject-output-visitor.c:158
#2 0x0000000100b8be94 in visit_type_uint64 (v=0x101dbfd70,
name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
at /home/aik/p/qemu/qapi/qapi-visit-core.c:207
#3 0x00000001004678f4 in vfio_pci_nvlink2_get_tgt (obj=0x102a84910,
v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", opaque=0x40000000000,
errp=0x7fffffffe888) at /home/aik/p/qemu/hw/vfio/pci-quirks.c:2195
#4 0x0000000100a45720 in object_property_get (obj=0x102a84910,
v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", errp=0x7fffffffe888) at
/home/aik/p/qemu/qom/object.c:1257
#5 0x0000000100a49fe8 in object_property_get_qobject (obj=0x102a84910,
name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
/home/aik/p/qemu/qom/qom-qobject.c:38
#6 0x0000000100a460c8 in object_property_get_uint (obj=0x102a84910,
name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
/home/aik/p/qemu/qom/object.c:1407
#7 0x00000001004eca98 in spapr_phb_pci_collect_nvgpu (bus=0x101d817f0,
pdev=0x102a84910, opaque=0x101dbfaa0) at
/home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:139
#8 0x000000010087795c in pci_for_each_device_under_bus
(bus=0x101d817f0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1638
#9 0x00000001008779fc in pci_for_each_device (bus=0x101d817f0,
bus_num=0x0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1650
#10 0x00000001004ecdd0 in spapr_phb_nvgpu_setup (sphb=0x101d34f20,
errp=0x7fffffffeb68) at /home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:187
#11 0x00000001004cb8f8 in spapr_phb_reset (qdev=0x101d34f20) at
/home/aik/p/qemu/hw/ppc/spapr_pci.c:2049
#12 0x0000000100718858 in device_reset (dev=0x101d34f20) at
/home/aik/p/qemu/hw/core/qdev.c:1112
#13 0x00000001007159f0 in qdev_reset_one (dev=0x101d34f20, opaque=0x0)
at /home/aik/p/qemu/hw/core/qdev.c:277
#14 0x0000000100716d18 in qdev_walk_children (dev=0x101d34f20,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
/home/aik/p/qemu/hw/core/qdev.c:593
#15 0x000000010071d1ac in qbus_walk_children (bus=0x1016df320,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
/home/aik/p/qemu/hw/core/bus.c:53
#16 0x0000000100715bf4 in qbus_reset_all (bus=0x1016df320) at
/home/aik/p/qemu/hw/core/qdev.c:303
#17 0x0000000100715c4c in qbus_reset_all_fn (opaque=0x1016df320) at
/home/aik/p/qemu/hw/core/qdev.c:309
#18 0x000000010071df6c in qemu_devices_reset () at
/home/aik/p/qemu/hw/core/reset.c:69
#19 0x00000001004a3554 in spapr_machine_reset (machine=0x1016bec00) at
/home/aik/p/qemu/hw/ppc/spapr.c:1684
#20 0x0000000100688488 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE)
at /home/aik/p/qemu/vl.c:1550
#21 0x0000000100692b3c in main (argc=0x2e, argv=0x7ffffffff568,
envp=0x7ffffffff6e0) at /home/aik/p/qemu/vl.c:4419
>
> If this is confirmed to be wrong, I can fix this in an extra patch in
> this series. Thoughts welcome.
>
> F.
>
>>
>>> trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
>>> nv2reg->size);
>>> free_exit:
>>> @@ -2301,9 +2292,8 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
>>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>>> }
>>>
>>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>>> - (void *) (uintptr_t) captgt->tgt, NULL);
>>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>>> + (void *)(uintptr_t)captgt->tgt, true, NULL);
>>
>> same
>>
>>> trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
>>> atsdreg->size);
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 06484c2bff..0a34ee3c86 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
>>> memory_region_do_init(mr, owner, name, size);
>>> }
>>>
>>> -static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - MemoryRegion *mr = MEMORY_REGION(obj);
>>> - uint64_t value = mr->addr;
>>> -
>>> - visit_type_uint64(v, name, &value, errp);
>>> -}
>>> -
>>> static void memory_region_get_container(Object *obj, Visitor *v,
>>> const char *name, void *opaque,
>>> Error **errp)
>>> @@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
>>> NULL, NULL, &error_abort);
>>> op->resolve = memory_region_resolve_container;
>>>
>>> - object_property_add(OBJECT(mr), "addr", "uint64",
>>> - memory_region_get_addr,
>>> - NULL, /* memory_region_set_addr */
>>> - NULL, NULL, &error_abort);
>>> + object_property_add_uint64_ptr(OBJECT(mr), "addr",
>>> + &mr->addr, true, &error_abort);
>>> object_property_add(OBJECT(mr), "priority", "uint32",
>>> memory_region_get_priority,
>>> NULL, /* memory_region_set_priority */
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 7a4ac9339b..aa7278e540 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>>> cpu->has_pmu = value;
>>> }
>>>
>>> -static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ARMCPU *cpu = ARM_CPU(obj);
>>> -
>>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>>> -}
>>> -
>>> -static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - ARMCPU *cpu = ARM_CPU(obj);
>>> -
>>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>>> -}
>>> -
>>> void arm_cpu_post_init(Object *obj)
>>> {
>>> ARMCPU *cpu = ARM_CPU(obj);
>>> @@ -1165,9 +1149,8 @@ void arm_cpu_post_init(Object *obj)
>>> * a simple DEFINE_PROP_UINT32 for this because we want to permit
>>> * the property to be set after realize.
>>> */
>>> - object_property_add(obj, "init-svtor", "uint32",
>>> - arm_get_init_svtor, arm_set_init_svtor,
>>> - NULL, NULL, &error_abort);
>>> + object_property_add_uint32_ptr(obj, "init-svtor",
>>> + &cpu->init_svtor, false, &error_abort);
>>> }
>>>
>>> qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
>>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>>> index 024bb24e51..23d7ab8b72 100644
>>> --- a/target/i386/sev.c
>>> +++ b/target/i386/sev.c
>>> @@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
>>> "guest owners session parameters (encoded with base64)", NULL);
>>> }
>>>
>>> -static void
>>> -qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> - uint32_t value;
>>> -
>>> - visit_type_uint32(v, name, &value, errp);
>>> - sev->handle = value;
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> - uint32_t value;
>>> -
>>> - visit_type_uint32(v, name, &value, errp);
>>> - sev->policy = value;
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> - uint32_t value;
>>> -
>>> - visit_type_uint32(v, name, &value, errp);
>>> - sev->cbitpos = value;
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> - uint32_t value;
>>> -
>>> - visit_type_uint32(v, name, &value, errp);
>>> - sev->reduced_phys_bits = value;
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint32_t value;
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> -
>>> - value = sev->policy;
>>> - visit_type_uint32(v, name, &value, errp);
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint32_t value;
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> -
>>> - value = sev->handle;
>>> - visit_type_uint32(v, name, &value, errp);
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint32_t value;
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> -
>>> - value = sev->cbitpos;
>>> - visit_type_uint32(v, name, &value, errp);
>>> -}
>>> -
>>> -static void
>>> -qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>>> - void *opaque, Error **errp)
>>> -{
>>> - uint32_t value;
>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>> -
>>> - value = sev->reduced_phys_bits;
>>> - visit_type_uint32(v, name, &value, errp);
>>> -}
>>> -
>>> static void
>>> qsev_guest_init(Object *obj)
>>> {
>>> @@ -361,15 +273,11 @@ qsev_guest_init(Object *obj)
>>>
>>> sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>>> sev->policy = DEFAULT_GUEST_POLICY;
>>> - object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
>>> - qsev_guest_set_policy, NULL, NULL, NULL);
>>> - object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
>>> - qsev_guest_set_handle, NULL, NULL, NULL);
>>> - object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
>>> - qsev_guest_set_cbitpos, NULL, NULL, NULL);
>>> - object_property_add(obj, "reduced-phys-bits", "uint32",
>>> - qsev_guest_get_reduced_phys_bits,
>>> - qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
>>> + object_property_add_uint32_ptr(obj, "policy", &sev->policy, false, NULL);
>>> + object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, NULL);
>>> + object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, NULL);
>>> + object_property_add_uint32_ptr(obj, "reduced-phys-bits",
>>> + &sev->reduced_phys_bits, false, NULL);
>>> }
>>>
>>> /* sev guest info */
>>> --
>>> 2.20.1
>>>
>>
>>
>> --
>> Marc-André Lureau
>
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] qom/object: Use common get/set uint helpers
2019-11-27 23:58 ` Alexey Kardashevskiy
@ 2019-11-28 16:12 ` Felipe Franciosi
0 siblings, 0 replies; 18+ messages in thread
From: Felipe Franciosi @ 2019-11-28 16:12 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Eduardo Habkost, qemu-devel, Markus Armbruster, Alex Williamson,
Marc-André Lureau, Stefan Hajnoczi
> On Nov 27, 2019, at 11:58 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 26/11/2019 20:39, Felipe Franciosi wrote:
>>
>>
>>> On Nov 26, 2019, at 8:39 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>>>
>>> Hi
>>
>> Heya, thanks for the review.
>>
>>>
>>> On Mon, Nov 25, 2019 at 7:37 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>
>>>
>>>
>>> Some comments below, otherwise:
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>>> ---
>>>> hw/acpi/ich9.c | 93 +++------------------------------------
>>>> hw/isa/lpc_ich9.c | 14 +-----
>>>> hw/misc/edu.c | 12 +----
>>>> hw/pci-host/q35.c | 14 ++----
>>>> hw/ppc/spapr.c | 17 ++------
>>>> hw/vfio/pci-quirks.c | 18 ++------
>>>> memory.c | 15 +------
>>>> target/arm/cpu.c | 21 +--------
>>>> target/i386/sev.c | 102 +++----------------------------------------
>>>> 9 files changed, 29 insertions(+), 277 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>> index 94dc5147ce..aa3c7a59ab 100644
>>>> --- a/hw/acpi/ich9.c
>>>> +++ b/hw/acpi/ich9.c
>>>> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
>>>> s->pm.cpu_hotplug_legacy = value;
>>>> }
>>>>
>>>> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - uint8_t value = pm->disable_s3;
>>>> -
>>>> - visit_type_uint8(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - Error *local_err = NULL;
>>>> - uint8_t value;
>>>> -
>>>> - visit_type_uint8(v, name, &value, &local_err);
>>>> - if (local_err) {
>>>> - goto out;
>>>> - }
>>>> - pm->disable_s3 = value;
>>>> -out:
>>>> - error_propagate(errp, local_err);
>>>> -}
>>>> -
>>>> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - uint8_t value = pm->disable_s4;
>>>> -
>>>> - visit_type_uint8(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - Error *local_err = NULL;
>>>> - uint8_t value;
>>>> -
>>>> - visit_type_uint8(v, name, &value, &local_err);
>>>> - if (local_err) {
>>>> - goto out;
>>>> - }
>>>> - pm->disable_s4 = value;
>>>> -out:
>>>> - error_propagate(errp, local_err);
>>>> -}
>>>> -
>>>> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - uint8_t value = pm->s4_val;
>>>> -
>>>> - visit_type_uint8(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCPMRegs *pm = opaque;
>>>> - Error *local_err = NULL;
>>>> - uint8_t value;
>>>> -
>>>> - visit_type_uint8(v, name, &value, &local_err);
>>>> - if (local_err) {
>>>> - goto out;
>>>> - }
>>>> - pm->s4_val = value;
>>>> -out:
>>>> - error_propagate(errp, local_err);
>>>> -}
>>>> -
>>>> static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
>>>> {
>>>> ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>>>> @@ -468,18 +393,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>>>> ich9_pm_get_cpu_hotplug_legacy,
>>>> ich9_pm_set_cpu_hotplug_legacy,
>>>> NULL);
>>>> - object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
>>>> - ich9_pm_get_disable_s3,
>>>> - ich9_pm_set_disable_s3,
>>>> - NULL, pm, NULL);
>>>> - object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
>>>> - ich9_pm_get_disable_s4,
>>>> - ich9_pm_set_disable_s4,
>>>> - NULL, pm, NULL);
>>>> - object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
>>>> - ich9_pm_get_s4_val,
>>>> - ich9_pm_set_s4_val,
>>>> - NULL, pm, NULL);
>>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
>>>> + &pm->disable_s3, false, errp);
>>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
>>>> + &pm->disable_s4, false, errp);
>>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_VAL,
>>>> + &pm->s4_val, false, errp);
>>>
>>> Sometime object properties are registered with error_abort, sometime
>>> with errp, sometime with NULL.
>>>
>>> Here you changed the argument. Not a big deal, but I think you should
>>> leave it as is for now. And we can address the error handling
>>> inconsisteny another day.
>>
>> You are right. Let me go over that once more and send a v2. I don't
>> believe in changing too many things at once and improvements or not,
>> it should be done separately (if anything to allow an easier revert
>> later on). :)
>>
>>>
>>>> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
>>>> ich9_pm_get_enable_tco,
>>>> ich9_pm_set_enable_tco,
>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>> index 232c7916f3..9abe07247e 100644
>>>> --- a/hw/isa/lpc_ich9.c
>>>> +++ b/hw/isa/lpc_ich9.c
>>>> @@ -627,15 +627,6 @@ static const MemoryRegionOps ich9_rst_cnt_ops = {
>>>> .endianness = DEVICE_LITTLE_ENDIAN
>>>> };
>>>>
>>>> -static void ich9_lpc_get_sci_int(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>>>> - uint8_t value = lpc->sci_gsi;
>>>> -
>>>> - visit_type_uint8(v, name, &value, errp);
>>>> -}
>>>> -
>>>> static void ich9_lpc_initfn(Object *obj)
>>>> {
>>>> ICH9LPCState *lpc = ICH9_LPC_DEVICE(obj);
>>>> @@ -643,9 +634,8 @@ static void ich9_lpc_initfn(Object *obj)
>>>> static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
>>>> static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
>>>>
>>>> - object_property_add(obj, ACPI_PM_PROP_SCI_INT, "uint8",
>>>> - ich9_lpc_get_sci_int,
>>>> - NULL, NULL, NULL, NULL);
>>>> + object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
>>>> + &lpc->sci_gsi, true, NULL);
>>>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_ENABLE_CMD,
>>>> &acpi_enable_cmd, true, NULL);
>>>> object_property_add_uint8_ptr(obj, ACPI_PM_PROP_ACPI_DISABLE_CMD,
>>>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>>>> index d5e2bdbb57..200503ecfd 100644
>>>> --- a/hw/misc/edu.c
>>>> +++ b/hw/misc/edu.c
>>>> @@ -396,21 +396,13 @@ static void pci_edu_uninit(PCIDevice *pdev)
>>>> msi_uninit(pdev);
>>>> }
>>>>
>>>> -static void edu_obj_uint64(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint64_t *val = opaque;
>>>> -
>>>> - visit_type_uint64(v, name, val, errp);
>>>> -}
>>>> -
>>>> static void edu_instance_init(Object *obj)
>>>> {
>>>> EduState *edu = EDU(obj);
>>>>
>>>> edu->dma_mask = (1UL << 28) - 1;
>>>> - object_property_add(obj, "dma_mask", "uint64", edu_obj_uint64,
>>>> - edu_obj_uint64, NULL, &edu->dma_mask, NULL);
>>>> + object_property_add_uint64_ptr(obj, "dma_mask",
>>>> + &edu->dma_mask, false, NULL);
>>>> }
>>>>
>>>> static void edu_class_init(ObjectClass *class, void *data)
>>>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>>>> index 158d270b9f..61fbe04fe9 100644
>>>> --- a/hw/pci-host/q35.c
>>>> +++ b/hw/pci-host/q35.c
>>>> @@ -165,14 +165,6 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>>>> visit_type_uint64(v, name, &value, errp);
>>>> }
>>>>
>>>> -static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
>>>> -
>>>> - visit_type_uint64(v, name, &e->size, errp);
>>>> -}
>>>> -
>>>> /*
>>>> * NOTE: setting defaults for the mch.* fields in this table
>>>> * doesn't work, because mch is a separate QOM object that is
>>>> @@ -213,6 +205,7 @@ static void q35_host_initfn(Object *obj)
>>>> {
>>>> Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>>> PCIHostState *phb = PCI_HOST_BRIDGE(obj);
>>>> + PCIExpressHost *pehb = PCIE_HOST_BRIDGE(obj);
>>>>
>>>> memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
>>>> "pci-conf-idx", 4);
>>>> @@ -242,9 +235,8 @@ static void q35_host_initfn(Object *obj)
>>>> q35_host_get_pci_hole64_end,
>>>> NULL, NULL, NULL, NULL);
>>>>
>>>> - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64",
>>>> - q35_host_get_mmcfg_size,
>>>> - NULL, NULL, NULL, NULL);
>>>> + object_property_add_uint64_ptr(obj, PCIE_HOST_MCFG_SIZE,
>>>> + &pehb->size, true, NULL);
>>>>
>>>> object_property_add_link(obj, MCH_HOST_PROP_RAM_MEM, TYPE_MEMORY_REGION,
>>>> (Object **) &s->mch.ram_memory,
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index e076f6023c..1b9400526f 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -3227,18 +3227,6 @@ static void spapr_set_resize_hpt(Object *obj, const char *value, Error **errp)
>>>> }
>>>> }
>>>>
>>>> -static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>>>> -}
>>>> -
>>>> -static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>>>> -}
>>>> -
>>>> static char *spapr_get_ic_mode(Object *obj, Error **errp)
>>>> {
>>>> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
>>>> @@ -3336,8 +3324,9 @@ static void spapr_instance_init(Object *obj)
>>>> object_property_set_description(obj, "resize-hpt",
>>>> "Resizing of the Hash Page Table (enabled, disabled, required)",
>>>> NULL);
>>>> - object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
>>>> - spapr_set_vsmt, NULL, &spapr->vsmt, &error_abort);
>>>> + object_property_add_uint32_ptr(obj, "vsmt",
>>>> + &spapr->vsmt, false, &error_abort);
>>>> +
>>>> object_property_set_description(obj, "vsmt",
>>>> "Virtual SMT: KVM behaves as if this were"
>>>> " the host's SMT mode", &error_abort);
>>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>>>> index 136f3a9ad6..34335e071e 100644
>>>> --- a/hw/vfio/pci-quirks.c
>>>> +++ b/hw/vfio/pci-quirks.c
>>>> @@ -2187,14 +2187,6 @@ int vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
>>>> return 0;
>>>> }
>>>>
>>>> -static void vfio_pci_nvlink2_get_tgt(Object *obj, Visitor *v,
>>>> - const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint64_t tgt = (uintptr_t) opaque;
>>>> - visit_type_uint64(v, name, &tgt, errp);
>>>> -}
>>>> -
>>>> static void vfio_pci_nvlink2_get_link_speed(Object *obj, Visitor *v,
>>>> const char *name,
>>>> void *opaque, Error **errp)
>>>> @@ -2240,9 +2232,8 @@ int vfio_pci_nvidia_v100_ram_init(VFIOPCIDevice *vdev, Error **errp)
>>>> nv2reg->size, p);
>>>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>>>>
>>>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>>>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>>>> - (void *) (uintptr_t) cap->tgt, NULL);
>>>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>>>> + (void *)(uintptr_t)cap->tgt, true, NULL);
>>>
>>> nit: (void *) is enough, no?
>>
>> Actually, I think this is completely wrong. I asked AW on IRC (he was
>> away and I didn't wait; oops), but I'll Cc him here and Alexey as well
>> (who wrote the code).
>>
>> Maybe I'm missing something, but it looks like this is passing the
>> absolute value of cap->tgt as a pointer. Shouldn't it just be
>> &cap->tg?
>
>
> Passing &cap->tgt requres @cap to stay in memory until the user of that
> property reads it which is not the case here - the property is set when
> the device is "realized" and used every time the pseries machine is
> reset. This is rather highjacking QOM and properties to pass a 64bit
> value from VFIO to PPC64/pseries without sharing any C structures.
>
>
>> I don't understand the casting to (void *).
>
> object_property_add() takes void*, and cap->tgt is not exactly an
> address so it is not a pointer, it is an abbreviated host hardware
> address which acts here more like a handle/cookie as in fact it is only
> 56bit but whatever :)
>
>
>
>> Later, in
>> vfio_pci_nvlink2_get_*, it does:
>>
>> uint64_t val = (uintptr_t)opaque;
>> visit_type_unitXX(..., &val, ...);
>>
>> It looks to me like that only gets the local variable and doesn't
>> return anything in practice. But again, I'm not familiar with this at
>> all so I may be saying non-sense.
>
>
> This visit_type_unitXX() thing puts @val to the visitor object which is
> then read by object_property_get(). I am having hardtime tracing this
> code, below is the example of a read (hopefully TB won't break
> formatting much). Thanks,
>
>
> (gdb) bt
> #0 0x0000000100b902e0 in qobject_output_add_obj (qov=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", value=0x103b8f000) at
> /home/aik/p/qemu/qapi/qobject-output-visitor.c:83
> #1 0x0000000100b908c0 in qobject_output_type_uint64 (v=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
> at /home/aik/p/qemu/qapi/qobject-output-visitor.c:158
> #2 0x0000000100b8be94 in visit_type_uint64 (v=0x101dbfd70,
> name=0x100dbdaf0 "nvlink2-tgt", obj=0x7fffffffe780, errp=0x7fffffffe888)
> at /home/aik/p/qemu/qapi/qapi-visit-core.c:207
> #3 0x00000001004678f4 in vfio_pci_nvlink2_get_tgt (obj=0x102a84910,
> v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", opaque=0x40000000000,
> errp=0x7fffffffe888) at /home/aik/p/qemu/hw/vfio/pci-quirks.c:2195
> #4 0x0000000100a45720 in object_property_get (obj=0x102a84910,
> v=0x101dbfd70, name=0x100dbdaf0 "nvlink2-tgt", errp=0x7fffffffe888) at
> /home/aik/p/qemu/qom/object.c:1257
> #5 0x0000000100a49fe8 in object_property_get_qobject (obj=0x102a84910,
> name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
> /home/aik/p/qemu/qom/qom-qobject.c:38
> #6 0x0000000100a460c8 in object_property_get_uint (obj=0x102a84910,
> name=0x100dbdaf0 "nvlink2-tgt", errp=0x0) at
> /home/aik/p/qemu/qom/object.c:1407
> #7 0x00000001004eca98 in spapr_phb_pci_collect_nvgpu (bus=0x101d817f0,
> pdev=0x102a84910, opaque=0x101dbfaa0) at
> /home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:139
> #8 0x000000010087795c in pci_for_each_device_under_bus
> (bus=0x101d817f0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
> opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1638
> #9 0x00000001008779fc in pci_for_each_device (bus=0x101d817f0,
> bus_num=0x0, fn=0x1004eca48 <spapr_phb_pci_collect_nvgpu>,
> opaque=0x101dbfaa0) at /home/aik/p/qemu/hw/pci/pci.c:1650
> #10 0x00000001004ecdd0 in spapr_phb_nvgpu_setup (sphb=0x101d34f20,
> errp=0x7fffffffeb68) at /home/aik/p/qemu/hw/ppc/spapr_pci_nvlink2.c:187
> #11 0x00000001004cb8f8 in spapr_phb_reset (qdev=0x101d34f20) at
> /home/aik/p/qemu/hw/ppc/spapr_pci.c:2049
> #12 0x0000000100718858 in device_reset (dev=0x101d34f20) at
> /home/aik/p/qemu/hw/core/qdev.c:1112
> #13 0x00000001007159f0 in qdev_reset_one (dev=0x101d34f20, opaque=0x0)
> at /home/aik/p/qemu/hw/core/qdev.c:277
> #14 0x0000000100716d18 in qdev_walk_children (dev=0x101d34f20,
> pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
> post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
> /home/aik/p/qemu/hw/core/qdev.c:593
> #15 0x000000010071d1ac in qbus_walk_children (bus=0x1016df320,
> pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x1007159c4 <qdev_reset_one>,
> post_busfn=0x100715a18 <qbus_reset_one>, opaque=0x0) at
> /home/aik/p/qemu/hw/core/bus.c:53
> #16 0x0000000100715bf4 in qbus_reset_all (bus=0x1016df320) at
> /home/aik/p/qemu/hw/core/qdev.c:303
> #17 0x0000000100715c4c in qbus_reset_all_fn (opaque=0x1016df320) at
> /home/aik/p/qemu/hw/core/qdev.c:309
> #18 0x000000010071df6c in qemu_devices_reset () at
> /home/aik/p/qemu/hw/core/reset.c:69
> #19 0x00000001004a3554 in spapr_machine_reset (machine=0x1016bec00) at
> /home/aik/p/qemu/hw/ppc/spapr.c:1684
> #20 0x0000000100688488 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE)
> at /home/aik/p/qemu/vl.c:1550
> #21 0x0000000100692b3c in main (argc=0x2e, argv=0x7ffffffff568,
> envp=0x7ffffffff6e0) at /home/aik/p/qemu/vl.c:4419
>
>
>>
>> If this is confirmed to be wrong, I can fix this in an extra patch in
>> this series. Thoughts welcome.
>>
>> F.
>>
>>>
>>>> trace_vfio_pci_nvidia_gpu_setup_quirk(vdev->vbasedev.name, cap->tgt,
>>>> nv2reg->size);
>>>> free_exit:
>>>> @@ -2301,9 +2292,8 @@ int vfio_pci_nvlink2_init(VFIOPCIDevice *vdev, Error **errp)
>>>> QLIST_INSERT_HEAD(&vdev->bars[0].quirks, quirk, next);
>>>> }
>>>>
>>>> - object_property_add(OBJECT(vdev), "nvlink2-tgt", "uint64",
>>>> - vfio_pci_nvlink2_get_tgt, NULL, NULL,
>>>> - (void *) (uintptr_t) captgt->tgt, NULL);
>>>> + object_property_add_uint64_ptr(OBJECT(vdev), "nvlink2-tgt",
>>>> + (void *)(uintptr_t)captgt->tgt, true, NULL);
>>>
>>> same
>>>
>>>> trace_vfio_pci_nvlink2_setup_quirk_ssatgt(vdev->vbasedev.name, captgt->tgt,
>>>> atsdreg->size);
>>>>
>>>> diff --git a/memory.c b/memory.c
>>>> index 06484c2bff..0a34ee3c86 100644
>>>> --- a/memory.c
>>>> +++ b/memory.c
>>>> @@ -1158,15 +1158,6 @@ void memory_region_init(MemoryRegion *mr,
>>>> memory_region_do_init(mr, owner, name, size);
>>>> }
>>>>
>>>> -static void memory_region_get_addr(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - MemoryRegion *mr = MEMORY_REGION(obj);
>>>> - uint64_t value = mr->addr;
>>>> -
>>>> - visit_type_uint64(v, name, &value, errp);
>>>> -}
>>>> -
>>>> static void memory_region_get_container(Object *obj, Visitor *v,
>>>> const char *name, void *opaque,
>>>> Error **errp)
>>>> @@ -1230,10 +1221,8 @@ static void memory_region_initfn(Object *obj)
>>>> NULL, NULL, &error_abort);
>>>> op->resolve = memory_region_resolve_container;
>>>>
>>>> - object_property_add(OBJECT(mr), "addr", "uint64",
>>>> - memory_region_get_addr,
>>>> - NULL, /* memory_region_set_addr */
>>>> - NULL, NULL, &error_abort);
>>>> + object_property_add_uint64_ptr(OBJECT(mr), "addr",
>>>> + &mr->addr, true, &error_abort);
>>>> object_property_add(OBJECT(mr), "priority", "uint32",
>>>> memory_region_get_priority,
>>>> NULL, /* memory_region_set_priority */
>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>> index 7a4ac9339b..aa7278e540 100644
>>>> --- a/target/arm/cpu.c
>>>> +++ b/target/arm/cpu.c
>>>> @@ -1039,22 +1039,6 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>>>> cpu->has_pmu = value;
>>>> }
>>>>
>>>> -static void arm_get_init_svtor(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ARMCPU *cpu = ARM_CPU(obj);
>>>> -
>>>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>>>> -}
>>>> -
>>>> -static void arm_set_init_svtor(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - ARMCPU *cpu = ARM_CPU(obj);
>>>> -
>>>> - visit_type_uint32(v, name, &cpu->init_svtor, errp);
>>>> -}
>>>> -
>>>> void arm_cpu_post_init(Object *obj)
>>>> {
>>>> ARMCPU *cpu = ARM_CPU(obj);
>>>> @@ -1165,9 +1149,8 @@ void arm_cpu_post_init(Object *obj)
>>>> * a simple DEFINE_PROP_UINT32 for this because we want to permit
>>>> * the property to be set after realize.
>>>> */
>>>> - object_property_add(obj, "init-svtor", "uint32",
>>>> - arm_get_init_svtor, arm_set_init_svtor,
>>>> - NULL, NULL, &error_abort);
>>>> + object_property_add_uint32_ptr(obj, "init-svtor",
>>>> + &cpu->init_svtor, false, &error_abort);
>>>> }
>>>>
>>>> qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
>>>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>>>> index 024bb24e51..23d7ab8b72 100644
>>>> --- a/target/i386/sev.c
>>>> +++ b/target/i386/sev.c
>>>> @@ -266,94 +266,6 @@ qsev_guest_class_init(ObjectClass *oc, void *data)
>>>> "guest owners session parameters (encoded with base64)", NULL);
>>>> }
>>>>
>>>> -static void
>>>> -qsev_guest_set_handle(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> - uint32_t value;
>>>> -
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> - sev->handle = value;
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_set_policy(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> - uint32_t value;
>>>> -
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> - sev->policy = value;
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_set_cbitpos(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> - uint32_t value;
>>>> -
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> - sev->cbitpos = value;
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_set_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> - uint32_t value;
>>>> -
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> - sev->reduced_phys_bits = value;
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_get_policy(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint32_t value;
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> -
>>>> - value = sev->policy;
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_get_handle(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint32_t value;
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> -
>>>> - value = sev->handle;
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_get_cbitpos(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint32_t value;
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> -
>>>> - value = sev->cbitpos;
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> -}
>>>> -
>>>> -static void
>>>> -qsev_guest_get_reduced_phys_bits(Object *obj, Visitor *v, const char *name,
>>>> - void *opaque, Error **errp)
>>>> -{
>>>> - uint32_t value;
>>>> - QSevGuestInfo *sev = QSEV_GUEST_INFO(obj);
>>>> -
>>>> - value = sev->reduced_phys_bits;
>>>> - visit_type_uint32(v, name, &value, errp);
>>>> -}
>>>> -
>>>> static void
>>>> qsev_guest_init(Object *obj)
>>>> {
>>>> @@ -361,15 +273,11 @@ qsev_guest_init(Object *obj)
>>>>
>>>> sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
>>>> sev->policy = DEFAULT_GUEST_POLICY;
>>>> - object_property_add(obj, "policy", "uint32", qsev_guest_get_policy,
>>>> - qsev_guest_set_policy, NULL, NULL, NULL);
>>>> - object_property_add(obj, "handle", "uint32", qsev_guest_get_handle,
>>>> - qsev_guest_set_handle, NULL, NULL, NULL);
>>>> - object_property_add(obj, "cbitpos", "uint32", qsev_guest_get_cbitpos,
>>>> - qsev_guest_set_cbitpos, NULL, NULL, NULL);
>>>> - object_property_add(obj, "reduced-phys-bits", "uint32",
>>>> - qsev_guest_get_reduced_phys_bits,
>>>> - qsev_guest_set_reduced_phys_bits, NULL, NULL, NULL);
>>>> + object_property_add_uint32_ptr(obj, "policy", &sev->policy, false, NULL);
>>>> + object_property_add_uint32_ptr(obj, "handle", &sev->handle, false, NULL);
>>>> + object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, false, NULL);
>>>> + object_property_add_uint32_ptr(obj, "reduced-phys-bits",
>>>> + &sev->reduced_phys_bits, false, NULL);
>>>> }
>>>>
>>>> /* sev guest info */
>>>> --
>>>> 2.20.1
>>>>
>>>
>>>
>>> --
>>> Marc-André Lureau
>>
>
> --
> Alexey
Thanks for the analysis. If you are happy with the usage, I'll send a
v2 of my series shortly which doesn't really touch this code.
Cheers,
F.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] qom/object: enable setter for uint types
2020-02-03 15:54 [PATCH 0/4] Improve default object property_add " Felipe Franciosi
@ 2020-02-03 15:54 ` Felipe Franciosi
0 siblings, 0 replies; 18+ messages in thread
From: Felipe Franciosi @ 2020-02-03 15:54 UTC (permalink / raw)
To: Marc-Andre Lureau, Phillipe 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 | 48 ++++++++--
qom/object.c | 214 ++++++++++++++++++++++++++++++++++++++-----
ui/console.c | 4 +-
8 files changed, 247 insertions(+), 49 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 6d621c31e7..c37749cb96 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 f85b484eac..0b6cf6f0dd 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -644,9 +644,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 17aeac3801..4d7f9fd765 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 29546496c1..784c97c0e1 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1664,69 +1664,101 @@ ObjectProperty *object_class_property_add_tm(ObjectClass *klass,
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);
+
ObjectProperty *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);
+
ObjectProperty *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);
+
ObjectProperty *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);
+
ObjectProperty *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 555c8b9d07..8492811220 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2498,6 +2498,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)
{
@@ -2505,6 +2521,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)
{
@@ -2512,6 +2544,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)
{
@@ -2519,68 +2567,184 @@ 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);
}
ObjectProperty *
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)
{
+ 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;
+ }
+
return object_class_property_add(klass, name, "uint8",
- property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
+ 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);
}
ObjectProperty *
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)
{
+ 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;
+ }
+
return object_class_property_add(klass, name, "uint16",
- property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
+ 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);
}
ObjectProperty *
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)
{
- return 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);
}
ObjectProperty *
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)
{
+ 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;
+ }
+
return object_class_property_add(klass, name, "uint64",
- property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
+ getter, setter, NULL, (void *)v, errp);
}
typedef struct {
diff --git a/ui/console.c b/ui/console.c
index 179901c35e..184e173687 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1299,8 +1299,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] 18+ messages in thread
end of thread, other threads:[~2020-02-03 15:55 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25 15:36 [PATCH 0/4] Improve default object property_add uint helpers Felipe Franciosi
2019-11-25 15:36 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
2019-11-26 8:40 ` Marc-André Lureau
2019-11-26 12:03 ` Felipe Franciosi
2019-11-26 12:18 ` Marc-André Lureau
2019-11-26 13:41 ` Felipe Franciosi
2019-11-25 15:36 ` [PATCH 2/4] ich9: fix getter type for sci_int property Felipe Franciosi
2019-11-25 16:43 ` Philippe Mathieu-Daudé
2019-11-26 9:55 ` Felipe Franciosi
2019-11-26 8:40 ` Marc-André Lureau
2019-11-25 15:36 ` [PATCH 3/4] ich9: Simplify ich9_lpc_initfn Felipe Franciosi
2019-11-26 8:39 ` Marc-André Lureau
2019-11-25 15:36 ` [PATCH 4/4] qom/object: Use common get/set uint helpers Felipe Franciosi
2019-11-26 8:39 ` Marc-André Lureau
2019-11-26 9:39 ` Felipe Franciosi
2019-11-27 23:58 ` Alexey Kardashevskiy
2019-11-28 16:12 ` Felipe Franciosi
2020-02-03 15:54 [PATCH 0/4] Improve default object property_add " Felipe Franciosi
2020-02-03 15:54 ` [PATCH 1/4] qom/object: enable setter for uint types Felipe Franciosi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.