1. Patchew
  2. QEMU
  3. [PATCH v2 0/2] qmp: Remove virtio_list & update virtio introspection
  4. View patch

[PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection

Jonah Palmer posted 2 patches 2 years, 8 months ago
[PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection
Posted by Jonah Palmer 2 years, 8 months ago 
Add new virtio transport feature to transport feature map:
 - VIRTIO_F_RING_RESET

Add new vhost-user protocol feature to vhost-user protocol feature map
and enumeration:
 - VHOST_USER_PROTOCOL_F_STATUS

Add new virtio device features for several virtio devices to their
respective feature mappings:

virtio-blk:
 - VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
 - VIRTIO_NET_F_NOTF_COAL
 - VIRTIO_NET_F_GUEST_USO4
 - VIRTIO_NET_F_GUEST_USO6
 - VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
 - VIRTIO_GPIO_F_IRQ
 - VHOST_F_LOG_ALL
 - VHOST_USER_F_PROTOCOL_FEATURES

virtio-bt:
 - VIRTIO_BT_F_VND_HCI
 - VIRTIO_BT_F_MSFT_EXT
 - VIRTIO_BT_F_AOSP_EXT
 - VIRTIO_BT_F_CONFIG_V2

virtio-scmi:
 - VIRTIO_SCMI_F_P2A_CHANNELS
 - VIRTIO_SCMI_F_SHARED_MEMORY

Add support for introspection on vhost-user-gpio devices.

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/vhost-user-gpio.c |  7 ++++
 hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index d6927b610a..e88ca5370f 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
     vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
 }
 
+static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    return &gpio->vhost_dev;
+}
+
 static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
 {
     virtio_delete_queue(gpio->command_vq);
@@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
     vdc->get_config = vu_gpio_get_config;
     vdc->set_status = vu_gpio_set_status;
     vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+    vdc->get_vhost = vu_gpio_get_vhost;
 }
 
 static const TypeInfo vu_gpio_info = {
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index e936cc8ce5..140c420d87 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
     VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+    VHOST_USER_PROTOCOL_F_STATUS = 16,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
             "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
     FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
             "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
+    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
+            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
     /* Virtio ring transport features */
     FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
             "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
@@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
     FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
             "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
             "memory slots supported"),
+    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
+            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
+            "device statuses supported"),
     { -1, "" }
 };
 
@@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
             "VIRTIO_BLK_F_DISCARD: Discard command supported"),
     FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
             "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
+    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
+            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
     FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
             "VIRTIO_BLK_F_ZONED: Zoned block devices"),
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
     FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
             "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
             "channel"),
+    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
+            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
+    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
+            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
+    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
+            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
+    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
+            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
     FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
             "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
     FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
@@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
 };
 #endif
 
