Akihiko Odaki <akihiko.od...@daynix.com> writes: > 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.
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-3f5539093...@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?
