Re: [PATCH v6 4/6] qmp: add QMP command x-debug-virtio-queue-status

[Jason Wang <jasowang@redhat.com>]

在 2021/7/21 下午4:59, Jonah Palmer 写道:
>
>
> On 7/13/21 10:37 PM, Jason Wang wrote:
>>
>> 在 2021/7/12 下午6:35, Jonah Palmer 写道:
>>> From: Laurent Vivier <lvivier@redhat.com>
>>>
>>> This new command shows internal status of a VirtQueue.
>>> (vrings and indexes).
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>> ---
>>>   hw/virtio/virtio-stub.c |   6 +++
>>>   hw/virtio/virtio.c      |  37 ++++++++++++++++++
>>>   qapi/virtio.json        | 102 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 145 insertions(+)
>>>
>>>   [Jonah: Added 'device-type' field to VirtQueueStatus and
>>>   qmp command x-debug-virtio-queue-status.]
>>>
>>> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
>>> index ddb592f..3c1bf17 100644
>>> --- a/hw/virtio/virtio-stub.c
>>> +++ b/hw/virtio/virtio-stub.c
>>> @@ -17,3 +17,9 @@ VirtioStatus *qmp_x_debug_virtio_status(const 
>>> char* path, Error **errp)
>>>   {
>>>       return qmp_virtio_unsupported(errp);
>>>   }
>>> +
>>> +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
>>> +                                                 uint16_t queue, 
>>> Error **errp)
>>> +{
>>> +    return qmp_virtio_unsupported(errp);
>>> +}
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 81a0ee8..ccd4371 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -3935,6 +3935,43 @@ static VirtIODevice *virtio_device_find(const 
>>> char *path)
>>>       return NULL;
>>>   }
>>>   +VirtQueueStatus *qmp_x_debug_virtio_queue_status(const char *path,
>>> +                                                 uint16_t queue, 
>>> Error **errp)
>>> +{
>>> +    VirtIODevice *vdev;
>>> +    VirtQueueStatus *status;
>>> +
>>> +    vdev = virtio_device_find(path);
>>> +    if (vdev == NULL) {
>>> +        error_setg(errp, "Path %s is not a VirtIO device", path);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    if (queue >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, 
>>> queue)) {
>>> +        error_setg(errp, "Invalid virtqueue number %d", queue);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    status = g_new0(VirtQueueStatus, 1);
>>> +    status->device_type = qapi_enum_parse(&VirtioType_lookup, 
>>> vdev->name,
>>> + VIRTIO_TYPE_UNKNOWN, NULL);
>>> +    status->queue_index = vdev->vq[queue].queue_index;
>>> +    status->inuse = vdev->vq[queue].inuse;
>>> +    status->vring_num = vdev->vq[queue].vring.num;
>>> +    status->vring_num_default = vdev->vq[queue].vring.num_default;
>>> +    status->vring_align = vdev->vq[queue].vring.align;
>>> +    status->vring_desc = vdev->vq[queue].vring.desc;
>>> +    status->vring_avail = vdev->vq[queue].vring.avail;
>>> +    status->vring_used = vdev->vq[queue].vring.used;
>>> +    status->last_avail_idx = vdev->vq[queue].last_avail_idx;
>>
>>
>> As mentioned in previous versions. We need add vhost support 
>> otherwise the value here is wrong.
> Got it. I'll add a case to determine if vhost is active for a given device.
> So, in the case that vhost is active, should I just not print out the value or would I substitute it with
> another value (whatever that might be)?


You can query the vhost for those index.

(vhost_get_vring_base())


>   Same question for shadow_avail_idx below as well.


It's an implementation specific. I think we can simply not show it if 
vhost is enabled.

Thanks


>
> Jonah
>>
>>
>>> +    status->shadow_avail_idx = vdev->vq[queue].shadow_avail_idx;
>>
>>
>> The shadow index is something that is implementation specific e.g in 
>> the case of vhost it's kind of meaningless.
>>
>> Thanks
>>
>>
>>> +    status->used_idx = vdev->vq[queue].used_idx;
>>> +    status->signalled_used = vdev->vq[queue].signalled_used;
>>> +    status->signalled_used_valid = 
>>> vdev->vq[queue].signalled_used_valid;
>>> +
>>> +    return status;
>>> +}
>>> +
>>>   #define CONVERT_FEATURES(type, map)                \
>>>       ({                                           \
>>>           type *list = NULL;                         \
>>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>>> index 78873cd..7007e0c 100644
>>> --- a/qapi/virtio.json
>>> +++ b/qapi/virtio.json
>>> @@ -406,3 +406,105 @@
>>>     'data': { 'path': 'str' },
>>>     'returns': 'VirtioStatus'
>>>   }
>>> +
>>> +##
>>> +# @VirtQueueStatus:
>>> +#
>>> +# Status of a VirtQueue
>>> +#
>>> +# @device-type: VirtIO device type
>>> +#
>>> +# @queue-index: VirtQueue queue_index
>>> +#
>>> +# @inuse: VirtQueue inuse
>>> +#
>>> +# @vring-num: VirtQueue vring.num
>>> +#
>>> +# @vring-num-default: VirtQueue vring.num_default
>>> +#
>>> +# @vring-align: VirtQueue vring.align
>>> +#
>>> +# @vring-desc: VirtQueue vring.desc
>>> +#
>>> +# @vring-avail: VirtQueue vring.avail
>>> +#
>>> +# @vring-used: VirtQueue vring.used
>>> +#
>>> +# @last-avail-idx: VirtQueue last_avail_idx
>>> +#
>>> +# @shadow-avail-idx: VirtQueue shadow_avail_idx
>>> +#
>>> +# @used-idx: VirtQueue used_idx
>>> +#
>>> +# @signalled-used: VirtQueue signalled_used
>>> +#
>>> +# @signalled-used-valid: VirtQueue signalled_used_valid
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +##
>>> +
>>> +{ 'struct': 'VirtQueueStatus',
>>> +  'data': {
>>> +    'device-type': 'VirtioType',
>>> +    'queue-index': 'uint16',
>>> +    'inuse': 'uint32',
>>> +    'vring-num': 'int',
>>> +    'vring-num-default': 'int',
>>> +    'vring-align': 'int',
>>> +    'vring-desc': 'uint64',
>>> +    'vring-avail': 'uint64',
>>> +    'vring-used': 'uint64',
>>> +    'last-avail-idx': 'uint16',
>>> +    'shadow-avail-idx': 'uint16',
>>> +    'used-idx': 'uint16',
>>> +    'signalled-used': 'uint16',
>>> +    'signalled-used-valid': 'uint16'
>>> +  }
>>> +}
>>> +
>>> +##
>>> +# @x-debug-virtio-queue-status:
>>> +#
>>> +# Return the status of a given VirtQueue
>>> +#
>>> +# @path: QOBject path of the VirtIODevice
>>> +#
>>> +# @queue: queue number to examine
>>> +#
>>> +# Returns: Status of the VirtQueue
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "x-debug-virtio-queue-status",
>>> +#      "arguments": {
>>> +#          "path": 
>>> "/machine/peripheral-anon/device[3]/virtio-backend",
>>> +#          "queue": 0
>>> +#      }
>>> +#   }
>>> +# <- { "return": {
>>> +#      "signalled-used": 373,
>>> +#      "inuse": 0,
>>> +#      "vring-align": 4096,
>>> +#      "vring-desc": 864411648,
>>> +#      "signalled-used-valid": 0,
>>> +#      "vring-num-default": 256,
>>> +#      "vring-avail": 864415744,
>>> +#      "queue-index": 0,
>>> +#      "last-avail-idx": 373,
>>> +#      "vring-used": 864416320,
>>> +#      "used-idx": 373,
>>> +#      "device-type": "virtio-net",
>>> +#      "shadow-avail-idx": 619,
>>> +#      "vring-num": 256
>>> +#      }
>>> +#    }
>>> +#
>>> +##
>>> +
>>> +{ 'command': 'x-debug-virtio-queue-status',
>>> +  'data': { 'path': 'str', 'queue': 'uint16' },
>>> +  'returns': 'VirtQueueStatus'
>>> +}
>>