+/* virtio/vhost-gpio features mapping */
+#ifdef CONFIG_VIRTIO_GPIO
+static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
+            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
+    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
+            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
+    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
+            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
+            "negotiation supported"),
+    { -1, "" }
+};
+#endif
+
+/* virtio-bluetooth features mapping */
+#ifdef CONFIG_VIRTIO_BT
+static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
+            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
+            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
+            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
+    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
+            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
+    { -1, "" }
+};
+#endif
+
+/* virtio-scmi features mapping */
+#ifdef CONFIG_VIRTIO_SCMI
+static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
+    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
+            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
+            "responses implemented"),
+    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
+            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
+            "implemented"),
+    { -1, "" }
+};
+#endif
+
 #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
     ({                                                   \
         type *list = NULL;                               \
@@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
         features->dev_features =
             CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
         break;
+#endif
+#ifdef CONFIG_VIRTIO_GPIO
+    case VIRTIO_ID_GPIO:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_BT
+    case VIRTIO_ID_BT:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
+        break;
+#endif
+#ifdef CONFIG_VIRTIO_SCMI
+    case VIRTIO_ID_SCMI:
+        features->dev_features =
+            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
+        break;
 #endif
     /* No features */
     case VIRTIO_ID_9P:
@@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
     case VIRTIO_ID_SIGNAL_DIST:
     case VIRTIO_ID_PSTORE:
     case VIRTIO_ID_SOUND:
-    case VIRTIO_ID_BT:
     case VIRTIO_ID_RPMB:
     case VIRTIO_ID_VIDEO_ENCODER:
     case VIRTIO_ID_VIDEO_DECODER:
-    case VIRTIO_ID_SCMI:
     case VIRTIO_ID_NITRO_SEC_MOD:
     case VIRTIO_ID_WATCHDOG:
     case VIRTIO_ID_CAN:
     case VIRTIO_ID_DMABUF:
     case VIRTIO_ID_PARAM_SERV:
     case VIRTIO_ID_AUDIO_POLICY:
-    case VIRTIO_ID_GPIO:
         break;
     default:
         g_assert_not_reached();
-- 
2.39.3
Re: [PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection
Posted by Michael S. Tsirkin 2 years, 7 months ago 
On Fri, Jun 09, 2023 at 09:20:40AM -0400, Jonah Palmer wrote:
> Add new virtio transport feature to transport feature map:
>  - VIRTIO_F_RING_RESET
> 
> Add new vhost-user protocol feature to vhost-user protocol feature map
> and enumeration:
>  - VHOST_USER_PROTOCOL_F_STATUS
> 
> Add new virtio device features for several virtio devices to their
> respective feature mappings:
> 
> virtio-blk:
>  - VIRTIO_BLK_F_SECURE_ERASE
> 
> virtio-net:
>  - VIRTIO_NET_F_NOTF_COAL
>  - VIRTIO_NET_F_GUEST_USO4
>  - VIRTIO_NET_F_GUEST_USO6
>  - VIRTIO_NET_F_HOST_USO
> 
> virtio/vhost-user-gpio:
>  - VIRTIO_GPIO_F_IRQ
>  - VHOST_F_LOG_ALL
>  - VHOST_USER_F_PROTOCOL_FEATURES
> 
> virtio-bt:
>  - VIRTIO_BT_F_VND_HCI
>  - VIRTIO_BT_F_MSFT_EXT
>  - VIRTIO_BT_F_AOSP_EXT
>  - VIRTIO_BT_F_CONFIG_V2
> 
> virtio-scmi:
>  - VIRTIO_SCMI_F_P2A_CHANNELS
>  - VIRTIO_SCMI_F_SHARED_MEMORY
> 
> Add support for introspection on vhost-user-gpio devices.
> 
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

Thanks for the patch! Some comments:


> ---
>  hw/virtio/vhost-user-gpio.c |  7 ++++
>  hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d6927b610a..e88ca5370f 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
>      vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
>  }
>  
> +static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
> +{
> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
> +    return &gpio->vhost_dev;
> +}
> +
>  static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
>  {
>      virtio_delete_queue(gpio->command_vq);
> @@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
>      vdc->get_config = vu_gpio_get_config;
>      vdc->set_status = vu_gpio_set_status;
>      vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
> +    vdc->get_vhost = vu_gpio_get_vhost;
>  }
>  
>  static const TypeInfo vu_gpio_info = {
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index e936cc8ce5..140c420d87 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
>      VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
>      VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
>      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> +    VHOST_USER_PROTOCOL_F_STATUS = 16,
>      VHOST_USER_PROTOCOL_F_MAX
>  };
>

OMG I just realized that by now we have accumulated each value
in 4 places! This is really badly asking to be moved
to a header. Not sure what to do about the document yet
but that will at least get us down to two.
  
> @@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
>              "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
>      FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
>              "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
> +    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
> +            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
>      /* Virtio ring transport features */
>      FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
>              "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
> @@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
>      FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
>              "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
>              "memory slots supported"),
> +    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
> +            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
> +            "device statuses supported"),

status - there's only one per device

>      { -1, "" }
>  };
>  
> @@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
>              "VIRTIO_BLK_F_DISCARD: Discard command supported"),
>      FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
>              "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
> +    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
> +            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
>      FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
>              "VIRTIO_BLK_F_ZONED: Zoned block devices"),
>  #ifndef VIRTIO_BLK_NO_LEGACY

