diff mbox series

[v3,3/3] virtio: Convert feature properties to OnOffAuto

Message ID 20250104-virtio-v3-3-63ef70e9ddf3@daynix.com (mailing list archive)
State New
Headers show
Series virtio: Convert feature properties to OnOffAuto | expand

Commit Message

Akihiko Odaki Jan. 4, 2025, 7:36 a.m. UTC 
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>
---
 include/hw/virtio/virtio.h | 38 +++++++++++++++++++++-----------------
 hw/core/machine.c          |  4 +++-
 hw/virtio/virtio-bus.c     | 14 ++++++++++++--
 hw/virtio/virtio.c         |  4 +++-
 4 files changed, 39 insertions(+), 21 deletions(-)

Comments

Lei Yang Jan. 7, 2025, 5:52 a.m. UTC | #1 
Hi Akihiko

I hit qemu core dump when I tested this series of patches with
virtio-net regression tests, and based on the test result this bug can
be reproduced by just booting a guest. For the details of core dump
info please review the attachment.

Regards

Lei

On Sat, Jan 4, 2025 at 3:37 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> 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>
> ---
>  include/hw/virtio/virtio.h | 38 +++++++++++++++++++++-----------------
>  hw/core/machine.c          |  4 +++-
>  hw/virtio/virtio-bus.c     | 14 ++++++++++++--
>  hw/virtio/virtio.c         |  4 +++-
>  4 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 638691028050..b854c2cb1d04 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -113,7 +113,8 @@ struct VirtIODevice
>      uint16_t queue_sel;
>      /**
>       * These fields represent a set of VirtIO features at various
> -     * levels of the stack. @host_features indicates the complete
> +     * levels of the stack. @requested_features indicates the feature
> +     * set the user requested. @host_features indicates the complete
>       * feature set the VirtIO device can offer to the driver.
>       * @guest_features indicates which features the VirtIO driver has
>       * selected by writing to the feature register. Finally
> @@ -121,6 +122,7 @@ struct VirtIODevice
>       * backend (e.g. vhost) and could potentially be a subset of the
>       * total feature set offered by QEMU.
>       */
> +    OnOffAutoBit64 requested_features;
>      uint64_t host_features;
>      uint64_t guest_features;
>      uint64_t backend_features;
> @@ -149,6 +151,7 @@ struct VirtIODevice
>      bool started;
>      bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
>      bool disable_legacy_check;
> +    bool force_features_auto;
>      bool vhost_started;
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
> @@ -376,22 +379,23 @@ typedef struct VirtIOSCSIConf VirtIOSCSIConf;
>  typedef struct VirtIORNGConf VirtIORNGConf;
>
>  #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
> -    DEFINE_PROP_BIT64("indirect_desc", _state, _field,    \
> -                      VIRTIO_RING_F_INDIRECT_DESC, true), \
> -    DEFINE_PROP_BIT64("event_idx", _state, _field,        \
> -                      VIRTIO_RING_F_EVENT_IDX, true),     \
> -    DEFINE_PROP_BIT64("notify_on_empty", _state, _field,  \
> -                      VIRTIO_F_NOTIFY_ON_EMPTY, true), \
> -    DEFINE_PROP_BIT64("any_layout", _state, _field, \
> -                      VIRTIO_F_ANY_LAYOUT, true), \
> -    DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> -                      VIRTIO_F_IOMMU_PLATFORM, false), \
> -    DEFINE_PROP_BIT64("packed", _state, _field, \
> -                      VIRTIO_F_RING_PACKED, false), \
> -    DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> -                      VIRTIO_F_RING_RESET, true), \
> -    DEFINE_PROP_BIT64("in_order", _state, _field, \
> -                      VIRTIO_F_IN_ORDER, false)
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("indirect_desc", _state, _field, \
> +                                  VIRTIO_RING_F_INDIRECT_DESC, \
> +                                  ON_OFF_AUTO_AUTO), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("event_idx", _state, _field, \
> +                                  VIRTIO_RING_F_EVENT_IDX, ON_OFF_AUTO_AUTO), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("notify_on_empty", _state, _field, \
> +                                  VIRTIO_F_NOTIFY_ON_EMPTY, ON_OFF_AUTO_AUTO), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("any_layout", _state, _field, \
> +                                  VIRTIO_F_ANY_LAYOUT, ON_OFF_AUTO_AUTO), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("iommu_platform", _state, _field, \
> +                                  VIRTIO_F_IOMMU_PLATFORM, ON_OFF_AUTO_OFF), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("packed", _state, _field, \
> +                                  VIRTIO_F_RING_PACKED, ON_OFF_AUTO_OFF), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("queue_reset", _state, _field, \
> +                                  VIRTIO_F_RING_RESET, ON_OFF_AUTO_AUTO), \
> +    DEFINE_PROP_ON_OFF_AUTO_BIT64("in_order", _state, _field, \
> +                                  VIRTIO_F_IN_ORDER, ON_OFF_AUTO_OFF)
>
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>  bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c949af97668d..bff26b95dd74 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -36,7 +36,9 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "audio/audio.h"
>
> -GlobalProperty hw_compat_9_2[] = {};
> +GlobalProperty hw_compat_9_2[] = {
> +    { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
> +};
>  const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
>
>  GlobalProperty hw_compat_9_1[] = {
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 896feb37a1ca..75d433b252d5 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -50,6 +50,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>      bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>      bool vdev_has_iommu;
>      Error *local_err = NULL;
> +    uint64_t features;
>
>      DPRINTF("%s: plug device.\n", qbus->name);
>
> @@ -63,13 +64,22 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>
>      /* Get the features of the plugged device. */
>      assert(vdc->get_features != NULL);
> -    vdev->host_features = vdc->get_features(vdev, vdev->host_features,
> -                                            &local_err);
> +    features = vdev->host_features | vdev->requested_features.auto_bits |
> +               vdev->requested_features.on_bits;
> +    features = vdc->get_features(vdev, features, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>
> +    if (!vdev->force_features_auto &&
> +        (features & vdev->requested_features.on_bits) != vdev->requested_features.on_bits) {
> +        error_setg(errp, "A requested feature is not supported by the device");
> +        return;
> +    }
> +
> +    vdev->host_features = features;
> +
>      if (klass->device_plugged != NULL) {
>          klass->device_plugged(qbus->parent, &local_err);
>      }
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 85110bce3744..83f803fc703d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4013,11 +4013,13 @@ static void virtio_device_instance_finalize(Object *obj)
>  }
>
>  static const Property virtio_properties[] = {
> -    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
> +    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, requested_features),
>      DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
>      DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
>      DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
>                       disable_legacy_check, false),
> +    DEFINE_PROP_BOOL("x-force-features-auto", VirtIODevice,
> +                     force_features_auto, false),
>  };
>
>  static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
>
> --
> 2.47.1
>
>
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 638691028050..b854c2cb1d04 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -113,7 +113,8 @@  struct VirtIODevice
     uint16_t queue_sel;
     /**
      * These fields represent a set of VirtIO features at various
-     * levels of the stack. @host_features indicates the complete
+     * levels of the stack. @requested_features indicates the feature
+     * set the user requested. @host_features indicates the complete
      * feature set the VirtIO device can offer to the driver.
      * @guest_features indicates which features the VirtIO driver has
      * selected by writing to the feature register. Finally
@@ -121,6 +122,7 @@  struct VirtIODevice
      * backend (e.g. vhost) and could potentially be a subset of the
      * total feature set offered by QEMU.
      */
