On Thu, Aug 01, 2024 at 12:59:57PM +0200, Markus Armbruster wrote: > 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?
ack
