| Message ID | 20240714-rombar-v2-0-af1504ef55de@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/pci: Convert rom_bar into OnOffAuto | expand |
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 Wed, Jul 31, 2024 at 10:32:19AM +0200, Markus Armbruster wrote: > Whether it would be worth your trouble and mine just to clean up > "rombar" seems highly dubious, though. Exactly.
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
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?
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/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?
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.
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
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,