* [PATCH v3 0/2] virtio: alignment issues @ 2020-04-06 16:11 Michael S. Tsirkin 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin 2020-04-06 16:12 ` [PATCH v3 2/2] vhost: force spec specified alignment on types Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2020-04-06 16:11 UTC (permalink / raw) To: linux-kernel This is an alternative to vhost: force spec specified alignment on types which is a bit safer as it does not change UAPI. I still think it's best to change the UAPI header as well, we can do that as a follow-up cleanup. Changes from v2: don't change struct name, instead add ifndef so kernel does not see the legacy UAPI version. Jason, can you pls ack one of the approaches? Michael S. Tsirkin (2): virtio: stop using legacy struct vring in kernel vhost: force spec specified alignment on types drivers/vhost/vhost.h | 6 ++-- include/linux/virtio_ring.h | 46 +++++++++++++++++++++++++ include/uapi/linux/virtio_ring.h | 26 ++++++++------ tools/virtio/ringtest/virtio_ring_0_9.c | 6 ++-- tools/virtio/vringh_test.c | 20 +++++------ 5 files changed, 78 insertions(+), 26 deletions(-) -- MST ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel 2020-04-06 16:11 [PATCH v3 0/2] virtio: alignment issues Michael S. Tsirkin @ 2020-04-06 16:11 ` Michael S. Tsirkin 2020-04-06 20:54 ` kbuild test robot ` (2 more replies) 2020-04-06 16:12 ` [PATCH v3 2/2] vhost: force spec specified alignment on types Michael S. Tsirkin 1 sibling, 3 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2020-04-06 16:11 UTC (permalink / raw) To: linux-kernel; +Cc: Jason Wang, virtualization 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@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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin @ 2020-04-06 20:54 ` kbuild test robot 2020-04-06 22:11 ` kbuild test robot 2020-04-08 0:13 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2020-04-06 20:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: kbuild-all, linux-kernel, Jason Wang, virtualization [-- Attachment #1: Type: text/plain, Size: 4219 bytes --] 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@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 type 42 | 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@rustcorp.com.au> :::::: CC: Rusty Russell <rusty@rustcorp.com.au> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 10912 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel @ 2020-04-06 20:54 ` kbuild test robot 0 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2020-04-06 20:54 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 4304 bytes --] 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@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 type 42 | 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@rustcorp.com.au> :::::: CC: Rusty Russell <rusty@rustcorp.com.au> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 10912 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin 2020-04-06 20:54 ` kbuild test robot @ 2020-04-06 22:11 ` kbuild test robot 2020-04-08 0:13 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2020-04-06 22:11 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 7106 bytes --] 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: powerpc-defconfig (attached as .config) compiler: powerpc64-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=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/virtio.h:12, from drivers/virtio/virtio_ring.c:6: include/linux/vringh.h:42:15: error: field 'vring' has incomplete type 42 | struct vring vring; | ^~~~~ drivers/virtio/virtio_ring.c: In function 'vring_create_virtqueue_split': >> drivers/virtio/virtio_ring.c:870:16: error: implicit declaration of function 'vring_size' [-Werror=implicit-function-declaration] 870 | for (; num && vring_size(num, vring_align) > PAGE_SIZE; num /= 2) { | ^~~~~~~~~~ >> drivers/virtio/virtio_ring.c:892:2: error: implicit declaration of function 'vring_init'; did you mean 'paging_init'? [-Werror=implicit-function-declaration] 892 | vring_init(&vring, num, queue, vring_align); | ^~~~~~~~~~ | paging_init cc1: some warnings being treated as errors vim +/vring_size +870 drivers/virtio/virtio_ring.c 138fd25148638a Tiwei Bie 2018-11-21 844 d79dca75c79680 Tiwei Bie 2018-11-21 845 static struct virtqueue *vring_create_virtqueue_split( d79dca75c79680 Tiwei Bie 2018-11-21 846 unsigned int index, d79dca75c79680 Tiwei Bie 2018-11-21 847 unsigned int num, d79dca75c79680 Tiwei Bie 2018-11-21 848 unsigned int vring_align, d79dca75c79680 Tiwei Bie 2018-11-21 849 struct virtio_device *vdev, d79dca75c79680 Tiwei Bie 2018-11-21 850 bool weak_barriers, d79dca75c79680 Tiwei Bie 2018-11-21 851 bool may_reduce_num, d79dca75c79680 Tiwei Bie 2018-11-21 852 bool context, d79dca75c79680 Tiwei Bie 2018-11-21 853 bool (*notify)(struct virtqueue *), d79dca75c79680 Tiwei Bie 2018-11-21 854 void (*callback)(struct virtqueue *), d79dca75c79680 Tiwei Bie 2018-11-21 855 const char *name) d79dca75c79680 Tiwei Bie 2018-11-21 856 { d79dca75c79680 Tiwei Bie 2018-11-21 857 struct virtqueue *vq; d79dca75c79680 Tiwei Bie 2018-11-21 858 void *queue = NULL; d79dca75c79680 Tiwei Bie 2018-11-21 859 dma_addr_t dma_addr; d79dca75c79680 Tiwei Bie 2018-11-21 860 size_t queue_size_in_bytes; d79dca75c79680 Tiwei Bie 2018-11-21 861 struct vring vring; d79dca75c79680 Tiwei Bie 2018-11-21 862 d79dca75c79680 Tiwei Bie 2018-11-21 863 /* We assume num is a power of 2. */ d79dca75c79680 Tiwei Bie 2018-11-21 864 if (num & (num - 1)) { d79dca75c79680 Tiwei Bie 2018-11-21 865 dev_warn(&vdev->dev, "Bad virtqueue length %u\n", num); d79dca75c79680 Tiwei Bie 2018-11-21 866 return NULL; d79dca75c79680 Tiwei Bie 2018-11-21 867 } d79dca75c79680 Tiwei Bie 2018-11-21 868 d79dca75c79680 Tiwei Bie 2018-11-21 869 /* TODO: allocate each queue chunk individually */ d79dca75c79680 Tiwei Bie 2018-11-21 @870 for (; num && vring_size(num, vring_align) > PAGE_SIZE; num /= 2) { d79dca75c79680 Tiwei Bie 2018-11-21 871 queue = vring_alloc_queue(vdev, vring_size(num, vring_align), d79dca75c79680 Tiwei Bie 2018-11-21 872 &dma_addr, d79dca75c79680 Tiwei Bie 2018-11-21 873 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO); d79dca75c79680 Tiwei Bie 2018-11-21 874 if (queue) d79dca75c79680 Tiwei Bie 2018-11-21 875 break; cf94db21905333 Cornelia Huck 2019-04-08 876 if (!may_reduce_num) cf94db21905333 Cornelia Huck 2019-04-08 877 return NULL; d79dca75c79680 Tiwei Bie 2018-11-21 878 } d79dca75c79680 Tiwei Bie 2018-11-21 879 d79dca75c79680 Tiwei Bie 2018-11-21 880 if (!num) d79dca75c79680 Tiwei Bie 2018-11-21 881 return NULL; d79dca75c79680 Tiwei Bie 2018-11-21 882 d79dca75c79680 Tiwei Bie 2018-11-21 883 if (!queue) { d79dca75c79680 Tiwei Bie 2018-11-21 884 /* Try to get a single page. You are my only hope! */ d79dca75c79680 Tiwei Bie 2018-11-21 885 queue = vring_alloc_queue(vdev, vring_size(num, vring_align), d79dca75c79680 Tiwei Bie 2018-11-21 886 &dma_addr, GFP_KERNEL|__GFP_ZERO); d79dca75c79680 Tiwei Bie 2018-11-21 887 } d79dca75c79680 Tiwei Bie 2018-11-21 888 if (!queue) d79dca75c79680 Tiwei Bie 2018-11-21 889 return NULL; d79dca75c79680 Tiwei Bie 2018-11-21 890 d79dca75c79680 Tiwei Bie 2018-11-21 891 queue_size_in_bytes = vring_size(num, vring_align); d79dca75c79680 Tiwei Bie 2018-11-21 @892 vring_init(&vring, num, queue, vring_align); d79dca75c79680 Tiwei Bie 2018-11-21 893 d79dca75c79680 Tiwei Bie 2018-11-21 894 vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers, context, d79dca75c79680 Tiwei Bie 2018-11-21 895 notify, callback, name); d79dca75c79680 Tiwei Bie 2018-11-21 896 if (!vq) { d79dca75c79680 Tiwei Bie 2018-11-21 897 vring_free_queue(vdev, queue_size_in_bytes, queue, d79dca75c79680 Tiwei Bie 2018-11-21 898 dma_addr); d79dca75c79680 Tiwei Bie 2018-11-21 899 return NULL; d79dca75c79680 Tiwei Bie 2018-11-21 900 } d79dca75c79680 Tiwei Bie 2018-11-21 901 d79dca75c79680 Tiwei Bie 2018-11-21 902 to_vvq(vq)->split.queue_dma_addr = dma_addr; d79dca75c79680 Tiwei Bie 2018-11-21 903 to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes; d79dca75c79680 Tiwei Bie 2018-11-21 904 to_vvq(vq)->we_own_ring = true; d79dca75c79680 Tiwei Bie 2018-11-21 905 d79dca75c79680 Tiwei Bie 2018-11-21 906 return vq; d79dca75c79680 Tiwei Bie 2018-11-21 907 } d79dca75c79680 Tiwei Bie 2018-11-21 908 :::::: The code at line 870 was first introduced by commit :::::: d79dca75c79680f52a27a7ee1b6ae75066f36b3e virtio_ring: extract split ring handling from ring creation :::::: TO: Tiwei Bie <tiwei.bie@intel.com> :::::: CC: David S. Miller <davem@davemloft.net> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 26179 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin 2020-04-06 20:54 ` kbuild test robot 2020-04-06 22:11 ` kbuild test robot @ 2020-04-08 0:13 ` kbuild test robot 2 siblings, 0 replies; 7+ messages in thread From: kbuild test robot @ 2020-04-08 0:13 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 8092 bytes --] Hi "Michael, I love your patch! Perhaps something to improve: [auto build test WARNING on next-20200406] [also build test WARNING 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 reproduce: # apt-get install sparse # sparse version: v0.6.1-188-g79f7ac98-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) drivers/remoteproc/remoteproc_core.c:331:16: sparse: sparse: undefined identifier 'vring_size' drivers/remoteproc/remoteproc_core.c:331:16: sparse: sparse: undefined identifier 'vring_size' drivers/remoteproc/remoteproc_core.c:331:16: sparse: sparse: incompatible types for operation (-): >> drivers/remoteproc/remoteproc_core.c:331:16: sparse: bad type >> drivers/remoteproc/remoteproc_core.c:331:16: sparse: int drivers/remoteproc/remoteproc_core.c:331:16: sparse: sparse: undefined identifier 'vring_size' drivers/remoteproc/remoteproc_core.c:331:16: sparse: sparse: incompatible types for operation (-): >> drivers/remoteproc/remoteproc_core.c:331:16: sparse: bad type >> drivers/remoteproc/remoteproc_core.c:331:16: sparse: int -- drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: sparse: undefined identifier 'vring_size' drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: sparse: undefined identifier 'vring_size' drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: sparse: incompatible types for operation (-): >> drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: bad type >> drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: int drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: sparse: undefined identifier 'vring_size' drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: sparse: incompatible types for operation (-): >> drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: bad type >> drivers/misc/mic/vop/vop_vringh.c:299:27: sparse: int drivers/misc/mic/vop/vop_vringh.c:311:37: sparse: sparse: undefined identifier 'vring_size' drivers/misc/mic/vop/vop_vringh.c:324:17: sparse: sparse: undefined identifier 'vring_init' vim +331 drivers/remoteproc/remoteproc_core.c c874bf59add0e6 Loic Pallardy 2018-07-27 319 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 320 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i) 400e64df6b237e Ohad Ben-Cohen 2011-10-20 321 { 7a186941626d19 Ohad Ben-Cohen 2012-02-13 322 struct rproc *rproc = rvdev->rproc; b5ab5e24e960b9 Ohad Ben-Cohen 2012-05-30 323 struct device *dev = &rproc->dev; 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 324 struct rproc_vring *rvring = &rvdev->vring[i]; c0d631570ad54a Sjur Brændeland 2013-02-21 325 struct fw_rsc_vdev *rsc; 096ee78669d2bc Clement Leger 2020-03-02 326 int ret, notifyid; c6aed238b7a9b1 Loic Pallardy 2018-07-27 327 struct rproc_mem_entry *mem; 096ee78669d2bc Clement Leger 2020-03-02 328 size_t size; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 329 7a186941626d19 Ohad Ben-Cohen 2012-02-13 330 /* actual size of vring (in bytes) */ 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 @331 size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); 400e64df6b237e Ohad Ben-Cohen 2011-10-20 332 c6aed238b7a9b1 Loic Pallardy 2018-07-27 333 rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; c6aed238b7a9b1 Loic Pallardy 2018-07-27 334 c6aed238b7a9b1 Loic Pallardy 2018-07-27 335 /* Search for pre-registered carveout */ c6aed238b7a9b1 Loic Pallardy 2018-07-27 336 mem = rproc_find_carveout_by_name(rproc, "vdev%dvring%d", rvdev->index, c6aed238b7a9b1 Loic Pallardy 2018-07-27 337 i); c6aed238b7a9b1 Loic Pallardy 2018-07-27 338 if (mem) { c6aed238b7a9b1 Loic Pallardy 2018-07-27 339 if (rproc_check_carveout_da(rproc, mem, rsc->vring[i].da, size)) c6aed238b7a9b1 Loic Pallardy 2018-07-27 340 return -ENOMEM; c6aed238b7a9b1 Loic Pallardy 2018-07-27 341 } else { c6aed238b7a9b1 Loic Pallardy 2018-07-27 342 /* Register carveout in in list */ 99cf0361e7af29 Ben Dooks (Codethink 2019-10-17 343) mem = rproc_mem_entry_init(dev, NULL, 0, 99cf0361e7af29 Ben Dooks (Codethink 2019-10-17 344) size, rsc->vring[i].da, c6aed238b7a9b1 Loic Pallardy 2018-07-27 345 rproc_alloc_carveout, c6aed238b7a9b1 Loic Pallardy 2018-07-27 346 rproc_release_carveout, c6aed238b7a9b1 Loic Pallardy 2018-07-27 347 "vdev%dvring%d", c6aed238b7a9b1 Loic Pallardy 2018-07-27 348 rvdev->index, i); c6aed238b7a9b1 Loic Pallardy 2018-07-27 349 if (!mem) { c6aed238b7a9b1 Loic Pallardy 2018-07-27 350 dev_err(dev, "Can't allocate memory entry structure\n"); c6aed238b7a9b1 Loic Pallardy 2018-07-27 351 return -ENOMEM; c6aed238b7a9b1 Loic Pallardy 2018-07-27 352 } c6aed238b7a9b1 Loic Pallardy 2018-07-27 353 c6aed238b7a9b1 Loic Pallardy 2018-07-27 354 rproc_add_carveout(rproc, mem); 400e64df6b237e Ohad Ben-Cohen 2011-10-20 355 } 400e64df6b237e Ohad Ben-Cohen 2011-10-20 356 400e64df6b237e Ohad Ben-Cohen 2011-10-20 357 /* 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 358 * Assign an rproc-wide unique index for this vring 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 359 * TODO: assign a notifyid for rvdev updates as well 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 360 * TODO: support predefined notifyids (via resource table) 400e64df6b237e Ohad Ben-Cohen 2011-10-20 361 */ 15fc61106a203b Tejun Heo 2013-02-27 362 ret = idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KERNEL); b39599b7cb8f29 Suman Anna 2013-03-06 363 if (ret < 0) { 15fc61106a203b Tejun Heo 2013-02-27 364 dev_err(dev, "idr_alloc failed: %d\n", ret); 7a186941626d19 Ohad Ben-Cohen 2012-02-13 365 return ret; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 366 } 15fc61106a203b Tejun Heo 2013-02-27 367 notifyid = ret; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 368 48f18f89896338 Bjorn Andersson 2016-10-19 369 /* Potentially bump max_notifyid */ 48f18f89896338 Bjorn Andersson 2016-10-19 370 if (notifyid > rproc->max_notifyid) 48f18f89896338 Bjorn Andersson 2016-10-19 371 rproc->max_notifyid = notifyid; 48f18f89896338 Bjorn Andersson 2016-10-19 372 6db20ea8d85064 Ohad Ben-Cohen 2012-05-17 373 rvring->notifyid = notifyid; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 374 c6aed238b7a9b1 Loic Pallardy 2018-07-27 375 /* Let the rproc know the notifyid of this vring.*/ c0d631570ad54a Sjur Brændeland 2013-02-21 376 rsc->vring[i].notifyid = notifyid; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 377 return 0; 400e64df6b237e Ohad Ben-Cohen 2011-10-20 378 } 400e64df6b237e Ohad Ben-Cohen 2011-10-20 379 :::::: The code at line 331 was first introduced by commit :::::: 6db20ea8d85064175c7ef594c433c6c2e6bbab83 remoteproc: allocate vrings on demand, free when not needed :::::: TO: Ohad Ben-Cohen <ohad@wizery.com> :::::: CC: Ohad Ben-Cohen <ohad@wizery.com> --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] vhost: force spec specified alignment on types 2020-04-06 16:11 [PATCH v3 0/2] virtio: alignment issues Michael S. Tsirkin 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin @ 2020-04-06 16:12 ` Michael S. Tsirkin 1 sibling, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2020-04-06 16:12 UTC (permalink / raw) To: linux-kernel; +Cc: Jason Wang, kvm, virtualization, netdev The ring element addresses are passed between components with different alignments assumptions. Thus, if guest/userspace selects a pointer and host then gets and dereferences it, we might need to decrease the compiler-selected alignment to prevent compiler on the host from assuming pointer is aligned. This actually triggers on ARM with -mabi=apcs-gnu - which is a deprecated configuration, but it seems safer to handle this generally. I verified that the produced binary is exactly identical on x86. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.h | 6 +++--- include/linux/virtio_ring.h | 24 +++++++++++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 181382185bbc..3ceaafecc1fb 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -67,9 +67,9 @@ struct vhost_virtqueue { /* The actual ring of buffers. */ struct mutex mutex; unsigned int num; - struct vring_desc __user *desc; - struct vring_avail __user *avail; - struct vring_used __user *used; + vring_desc_t __user *desc; + vring_avail_t __user *avail; + vring_used_t __user *used; const struct vhost_iotlb_map *meta_iotlb[VHOST_NUM_ADDRS]; struct file *kick; struct eventfd_ctx *call_ctx; diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h index b6a31b3cf87c..dfb58eff7a7f 100644 --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -113,14 +113,32 @@ void vring_transport_features(struct virtio_device *vdev); irqreturn_t vring_interrupt(int irq, void *_vq); +/* + * The ring element addresses are passed between components with different + * alignments assumptions. Thus, we might need to decrease the compiler-selected + * alignment, and so must use a typedef to make sure the __aligned attribute + * actually takes hold: + * + * https://gcc.gnu.org/onlinedocs//gcc/Common-Type-Attributes.html#Common-Type-Attributes + * + * When used on a struct, or struct member, the aligned attribute can only + * increase the alignment; in order to decrease it, the packed attribute must + * be specified as well. When used as part of a typedef, the aligned attribute + * can both increase and decrease alignment, and specifying the packed + * attribute generates a warning. + */ +typedef struct vring_desc __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t; +typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t; +typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t; + struct vring { unsigned int num; - struct vring_desc *desc; + vring_desc_t *desc; - struct vring_avail *avail; + vring_avail_t *avail; - struct vring_used *used; + vring_used_t *used; }; static inline void vring_legacy_init(struct vring *vr, unsigned int num, void *p, -- MST ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-08 0:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-06 16:11 [PATCH v3 0/2] virtio: alignment issues Michael S. Tsirkin 2020-04-06 16:11 ` [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel Michael S. Tsirkin 2020-04-06 20:54 ` kbuild test robot 2020-04-06 20:54 ` kbuild test robot 2020-04-06 22:11 ` kbuild test robot 2020-04-08 0:13 ` kbuild test robot 2020-04-06 16:12 ` [PATCH v3 2/2] vhost: force spec specified alignment on types Michael S. Tsirkin
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.