[PATCH v3 0/2] virtio: alignment issues

[PATCH v3 0/2] virtio: alignment issues

[Michael S. Tsirkin]

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

[PATCH v3 1/2] virtio: stop using legacy struct vring in kernel

[Michael S. Tsirkin]

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

[PATCH v3 2/2] vhost: force spec specified alignment on types

[Michael S. Tsirkin]

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

Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel

[kbuild test robot]

[-- 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 --]

Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel

[kbuild test robot]

[-- 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 --]

Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel

[kbuild test robot]

[-- 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 --]

Re: [PATCH v3 1/2] virtio: stop using legacy struct vring in kernel

[kbuild test robot]

[-- 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