> @@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
>      FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
>              "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
>              "channel"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
> +            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
> +    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
> +            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
>      FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
>              "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
>      FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
> @@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
>  };
>  #endif
>  
> +/* virtio/vhost-gpio features mapping */
> +#ifdef CONFIG_VIRTIO_GPIO

Where's this coming from?

> +static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
> +            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
> +    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
> +            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
> +    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
> +            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
> +            "negotiation supported"),
> +    { -1, "" }
> +};
> +#endif
> +
> +/* virtio-bluetooth features mapping */
> +#ifdef CONFIG_VIRTIO_BT

Where's this coming from?


> +static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
> +            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
> +            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
> +            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
> +    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
> +            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
> +    { -1, "" }
> +};
> +#endif
> +
> +/* virtio-scmi features mapping */
> +#ifdef CONFIG_VIRTIO_SCMI

Where's this coming from?

> +static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
> +    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
> +            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
> +            "responses implemented"),
> +    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
> +            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
> +            "implemented"),
> +    { -1, "" }
> +};
> +#endif
> +
>  #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
>      ({                                                   \
>          type *list = NULL;                               \
> @@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>          features->dev_features =
>              CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
>          break;
> +#endif
> +#ifdef CONFIG_VIRTIO_GPIO
> +    case VIRTIO_ID_GPIO:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
> +        break;
> +#endif
> +#ifdef CONFIG_VIRTIO_BT
> +    case VIRTIO_ID_BT:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
> +        break;
> +#endif
> +#ifdef CONFIG_VIRTIO_SCMI
> +    case VIRTIO_ID_SCMI:
> +        features->dev_features =
> +            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
> +        break;
>  #endif
>      /* No features */
>      case VIRTIO_ID_9P:
> @@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>      case VIRTIO_ID_SIGNAL_DIST:
>      case VIRTIO_ID_PSTORE:
>      case VIRTIO_ID_SOUND:
> -    case VIRTIO_ID_BT:
>      case VIRTIO_ID_RPMB:
>      case VIRTIO_ID_VIDEO_ENCODER:
>      case VIRTIO_ID_VIDEO_DECODER:
> -    case VIRTIO_ID_SCMI:
>      case VIRTIO_ID_NITRO_SEC_MOD:
>      case VIRTIO_ID_WATCHDOG:
>      case VIRTIO_ID_CAN:
>      case VIRTIO_ID_DMABUF:
>      case VIRTIO_ID_PARAM_SERV:
>      case VIRTIO_ID_AUDIO_POLICY:
> -    case VIRTIO_ID_GPIO:
>          break;
>      default:
>          g_assert_not_reached();
> -- 
> 2.39.3
Re: [PATCH v2 2/2] qmp: update virtio feature maps, vhost-user-gpio instrospection
Posted by Jonah Palmer 2 years, 7 months ago 
On 6/23/23 01:43, Michael S. Tsirkin wrote:

> On Fri, Jun 09, 2023 at 09:20:40AM -0400, Jonah Palmer wrote:
>> Add new virtio transport feature to transport feature map:
>>   - VIRTIO_F_RING_RESET
>>
>> Add new vhost-user protocol feature to vhost-user protocol feature map
>> and enumeration:
>>   - VHOST_USER_PROTOCOL_F_STATUS
>>
>> Add new virtio device features for several virtio devices to their
>> respective feature mappings:
>>
>> virtio-blk:
>>   - VIRTIO_BLK_F_SECURE_ERASE
>>
>> virtio-net:
>>   - VIRTIO_NET_F_NOTF_COAL
>>   - VIRTIO_NET_F_GUEST_USO4
>>   - VIRTIO_NET_F_GUEST_USO6
>>   - VIRTIO_NET_F_HOST_USO
>>
>> virtio/vhost-user-gpio:
>>   - VIRTIO_GPIO_F_IRQ
>>   - VHOST_F_LOG_ALL
>>   - VHOST_USER_F_PROTOCOL_FEATURES
>>
>> virtio-bt:
>>   - VIRTIO_BT_F_VND_HCI
>>   - VIRTIO_BT_F_MSFT_EXT
>>   - VIRTIO_BT_F_AOSP_EXT
>>   - VIRTIO_BT_F_CONFIG_V2
>>
>> virtio-scmi:
>>   - VIRTIO_SCMI_F_P2A_CHANNELS
>>   - VIRTIO_SCMI_F_SHARED_MEMORY
>>
>> Add support for introspection on vhost-user-gpio devices.
>>
>> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com>
> Thanks for the patch! Some comments:
>
>
>> ---
>>   hw/virtio/vhost-user-gpio.c |  7 ++++
>>   hw/virtio/virtio-qmp.c      | 79 +++++++++++++++++++++++++++++++++++--
>>   2 files changed, 83 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> index d6927b610a..e88ca5370f 100644
>> --- a/hw/virtio/vhost-user-gpio.c
>> +++ b/hw/virtio/vhost-user-gpio.c
>> @@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
>>       vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
>>   }
>>   
>> +static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
>> +{
>> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
>> +    return &gpio->vhost_dev;
>> +}
>> +
>>   static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
>>   {
>>       virtio_delete_queue(gpio->command_vq);
>> @@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void *data)
>>       vdc->get_config = vu_gpio_get_config;
>>       vdc->set_status = vu_gpio_set_status;
>>       vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
>> +    vdc->get_vhost = vu_gpio_get_vhost;
>>   }
>>   
>>   static const TypeInfo vu_gpio_info = {
>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> index e936cc8ce5..140c420d87 100644
>> --- a/hw/virtio/virtio-qmp.c
>> +++ b/hw/virtio/virtio-qmp.c
>> @@ -53,6 +53,7 @@ enum VhostUserProtocolFeature {
>>       VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
>>       VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
>>       VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
>> +    VHOST_USER_PROTOCOL_F_STATUS = 16,
>>       VHOST_USER_PROTOCOL_F_MAX
>>   };
>>
> OMG I just realized that by now we have accumulated each value
> in 4 places! This is really badly asking to be moved
> to a header. Not sure what to do about the document yet
> but that will at least get us down to two.
>    

I can add this in another patch as a part of this series.

>> @@ -79,6 +80,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] = {
>>               "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
>>       FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
>>               "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
>> +    FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
>> +            "VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
>>       /* Virtio ring transport features */
>>       FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
>>               "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
>> @@ -134,6 +137,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
>>       FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
>>               "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
>>               "memory slots supported"),
>> +    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
>> +            "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
>> +            "device statuses supported"),
> status - there's only one per device

