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

[Markus Armbruster]

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: 
> 20240706-rombar-v1-0-802daef2aec1@daynix.com">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.