On 2024/07/31 17:32, Markus Armbruster wrote:
Akihiko Odaki <akihiko.od...@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.od...@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-802daef2a...@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-3f5539093...@daynix.com
