From: Jason Wang <jasowang@redhat.com> To: Guo Zhi <qtxuning1999@sjtu.edu.cn>, eperezma@redhat.com, sgarzare@redhat.com, mst@redhat.com Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [RFC 4/5] virtio: get desc id in order Date: Tue, 26 Jul 2022 16:07:46 +0800 [thread overview] Message-ID: <9d4c24de-f2cc-16a0-818a-16695946f3a3@redhat.com> (raw) In-Reply-To: <20220721084341.24183-5-qtxuning1999@sjtu.edu.cn> 在 2022/7/21 16:43, Guo Zhi 写道: > If in order feature negotiated, we can skip the used ring to get > buffer's desc id sequentially. Let's rename the patch to something like "in order support for virtio_ring" > > Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn> > --- > drivers/virtio/virtio_ring.c | 37 ++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) I don't see packed support in this patch, we need to implement that. > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index a5ec724c0..4d57a4edc 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -144,6 +144,9 @@ struct vring_virtqueue { > /* DMA address and size information */ > dma_addr_t queue_dma_addr; > size_t queue_size_in_bytes; > + > + /* In order feature batch begin here */ > + u16 next_batch_desc_begin; > } split; > > /* Available for packed ring */ > @@ -700,8 +703,10 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > } > > vring_unmap_one_split(vq, i); > - vq->split.desc_extra[i].next = vq->free_head; > - vq->free_head = head; > + if (!virtio_has_feature(vq->vq.vdev, VIRTIO_F_IN_ORDER)) { > + vq->split.desc_extra[i].next = vq->free_head; > + vq->free_head = head; > + } Let's add a comment to explain why we don't need anything if in order is neogitated. > > /* Plus final descriptor */ > vq->vq.num_free++; > @@ -743,7 +748,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > { > struct vring_virtqueue *vq = to_vvq(_vq); > void *ret; > - unsigned int i; > + __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT); > + unsigned int i, j; > u16 last_used; > > START_USE(vq); > @@ -762,11 +768,24 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > /* Only get used array entries after they have been exposed by host. */ > virtio_rmb(vq->weak_barriers); > > - last_used = (vq->last_used_idx & (vq->split.vring.num - 1)); > - i = virtio32_to_cpu(_vq->vdev, > - vq->split.vring.used->ring[last_used].id); > - *len = virtio32_to_cpu(_vq->vdev, > - vq->split.vring.used->ring[last_used].len); > + if (virtio_has_feature(_vq->vdev, VIRTIO_F_IN_ORDER)) { > + /* Skip used ring and get used desc in order*/ > + i = vq->split.next_batch_desc_begin; > + j = i; > + while (vq->split.vring.desc[j].flags & nextflag) Let's don't depend on the descriptor ring which is under the control of the malicious hypervisor. Let's use desc_extra that is not visible by the hypervisor. More can be seen in this commit: 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra for split virtqueue") > + j = (j + 1) % vq->split.vring.num; > + /* move to next */ > + j = (j + 1) % vq->split.vring.num; > + vq->split.next_batch_desc_begin = j; I'm not sure I get the logic here, basically I think we should check buffer instead of descriptor here. So if vring.used->ring[last_used].id != last_used, we know all [last_used, vring.used->ring[last_used].id] have been used in a batch? > + > + /* TODO: len of buffer */ So spec said: " The skipped buffers (for which no used ring entry was written) are assumed to have been used (read or written) by the device completely. " Thanks > + } else { > + last_used = (vq->last_used_idx & (vq->split.vring.num - 1)); > + i = virtio32_to_cpu(_vq->vdev, > + vq->split.vring.used->ring[last_used].id); > + *len = virtio32_to_cpu(_vq->vdev, > + vq->split.vring.used->ring[last_used].len); > + } > > if (unlikely(i >= vq->split.vring.num)) { > BAD_RING(vq, "id %u out of range\n", i); > @@ -2234,6 +2253,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > vq->split.avail_flags_shadow = 0; > vq->split.avail_idx_shadow = 0; > > + vq->split.next_batch_desc_begin = 0; > + > /* No callback? Tell other side not to bother us. */ > if (!callback) { > vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: Guo Zhi <qtxuning1999@sjtu.edu.cn>, eperezma@redhat.com, sgarzare@redhat.com, mst@redhat.com Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [RFC 4/5] virtio: get desc id in order Date: Tue, 26 Jul 2022 16:07:46 +0800 [thread overview] Message-ID: <9d4c24de-f2cc-16a0-818a-16695946f3a3@redhat.com> (raw) In-Reply-To: <20220721084341.24183-5-qtxuning1999@sjtu.edu.cn> 在 2022/7/21 16:43, Guo Zhi 写道: > If in order feature negotiated, we can skip the used ring to get > buffer's desc id sequentially. Let's rename the patch to something like "in order support for virtio_ring" > > Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn> > --- > drivers/virtio/virtio_ring.c | 37 ++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) I don't see packed support in this patch, we need to implement that. > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index a5ec724c0..4d57a4edc 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -144,6 +144,9 @@ struct vring_virtqueue { > /* DMA address and size information */ > dma_addr_t queue_dma_addr; > size_t queue_size_in_bytes; > + > + /* In order feature batch begin here */ > + u16 next_batch_desc_begin; > } split; > > /* Available for packed ring */ > @@ -700,8 +703,10 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > } > > vring_unmap_one_split(vq, i); > - vq->split.desc_extra[i].next = vq->free_head; > - vq->free_head = head; > + if (!virtio_has_feature(vq->vq.vdev, VIRTIO_F_IN_ORDER)) { > + vq->split.desc_extra[i].next = vq->free_head; > + vq->free_head = head; > + } Let's add a comment to explain why we don't need anything if in order is neogitated. > > /* Plus final descriptor */ > vq->vq.num_free++; > @@ -743,7 +748,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > { > struct vring_virtqueue *vq = to_vvq(_vq); > void *ret; > - unsigned int i; > + __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT); > + unsigned int i, j; > u16 last_used; > > START_USE(vq); > @@ -762,11 +768,24 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > /* Only get used array entries after they have been exposed by host. */ > virtio_rmb(vq->weak_barriers); > > - last_used = (vq->last_used_idx & (vq->split.vring.num - 1)); > - i = virtio32_to_cpu(_vq->vdev, > - vq->split.vring.used->ring[last_used].id); > - *len = virtio32_to_cpu(_vq->vdev, > - vq->split.vring.used->ring[last_used].len); > + if (virtio_has_feature(_vq->vdev, VIRTIO_F_IN_ORDER)) { > + /* Skip used ring and get used desc in order*/ > + i = vq->split.next_batch_desc_begin; > + j = i; > + while (vq->split.vring.desc[j].flags & nextflag) Let's don't depend on the descriptor ring which is under the control of the malicious hypervisor. Let's use desc_extra that is not visible by the hypervisor. More can be seen in this commit: 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra for split virtqueue") > + j = (j + 1) % vq->split.vring.num; > + /* move to next */ > + j = (j + 1) % vq->split.vring.num; > + vq->split.next_batch_desc_begin = j; I'm not sure I get the logic here, basically I think we should check buffer instead of descriptor here. So if vring.used->ring[last_used].id != last_used, we know all [last_used, vring.used->ring[last_used].id] have been used in a batch? > + > + /* TODO: len of buffer */ So spec said: " The skipped buffers (for which no used ring entry was written) are assumed to have been used (read or written) by the device completely. " Thanks > + } else { > + last_used = (vq->last_used_idx & (vq->split.vring.num - 1)); > + i = virtio32_to_cpu(_vq->vdev, > + vq->split.vring.used->ring[last_used].id); > + *len = virtio32_to_cpu(_vq->vdev, > + vq->split.vring.used->ring[last_used].len); > + } > > if (unlikely(i >= vq->split.vring.num)) { > BAD_RING(vq, "id %u out of range\n", i); > @@ -2234,6 +2253,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, > vq->split.avail_flags_shadow = 0; > vq->split.avail_idx_shadow = 0; > > + vq->split.next_batch_desc_begin = 0; > + > /* No callback? Tell other side not to bother us. */ > if (!callback) { > vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-07-26 8:08 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-21 8:43 [RFC 0/5] In virtio-spec 1.1, new feature bit VIRTIO_F_IN_ORDER was introduced Guo Zhi 2022-07-21 8:43 ` [RFC 1/5] vhost: reorder used descriptors in a batch Guo Zhi 2022-07-22 7:07 ` Eugenio Perez Martin 2022-08-02 3:30 ` Guo Zhi 2022-07-26 7:36 ` Jason Wang 2022-07-26 7:36 ` Jason Wang [not found] ` <2a8838c4-2e6f-6de7-dcdc-572699ff3dc9@sjtu.edu.cn> 2022-07-29 7:32 ` Jason Wang 2022-07-29 7:32 ` Jason Wang 2022-08-02 3:09 ` Guo Zhi 2022-08-02 14:12 ` Guo Zhi 2022-08-04 5:04 ` Jason Wang 2022-08-04 5:04 ` Jason Wang 2022-08-11 8:58 ` Guo Zhi 2022-08-02 13:54 ` Guo Zhi 2022-07-21 8:43 ` [RFC 2/5] vhost: announce VIRTIO_F_IN_ORDER support Guo Zhi 2022-07-21 8:43 ` [RFC 3/5] vhost_test: batch used buffer Guo Zhi 2022-07-22 7:12 ` Eugenio Perez Martin 2022-08-02 2:47 ` Guo Zhi 2022-08-02 3:08 ` Guo Zhi [not found] ` <1D1ABF88-B503-4BE0-AC83-3326EAA62510@sjtu.edu.cn> 2022-08-02 7:45 ` Stefano Garzarella 2022-08-02 7:45 ` Stefano Garzarella 2022-07-21 8:43 ` [RFC 4/5] virtio: get desc id in order Guo Zhi 2022-07-26 8:07 ` Jason Wang [this message] 2022-07-26 8:07 ` Jason Wang 2022-07-28 8:12 ` Guo Zhi 2022-08-11 8:49 ` Guo Zhi 2022-07-21 8:43 ` [RFC 5/5] virtio: annouce VIRTIO_F_IN_ORDER support Guo Zhi 2022-07-21 9:17 ` [RFC 0/5] In virtio-spec 1.1, new feature bit VIRTIO_F_IN_ORDER was introduced Jason Wang 2022-07-21 9:17 ` Jason Wang 2022-07-21 11:54 ` Guo Zhi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9d4c24de-f2cc-16a0-818a-16695946f3a3@redhat.com \ --to=jasowang@redhat.com \ --cc=eperezma@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=qtxuning1999@sjtu.edu.cn \ --cc=sgarzare@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.