On Tue, Apr 10, 2018 at 03:05:24PM +0800, Jason Wang wrote:
>
>
> On 2018年04月04日 20:53, w...@redhat.com wrote:
> >From: Wei Xu <w...@redhat.com>
> >
> >Only minimum definitions from the spec are included
> >for prototype.
> >
> >Signed-off-by: Wei Xu <w...@redhat.com>
> >---
> > hw/virtio/virtio.c | 47
> > +++++++++++++++++++++++---
> > include/hw/virtio/virtio.h | 12 ++++++-
> > include/standard-headers/linux/virtio_config.h | 2 ++
> > 3 files changed, 56 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 006d3d1..9a6bfe7 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -39,6 +39,14 @@ typedef struct VRingDesc
> > uint16_t next;
> > } VRingDesc;
> >+typedef struct VRingDescPacked
> >+{
> >+ uint64_t addr;
> >+ uint32_t len;
> >+ uint16_t id;
> >+ uint16_t flags;
> >+} VRingDescPacked;
> >+
> > typedef struct VRingAvail
> > {
> > uint16_t flags;
> >@@ -61,9 +69,18 @@ typedef struct VRingUsed
> > typedef struct VRingMemoryRegionCaches {
> > struct rcu_head rcu;
> >- MemoryRegionCache desc;
> >- MemoryRegionCache avail;
> >- MemoryRegionCache used;
> >+ union {
> >+ struct {
> >+ MemoryRegionCache desc;
> >+ MemoryRegionCache avail;
> >+ MemoryRegionCache used;
> >+ };
> >+ struct {
> >+ MemoryRegionCache desc_packed;
> >+ MemoryRegionCache driver;
> >+ MemoryRegionCache device;
> >+ };
> >+ };
>
> I think we can reuse exist memory region caches? Especially consider
> device/driver area could be treated as a renaming of avail/used area.
>
> E.g desc for desc_packed, avail for driver area and used for device area.
Yes, I will take it.
>
> > } VRingMemoryRegionCaches;
> > typedef struct VRing
> >@@ -77,10 +94,31 @@ typedef struct VRing
> > VRingMemoryRegionCaches *caches;
> > } VRing;
> >+typedef struct VRingPackedDescEvent {
> >+ uint16_t desc_event_off:15,
> >+ desc_event_wrap:1;
> >+ uint16_t desc_event_flags:2;
> >+} VRingPackedDescEvent ;
> >+
> >+typedef struct VRingPacked
> >+{
> >+ unsigned int num;
> >+ unsigned int num_default;
> >+ unsigned int align;
> >+ hwaddr desc;
> >+ hwaddr driver;
> >+ hwaddr device;
> >+ VRingMemoryRegionCaches *caches;
> >+} VRingPacked;
>
> Same here, can we reuse VRing here?
Yes.
>
> >+
> > struct VirtQueue
> > {
> >- VRing vring;
> >+ union {
> >+ struct VRing vring;
> >+ struct VRingPacked packed;
> >+ };
> >+ uint8_t wrap_counter:1;
> > /* Next head to pop */
> > uint16_t last_avail_idx;
> >@@ -1220,6 +1258,7 @@ void virtio_reset(void *opaque)
> > vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> > vdev->vq[i].inuse = 0;
> > virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> >+ vdev->vq[i].wrap_counter = 1;
> > }
> > }
> >diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >index 098bdaa..563e88e 100644
> >--- a/include/hw/virtio/virtio.h
> >+++ b/include/hw/virtio/virtio.h
> >@@ -46,6 +46,14 @@ typedef struct VirtQueueElement
> > unsigned int index;
> > unsigned int out_num;
> > unsigned int in_num;
> >+
> >+ /* Number of descriptors used by packed ring */
>
> Do you mean the number of chained descriptors?
These has been removed.
>
> >+ uint16_t count;
> >+ uint8_t wrap_counter:1;
>
> What's the use of this bit? If you refer to my v1 vhost code, I used to have
> this, but it won't work for OOO completion e.g when zerocopy is disabled.
> I've dropped it now.
>
> This is tricky and can only work when device complete descriptors in order.
Same here.
>
> >+ /* FIXME: Length of every used buffer for a descriptor,
> >+ move to dynamical allocating due to out/in sgs numbers */
> >+ uint32_t len[VIRTQUEUE_MAX_SIZE];
>
> Can you explain more about this?
Also here.
>
> >+
> > hwaddr *in_addr;
> > hwaddr *out_addr;
> > struct iovec *in_sg;
> >@@ -262,7 +270,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> > DEFINE_PROP_BIT64("any_layout", _state, _field, \
> > VIRTIO_F_ANY_LAYOUT, true), \
> > DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
> >- VIRTIO_F_IOMMU_PLATFORM, false)
> >+ VIRTIO_F_IOMMU_PLATFORM, false), \
> >+ DEFINE_PROP_BIT64("ring_packed", _state, _field, \
> >+ VIRTIO_F_RING_PACKED, true)
>
> Remember to disable this for old machine types in next version.
Sure, will do.
>
> Thanks
>
> > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> > hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
> >diff --git a/include/standard-headers/linux/virtio_config.h
> >b/include/standard-headers/linux/virtio_config.h
> >index b777069..6ee5529 100644
> >--- a/include/standard-headers/linux/virtio_config.h
> >+++ b/include/standard-headers/linux/virtio_config.h
> >@@ -71,4 +71,6 @@
> > * this is for compatibility with legacy systems.
> > */
> > #define VIRTIO_F_IOMMU_PLATFORM 33
> >+
> >+#define VIRTIO_F_RING_PACKED 34
> > #endif /* _LINUX_VIRTIO_CONFIG_H */
>