Will change, thanks!

>>       { -1, "" }
>>   };
>>   
>> @@ -176,6 +182,8 @@ static const qmp_virtio_feature_map_t virtio_blk_feature_map[] = {
>>               "VIRTIO_BLK_F_DISCARD: Discard command supported"),
>>       FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
>>               "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
>> +    FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
>> +            "VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
>>       FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
>>               "VIRTIO_BLK_F_ZONED: Zoned block devices"),
>>   #ifndef VIRTIO_BLK_NO_LEGACY
>> @@ -299,6 +307,14 @@ static const qmp_virtio_feature_map_t virtio_net_feature_map[] = {
>>       FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>               "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
>>               "channel"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
>> +            "VIRTIO_NET_F_NOTF_COAL: Device supports coalescing notifications"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
>> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
>> +            "VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
>> +    FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
>> +            "VIRTIO_NET_F_HOST_USO: Device can receive USO"),
>>       FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
>>               "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
>>       FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
>> @@ -469,6 +485,48 @@ static const qmp_virtio_feature_map_t virtio_rng_feature_map[] = {
>>   };
>>   #endif
>>   
>> +/* virtio/vhost-gpio features mapping */
>> +#ifdef CONFIG_VIRTIO_GPIO
> Where's this coming from?
>
Ah, this should probably be CONFIG_VHOST_USER_GPIO instead I believe.

As for the other two...

