From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id C94141B46B for ; Fri, 2 Nov 2018 15:02:04 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Nov 2018 07:02:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,456,1534834800"; d="scan'208";a="102879481" Received: from btwcube1.sh.intel.com (HELO debian) ([10.67.104.158]) by fmsmga004.fm.intel.com with ESMTP; 02 Nov 2018 07:02:02 -0700 Date: Fri, 2 Nov 2018 22:00:32 +0800 From: Tiwei Bie To: Jens Freimann Cc: dev@dpdk.org, maxime.coquelin@redhat.com, Gavin.Hu@arm.com Message-ID: <20181102140032.GB18161@debian> References: <20181102090749.17316-1-jfreimann@redhat.com> <20181102090749.17316-2-jfreimann@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181102090749.17316-2-jfreimann@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v10 1/9] net/virtio: vring init for packed queues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Nov 2018 14:02:06 -0000 On Fri, Nov 02, 2018 at 10:07:41AM +0100, Jens Freimann wrote: > Add and initialize descriptor data structures. > > Signed-off-by: Jens Freimann > --- > drivers/net/virtio/virtio_ethdev.c | 22 +++++++---- > drivers/net/virtio/virtio_pci.h | 7 ++++ > drivers/net/virtio/virtio_ring.h | 60 ++++++++++++++++++++++++++---- > drivers/net/virtio/virtqueue.h | 19 +++++++++- > 4 files changed, 92 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index 27088e3a6..0f5a3b80f 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -299,19 +299,21 @@ virtio_init_vring(struct virtqueue *vq) > > PMD_INIT_FUNC_TRACE(); > > - /* > - * Reinitialise since virtio port might have been stopped and restarted > - */ This comment is supposed to be kept in this version. > memset(ring_mem, 0, vq->vq_ring_size); > - vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN); > + > vq->vq_used_cons_idx = 0; > vq->vq_desc_head_idx = 0; > vq->vq_avail_idx = 0; > vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1); > vq->vq_free_cnt = vq->vq_nentries; > memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries); > - > - vring_desc_init(vr->desc, size); > + if (vtpci_packed_queue(vq->hw)) { > + vring_init_packed(&vq->ring_packed, ring_mem, VIRTIO_PCI_VRING_ALIGN, size); > + vring_desc_init_packed(vq, size); > + } else { > + vring_init_split(vr, ring_mem, VIRTIO_PCI_VRING_ALIGN, size); > + vring_desc_init_split(vr->desc, size); > + } > > /* > * Disable device(host) interrupting guest [...] > diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h > index 9e3c2a015..69ec84d1e 100644 > --- a/drivers/net/virtio/virtio_ring.h > +++ b/drivers/net/virtio/virtio_ring.h > @@ -54,9 +54,35 @@ struct vring_used { > struct vring_used_elem ring[0]; > }; > > +/* For support of packed virtqueues in Virtio 1.1 the format of descriptors > + * looks like this. > + */ > +struct vring_desc_packed { Please change the name to vring_packed_desc to make the name consistent with the name in vhost code. > + uint64_t addr; > + uint32_t len; > + uint16_t index; The `index` is supposed to be changed to `id` in this version. > + uint16_t flags; > +} __attribute__ ((aligned(16))); When defining the structure for vring desc, I think the __attribute__ ((aligned(16))) isn't needed. > + > +#define RING_EVENT_FLAGS_ENABLE 0x0 > +#define RING_EVENT_FLAGS_DISABLE 0x1 > +#define RING_EVENT_FLAGS_DESC 0x2 An empty line is supposed to be added here in this version. > +struct vring_packed_desc_event { > + uint16_t desc_event_off_wrap; > + uint16_t desc_event_flags; > +}; > + > +struct vring_packed { > + unsigned int num; > + struct vring_desc_packed *desc_packed; > + struct vring_packed_desc_event *driver_event; > + struct vring_packed_desc_event *device_event; > + > +}; > + > struct vring { > unsigned int num; > - struct vring_desc *desc; > + struct vring_desc *desc; Above change is unnecessary. > struct vring_avail *avail; > struct vring_used *used; > }; > @@ -95,10 +121,18 @@ struct vring { > #define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num]) > > static inline size_t > -vring_size(unsigned int num, unsigned long align) > +vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align) > { > size_t size; > > + if (vtpci_packed_queue(hw)) { > + size = num * sizeof(struct vring_desc_packed); > + size += sizeof(struct vring_packed_desc_event); > + size = RTE_ALIGN_CEIL(size, align); > + size += sizeof(struct vring_packed_desc_event); > + return size; > + } > + > size = num * sizeof(struct vring_desc); > size += sizeof(struct vring_avail) + (num * sizeof(uint16_t)); > size = RTE_ALIGN_CEIL(size, align); > @@ -106,17 +140,29 @@ vring_size(unsigned int num, unsigned long align) > (num * sizeof(struct vring_used_elem)); > return size; > } > - Why above empty line is removed? > static inline void > -vring_init(struct vring *vr, unsigned int num, uint8_t *p, > - unsigned long align) > +vring_init_split(struct vring *vr, uint8_t *p, unsigned long align, > + unsigned int num) Above change to the order of the params is unnecessary. > { > vr->num = num; > vr->desc = (struct vring_desc *) p; > vr->avail = (struct vring_avail *) (p + > - num * sizeof(struct vring_desc)); > + vr->num * sizeof(struct vring_desc)); Above change is unnecessary. > vr->used = (void *) > - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align); > + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[vr->num]), align); Above change is unnecessary. > +} > + > +static inline void > +vring_init_packed(struct vring_packed *vr, uint8_t *p, unsigned long align, > + unsigned int num) > +{ > + vr->num = num; > + vr->desc_packed = (struct vring_desc_packed *)p; > + vr->driver_event = (struct vring_packed_desc_event *)(p + > + vr->num * sizeof(struct vring_desc_packed)); > + vr->device_event = (struct vring_packed_desc_event *) > + RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event + > + sizeof(struct vring_packed_desc_event)), align); > } > > /* > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h > index 26518ed98..90bff7e38 100644 > --- a/drivers/net/virtio/virtqueue.h > +++ b/drivers/net/virtio/virtqueue.h > @@ -161,11 +161,16 @@ struct virtio_pmd_ctrl { > struct vq_desc_extra { > void *cookie; > uint16_t ndescs; > + uint16_t next; > }; > > struct virtqueue { > struct virtio_hw *hw; /**< virtio_hw structure pointer. */ > struct vring vq_ring; /**< vring keeping desc, used and avail */ > + struct vring_packed ring_packed; /**< vring keeping desc, used and avail */ The comment should be updated for packed ring. > + bool avail_wrap_counter; > + bool used_wrap_counter; > + > /** > * Last consumed descriptor in the used table, > * trails vq_ring.used->idx. > @@ -245,9 +250,21 @@ struct virtio_tx_region { > __attribute__((__aligned__(16))); > }; > > +static inline void > +vring_desc_init_packed(struct virtqueue *vq, int n) > +{ > + int i; > + for (i = 0; i < n - 1; i++) { > + vq->ring_packed.desc_packed[i].index = i; > + vq->vq_descx[i].next = i + 1; > + } > + vq->ring_packed.desc_packed[i].index = i; > + vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END; > +} > + > /* Chain all the descriptors in the ring with an END */ > static inline void > -vring_desc_init(struct vring_desc *dp, uint16_t n) > +vring_desc_init_split(struct vring_desc *dp, uint16_t n) > { > uint16_t i; > > -- > 2.17.1 >