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

[Jonah Palmer <jonah.palmer@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 9461 bytes --]

Hi Jason, could I get your thoughts on this implementation question below?

I'm not too sure on how I should proceed determining if vhost is active 
or not.

Thank you!


Jonah

On 7/26/21 5:33 AM, Jonah Palmer wrote:
>
>
> On 7/22/21 5:22 AM, Jason Wang wrote:
>>
>> 在 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
>
> Ah I see, thank you!
>
> So, it appears to me that it's not very easy to get the struct 
> vhost_dev pointer from struct VirtIODevice to indicate whether or not 
> vhost is active, e.g. there's no virtio class-independent way to get 
> struct vhost_dev.
>
> I was thinking of adding an op/callback function to struct 
> VirtioDeviceClass, e.g. bool has_vhost(VirtIODevice *vdev), and 
> implement it for each virtio class (net, scsi, blk, etc.).
>
> For example, for virtio-net, maybe it'd be something like:
>
> bool has_vhost(VirtIODevice *vdev) {
>    VirtIONet *n = VIRTIO_NET(vdev);
>    NetClientState *nc = qemu_get_queue(n->nic);
>    return nc->peer ? get_vhost_net(nc->peer) : false;
> }
>
> Also, for getting the last_avail_idx, I was also thinking of adding 
> another op/callback to struct VirtioDeviceClass, e.g. unsigned int 
> get_last_avail_idx(VirtIODevice *vdev, unsigned int vq_idx) that finds 
> if vhost is active or not and either gets last_avail_idx from virtio 
> directly or through vhost (e.g. 
> vhost_dev->vhost_ops->vhost_get_vring_base()).
>
> I wanted to run this by you and get your opinion on this before I 
> started implementing it in code. Let me know what you think about this.
>
>
> Jonah
>
>>
>>
>>>
>>> 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'
>>>>> +}
>>>>
>>

[-- Attachment #2: Type: text/html, Size: 16534 bytes --]