When zero-copy usage of indirect descriptors buffer table isn't
possible, library gather scattered memory chunks in a local copy.
This commit fixes the issue with pointer arithmetic for the local copy
buffer.
Signed-off-by: Temir Zharaspayev <masscry@gmail.com>
---
subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 6684057370..e952c098a3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -2307,7 +2307,7 @@ static int
virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc,
uint64_t addr, size_t len)
{
- struct vring_desc *ori_desc;
+ uint8_t *src_cursor, *dst_cursor;
uint64_t read_len;
if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc))) {
@@ -2318,17 +2318,18 @@ virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc,
return -1;
}
+ dst_cursor = (uint8_t *) desc;
while (len) {
read_len = len;
- ori_desc = vu_gpa_to_va(dev, &read_len, addr);
- if (!ori_desc) {
+ src_cursor = vu_gpa_to_va(dev, &read_len, addr);
+ if (!src_cursor) {
return -1;
}
- memcpy(desc, ori_desc, read_len);
+ memcpy(dst_cursor, src_cursor, read_len);
len -= read_len;
addr += read_len;
- desc += read_len;
+ dst_cursor += read_len;
}
return 0;
--
2.34.1
On Sat, Jan 13, 2024 at 04:27:40AM +0300, Temir Zharaspayev wrote: > When zero-copy usage of indirect descriptors buffer table isn't > possible, library gather scattered memory chunks in a local copy. > This commit fixes the issue with pointer arithmetic for the local copy > buffer. > > Signed-off-by: Temir Zharaspayev <masscry@gmail.com> > --- > subprojects/libvhost-user/libvhost-user.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > index 6684057370..e952c098a3 100644 > --- a/subprojects/libvhost-user/libvhost-user.c > +++ b/subprojects/libvhost-user/libvhost-user.c > @@ -2307,7 +2307,7 @@ static int > virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc, > uint64_t addr, size_t len) > { > - struct vring_desc *ori_desc; > + uint8_t *src_cursor, *dst_cursor; > uint64_t read_len; > > if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc))) { > @@ -2318,17 +2318,18 @@ virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc, > return -1; > } > > + dst_cursor = (uint8_t *) desc; > while (len) { > read_len = len; > - ori_desc = vu_gpa_to_va(dev, &read_len, addr); > - if (!ori_desc) { > + src_cursor = vu_gpa_to_va(dev, &read_len, addr); > + if (!src_cursor) { > return -1; > } > > - memcpy(desc, ori_desc, read_len); > + memcpy(dst_cursor, src_cursor, read_len); > len -= read_len; > addr += read_len; > - desc += read_len; > + dst_cursor += read_len; The ori_desc -> src_cursor changes don't look to have any functional effect. Having that change present obscures the functional part of the patch, which is this line. FWIW, it is generally preferrable to not mix functional and non-functional changes in the same patch It now interprets 'read_len' as the number of bytes to increment the address by, rather than incrementing by the number of elements of size 'sizeof(struct vring_desc)'. I don't know enough about this area of QEMU code to say which semantics were desired, so I'll defer to the Michael as maintainer to give a formal review. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The change looks right to me. As is, it looks like the code is skipping over descriptors when the intent should be to bounce data into a single descriptor. I agree the variable rename should go in as a separate change. On Thu, Apr 18, 2024 at 6:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Sat, Jan 13, 2024 at 04:27:40AM +0300, Temir Zharaspayev wrote: > > When zero-copy usage of indirect descriptors buffer table isn't > > possible, library gather scattered memory chunks in a local copy. > > This commit fixes the issue with pointer arithmetic for the local copy > > buffer. > > > > Signed-off-by: Temir Zharaspayev <masscry@gmail.com> > > --- > > subprojects/libvhost-user/libvhost-user.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > > index 6684057370..e952c098a3 100644 > > --- a/subprojects/libvhost-user/libvhost-user.c > > +++ b/subprojects/libvhost-user/libvhost-user.c > > @@ -2307,7 +2307,7 @@ static int > > virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc, > > uint64_t addr, size_t len) > > { > > - struct vring_desc *ori_desc; > > + uint8_t *src_cursor, *dst_cursor; > > uint64_t read_len; > > > > if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc))) { > > @@ -2318,17 +2318,18 @@ virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc, > > return -1; > > } > > > > + dst_cursor = (uint8_t *) desc; Nit - no space on cast > > while (len) { > > read_len = len; > > - ori_desc = vu_gpa_to_va(dev, &read_len, addr); > > - if (!ori_desc) { > > + src_cursor = vu_gpa_to_va(dev, &read_len, addr); > > + if (!src_cursor) { > > return -1; > > } > > > > - memcpy(desc, ori_desc, read_len); > > + memcpy(dst_cursor, src_cursor, read_len); > > len -= read_len; > > addr += read_len; > > - desc += read_len; > > + dst_cursor += read_len; > > The ori_desc -> src_cursor changes don't look to have any functional > effect. Having that change present obscures the functional part of > the patch, which is this line. FWIW, it is generally preferrable to > not mix functional and non-functional changes in the same patch > > It now interprets 'read_len' as the number of bytes to increment the > address by, rather than incrementing by the number of elements of > size 'sizeof(struct vring_desc)'. > > I don't know enough about this area of QEMU code to say which > semantics were desired, so I'll defer to the Michael as maintainer > to give a formal review. > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >
© 2016 - 2024 Red Hat, Inc.