Michael S. Tsirkin
2020-Apr-06 16:11 UTC
[PATCH v3 1/2] virtio: stop using legacy struct vring in kernel
struct vring (in the uapi directory) and supporting APIs are kept around to solely avoid breaking old userspace builds. It's not actually part of the UAPI - it was kept in the UAPI header by mistake, and using it in kernel isn't necessary and prevents us from making changes safely. In particular, the APIs actually assume the legacy layout. Add an internal kernel-only struct vring, add supporting legacy APIs and switch everyone to use that. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/virtio_ring.h | 28 +++++++++++++++++++++++++ include/uapi/linux/virtio_ring.h | 26 ++++++++++++++--------- tools/virtio/ringtest/virtio_ring_0_9.c | 6 +++--- tools/virtio/vringh_test.c | 20 +++++++++--------- 4 files changed, 57 insertions(+), 23 deletions(-) diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h index 3dc70adfe5f5..b6a31b3cf87c 100644 --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -112,4 +112,32 @@ void vring_del_virtqueue(struct virtqueue *vq); void vring_transport_features(struct virtio_device *vdev); irqreturn_t vring_interrupt(int irq, void *_vq); + +struct vring { + unsigned int num; + + struct vring_desc *desc; + + struct vring_avail *avail; + + struct vring_used *used; +}; + +static inline void vring_legacy_init(struct vring *vr, unsigned int num, void *p, + unsigned long align) +{ + vr->num = num; + vr->desc = p; + vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); + vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) + + align-1) & ~(align - 1)); +} + +static inline unsigned vring_legacy_size(unsigned int num, unsigned long align) +{ + return ((sizeof(struct vring_desc) * num + sizeof(__virtio16) * (3 + num) + + align - 1) & ~(align - 1)) + + sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num; +} + #endif /* _LINUX_VIRTIO_RING_H */ diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h index 559f42e73315..59939ba30b06 100644 --- a/include/uapi/linux/virtio_ring.h +++ b/include/uapi/linux/virtio_ring.h @@ -118,16 +118,6 @@ struct vring_used { struct vring_used_elem ring[]; }; -struct vring { - unsigned int num; - - struct vring_desc *desc; - - struct vring_avail *avail; - - struct vring_used *used; -}; - /* Alignment requirements for vring elements. * When using pre-virtio 1.0 layout, these fall out naturally. */ @@ -164,6 +154,21 @@ struct vring { #define vring_used_event(vr) ((vr)->avail->ring[(vr)->num]) #define vring_avail_event(vr) (*(__virtio16 *)&(vr)->used->ring[(vr)->num]) +#ifndef __KERNEL__ +/* + * The following definitions have been put in the UAPI header by mistake. We + * keep them around to avoid breaking old userspace builds. + */ +struct vring { + unsigned int num; + + struct vring_desc *desc; + + struct vring_avail *avail; + + struct vring_used *used; +}; + static inline void vring_init(struct vring *vr, unsigned int num, void *p, unsigned long align) { @@ -180,6 +185,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align) + align - 1) & ~(align - 1)) + sizeof(__virtio16) * 3 + sizeof(struct vring_used_elem) * num; } +#endif /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */ /* Assuming a given event_idx value from the other side, if diff --git a/tools/virtio/ringtest/virtio_ring_0_9.c b/tools/virtio/ringtest/virtio_ring_0_9.c index 13a035a390e9..e2ab6ac53966 100644 --- a/tools/virtio/ringtest/virtio_ring_0_9.c +++ b/tools/virtio/ringtest/virtio_ring_0_9.c @@ -67,13 +67,13 @@ void alloc_ring(void) int i; void *p; - ret = posix_memalign(&p, 0x1000, vring_size(ring_size, 0x1000)); + ret = posix_memalign(&p, 0x1000, vring_legacy_size(ring_size, 0x1000)); if (ret) { perror("Unable to allocate ring buffer.\n"); exit(3); } - memset(p, 0, vring_size(ring_size, 0x1000)); - vring_init(&ring, ring_size, p, 0x1000); + memset(p, 0, vring_legacy_size(ring_size, 0x1000)); + vring_legacy_init(&ring, ring_size, p, 0x1000); guest.avail_idx = 0; guest.kicked_avail_idx = -1; diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c index 293653463303..d26dc6530bd4 100644 --- a/tools/virtio/vringh_test.c +++ b/tools/virtio/vringh_test.c @@ -151,7 +151,7 @@ static int parallel_test(u64 features, err(1, "Opening /tmp/vringh_test-file"); /* Extra room at the end for some data, and indirects */ - mapsize = vring_size(RINGSIZE, ALIGN) + mapsize = vring_legacy_size(RINGSIZE, ALIGN) + RINGSIZE * 2 * sizeof(int) + RINGSIZE * 6 * sizeof(struct vring_desc); mapsize = (mapsize + getpagesize() - 1) & ~(getpagesize() - 1); @@ -185,7 +185,7 @@ static int parallel_test(u64 features, close(to_guest[0]); close(to_host[1]); - vring_init(&vrh.vring, RINGSIZE, host_map, ALIGN); + vring_legacy_init(&vrh.vring, RINGSIZE, host_map, ALIGN); vringh_init_user(&vrh, features, RINGSIZE, true, vrh.vring.desc, vrh.vring.avail, vrh.vring.used); CPU_SET(first_cpu, &cpu_set); @@ -297,7 +297,7 @@ static int parallel_test(u64 features, unsigned int finished = 0; /* We pass sg[]s pointing into here, but we need RINGSIZE+1 */ - data = guest_map + vring_size(RINGSIZE, ALIGN); + data = guest_map + vring_legacy_size(RINGSIZE, ALIGN); indirects = (void *)data + (RINGSIZE + 1) * 2 * sizeof(int); /* We are the guest. */ @@ -478,7 +478,7 @@ int main(int argc, char *argv[]) if (posix_memalign(&__user_addr_min, PAGE_SIZE, USER_MEM) != 0) abort(); __user_addr_max = __user_addr_min + USER_MEM; - memset(__user_addr_min, 0, vring_size(RINGSIZE, ALIGN)); + memset(__user_addr_min, 0, vring_legacy_size(RINGSIZE, ALIGN)); /* Set up guest side. */ vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true, false, @@ -487,7 +487,7 @@ int main(int argc, char *argv[]) "guest vq"); /* Set up host side. */ - vring_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN); + vring_legacy_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN); vringh_init_user(&vrh, vdev.features, RINGSIZE, true, vrh.vring.desc, vrh.vring.avail, vrh.vring.used); @@ -506,7 +506,7 @@ int main(int argc, char *argv[]) sgs[1] = &guest_sg[1]; /* May allocate an indirect, so force it to allocate user addr */ - __kmalloc_fake = __user_addr_min + vring_size(RINGSIZE, ALIGN); + __kmalloc_fake = __user_addr_min + vring_legacy_size(RINGSIZE, ALIGN); err = virtqueue_add_sgs(vq, sgs, 1, 1, &err, GFP_KERNEL); if (err) errx(1, "virtqueue_add_sgs: %i", err); @@ -556,7 +556,7 @@ int main(int argc, char *argv[]) errx(1, "vringh_complete_user: %i", err); /* Guest should see used token now. */ - __kfree_ignore_start = __user_addr_min + vring_size(RINGSIZE, ALIGN); + __kfree_ignore_start = __user_addr_min + vring_legacy_size(RINGSIZE, ALIGN); __kfree_ignore_end = __kfree_ignore_start + 1; ret = virtqueue_get_buf(vq, &i); if (ret != &err) @@ -575,7 +575,7 @@ int main(int argc, char *argv[]) ((char *)__user_addr_max - USER_MEM/4)[i] = i; /* This will allocate an indirect, so force it to allocate user addr */ - __kmalloc_fake = __user_addr_min + vring_size(RINGSIZE, ALIGN); + __kmalloc_fake = __user_addr_min + vring_legacy_size(RINGSIZE, ALIGN); err = virtqueue_add_outbuf(vq, guest_sg, RINGSIZE, &err, GFP_KERNEL); if (err) errx(1, "virtqueue_add_outbuf (large): %i", err); @@ -660,7 +660,7 @@ int main(int argc, char *argv[]) if (__virtio_test_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC)) { char *data = __user_addr_max - USER_MEM/4; struct vring_desc *d = __user_addr_max - USER_MEM/2; - struct vring vring; + struct vring_s vring; /* Force creation of direct, which we modify. */ __virtio_clear_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC); @@ -680,7 +680,7 @@ int main(int argc, char *argv[]) if (err) errx(1, "virtqueue_add_outbuf (indirect): %i", err); - vring_init(&vring, RINGSIZE, __user_addr_min, ALIGN); + vring_legacy_init(&vring, RINGSIZE, __user_addr_min, ALIGN); /* They're used in order, but double-check... */ assert(vring.desc[0].addr == (unsigned long)d); -- MST
kbuild test robot
2020-Apr-06 20:54 UTC
[PATCH v3 1/2] virtio: stop using legacy struct vring in kernel
Hi "Michael, I love your patch! Yet something to improve: [auto build test ERROR on next-20200406] [also build test ERROR on v5.6] [cannot apply to vhost/linux-next linus/master linux/master v5.6 v5.6-rc7 v5.6-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio-alignment-issues/20200407-025651 base: b2e2a818a01717ba15c74fd355f76822b81a95f6 config: nds32-defconfig (attached as .config) compiler: nds32le-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.3.0 make.cross ARCH=nds32 If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp at intel.com> All errors (new ones prefixed by >>): In file included from include/linux/virtio.h:12, from include/linux/virtio_config.h:7, from include/uapi/linux/virtio_net.h:30, from include/linux/virtio_net.h:6, from net//packet/af_packet.c:82:>> include/linux/vringh.h:42:15: error: field 'vring' has incomplete type42 | struct vring vring; | ^~~~~ vim +/vring +42 include/linux/vringh.h f87d0fbb579818 Rusty Russell 2013-03-20 20 f87d0fbb579818 Rusty Russell 2013-03-20 21 /* virtio_ring with information needed for host access. */ f87d0fbb579818 Rusty Russell 2013-03-20 22 struct vringh { b9f7ac8c72894c Michael S. Tsirkin 2014-12-12 23 /* Everything is little endian */ b9f7ac8c72894c Michael S. Tsirkin 2014-12-12 24 bool little_endian; b9f7ac8c72894c Michael S. Tsirkin 2014-12-12 25 f87d0fbb579818 Rusty Russell 2013-03-20 26 /* Guest publishes used event idx (note: we always do). */ f87d0fbb579818 Rusty Russell 2013-03-20 27 bool event_indices; f87d0fbb579818 Rusty Russell 2013-03-20 28 f87d0fbb579818 Rusty Russell 2013-03-20 29 /* Can we get away with weak barriers? */ f87d0fbb579818 Rusty Russell 2013-03-20 30 bool weak_barriers; f87d0fbb579818 Rusty Russell 2013-03-20 31 f87d0fbb579818 Rusty Russell 2013-03-20 32 /* Last available index we saw (ie. where we're up to). */ f87d0fbb579818 Rusty Russell 2013-03-20 33 u16 last_avail_idx; f87d0fbb579818 Rusty Russell 2013-03-20 34 f87d0fbb579818 Rusty Russell 2013-03-20 35 /* Last index we used. */ f87d0fbb579818 Rusty Russell 2013-03-20 36 u16 last_used_idx; f87d0fbb579818 Rusty Russell 2013-03-20 37 f87d0fbb579818 Rusty Russell 2013-03-20 38 /* How many descriptors we've completed since last need_notify(). */ f87d0fbb579818 Rusty Russell 2013-03-20 39 u32 completed; f87d0fbb579818 Rusty Russell 2013-03-20 40 f87d0fbb579818 Rusty Russell 2013-03-20 41 /* The vring (note: it may contain user pointers!) */ f87d0fbb579818 Rusty Russell 2013-03-20 @42 struct vring vring; 3beee86a4b9374 Sjur Br?ndeland 2013-03-20 43 9ad9c49cfe970b Jason Wang 2020-03-26 44 /* IOTLB for this vring */ 9ad9c49cfe970b Jason Wang 2020-03-26 45 struct vhost_iotlb *iotlb; 9ad9c49cfe970b Jason Wang 2020-03-26 46 3beee86a4b9374 Sjur Br?ndeland 2013-03-20 47 /* The function to call to notify the guest about added buffers */ 3beee86a4b9374 Sjur Br?ndeland 2013-03-20 48 void (*notify)(struct vringh *); 3beee86a4b9374 Sjur Br?ndeland 2013-03-20 49 }; 3beee86a4b9374 Sjur Br?ndeland 2013-03-20 50 :::::: The code at line 42 was first introduced by commit :::::: f87d0fbb579818fed3eeb0923cc253163ab93039 vringh: host-side implementation of virtio rings. :::::: TO: Rusty Russell <rusty at rustcorp.com.au> :::::: CC: Rusty Russell <rusty at rustcorp.com.au> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all at lists.01.org -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 10912 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20200407/0deab699/attachment-0001.gz>