| Message ID | 20240428-auto-v1-0-7b012216a120@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtio-net: Convert feature properties to OnOffAuto | expand |
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>
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.
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
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,