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(-)
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>
---
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,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
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.
Whether it would be worth your trouble and mine just to clean up
"rombar" seems highly dubious, though.
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.
>
> 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
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?
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
On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote: > 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 I don't know that this should worry us much. Is there some project that requires getting rid of dev->opts for some reason? -- MST
On 2024/08/01 16:52, Michael S. Tsirkin wrote: > On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote: >> 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 > > I don't know that this should worry us much. Is there some project that > requires getting rid of dev->opts for some reason? No, but I just thought it is too ugly to add a new reference to it for the aforementioned patch, circumventing the visitor code and the QOM property. Regards, Akihiko Odaki
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2024/08/01 16:52, Michael S. Tsirkin wrote: >> On Thu, Aug 01, 2024 at 04:01:44PM +0900, Akihiko Odaki wrote: >>> 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 >> I don't know that this should worry us much. Is there some project that >> requires getting rid of dev->opts for some reason? > > No, but I just thought it is too ugly to add a new reference to it for the aforementioned patch, circumventing the visitor code and the QOM property. Use of dev->opts is an ugly hack. I'd love to get rid of it, if the price is right.
© 2016 - 2024 Red Hat, Inc.