[SLOF] [PATCH slof v3 2/4] virtio: Store queue descriptors in virtio_device
Alexey Kardashevskiy
aik at ozlabs.ru
Wed Dec 4 12:58:15 AEDT 2019
On 04/12/2019 11:11, Michael Roth wrote:
> Quoting Alexey Kardashevskiy (2019-11-20 17:57:59)
>> At the moment desc/avail/used pointers are read from the device every
>> time we need them. This works for now unless iommu_platform=on is used,
>> desc/avail/used stored in the config space are bus addresses while
>> SLOF should keep using the guest physical addresses.
>>
>> virtio-net stores queue descriptors already, virtio-serial does it in
>> global statics, move them into virtio_device.
>>
>> The next patch will use this to allow IOMMU.
>>
>> While at this, move repeating avail->flags/idx setup into
>> virtio_queue_init_vq() except virtio-serial which vq_rx->avail->idx is
>> setup differently.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>
> Some cosmetic comments below, but either way:
>
> Reviewed-by: Michael Roth <mdroth at linux.vnet.ibm.com>
Thanks, some comments below.
>
>> ---
>> lib/libvirtio/virtio-internal.h | 12 ++--
>> lib/libvirtio/virtio-net.h | 2 -
>> lib/libvirtio/virtio.h | 26 +++----
>> lib/libvirtio/virtio-9p.c | 37 ++++------
>> lib/libvirtio/virtio-blk.c | 52 ++++++--------
>> lib/libvirtio/virtio-net.c | 27 ++++---
>> lib/libvirtio/virtio-scsi.c | 67 +++++++----------
>> lib/libvirtio/virtio-serial.c | 72 ++++++++-----------
>> lib/libvirtio/virtio.c | 124 +++++++++++++-------------------
>> 9 files changed, 173 insertions(+), 246 deletions(-)
>>
>> diff --git a/lib/libvirtio/virtio-internal.h b/lib/libvirtio/virtio-internal.h
>> index 08662eab7014..fe59c6b9909d 100644
>> --- a/lib/libvirtio/virtio-internal.h
>> +++ b/lib/libvirtio/virtio-internal.h
>> @@ -17,32 +17,32 @@
>>
>> static inline uint16_t virtio_cpu_to_modern16(struct virtio_device *dev, uint16_t val)
>> {
>> - return dev->is_modern ? cpu_to_le16(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? cpu_to_le16(val) : val;
>> }
>>
>> static inline uint32_t virtio_cpu_to_modern32(struct virtio_device *dev, uint32_t val)
>> {
>> - return dev->is_modern ? cpu_to_le32(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? cpu_to_le32(val) : val;
>> }
>>
>> static inline uint64_t virtio_cpu_to_modern64(struct virtio_device *dev, uint64_t val)
>> {
>> - return dev->is_modern ? cpu_to_le64(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? cpu_to_le64(val) : val;
>> }
>>
>> static inline uint16_t virtio_modern16_to_cpu(struct virtio_device *dev, uint16_t val)
>> {
>> - return dev->is_modern ? le16_to_cpu(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? le16_to_cpu(val) : val;
>> }
>>
>> static inline uint32_t virtio_modern32_to_cpu(struct virtio_device *dev, uint32_t val)
>> {
>> - return dev->is_modern ? le32_to_cpu(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? le32_to_cpu(val) : val;
>> }
>>
>> static inline uint64_t virtio_modern64_to_cpu(struct virtio_device *dev, uint64_t val)
>> {
>> - return dev->is_modern ? le64_to_cpu(val) : val;
>> + return (dev->features & VIRTIO_F_VERSION_1) ? le64_to_cpu(val) : val;
>> }
>>
>> #endif /* _LIBVIRTIO_INTERNAL_H */
>> diff --git a/lib/libvirtio/virtio-net.h b/lib/libvirtio/virtio-net.h
>> index f72d435564bb..c71fbded0bf1 100644
>> --- a/lib/libvirtio/virtio-net.h
>> +++ b/lib/libvirtio/virtio-net.h
>> @@ -27,8 +27,6 @@ enum {
>> struct virtio_net {
>> net_driver_t driver;
>> struct virtio_device vdev;
>> - struct vqs vq_rx;
>> - struct vqs vq_tx;
>> };
>>
>> /* VIRTIO_NET Feature bits */
>> diff --git a/lib/libvirtio/virtio.h b/lib/libvirtio/virtio.h
>> index b65c716e88c9..4927a97f9f5f 100644
>> --- a/lib/libvirtio/virtio.h
>> +++ b/lib/libvirtio/virtio.h
>> @@ -14,7 +14,6 @@
>> #define _LIBVIRTIO_H
>>
>> #include <stdint.h>
>> -#include <stdbool.h>
>>
>> /* Device status bits */
>> #define VIRTIO_STAT_ACKNOWLEDGE 1
>> @@ -78,8 +77,17 @@ struct virtio_cap {
>> uint8_t cap_id;
>> };
>>
>> +struct vqs {
>> + uint64_t id; /* Queue ID */
>> + uint32_t size;
>> + void *buf_mem;
>> + struct vring_desc *desc;
>> + struct vring_avail *avail;
>> + struct vring_used *used;
>> +};
>> +
>> struct virtio_device {
>> - uint32_t is_modern; /* Indicates whether to use virtio 1.0 */
>> + uint64_t features;
>> struct virtio_cap legacy;
>> struct virtio_cap common;
>> struct virtio_cap notify;
>> @@ -87,15 +95,7 @@ struct virtio_device {
>> struct virtio_cap device;
>> struct virtio_cap pci;
>> uint32_t notify_off_mul;
>> -};
>> -
>> -struct vqs {
>> - uint64_t id; /* Queue ID */
>> - uint32_t size;
>> - void *buf_mem;
>> - struct vring_desc *desc;
>> - struct vring_avail *avail;
>> - struct vring_used *used;
>> + struct vqs vq[3];
>> };
>>
>> /* Parts of the virtqueue are aligned on a 4096 byte page boundary */
>> @@ -106,10 +106,10 @@ extern unsigned int virtio_get_qsize(struct virtio_device *dev, int queue);
>> extern struct vring_desc *virtio_get_vring_desc(struct virtio_device *dev, int queue);
>> extern struct vring_avail *virtio_get_vring_avail(struct virtio_device *dev, int queue);
>> extern struct vring_used *virtio_get_vring_used(struct virtio_device *dev, int queue);
>> -extern void virtio_fill_desc(struct vring_desc *desc, bool is_modern,
>> +extern void virtio_fill_desc(struct vqs *vq, int id, uint64_t features,
>> uint64_t addr, uint32_t len,
>> uint16_t flags, uint16_t next);
>> -extern int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id);
>> +extern struct vqs *virtio_queue_init_vq(struct virtio_device *dev, unsigned int id);
>> extern void virtio_queue_term_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id);
>>
>> extern struct virtio_device *virtio_setup_vd(void);
>>
>
> <snip>
>
>> diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c
>> index e95352da8191..251661e8d466 100644
>> --- a/lib/libvirtio/virtio-scsi.c
>> +++ b/lib/libvirtio/virtio-scsi.c
>> @@ -23,63 +23,54 @@ int virtioscsi_send(struct virtio_device *dev,
>> struct virtio_scsi_resp_cmd *resp,
>> int is_read, void *buf, uint64_t buf_len)
>> {
>> - struct vring_desc *vq_desc; /* Descriptor vring */
>> - struct vring_avail *vq_avail; /* "Available" vring */
>> - struct vring_used *vq_used; /* "Used" vring */
>>
>> volatile uint16_t *current_used_idx;
>> uint16_t last_used_idx, avail_idx;
>> int id;
>> - uint32_t vq_size, time;
>> + uint32_t time;
>> + struct vqs *vq = &dev->vq[VIRTIO_SCSI_REQUEST_VQ];
>>
>> - int vq = VIRTIO_SCSI_REQUEST_VQ;
>> + avail_idx = virtio_modern16_to_cpu(dev, vq->avail->idx);
>>
>> - vq_size = virtio_get_qsize(dev, vq);
>> - vq_desc = virtio_get_vring_desc(dev, vq);
>> - vq_avail = virtio_get_vring_avail(dev, vq);
>> - vq_used = virtio_get_vring_used(dev, vq);
>> -
>> - avail_idx = virtio_modern16_to_cpu(dev, vq_avail->idx);
>> -
>> - last_used_idx = vq_used->idx;
>> - current_used_idx = &vq_used->idx;
>> + last_used_idx = vq->used->idx;
>> + current_used_idx = &vq->used->idx;
>>
>> /* Determine descriptor index */
>> - id = (avail_idx * 3) % vq_size;
>> - virtio_fill_desc(&vq_desc[id], dev->is_modern, (uint64_t)req, sizeof(*req), VRING_DESC_F_NEXT,
>> - (id + 1) % vq_size);
>> + id = (avail_idx * 3) % vq->size;
>> + virtio_fill_desc(vq, id, dev->features, (uint64_t)req, sizeof(*req), VRING_DESC_F_NEXT,
>> + id + 1);
>>
>> if (buf == NULL || buf_len == 0) {
>> /* Set up descriptor for response information */
>> - virtio_fill_desc(&vq_desc[(id + 1) % vq_size], dev->is_modern,
>> + virtio_fill_desc(vq, id + 1, dev->features,
>> (uint64_t)resp, sizeof(*resp),
>> VRING_DESC_F_WRITE, 0);
>> } else if (is_read) {
>> /* Set up descriptor for response information */
>> - virtio_fill_desc(&vq_desc[(id + 1) % vq_size], dev->is_modern,
>> + virtio_fill_desc(vq, id + 1, dev->features,
>> (uint64_t)resp, sizeof(*resp),
>> VRING_DESC_F_NEXT | VRING_DESC_F_WRITE,
>> - (id + 2) % vq_size);
>> + id + 2);
>> /* Set up virtqueue descriptor for data from device */
>> - virtio_fill_desc(&vq_desc[(id + 2) % vq_size], dev->is_modern,
>> + virtio_fill_desc(vq, id + 2, dev->features,
>> (uint64_t)buf, buf_len, VRING_DESC_F_WRITE, 0);
>> } else {
>> /* Set up virtqueue descriptor for data to device */
>> - virtio_fill_desc(&vq_desc[(id + 1) % vq_size], dev->is_modern,
>> + virtio_fill_desc(vq, id + 1, dev->features,
>> (uint64_t)buf, buf_len, VRING_DESC_F_NEXT,
>> - (id + 2) % vq_size);
>> + id + 2);
>> /* Set up descriptor for response information */
>> - virtio_fill_desc(&vq_desc[(id + 2) % vq_size], dev->is_modern,
>> + virtio_fill_desc(vq, id + 2, dev->features,
>> (uint64_t)resp, sizeof(*resp),
>> VRING_DESC_F_WRITE, 0);
>> }
>>
>> - vq_avail->ring[avail_idx % vq_size] = virtio_cpu_to_modern16(dev, id);
>> + vq->avail->ring[avail_idx % vq->size] = virtio_cpu_to_modern16(dev, id);
>> mb();
>> - vq_avail->idx = virtio_cpu_to_modern16(dev, avail_idx + 1);
>> + vq->avail->idx = virtio_cpu_to_modern16(dev, avail_idx + 1);
>>
>> /* Tell HV that the vq is ready */
>> - virtio_queue_notify(dev, vq);
>> + virtio_queue_notify(dev, vq->id);
>
> We know vq->id is VIRTIO_SCSI_REQUEST_VQ since we used that as an
> index to grab it earlier in the function. If we drop the use of vq->id
> here, and drop the use of it in virtio_queue_term_vq (in favor of the
> 'id' param), it seems like we can drop it from struct vqs entirely.
virtio_queue_term_vq() is not using it, did you mean virtio_queue_init_vq()?
I'll do the rest, seems like a good idea.
>
> Though I could see it being useful if it ever became the case that
> queue_id wasn't necessarily a direct index into dev->vqs, but that
> would require a bunch of rework anyway since we currently use it as
> an index in multiple places outside of the common virtio routines.
Right.
One immediate rework I'd like to do is removing "static uint16_t
last_rx_idx;", I see this in -net and -serial, very confusing why we
need this.
>
>>
>> /* Wait for host to consume the descriptor */
>> time = SLOF_GetTimer() + VIRTIO_TIMEOUT;
>> @@ -99,9 +90,8 @@ int virtioscsi_send(struct virtio_device *dev,
>> */
>> int virtioscsi_init(struct virtio_device *dev)
>> {
>> - struct vqs vq_ctrl, vq_event, vq_request;
>> + struct vqs *vq_ctrl, *vq_event, *vq_request;
>> int status = VIRTIO_STAT_ACKNOWLEDGE;
>> - uint16_t flags;
>>
>> /* Reset device */
>> // XXX That will clear the virtq base. We need to move
>> @@ -117,7 +107,7 @@ int virtioscsi_init(struct virtio_device *dev)
>> virtio_set_status(dev, status);
>>
>> /* Device specific setup - we do not support special features right now */
>> - if (dev->is_modern) {
>> + if (dev->features & VIRTIO_F_VERSION_1) {
>> if (virtio_negotiate_guest_features(dev, VIRTIO_F_VERSION_1))
>> goto dev_error;
>> virtio_get_status(dev, &status);
>
> <snip>
>
>> @@ -112,35 +108,28 @@ void virtio_serial_shutdown(struct virtio_device *dev)
>>
>> int virtio_serial_putchar(struct virtio_device *dev, char c)
>> {
>> - struct vring_desc *desc;
>> int id;
>> uint32_t vq_size, time;
>> - struct vring_desc *vq_desc;
>> - struct vring_avail *vq_avail;
>> - struct vring_used *vq_used;
>> volatile uint16_t *current_used_idx;
>> uint16_t last_used_idx, avail_idx;
>> + struct vqs *vq = &dev->vq[TX_Q];
>>
>> vq_size = virtio_get_qsize(dev, TX_Q);
>
> We drop virtio_get_qsize() in favor of vq->size in blk/scsi. Any reason not
> to do the same here?
No reason really, done. Thanks,
--
Alexey
More information about the SLOF
mailing list