Re: [PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto

[Michael S. Tsirkin]

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