+    OnOffAutoBit64 requested_features;
     uint64_t host_features;
     uint64_t guest_features;
     uint64_t backend_features;
@@ -149,6 +151,7 @@  struct VirtIODevice
     bool started;
     bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
     bool disable_legacy_check;
+    bool force_features_auto;
     bool vhost_started;
     VMChangeStateEntry *vmstate;
     char *bus_name;
@@ -376,22 +379,23 @@  typedef struct VirtIOSCSIConf VirtIOSCSIConf;
 typedef struct VirtIORNGConf VirtIORNGConf;
 
 #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
-    DEFINE_PROP_BIT64("indirect_desc", _state, _field,    \
-                      VIRTIO_RING_F_INDIRECT_DESC, true), \
-    DEFINE_PROP_BIT64("event_idx", _state, _field,        \
-                      VIRTIO_RING_F_EVENT_IDX, true),     \
-    DEFINE_PROP_BIT64("notify_on_empty", _state, _field,  \
-                      VIRTIO_F_NOTIFY_ON_EMPTY, true), \
-    DEFINE_PROP_BIT64("any_layout", _state, _field, \
-                      VIRTIO_F_ANY_LAYOUT, true), \
-    DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
-                      VIRTIO_F_IOMMU_PLATFORM, false), \
-    DEFINE_PROP_BIT64("packed", _state, _field, \
-                      VIRTIO_F_RING_PACKED, false), \
-    DEFINE_PROP_BIT64("queue_reset", _state, _field, \
-                      VIRTIO_F_RING_RESET, true), \
-    DEFINE_PROP_BIT64("in_order", _state, _field, \
-                      VIRTIO_F_IN_ORDER, false)
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("indirect_desc", _state, _field, \
+                                  VIRTIO_RING_F_INDIRECT_DESC, \
+                                  ON_OFF_AUTO_AUTO), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("event_idx", _state, _field, \
+                                  VIRTIO_RING_F_EVENT_IDX, ON_OFF_AUTO_AUTO), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("notify_on_empty", _state, _field, \
+                                  VIRTIO_F_NOTIFY_ON_EMPTY, ON_OFF_AUTO_AUTO), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("any_layout", _state, _field, \
+                                  VIRTIO_F_ANY_LAYOUT, ON_OFF_AUTO_AUTO), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("iommu_platform", _state, _field, \
+                                  VIRTIO_F_IOMMU_PLATFORM, ON_OFF_AUTO_OFF), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("packed", _state, _field, \
+                                  VIRTIO_F_RING_PACKED, ON_OFF_AUTO_OFF), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("queue_reset", _state, _field, \
+                                  VIRTIO_F_RING_RESET, ON_OFF_AUTO_AUTO), \
+    DEFINE_PROP_ON_OFF_AUTO_BIT64("in_order", _state, _field, \
+                                  VIRTIO_F_IN_ORDER, ON_OFF_AUTO_OFF)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c949af97668d..bff26b95dd74 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -36,7 +36,9 @@ 
 #include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_9_2[] = {};
