[SLOF] [PATCH slof v3 2/4] virtio: Store queue descriptors in virtio_device
Alexey Kardashevskiy
aik at ozlabs.ru
Wed Dec 4 13:02:42 AEDT 2019
On 04/12/2019 12:58, Alexey Kardashevskiy wrote:
>
>
> 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()?
Ah never mind, 4/4 added use of it, will take care of that. Thanks,
--
Alexey
More information about the SLOF
mailing list