[SLOF] [PATCH slof v3 2/4] virtio: Store queue descriptors in virtio_device
Michael Roth
mdroth at linux.vnet.ibm.com
Wed Dec 4 11:11:41 AEDT 2019
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>
> ---
> 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.
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.
>
> /* 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?
More information about the SLOF
mailing list