>> +static const qmp_virtio_feature_map_t virtio_gpio_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_GPIO_F_IRQ, \
>> +            "VIRTIO_GPIO_F_IRQ: Device supports interrupts on GPIO lines"),
>> +    FEATURE_ENTRY(VHOST_F_LOG_ALL, \
>> +            "VHOST_F_LOG_ALL: Logging write descriptors supported"),
>> +    FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
>> +            "VHOST_USER_F_PROTOCOL_FEATURES: Vhost-user protocol features "
>> +            "negotiation supported"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>> +/* virtio-bluetooth features mapping */
>> +#ifdef CONFIG_VIRTIO_BT
> Where's this coming from?
>
This is coming from... nowhere!

My apologies. I forgot to check if any of these virtio devices were
actually being declared as objects (via. OBJECT_DECLARE_SIMPLE_TYPE).

I will revert these changes for virtio-bt & virtio-scmi and be more
careful to check for these in the future. Thank you for pointing this
out.

>> +static const qmp_virtio_feature_map_t virtio_bt_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_BT_F_VND_HCI, \
>> +            "VIRTIO_BT_F_VND_HCI: Vendor command supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_MSFT_EXT, \
>> +            "VIRTIO_BT_F_MSFT_EXT: MSFT vendor supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_AOSP_EXT, \
>> +            "VIRTIO_BT_F_AOSP_EXT: AOSP vendor supported"),
>> +    FEATURE_ENTRY(VIRTIO_BT_F_CONFIG_V2, \
>> +            "VIRTIO_BT_F_CONFIG_V2: Using v2 configuration"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>> +/* virtio-scmi features mapping */
>> +#ifdef CONFIG_VIRTIO_SCMI
> Where's this coming from?

Will revert this. See comment above.

>> +static const qmp_virtio_feature_map_t virtio_scmi_feature_map[] = {
>> +    FEATURE_ENTRY(VIRTIO_SCMI_F_P2A_CHANNELS, \
>> +            "VIRTIO_SCMI_F_P2A_CHANNELS: SCMI notifications or delayed "
>> +            "responses implemented"),
>> +    FEATURE_ENTRY(VIRTIO_SCMI_F_SHARED_MEMORY, \
>> +            "VIRTIO_SCMI_F_SHARED_MEMORY: SCMI shared memory region statistics "
>> +            "implemented"),
>> +    { -1, "" }
>> +};
>> +#endif
>> +
>>   #define CONVERT_FEATURES(type, map, is_status, bitmap)   \
>>       ({                                                   \
>>           type *list = NULL;                               \
>> @@ -625,6 +683,24 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>           features->dev_features =
>>               CONVERT_FEATURES(strList, virtio_rng_feature_map, 0, bitmap);
>>           break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_GPIO
>> +    case VIRTIO_ID_GPIO:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_gpio_feature_map, 0, bitmap);
>> +        break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_BT
>> +    case VIRTIO_ID_BT:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_bt_feature_map, 0, bitmap);
>> +        break;
>> +#endif
>> +#ifdef CONFIG_VIRTIO_SCMI
>> +    case VIRTIO_ID_SCMI:
>> +        features->dev_features =
>> +            CONVERT_FEATURES(strList, virtio_scmi_feature_map, 0, bitmap);
>> +        break;
>>   #endif
>>       /* No features */
>>       case VIRTIO_ID_9P:
>> @@ -640,18 +716,15 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>       case VIRTIO_ID_SIGNAL_DIST:
>>       case VIRTIO_ID_PSTORE:
>>       case VIRTIO_ID_SOUND:
>> -    case VIRTIO_ID_BT:
>>       case VIRTIO_ID_RPMB:
>>       case VIRTIO_ID_VIDEO_ENCODER:
>>       case VIRTIO_ID_VIDEO_DECODER:
>> -    case VIRTIO_ID_SCMI:
>>       case VIRTIO_ID_NITRO_SEC_MOD:
>>       case VIRTIO_ID_WATCHDOG:
>>       case VIRTIO_ID_CAN:
>>       case VIRTIO_ID_DMABUF:
>>       case VIRTIO_ID_PARAM_SERV:
>>       case VIRTIO_ID_AUDIO_POLICY:
>> -    case VIRTIO_ID_GPIO:
>>           break;
>>       default:
>>           g_assert_not_reached();
>> -- 
>> 2.39.3

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.