[PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto

Akihiko Odaki posted 4 patches 11 months, 1 week ago
Failed in applying to current master (apply log)
include/hw/qdev-properties.h |  18 ++++++++
include/hw/virtio/virtio.h   |  38 +++++++++-------
hw/core/machine.c            |   1 +
hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
hw/virtio/virtio-bus.c       |  14 +++++-
hw/virtio/virtio.c           |   4 +-
qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
scripts/qapi/visit.py        |  24 ++++++++++
8 files changed, 229 insertions(+), 56 deletions(-)
[PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto
Posted by Akihiko Odaki 11 months, 1 week ago
This series was spun off from:
"[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
(https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)

Some features are not always available with vhost. Legacy features are
not available with vp_vdpa in particular. virtio devices used to disable
them when not available even if the corresponding properties were
explicitly set to "on".

QEMU already has OnOffAuto type, which includes the "auto" value to let
it automatically decide the effective value. Convert feature properties
to OnOffAuto and set them "auto" by default to utilize it. This allows
QEMU to report an error if they are set "on" and the corresponding
features are not available.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v6:
- Rebased.
- Link to v5: https://lore.kernel.org/qemu-devel/20250208-virtio-v5-0-4376cb218c0f@daynix.com

Changes in v5:
- Covered QAPI more than just qdev.
- Expanded the description of patch
  "qapi: Accept bool for OnOffAuto and OnOffSplit".
- Rebased.
- Link to v4: https://lore.kernel.org/r/20250108-virtio-v4-0-cbf0aa04c9f9@daynix.com

Changes in v4:
- Added patch "qapi: Do not consume a value if failed".
- Link to v3: https://lore.kernel.org/r/20250104-virtio-v3-0-63ef70e9ddf3@daynix.com

Changes in v3:
- Rebased.
- Link to v2: https://lore.kernel.org/r/20241022-virtio-v2-0-b2394236e053@daynix.com

Changes in v2:
- Expanded the message of patch "qdev-properties: Accept bool for
  OnOffAuto".
- Link to v1: https://lore.kernel.org/r/20241014-virtio-v1-0-e9ddf7a81891@daynix.com

---
Akihiko Odaki (4):
      qapi: Do not consume a value if failed
      qapi: Accept bool for OnOffAuto and OnOffSplit
      qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
      virtio: Convert feature properties to OnOffAuto

 include/hw/qdev-properties.h |  18 ++++++++
 include/hw/virtio/virtio.h   |  38 +++++++++-------
 hw/core/machine.c            |   1 +
 hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
 hw/virtio/virtio-bus.c       |  14 +++++-
 hw/virtio/virtio.c           |   4 +-
 qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
 scripts/qapi/visit.py        |  24 ++++++++++
 8 files changed, 229 insertions(+), 56 deletions(-)
---
base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
change-id: 20241013-virtio-164ea3f295c3

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>
Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto
Posted by Michael S. Tsirkin 9 months, 2 weeks ago
On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
> This series was spun off from:
> "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
> (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
> 
> Some features are not always available with vhost. Legacy features are
> not available with vp_vdpa in particular. virtio devices used to disable
> them when not available even if the corresponding properties were
> explicitly set to "on".
> 
> QEMU already has OnOffAuto type, which includes the "auto" value to let
> it automatically decide the effective value. Convert feature properties
> to OnOffAuto and set them "auto" by default to utilize it. This allows
> QEMU to report an error if they are set "on" and the corresponding
> features are not available.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>


Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?

> ---
> Changes in v6:
> - Rebased.
> - Link to v5: https://lore.kernel.org/qemu-devel/20250208-virtio-v5-0-4376cb218c0f@daynix.com
> 
> Changes in v5:
> - Covered QAPI more than just qdev.
> - Expanded the description of patch
>   "qapi: Accept bool for OnOffAuto and OnOffSplit".
> - Rebased.
> - Link to v4: https://lore.kernel.org/r/20250108-virtio-v4-0-cbf0aa04c9f9@daynix.com
> 
> Changes in v4:
> - Added patch "qapi: Do not consume a value if failed".
> - Link to v3: https://lore.kernel.org/r/20250104-virtio-v3-0-63ef70e9ddf3@daynix.com
> 
> Changes in v3:
> - Rebased.
> - Link to v2: https://lore.kernel.org/r/20241022-virtio-v2-0-b2394236e053@daynix.com
> 
> Changes in v2:
> - Expanded the message of patch "qdev-properties: Accept bool for
>   OnOffAuto".
> - Link to v1: https://lore.kernel.org/r/20241014-virtio-v1-0-e9ddf7a81891@daynix.com
> 
> ---
> Akihiko Odaki (4):
>       qapi: Do not consume a value if failed
>       qapi: Accept bool for OnOffAuto and OnOffSplit
>       qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
>       virtio: Convert feature properties to OnOffAuto
> 
>  include/hw/qdev-properties.h |  18 ++++++++
>  include/hw/virtio/virtio.h   |  38 +++++++++-------
>  hw/core/machine.c            |   1 +
>  hw/core/qdev-properties.c    |  83 +++++++++++++++++++++++++++++++++-
>  hw/virtio/virtio-bus.c       |  14 +++++-
>  hw/virtio/virtio.c           |   4 +-
>  qapi/qobject-input-visitor.c | 103 +++++++++++++++++++++++++++++--------------
>  scripts/qapi/visit.py        |  24 ++++++++++
>  8 files changed, 229 insertions(+), 56 deletions(-)
> ---
> base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
> change-id: 20241013-virtio-164ea3f295c3
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>
Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto
Posted by Daniel P. Berrangé 9 months, 2 weeks ago
On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
> > This series was spun off from:
> > "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
> > (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
> > 
> > Some features are not always available with vhost. Legacy features are
> > not available with vp_vdpa in particular. virtio devices used to disable
> > them when not available even if the corresponding properties were
> > explicitly set to "on".
> > 
> > QEMU already has OnOffAuto type, which includes the "auto" value to let
> > it automatically decide the effective value. Convert feature properties
> > to OnOffAuto and set them "auto" by default to utilize it. This allows
> > QEMU to report an error if they are set "on" and the corresponding
> > features are not available.
> > 
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> 
> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?

I've said on every previous version that I don't think we should be
changing OnOffAuto to secretly accept bool values. That is bypassing
QAPI schema definitions with a special code hack.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto
Posted by Markus Armbruster 9 months, 2 weeks ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
>> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
>> > This series was spun off from:
>> > "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
>> > (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
>> > 
>> > Some features are not always available with vhost. Legacy features are
>> > not available with vp_vdpa in particular. virtio devices used to disable
>> > them when not available even if the corresponding properties were
>> > explicitly set to "on".
>> > 
>> > QEMU already has OnOffAuto type, which includes the "auto" value to let
>> > it automatically decide the effective value. Convert feature properties
>> > to OnOffAuto and set them "auto" by default to utilize it. This allows
>> > QEMU to report an error if they are set "on" and the corresponding
>> > features are not available.
>> > 
>> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> 
>> 
>> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?
>
> I've said on every previous version that I don't think we should be
> changing OnOffAuto to secretly accept bool values. That is bypassing
> QAPI schema definitions with a special code hack.

I also objected to v4.  Thread starts at

    Message-ID: <87cyfwxveo.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/87cyfwxveo.fsf@pond.sub.org/

I could be persuaded to accept a patch that changes exactly the
properties that need to be changed to tri-state, with suitable
rationale.  This patch changes a bunch of unrelated properties, too.
Re: [PATCH v6 0/4] virtio: Convert feature properties to OnOffAuto
Posted by Akihiko Odaki 9 months, 1 week ago
On 2025/04/26 0:08, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Fri, Apr 25, 2025 at 08:14:13AM -0400, Michael S. Tsirkin wrote:
>>> On Thu, Mar 06, 2025 at 03:16:26PM +0900, Akihiko Odaki wrote:
>>>> This series was spun off from:
>>>> "[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto"
>>>> (https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/)
>>>>
>>>> Some features are not always available with vhost. Legacy features are
>>>> not available with vp_vdpa in particular. virtio devices used to disable
>>>> them when not available even if the corresponding properties were
>>>> explicitly set to "on".
>>>>
>>>> QEMU already has OnOffAuto type, which includes the "auto" value to let
>>>> it automatically decide the effective value. Convert feature properties
>>>> to OnOffAuto and set them "auto" by default to utilize it. This allows
>>>> QEMU to report an error if they are set "on" and the corresponding
>>>> features are not available.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>
>>>
>>> Marcus, Paolo, Daniel, Eduardo, any feedback on the QOM bits?
>>
>> I've said on every previous version that I don't think we should be
>> changing OnOffAuto to secretly accept bool values. That is bypassing
>> QAPI schema definitions with a special code hack.
> 
> I also objected to v4.  Thread starts at
> 
>      Message-ID: <87cyfwxveo.fsf@pond.sub.org>
>      https://lore.kernel.org/qemu-devel/87cyfwxveo.fsf@pond.sub.org/
> 
> I could be persuaded to accept a patch that changes exactly the
> properties that need to be changed to tri-state, with suitable
> rationale.  This patch changes a bunch of unrelated properties, too.
> 

I replied to the thread as I found that I haven't replied to the last 
message in the thread and the newer versions of the series do not 
address its discussion points either.

Regards,
Akihiko Odaki