* [PATCH v2 1/4] qapi: Add visit_type_str_preserving()
2024-07-14 7:48 [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
@ 2024-07-14 7:48 ` Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 2/4] qapi: Do not consume a value when visit_type_enum() fails Akihiko Odaki
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2024-07-14 7:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, Markus Armbruster
Cc: qemu-devel, qemu-block, Akihiko Odaki
visit_type_str_preserving() is mostly indentical with visit_type_str()
but leaves the value intact. This is useful when the caller may
interpret the value with a different type.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/qapi/visitor-impl.h | 3 ++-
include/qapi/visitor.h | 20 ++++++++++++++++++++
qapi/opts-visitor.c | 7 +++++--
qapi/qapi-clone-visitor.c | 2 +-
qapi/qapi-dealloc-visitor.c | 4 ++--
qapi/qapi-forward-visitor.c | 4 +++-
qapi/qapi-visit-core.c | 17 +++++++++++++++--
qapi/qobject-input-visitor.c | 23 ++++++++++++-----------
qapi/qobject-output-visitor.c | 2 +-
qapi/string-input-visitor.c | 2 +-
qapi/string-output-visitor.c | 2 +-
11 files changed, 63 insertions(+), 23 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2badec5ba460..7b61f55995b5 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -95,7 +95,8 @@ struct Visitor
bool (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp);
/* Must be set */
- bool (*type_str)(Visitor *v, const char *name, char **obj, Error **errp);
+ bool (*type_str)(Visitor *v, const char *name, char **obj, bool consume,
+ Error **errp);
/* Must be set to visit numbers */
bool (*type_number)(Visitor *v, const char *name, double *obj,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 27b85d4700f2..b3ae3188edfb 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -654,6 +654,26 @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
*/
bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
+/*
+ * Visit a string value but do not consume it.
+ *
+ * @name expresses the relationship of this string to its parent
+ * container; see the general description of @name above.
+ *
+ * @obj must be non-NULL. Input and clone visitors set *@obj to the
+ * value (always using "" rather than NULL for an empty string).
+ * Other visitors leave *@obj unchanged, and commonly treat NULL like
+ * "".
+ *
+ * This function must be called only with an input visitor.
+ *
+ * This is mostly identical with visit_type_str() but leaves the value intact.
+ * This is useful when the caller may interpret the value with a different
+ * type.
+ */
+bool visit_type_str_preserving(Visitor *v, const char *name, char **obj,
+ Error **errp);
+
/*
* Visit a number (i.e. double) value.
*
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 3d1a28b41918..e9fad756e189 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -347,7 +347,8 @@ processed(OptsVisitor *ov, const char *name)
static bool
-opts_type_str(Visitor *v, const char *name, char **obj, Error **errp)
+opts_type_str(Visitor *v, const char *name, char **obj, bool consume,
+ Error **errp)
{
OptsVisitor *ov = to_ov(v);
const QemuOpt *opt;
@@ -363,7 +364,9 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp)
* valid enum value; this is harmless because tracking what gets
* consumed only matters to visit_end_struct() as the final error
* check if there were no other failures during the visit. */
- processed(ov, name);
+ if (consume) {
+ processed(ov, name);
+ }
return true;
}
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index bbf953698f38..90e6641835a7 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -108,7 +108,7 @@ static bool qapi_clone_type_bool(Visitor *v, const char *name, bool *obj,
}
static bool qapi_clone_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
QapiCloneVisitor *qcv = to_qcv(v);
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index ef283f296601..6b0f957047d5 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -62,9 +62,9 @@ static void qapi_dealloc_end_list(Visitor *v, void **obj)
}
static bool qapi_dealloc_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
- if (obj) {
+ if (obj && consume) {
g_free(*obj);
}
return true;
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
index e36d9bc9ba7e..891282e0c3b3 100644
--- a/qapi/qapi-forward-visitor.c
+++ b/qapi/qapi-forward-visitor.c
@@ -180,6 +180,7 @@ static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj,
}
static bool forward_field_type_str(Visitor *v, const char *name, char **obj,
+ bool consume,
Error **errp)
{
ForwardFieldVisitor *ffv = to_ffv(v);
@@ -187,7 +188,8 @@ static bool forward_field_type_str(Visitor *v, const char *name, char **obj,
if (!forward_field_translate_name(ffv, &name, errp)) {
return false;
}
- return visit_type_str(ffv->target, name, obj, errp);
+ return (consume ? visit_type_str : visit_type_str_preserving)(
+ ffv->target, name, obj, errp);
}
static bool forward_field_type_size(Visitor *v, const char *name, uint64_t *obj,
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6c13510a2bc7..89b52fc99202 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -336,7 +336,8 @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
return v->type_bool(v, name, obj, errp);
}
-bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
+static bool visit_type_str_common(Visitor *v, const char *name, char **obj,
+ bool consume, Error **errp)
{
bool ok;
@@ -346,13 +347,25 @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
assert(!(v->type & VISITOR_OUTPUT) || *obj);
*/
trace_visit_type_str(v, name, obj);
- ok = v->type_str(v, name, obj, errp);
+ ok = v->type_str(v, name, obj, consume, errp);
if (v->type & VISITOR_INPUT) {
assert(ok != !*obj);
}
return ok;
}
+bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
+{
+ return visit_type_str_common(v, name, obj, true, errp);
+}
+
+bool visit_type_str_preserving(Visitor *v, const char *name, char **obj,
+ Error **errp)
+{
+ assert(v->type & VISITOR_INPUT);
+ return visit_type_str_common(v, name, obj, false, errp);
+}
+
bool visit_type_number(Visitor *v, const char *name, double *obj,
Error **errp)
{
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f110a804b2ae..facbbf01bd27 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -174,13 +174,13 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
}
static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
- const char *name,
+ const char *name, bool consume,
Error **errp)
{
QObject *qobj;
QString *qstr;
- qobj = qobject_input_get_object(qiv, name, true, errp);
+ qobj = qobject_input_get_object(qiv, name, consume, errp);
if (!qobj) {
return NULL;
}
@@ -416,7 +416,7 @@ static bool qobject_input_type_int64_keyval(Visitor *v, const char *name,
int64_t *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, true, errp);
if (!str) {
return false;
@@ -467,7 +467,7 @@ static bool qobject_input_type_uint64_keyval(Visitor *v, const char *name,
uint64_t *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, true, errp);
if (!str) {
return false;
@@ -507,7 +507,7 @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name,
bool *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, true, errp);
if (!str) {
return false;
@@ -522,10 +522,10 @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name,
}
static bool qobject_input_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+ QObject *qobj = qobject_input_get_object(qiv, name, consume, errp);
QString *qstr;
*obj = NULL;
@@ -544,10 +544,11 @@ static bool qobject_input_type_str(Visitor *v, const char *name, char **obj,
}
static bool qobject_input_type_str_keyval(Visitor *v, const char *name,
- char **obj, Error **errp)
+ char **obj, bool consume,
+ Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, consume, errp);
*obj = g_strdup(str);
return !!str;
@@ -578,7 +579,7 @@ static bool qobject_input_type_number_keyval(Visitor *v, const char *name,
double *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, true, errp);
double val;
if (!str) {
@@ -635,7 +636,7 @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name,
uint64_t *obj, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
- const char *str = qobject_input_get_keyval(qiv, name, errp);
+ const char *str = qobject_input_get_keyval(qiv, name, true, errp);
if (!str) {
return false;
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 74770edd73c6..36a9fc245b79 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -173,7 +173,7 @@ static bool qobject_output_type_bool(Visitor *v, const char *name, bool *obj,
}
static bool qobject_output_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
if (*obj) {
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 3f1b9e9b4137..6562bbe3cfd4 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -336,7 +336,7 @@ static bool parse_type_bool(Visitor *v, const char *name, bool *obj,
}
static bool parse_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
StringInputVisitor *siv = to_siv(v);
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 5115536b1539..9a8cca0d5c68 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -269,7 +269,7 @@ static bool print_type_bool(Visitor *v, const char *name, bool *obj,
}
static bool print_type_str(Visitor *v, const char *name, char **obj,
- Error **errp)
+ bool consume, Error **errp)
{
StringOutputVisitor *sov = to_sov(v);
char *out;
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 2/4] qapi: Do not consume a value when visit_type_enum() fails
2024-07-14 7:48 [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 1/4] qapi: Add visit_type_str_preserving() Akihiko Odaki
@ 2024-07-14 7:48 ` Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 3/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2024-07-14 7:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, Markus Armbruster
Cc: qemu-devel, qemu-block, Akihiko Odaki
Consuming a value when visit_type_enum() fails makes it impossible to
reinterpret the value with a different type.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/qapi/visitor.h | 5 -----
qapi/opts-visitor.c | 5 -----
qapi/qapi-visit-core.c | 4 +++-
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b3ae3188edfb..8e841b26428b 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -513,11 +513,6 @@ void visit_set_policy(Visitor *v, CompatPolicy *policy);
* is an input visitor.
*
* Return true on success, false on failure.
- *
- * May call visit_type_str() under the hood, and the enum visit may
- * fail even if the corresponding string visit succeeded; this implies
- * that an input visitor's visit_type_str() must have no unwelcome
- * side effects.
*/
bool visit_type_enum(Visitor *v, const char *name, int *obj,
const QEnumLookup *lookup, Error **errp);
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index e9fad756e189..d83434b95a56 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -359,11 +359,6 @@ opts_type_str(Visitor *v, const char *name, char **obj, bool consume,
return false;
}
*obj = g_strdup(opt->str ? opt->str : "");
- /* Note that we consume a string even if this is called as part of
- * an enum visit that later fails because the string is not a
- * valid enum value; this is harmless because tracking what gets
- * consumed only matters to visit_end_struct() as the final error
- * check if there were no other failures during the visit. */
if (consume) {
processed(ov, name);
}
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 89b52fc99202..1137d472290b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -411,7 +411,7 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
int64_t value;
g_autofree char *enum_str = NULL;
- if (!visit_type_str(v, name, &enum_str, errp)) {
+ if (!visit_type_str_preserving(v, name, &enum_str, errp)) {
return false;
}
@@ -430,6 +430,8 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
return false;
}
+ enum_str = NULL;
+ visit_type_str(v, name, &enum_str, &error_abort);
*obj = value;
return true;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 3/4] hw/pci: Convert rom_bar into OnOffAuto
2024-07-14 7:48 [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 1/4] qapi: Add visit_type_str_preserving() Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 2/4] qapi: Do not consume a value when visit_type_enum() fails Akihiko Odaki
@ 2024-07-14 7:48 ` Akihiko Odaki
2024-07-14 7:48 ` [PATCH v2 4/4] hw/qdev: Remove opts member Akihiko Odaki
2024-07-31 8:32 ` [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Markus Armbruster
4 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2024-07-14 7:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, Markus Armbruster
Cc: qemu-devel, qemu-block, Akihiko Odaki
rom_bar is tristate but was defined as uint32_t so convert it into
OnOffAuto to clarify that. For compatibility, a uint32 value set via
QOM will be converted into OnOffAuto.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
docs/about/deprecated.rst | 7 +++++
docs/igd-assign.txt | 2 +-
include/hw/pci/pci_device.h | 2 +-
hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++--
hw/vfio/pci-quirks.c | 2 +-
hw/vfio/pci.c | 11 ++++----
hw/xen/xen_pt_load_rom.c | 4 +--
tests/qtest/virtio-net-failover.c | 32 +++++++++++-----------
8 files changed, 88 insertions(+), 29 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 20b7a17cf07e..ac8d40fd0e84 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -389,6 +389,13 @@ recommending to switch to their stable counterparts:
SD physical layer specification v2.00 supersedes the v1.10 one.
v2.00 is the default since QEMU 3.0.0.
+Integer specification of PCI device's ``rombar`` property (since 9.1)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Specifying an integer for the ``rombar`` property of a PCI device is being
+deprecated starting in 9.1 due to obscurity of such an specification. Replace
+zero with ``off`` and a non-zero value with ``on``, respectively.
+
Block device options
''''''''''''''''''''
diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
index e17bb50789ad..35c6c8e28493 100644
--- a/docs/igd-assign.txt
+++ b/docs/igd-assign.txt
@@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
PCI address 1f.0.
* The IGD device must have a VGA ROM, either provided via the romfile
- option or loaded automatically through vfio (standard). rombar=0
+ option or loaded automatically through vfio (standard). rombar=off
will disable legacy mode support.
* Hotplug of the IGD device is not supported.
* The IGD device must be a SandyBridge or newer model device.
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index ca151325085d..49b341ce2e27 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -147,7 +147,7 @@ struct PCIDevice {
uint32_t romsize;
bool has_rom;
MemoryRegion rom;
- uint32_t rom_bar;
+ OnOffAuto rom_bar;
/* INTx routing notifier */
PCIINTxRoutingNotifier intx_routing_notifier;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c7be5295110..ca8fb5383765 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -67,11 +67,64 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
static void pcibus_reset_hold(Object *obj, ResetType type);
static bool pcie_has_upstream_port(PCIDevice *dev);
+static void rom_bar_get(Object *obj, Visitor *v, const char *name, void *opaque,
+ Error **errp)
+{
+ Property *prop = opaque;
+ int *ptr = object_field_prop_ptr(obj, prop);
+
+ visit_type_enum(v, name, ptr, prop->info->enum_table, errp);
+}
+
+static void rom_bar_set(Object *obj, Visitor *v, const char *name, void *opaque,
+ Error **errp)
+{
+ Property *prop = opaque;
+ Error *local_err = NULL;
+ int *ptr = object_field_prop_ptr(obj, prop);
+ uint32_t value;
+
+ visit_type_enum(v, name, ptr, prop->info->enum_table, &local_err);
+ if (!local_err) {
+ return;
+ }
+
+ if (visit_type_uint32(v, name, &value, NULL)) {
+ if (value) {
+ *ptr = ON_OFF_AUTO_ON;
+ warn_report("Specifying a number for rombar is deprecated; replace a non-zero value with 'on'");
+ } else {
+ *ptr = ON_OFF_AUTO_OFF;
+ warn_report("Specifying a number for rombar is deprecated; replace 0 with 'off'");
+ }
+
+ return;
+ }
+
+ error_propagate(errp, local_err);
+}
+
+static void rom_bar_set_default_value(ObjectProperty *op, const Property *prop)
+{
+ object_property_set_default_str(op,
+ qapi_enum_lookup(prop->info->enum_table, prop->defval.i));
+}
+
+static const PropertyInfo qdev_prop_rom_bar = {
+ .name = "OnOffAuto",
+ .description = "on/off/auto",
+ .enum_table = &OnOffAuto_lookup,
+ .get = rom_bar_get,
+ .set = rom_bar_set,
+ .set_default_value = rom_bar_set_default_value,
+};
+
static Property pci_props[] = {
DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX),
- DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
+ DEFINE_PROP_SIGNED("rombar", PCIDevice, rom_bar, ON_OFF_AUTO_AUTO,
+ qdev_prop_rom_bar, OnOffAuto),
DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present,
@@ -2334,7 +2387,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
return;
}
- if (!pdev->rom_bar) {
+ if (pdev->rom_bar == ON_OFF_AUTO_OFF) {
/*
* Load rom via fw_cfg instead of creating a rom bar,
* for 0.11 compatibility.
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 39dae72497e0..0e920ed0691a 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -33,7 +33,7 @@
* execution as noticed with the BCM 57810 card for lack of a
* more better way to handle such issues.
* The user can still override by specifying a romfile or
- * rombar=1.
+ * rombar=on.
* Please see https://bugs.launchpad.net/qemu/+bug/1284874
* for an analysis of the 57810 card hang. When adding
* a new vendor id/device id combination below, please also add
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e03d9f3ba546..502ee2ed0489 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -902,7 +902,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
error_report("vfio-pci: Cannot read device rom at "
"%s", vdev->vbasedev.name);
error_printf("Device option ROM contents are probably invalid "
- "(check dmesg).\nSkip option ROM probe with rombar=0, "
+ "(check dmesg).\nSkip option ROM probe with rombar=off, "
"or load from file with romfile=\n");
return;
}
@@ -1012,11 +1012,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
{
uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
- DeviceState *dev = DEVICE(vdev);
char *name;
int fd = vdev->vbasedev.fd;
- if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
+ if (vdev->pdev.romfile || vdev->pdev.rom_bar == ON_OFF_AUTO_OFF) {
/* Since pci handles romfile, just print a message and return */
if (vfio_opt_rom_in_denylist(vdev) && vdev->pdev.romfile) {
warn_report("Device at %s is known to cause system instability"
@@ -1046,17 +1045,17 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
}
if (vfio_opt_rom_in_denylist(vdev)) {
- if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
+ if (vdev->pdev.rom_bar == ON_OFF_AUTO_ON) {
warn_report("Device at %s is known to cause system instability"
" issues during option rom execution",
vdev->vbasedev.name);
error_printf("Proceeding anyway since user specified"
- " non zero value for rombar\n");
+ " on for rombar\n");
} else {
warn_report("Rom loading for device at %s has been disabled"
" due to system instability issues",
vdev->vbasedev.name);
- error_printf("Specify rombar=1 or romfile to force\n");
+ error_printf("Specify rombar=on or romfile to force\n");
return;
}
}
diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index 6bc64acd3352..025a6b25a916 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -26,7 +26,7 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
Object *owner = OBJECT(dev);
/* If loading ROM from file, pci handles it */
- if (dev->romfile || !dev->rom_bar) {
+ if (dev->romfile || dev->rom_bar == ON_OFF_AUTO_OFF) {
return NULL;
}
@@ -71,7 +71,7 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
if (!fread(ptr, 1, st.st_size, fp)) {
error_report("pci-assign: Cannot read from host %s", rom_file);
error_printf("Device option ROM contents are probably invalid "
- "(check dmesg).\nSkip option ROM probe with rombar=0, "
+ "(check dmesg).\nSkip option ROM probe with rombar=off, "
"or load from file with romfile=\n");
goto close_rom;
}
diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 73dfabc2728b..f65b97683fb6 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -568,7 +568,7 @@ static void test_hotplug_2_reverse(void)
"{'bus': 'root0',"
"'failover': true,"
"'netdev': 'hs0',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_STANDBY0"'}");
@@ -655,7 +655,7 @@ static void test_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -765,7 +765,7 @@ static void test_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -819,7 +819,7 @@ static void test_off_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -887,7 +887,7 @@ static void test_off_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -938,7 +938,7 @@ static void test_guest_off_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1014,7 +1014,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1065,7 +1065,7 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1170,7 +1170,7 @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1259,7 +1259,7 @@ static void test_migrate_abort_active(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1358,7 +1358,7 @@ static void test_migrate_off_abort(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1450,7 +1450,7 @@ static void test_migrate_abort_timeout(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1543,7 +1543,7 @@ static void test_multi_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1574,7 +1574,7 @@ static void test_multi_out(gconstpointer opaque)
"{'bus': 'root3',"
"'failover_pair_id': 'standby1',"
"'netdev': 'hs3',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY1"'}");
@@ -1713,7 +1713,7 @@ static void test_multi_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1737,7 +1737,7 @@ static void test_multi_in(gconstpointer opaque)
"{'bus': 'root3',"
"'failover_pair_id': 'standby1',"
"'netdev': 'hs3',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY1"'}");
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 4/4] hw/qdev: Remove opts member
2024-07-14 7:48 [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
` (2 preceding siblings ...)
2024-07-14 7:48 ` [PATCH v2 3/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
@ 2024-07-14 7:48 ` Akihiko Odaki
2024-07-31 8:32 ` [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Markus Armbruster
4 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2024-07-14 7:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, Markus Armbruster
Cc: qemu-devel, qemu-block, Akihiko Odaki
It is no longer used.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/qdev-core.h | 4 ----
hw/core/qdev.c | 1 -
system/qdev-monitor.c | 12 +++++++-----
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5336728a23f6..40f2d185f17c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -237,10 +237,6 @@ struct DeviceState {
* @pending_deleted_expires_ms: optional timeout for deletion events
*/
int64_t pending_deleted_expires_ms;
- /**
- * @opts: QDict of options for the device
- */
- QDict *opts;
/**
* @hotplugged: was device added after PHASE_MACHINE_READY?
*/
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57dee..2fc84699d432 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -706,7 +706,6 @@ static void device_finalize(Object *obj)
dev->canonical_path = NULL;
}
- qobject_unref(dev->opts);
g_free(dev->id);
}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d667f..3551989d5153 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -624,6 +624,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
char *id;
DeviceState *dev = NULL;
BusState *bus = NULL;
+ QDict *properties;
driver = qdict_get_try_str(opts, "driver");
if (!driver) {
@@ -705,13 +706,14 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
}
/* set properties */
- dev->opts = qdict_clone_shallow(opts);
- qdict_del(dev->opts, "driver");
- qdict_del(dev->opts, "bus");
- qdict_del(dev->opts, "id");
+ properties = qdict_clone_shallow(opts);
+ qdict_del(properties, "driver");
+ qdict_del(properties, "bus");
+ qdict_del(properties, "id");
- object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json,
+ object_set_properties_from_keyval(&dev->parent_obj, properties, from_json,
errp);
+ qobject_unref(properties);
if (*errp) {
goto err_del_dev;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-07-14 7:48 [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Akihiko Odaki
` (3 preceding siblings ...)
2024-07-14 7:48 ` [PATCH v2 4/4] hw/qdev: Remove opts member Akihiko Odaki
@ 2024-07-31 8:32 ` Markus Armbruster
2024-07-31 8:40 ` Michael S. Tsirkin
2024-08-01 7:01 ` Akihiko Odaki
4 siblings, 2 replies; 13+ messages in thread
From: Markus Armbruster @ 2024-07-31 8:32 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> rom_bar is tristate but was defined as uint32_t so convert it into
> OnOffAuto to clarify that. For compatibility, a uint32 value set via
> QOM will be converted into OnOffAuto.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
I agree making property "rombar" an integer was a design mistake. I
further agree that vfio_pci_size_rom() peeking into dev->opts to
distinguish "user didn't set a value" from "user set the default value")
is an unadvisable hack.
However, ...
> ---
> Changes in v2:
> - Documented in docs/about/deprecated.rst (Daniel P. Berrangé)
> - Link to v1: https://lore.kernel.org/r/20240706-rombar-v1-0-802daef2aec1@daynix.com
>
> ---
> Akihiko Odaki (4):
> qapi: Add visit_type_str_preserving()
> qapi: Do not consume a value when visit_type_enum() fails
> hw/pci: Convert rom_bar into OnOffAuto
> hw/qdev: Remove opts member
>
> docs/about/deprecated.rst | 7 +++++
> docs/igd-assign.txt | 2 +-
> include/hw/pci/pci_device.h | 2 +-
> include/hw/qdev-core.h | 4 ---
> include/qapi/visitor-impl.h | 3 ++-
> include/qapi/visitor.h | 25 +++++++++++++----
> hw/core/qdev.c | 1 -
> hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++--
> hw/vfio/pci-quirks.c | 2 +-
> hw/vfio/pci.c | 11 ++++----
> hw/xen/xen_pt_load_rom.c | 4 +--
> qapi/opts-visitor.c | 12 ++++-----
> qapi/qapi-clone-visitor.c | 2 +-
> qapi/qapi-dealloc-visitor.c | 4 +--
> qapi/qapi-forward-visitor.c | 4 ++-
> qapi/qapi-visit-core.c | 21 ++++++++++++---
> qapi/qobject-input-visitor.c | 23 ++++++++--------
> qapi/qobject-output-visitor.c | 2 +-
> qapi/string-input-visitor.c | 2 +-
> qapi/string-output-visitor.c | 2 +-
> system/qdev-monitor.c | 12 +++++----
> tests/qtest/virtio-net-failover.c | 32 +++++++++++-----------
> 22 files changed, 161 insertions(+), 73 deletions(-)
> ---
> base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
> change-id: 20240704-rombar-1a4ba2890d6c
>
> Best regards,
... this is an awful lot of QAPI visitor infrastructure. Infrastructure
that will likely only ever be used for this one property.
Moreover, the property setter rom_bar_set() is a hack: it first tries to
parse the value as enum, and if that fails, as uint32_t. QAPI already
provides a way to express "either this type or that type": alternate.
Like this:
{ 'alternate': 'OnOffAutoUint32',
'data': { 'sym': 'OnOffAuto',
'uint': 'uint32' } }
Unfortunately, such alternates don't work on the command line due to
keyval visitor restrictions. These cannot be lifted entirely, but we
might be able to lift them sufficiently to make this alternate work.
Whether it would be worth your trouble and mine just to clean up
"rombar" seems highly dubious, though.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-07-31 8:32 ` [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Markus Armbruster
@ 2024-07-31 8:40 ` Michael S. Tsirkin
2024-08-01 7:01 ` Akihiko Odaki
1 sibling, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-07-31 8:40 UTC (permalink / raw)
To: Markus Armbruster
Cc: Akihiko Odaki, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
On Wed, Jul 31, 2024 at 10:32:19AM +0200, Markus Armbruster wrote:
> Whether it would be worth your trouble and mine just to clean up
> "rombar" seems highly dubious, though.
Exactly.
--
MST
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-07-31 8:32 ` [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto Markus Armbruster
2024-07-31 8:40 ` Michael S. Tsirkin
@ 2024-08-01 7:01 ` Akihiko Odaki
2024-08-01 7:52 ` Michael S. Tsirkin
2024-08-01 10:59 ` Markus Armbruster
1 sibling, 2 replies; 13+ messages in thread
From: Akihiko Odaki @ 2024-08-01 7:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
On 2024/07/31 17:32, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
>> rom_bar is tristate but was defined as uint32_t so convert it into
>> OnOffAuto to clarify that. For compatibility, a uint32 value set via
>> QOM will be converted into OnOffAuto.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> I agree making property "rombar" an integer was a design mistake. I
> further agree that vfio_pci_size_rom() peeking into dev->opts to
> distinguish "user didn't set a value" from "user set the default value")
> is an unadvisable hack.
>
> However, ...
>
>> ---
>> Changes in v2:
>> - Documented in docs/about/deprecated.rst (Daniel P. Berrangé)
>> - Link to v1: https://lore.kernel.org/r/20240706-rombar-v1-0-802daef2aec1@daynix.com
>>
>> ---
>> Akihiko Odaki (4):
>> qapi: Add visit_type_str_preserving()
>> qapi: Do not consume a value when visit_type_enum() fails
>> hw/pci: Convert rom_bar into OnOffAuto
>> hw/qdev: Remove opts member
>>
>> docs/about/deprecated.rst | 7 +++++
>> docs/igd-assign.txt | 2 +-
>> include/hw/pci/pci_device.h | 2 +-
>> include/hw/qdev-core.h | 4 ---
>> include/qapi/visitor-impl.h | 3 ++-
>> include/qapi/visitor.h | 25 +++++++++++++----
>> hw/core/qdev.c | 1 -
>> hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++--
>> hw/vfio/pci-quirks.c | 2 +-
>> hw/vfio/pci.c | 11 ++++----
>> hw/xen/xen_pt_load_rom.c | 4 +--
>> qapi/opts-visitor.c | 12 ++++-----
>> qapi/qapi-clone-visitor.c | 2 +-
>> qapi/qapi-dealloc-visitor.c | 4 +--
>> qapi/qapi-forward-visitor.c | 4 ++-
>> qapi/qapi-visit-core.c | 21 ++++++++++++---
>> qapi/qobject-input-visitor.c | 23 ++++++++--------
>> qapi/qobject-output-visitor.c | 2 +-
>> qapi/string-input-visitor.c | 2 +-
>> qapi/string-output-visitor.c | 2 +-
>> system/qdev-monitor.c | 12 +++++----
>> tests/qtest/virtio-net-failover.c | 32 +++++++++++-----------
>> 22 files changed, 161 insertions(+), 73 deletions(-)
>> ---
>> base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
>> change-id: 20240704-rombar-1a4ba2890d6c
>>
>> Best regards,
>
> ... this is an awful lot of QAPI visitor infrastructure. Infrastructure
> that will likely only ever be used for this one property.
>
> Moreover, the property setter rom_bar_set() is a hack: it first tries to
> parse the value as enum, and if that fails, as uint32_t. QAPI already
> provides a way to express "either this type or that type": alternate.
> Like this:
>
> { 'alternate': 'OnOffAutoUint32',
> 'data': { 'sym': 'OnOffAuto',
> 'uint': 'uint32' } }
>
> Unfortunately, such alternates don't work on the command line due to
> keyval visitor restrictions. These cannot be lifted entirely, but we
> might be able to lift them sufficiently to make this alternate work.
The keyval visitor cannot implement alternates because the command line
input does not have type information. For example, you cannot
distinguish string "0" and integer 0.
>
> Whether it would be worth your trouble and mine just to clean up
> "rombar" seems highly dubious, though.
rom_bar_set() and and underlying visit_type_str_preserving() are ugly,
but we can remove them once the deprecation period ends. On the other
hand, if we don't make this change, dev->opts will keep floating around,
and we will even have another use of it for "[PATCH v5 1/8] hw/pci: Do
not add ROM BAR for SR-IOV VF"[1]. Eventually, having this refactoring
early will result in less mess in the future.
[1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-08-01 7:01 ` Akihiko Odaki
@ 2024-08-01 7:52 ` Michael S. Tsirkin
2024-08-01 8:39 ` Akihiko Odaki
2024-08-01 10:59 ` Markus Armbruster
1 sibling, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-08-01 7:52 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Markus Armbruster, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote:
> rom_bar_set() and and underlying visit_type_str_preserving() are ugly, but
> we can remove them once the deprecation period ends. On the other hand, if
> we don't make this change, dev->opts will keep floating around, and we will
> even have another use of it for "[PATCH v5 1/8] hw/pci: Do not add ROM BAR
> for SR-IOV VF"[1]. Eventually, having this refactoring early will result in
> less mess in the future.
>
> [1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
I don't know that this should worry us much. Is there some project that
requires getting rid of dev->opts for some reason?
--
MST
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-08-01 7:52 ` Michael S. Tsirkin
@ 2024-08-01 8:39 ` Akihiko Odaki
2024-08-01 11:05 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Akihiko Odaki @ 2024-08-01 8:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Markus Armbruster, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
On 2024/08/01 16:52, Michael S. Tsirkin wrote:
> On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote:
>> rom_bar_set() and and underlying visit_type_str_preserving() are ugly, but
>> we can remove them once the deprecation period ends. On the other hand, if
>> we don't make this change, dev->opts will keep floating around, and we will
>> even have another use of it for "[PATCH v5 1/8] hw/pci: Do not add ROM BAR
>> for SR-IOV VF"[1]. Eventually, having this refactoring early will result in
>> less mess in the future.
>>
>> [1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
>
> I don't know that this should worry us much. Is there some project that
> requires getting rid of dev->opts for some reason?
No, but I just thought it is too ugly to add a new reference to it for
the aforementioned patch, circumventing the visitor code and the QOM
property.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-08-01 8:39 ` Akihiko Odaki
@ 2024-08-01 11:05 ` Markus Armbruster
0 siblings, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2024-08-01 11:05 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Markus Armbruster,
Philippe Mathieu-Daudé, Marcel Apfelbaum, Alex Williamson,
Cédric Le Goater, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Sriram Yagnaraman, Jason Wang, Keith Busch,
Klaus Jensen, qemu-devel, qemu-block
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> On 2024/08/01 16:52, Michael S. Tsirkin wrote:
>> On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote:
>>> rom_bar_set() and and underlying visit_type_str_preserving() are ugly, but
>>> we can remove them once the deprecation period ends. On the other hand, if
>>> we don't make this change, dev->opts will keep floating around, and we will
>>> even have another use of it for "[PATCH v5 1/8] hw/pci: Do not add ROM BAR
>>> for SR-IOV VF"[1]. Eventually, having this refactoring early will result in
>>> less mess in the future.
>>>
>>> [1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
>> I don't know that this should worry us much. Is there some project that
>> requires getting rid of dev->opts for some reason?
>
> No, but I just thought it is too ugly to add a new reference to it for the aforementioned patch, circumventing the visitor code and the QOM property.
Use of dev->opts is an ugly hack. I'd love to get rid of it, if the
price is right.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-08-01 7:01 ` Akihiko Odaki
2024-08-01 7:52 ` Michael S. Tsirkin
@ 2024-08-01 10:59 ` Markus Armbruster
2024-08-01 12:22 ` Michael S. Tsirkin
1 sibling, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2024-08-01 10:59 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> On 2024/07/31 17:32, Markus Armbruster wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>
>>> rom_bar is tristate but was defined as uint32_t so convert it into
>>> OnOffAuto to clarify that. For compatibility, a uint32 value set via
>>> QOM will be converted into OnOffAuto.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> I agree making property "rombar" an integer was a design mistake. I
>> further agree that vfio_pci_size_rom() peeking into dev->opts to
>> distinguish "user didn't set a value" from "user set the default value")
>> is an unadvisable hack.
>>
>> However, ...
>>
>>> ---
>>> Changes in v2:
>>> - Documented in docs/about/deprecated.rst (Daniel P. Berrangé)
>>> - Link to v1: https://lore.kernel.org/r/20240706-rombar-v1-0-802daef2aec1@daynix.com
>>>
>>> ---
>>> Akihiko Odaki (4):
>>> qapi: Add visit_type_str_preserving()
>>> qapi: Do not consume a value when visit_type_enum() fails
>>> hw/pci: Convert rom_bar into OnOffAuto
>>> hw/qdev: Remove opts member
>>>
>>> docs/about/deprecated.rst | 7 +++++
>>> docs/igd-assign.txt | 2 +-
>>> include/hw/pci/pci_device.h | 2 +-
>>> include/hw/qdev-core.h | 4 ---
>>> include/qapi/visitor-impl.h | 3 ++-
>>> include/qapi/visitor.h | 25 +++++++++++++----
>>> hw/core/qdev.c | 1 -
>>> hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++--
>>> hw/vfio/pci-quirks.c | 2 +-
>>> hw/vfio/pci.c | 11 ++++----
>>> hw/xen/xen_pt_load_rom.c | 4 +--
>>> qapi/opts-visitor.c | 12 ++++-----
>>> qapi/qapi-clone-visitor.c | 2 +-
>>> qapi/qapi-dealloc-visitor.c | 4 +--
>>> qapi/qapi-forward-visitor.c | 4 ++-
>>> qapi/qapi-visit-core.c | 21 ++++++++++++---
>>> qapi/qobject-input-visitor.c | 23 ++++++++--------
>>> qapi/qobject-output-visitor.c | 2 +-
>>> qapi/string-input-visitor.c | 2 +-
>>> qapi/string-output-visitor.c | 2 +-
>>> system/qdev-monitor.c | 12 +++++----
>>> tests/qtest/virtio-net-failover.c | 32 +++++++++++-----------
>>> 22 files changed, 161 insertions(+), 73 deletions(-)
>>> ---
>>> base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
>>> change-id: 20240704-rombar-1a4ba2890d6c
>>>
>>> Best regards,
>>
>> ... this is an awful lot of QAPI visitor infrastructure. Infrastructure
>> that will likely only ever be used for this one property.
>>
>> Moreover, the property setter rom_bar_set() is a hack: it first tries to
>> parse the value as enum, and if that fails, as uint32_t. QAPI already
>> provides a way to express "either this type or that type": alternate.
>> Like this:
>>
>> { 'alternate': 'OnOffAutoUint32',
>> 'data': { 'sym': 'OnOffAuto',
>> 'uint': 'uint32' } }
>>
>> Unfortunately, such alternates don't work on the command line due to
>> keyval visitor restrictions. These cannot be lifted entirely, but we
>> might be able to lift them sufficiently to make this alternate work.
>
> The keyval visitor cannot implement alternates because the command line
> input does not have type information. For example, you cannot
> distinguish string "0" and integer 0.
Correct.
For alternate types, an input visitor picks the branch based on the
QType.
With JSON, we have scalar types null, number, string, and bool.
With keyval, we only have string. Alternates with more than one scalar
branch don't work.
They could be made to work to some degree, though. Observe:
* Any value can be a string.
* "frob" can be nothing else.
* "on" and "off" can also be bool.
* "123" and "1e3" can also be number or enum.
Instead of picking the branch based on the QType, we could pick based on
QType and value, where the value part is delegated to a visitor method.
This is also new infrastructure. But it's more generally useful
infrastructure.
>> Whether it would be worth your trouble and mine just to clean up
>> "rombar" seems highly dubious, though.
>
> rom_bar_set() and and underlying visit_type_str_preserving() are ugly,
> but we can remove them once the deprecation period ends. On the other
> hand, if we don't make this change, dev->opts will keep floating around,
> and we will even have another use of it for "[PATCH v5 1/8] hw/pci: Do
> not add ROM BAR for SR-IOV VF"[1]. Eventually, having this refactoring
> early will result in less mess in the future.
>
> [1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
Here's another idea.
Property "rombar" is backed by PCIDevice member uint32_t rom_bar, and
defaults to 1.
The code uses member rom_bar as if it was a boolean: it tests
zero/non-zero.
vfio_pci_size_rom() additionally uses qdict_haskey(dev->opts, "rombar")
to see whether "rombar" was set by the user.
Taken together, "rom_bar" has three abstract states: zero,
non-zero-default, non-zero-user. The concrete representation uses
dev->opts in addition to member rom_bar. This is unusual.
You propose to represent as OnOffAuto instead, with On for
non-zero-user, Off for zero, Auto for non-zero-default. Fine, except
for the compatibility headaches.
OnOffAuto is not the only option for encoding these three states. We
could also do positive, 0, negative. Like this:
* Change "rombar" from unsigned to signed.
* Change its default to -1.
* Now 0 means off, positive means on, and negative means auto.
The change to signed breaks rombar=N for 2^31<=N<2^32. Do we care?
Only if we have reason to fear something passes such values. I doubt
it. I'd expect only rombar=0 and rombar=1.
If we do care, we could create a special kind of property that maps any
positive value to 1.
With the change, we no longer reject rombar=N for -2^31<=N<0. That's
not a compatiblity break.
Thoughts?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto
2024-08-01 10:59 ` Markus Armbruster
@ 2024-08-01 12:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2024-08-01 12:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Akihiko Odaki, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Paolo Bonzini,
Daniel P. Berrangé, Eduardo Habkost, Sriram Yagnaraman,
Jason Wang, Keith Busch, Klaus Jensen, qemu-devel, qemu-block
On Thu, Aug 01, 2024 at 12:59:57PM +0200, Markus Armbruster wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> > On 2024/07/31 17:32, Markus Armbruster wrote:
> >> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> >>
> >>> rom_bar is tristate but was defined as uint32_t so convert it into
> >>> OnOffAuto to clarify that. For compatibility, a uint32 value set via
> >>> QOM will be converted into OnOffAuto.
> >>>
> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>
> >> I agree making property "rombar" an integer was a design mistake. I
> >> further agree that vfio_pci_size_rom() peeking into dev->opts to
> >> distinguish "user didn't set a value" from "user set the default value")
> >> is an unadvisable hack.
> >>
> >> However, ...
> >>
> >>> ---
> >>> Changes in v2:
> >>> - Documented in docs/about/deprecated.rst (Daniel P. Berrangé)
> >>> - Link to v1: https://lore.kernel.org/r/20240706-rombar-v1-0-802daef2aec1@daynix.com
> >>>
> >>> ---
> >>> Akihiko Odaki (4):
> >>> qapi: Add visit_type_str_preserving()
> >>> qapi: Do not consume a value when visit_type_enum() fails
> >>> hw/pci: Convert rom_bar into OnOffAuto
> >>> hw/qdev: Remove opts member
> >>>
> >>> docs/about/deprecated.rst | 7 +++++
> >>> docs/igd-assign.txt | 2 +-
> >>> include/hw/pci/pci_device.h | 2 +-
> >>> include/hw/qdev-core.h | 4 ---
> >>> include/qapi/visitor-impl.h | 3 ++-
> >>> include/qapi/visitor.h | 25 +++++++++++++----
> >>> hw/core/qdev.c | 1 -
> >>> hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++--
> >>> hw/vfio/pci-quirks.c | 2 +-
> >>> hw/vfio/pci.c | 11 ++++----
> >>> hw/xen/xen_pt_load_rom.c | 4 +--
> >>> qapi/opts-visitor.c | 12 ++++-----
> >>> qapi/qapi-clone-visitor.c | 2 +-
> >>> qapi/qapi-dealloc-visitor.c | 4 +--
> >>> qapi/qapi-forward-visitor.c | 4 ++-
> >>> qapi/qapi-visit-core.c | 21 ++++++++++++---
> >>> qapi/qobject-input-visitor.c | 23 ++++++++--------
> >>> qapi/qobject-output-visitor.c | 2 +-
> >>> qapi/string-input-visitor.c | 2 +-
> >>> qapi/string-output-visitor.c | 2 +-
> >>> system/qdev-monitor.c | 12 +++++----
> >>> tests/qtest/virtio-net-failover.c | 32 +++++++++++-----------
> >>> 22 files changed, 161 insertions(+), 73 deletions(-)
> >>> ---
> >>> base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
> >>> change-id: 20240704-rombar-1a4ba2890d6c
> >>>
> >>> Best regards,
> >>
> >> ... this is an awful lot of QAPI visitor infrastructure. Infrastructure
> >> that will likely only ever be used for this one property.
> >>
> >> Moreover, the property setter rom_bar_set() is a hack: it first tries to
> >> parse the value as enum, and if that fails, as uint32_t. QAPI already
> >> provides a way to express "either this type or that type": alternate.
> >> Like this:
> >>
> >> { 'alternate': 'OnOffAutoUint32',
> >> 'data': { 'sym': 'OnOffAuto',
> >> 'uint': 'uint32' } }
> >>
> >> Unfortunately, such alternates don't work on the command line due to
> >> keyval visitor restrictions. These cannot be lifted entirely, but we
> >> might be able to lift them sufficiently to make this alternate work.
> >
> > The keyval visitor cannot implement alternates because the command line
> > input does not have type information. For example, you cannot
> > distinguish string "0" and integer 0.
>
> Correct.
>
> For alternate types, an input visitor picks the branch based on the
> QType.
>
> With JSON, we have scalar types null, number, string, and bool.
>
> With keyval, we only have string. Alternates with more than one scalar
> branch don't work.
>
> They could be made to work to some degree, though. Observe:
>
> * Any value can be a string.
>
> * "frob" can be nothing else.
>
> * "on" and "off" can also be bool.
>
> * "123" and "1e3" can also be number or enum.
>
> Instead of picking the branch based on the QType, we could pick based on
> QType and value, where the value part is delegated to a visitor method.
>
> This is also new infrastructure. But it's more generally useful
> infrastructure.
>
> >> Whether it would be worth your trouble and mine just to clean up
> >> "rombar" seems highly dubious, though.
> >
> > rom_bar_set() and and underlying visit_type_str_preserving() are ugly,
> > but we can remove them once the deprecation period ends. On the other
> > hand, if we don't make this change, dev->opts will keep floating around,
> > and we will even have another use of it for "[PATCH v5 1/8] hw/pci: Do
> > not add ROM BAR for SR-IOV VF"[1]. Eventually, having this refactoring
> > early will result in less mess in the future.
> >
> > [1]: lore.kernel.org/r/20240715-sriov-v5-1-3f5539093ffc@daynix.com
>
> Here's another idea.
>
> Property "rombar" is backed by PCIDevice member uint32_t rom_bar, and
> defaults to 1.
>
> The code uses member rom_bar as if it was a boolean: it tests
> zero/non-zero.
>
> vfio_pci_size_rom() additionally uses qdict_haskey(dev->opts, "rombar")
> to see whether "rombar" was set by the user.
>
> Taken together, "rom_bar" has three abstract states: zero,
> non-zero-default, non-zero-user. The concrete representation uses
> dev->opts in addition to member rom_bar. This is unusual.
>
> You propose to represent as OnOffAuto instead, with On for
> non-zero-user, Off for zero, Auto for non-zero-default. Fine, except
> for the compatibility headaches.
>
> OnOffAuto is not the only option for encoding these three states. We
> could also do positive, 0, negative. Like this:
>
> * Change "rombar" from unsigned to signed.
>
> * Change its default to -1.
>
> * Now 0 means off, positive means on, and negative means auto.
>
> The change to signed breaks rombar=N for 2^31<=N<2^32. Do we care?
> Only if we have reason to fear something passes such values. I doubt
> it. I'd expect only rombar=0 and rombar=1.
>
> If we do care, we could create a special kind of property that maps any
> positive value to 1.
>
> With the change, we no longer reject rombar=N for -2^31<=N<0. That's
> not a compatiblity break.
>
> Thoughts?
ack
^ permalink raw reply [flat|nested] 13+ messages in thread