virtio-net: Convert feature properties to OnOffAuto

[PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto

Posted by Akihiko Odaki 1 week, 6 days ago

Based-on: <20240428-rss-v10-0-73cbaa91aeb6@daynix.com>
("[PATCH v10 00/18] virtio-net RSS/hash report fixes and improvements")

Some features are not always available, and virtio-net used to disable
them when not available even if the corresponding properties were
explicitly set to "on".

Convert feature properties to OnOffAuto so that the user can explicitly
tell QEMU to automatically select the value by setting them "auto".
QEMU will give an error if they are set "on".

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (3):
      qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
      virtio-net: Convert feature properties to OnOffAuto
      virtio-net: Report RSS warning at device realization

 include/hw/qdev-properties.h   |  18 +++
 include/hw/virtio/virtio-net.h |   2 +-
 hw/core/qdev-properties.c      |  65 ++++++++++-
 hw/net/virtio-net.c            | 259 +++++++++++++++++++++++++----------------
 4 files changed, 239 insertions(+), 105 deletions(-)
---
base-commit: ec6325eec995018983a3f88f0e78ebf733a47b7e
change-id: 20240428-auto-be0dc010dda5

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto

Posted by Michael S. Tsirkin 1 week, 5 days ago

On Sun, Apr 28, 2024 at 04:21:06PM +0900, Akihiko Odaki wrote:
> Based-on: <20240428-rss-v10-0-73cbaa91aeb6@daynix.com>
> ("[PATCH v10 00/18] virtio-net RSS/hash report fixes and improvements")
> 
> Some features are not always available, and virtio-net used to disable
> them when not available even if the corresponding properties were
> explicitly set to "on".
> 
> Convert feature properties to OnOffAuto so that the user can explicitly
> tell QEMU to automatically select the value by setting them "auto".
> QEMU will give an error if they are set "on".
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Should we maybe bite the bullet allow "auto" for all binary/boolean
properties? Just ignore "auto" if no one cares ATM.



> ---
> Akihiko Odaki (3):
>       qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
>       virtio-net: Convert feature properties to OnOffAuto
>       virtio-net: Report RSS warning at device realization
> 
>  include/hw/qdev-properties.h   |  18 +++
>  include/hw/virtio/virtio-net.h |   2 +-
>  hw/core/qdev-properties.c      |  65 ++++++++++-
>  hw/net/virtio-net.c            | 259 +++++++++++++++++++++++++----------------
>  4 files changed, 239 insertions(+), 105 deletions(-)
> ---
> base-commit: ec6325eec995018983a3f88f0e78ebf733a47b7e
> change-id: 20240428-auto-be0dc010dda5
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto

Posted by Akihiko Odaki 1 week, 3 days ago

On 2024/04/29 16:05, Michael S. Tsirkin wrote:
> On Sun, Apr 28, 2024 at 04:21:06PM +0900, Akihiko Odaki wrote:
>> Based-on: <20240428-rss-v10-0-73cbaa91aeb6@daynix.com>
>> ("[PATCH v10 00/18] virtio-net RSS/hash report fixes and improvements")
>>
>> Some features are not always available, and virtio-net used to disable
>> them when not available even if the corresponding properties were
>> explicitly set to "on".
>>
>> Convert feature properties to OnOffAuto so that the user can explicitly
>> tell QEMU to automatically select the value by setting them "auto".
>> QEMU will give an error if they are set "on".
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Should we maybe bite the bullet allow "auto" for all binary/boolean
> properties? Just ignore "auto" if no one cares ATM.

It is not always obvious whether "auto" should be considered as "on" or 
"off" for existing boolean properties. The properties this patch deals 
with are to enable features so "auto" should be considered as "on" if 
possible. However, other properties may mean to disable features, and in 
such a case, "auto" should be considered as "off".

It may still make sense to accept "auto" for all virtio-net feature bits 
for consistency. In particular, I left guest_rsc_ext property boolean 
since nobody cares "auto" for that feature, but this can be converted to 
OnOffAuto.

Re: [PATCH 0/3] virtio-net: Convert feature properties to OnOffAuto

Posted by Jason Wang 6 days, 1 hour ago

On Wed, May 1, 2024 at 3:20 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/04/29 16:05, Michael S. Tsirkin wrote:
> > On Sun, Apr 28, 2024 at 04:21:06PM +0900, Akihiko Odaki wrote:
> >> Based-on: <20240428-rss-v10-0-73cbaa91aeb6@daynix.com>
> >> ("[PATCH v10 00/18] virtio-net RSS/hash report fixes and improvements")
> >>
> >> Some features are not always available, and virtio-net used to disable
> >> them when not available even if the corresponding properties were
> >> explicitly set to "on".

I think we'd better fail the initialization in this case, otherwise it
might confuse libvirt.

Adding Jonathon for more comments.

> >>
> >> Convert feature properties to OnOffAuto so that the user can explicitly
> >> tell QEMU to automatically select the value by setting them "auto".
> >> QEMU will give an error if they are set "on".
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > Should we maybe bite the bullet allow "auto" for all binary/boolean
> > properties? Just ignore "auto" if no one cares ATM.
>
> It is not always obvious whether "auto" should be considered as "on" or
> "off" for existing boolean properties. The properties this patch deals
> with are to enable features so "auto" should be considered as "on" if
> possible. However, other properties may mean to disable features, and in
> such a case, "auto" should be considered as "off".
>
> It may still make sense to accept "auto" for all virtio-net feature bits
> for consistency. In particular, I left guest_rsc_ext property boolean
> since nobody cares "auto" for that feature, but this can be converted to
> OnOffAuto.
>

Thanks