+GlobalProperty hw_compat_9_2[] = {
+    { TYPE_VIRTIO_DEVICE, "x-force-features-auto", "on" },
+};
 const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2);
 
 GlobalProperty hw_compat_9_1[] = {
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 896feb37a1ca..75d433b252d5 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -50,6 +50,7 @@  void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
     bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
     bool vdev_has_iommu;
     Error *local_err = NULL;
+    uint64_t features;
 
     DPRINTF("%s: plug device.\n", qbus->name);
 
@@ -63,13 +64,22 @@  void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
 
     /* Get the features of the plugged device. */
     assert(vdc->get_features != NULL);
-    vdev->host_features = vdc->get_features(vdev, vdev->host_features,
-                                            &local_err);
+    features = vdev->host_features | vdev->requested_features.auto_bits |
+               vdev->requested_features.on_bits;
+    features = vdc->get_features(vdev, features, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
+    if (!vdev->force_features_auto &&
+        (features & vdev->requested_features.on_bits) != vdev->requested_features.on_bits) {
+        error_setg(errp, "A requested feature is not supported by the device");
+        return;
+    }
+
+    vdev->host_features = features;
+
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent, &local_err);
     }
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 85110bce3744..83f803fc703d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4013,11 +4013,13 @@  static void virtio_device_instance_finalize(Object *obj)
 }
 
 static const Property virtio_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, requested_features),
     DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
     DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
     DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
                      disable_legacy_check, false),
+    DEFINE_PROP_BOOL("x-force-features-auto", VirtIODevice,
+                     force_features_auto, false),
 };
 
 static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)