2016-12-06 15:40:36

by Michael S. Tsirkin

Subject: [PATCH 00/10] virtio: sparse fixes

I run latest sparse from git on virtio drivers
(turns out the version I had was rather outdated).
This patchset fixes a couple of bugs this uncovered,
and adds some annotations to make it sparse-clean.
In particular, endian-ness is often tricky,
so this patchset enabled endian-ness checks for sparse
builds.

Michael S. Tsirkin (10):
virtio_console: drop unused config fields
drm/virtio: fix endianness in primary_plane_update
drm/virtio: fix lock context imbalance
drm/virtio: annotate virtio_gpu_queue_ctrl_buffer_locked
vhost: make interval tree static inline
vhost: add missing __user annotations
vsock/virtio: add a missing __le annotation
vsock/virtio: mark an internal function static
vsock/virtio: fix src/dst cid format
virtio: enable endian checks for sparse builds

drivers/char/virtio_console.c | 14 +++++++-------
drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++--
drivers/gpu/drm/virtio/virtgpu_vq.c | 6 +++++-
drivers/vhost/vhost.c | 12 ++++++------
net/vmw_vsock/virtio_transport.c | 2 +-
net/vmw_vsock/virtio_transport_common.c | 16 ++++++++--------
drivers/block/Makefile | 1 +
drivers/char/Makefile | 1 +
drivers/char/hw_random/Makefile | 2 ++
drivers/gpu/drm/virtio/Makefile | 1 +
drivers/net/Makefile | 3 +++
drivers/net/caif/Makefile | 1 +
drivers/rpmsg/Makefile | 1 +
drivers/s390/virtio/Makefile | 2 ++
drivers/scsi/Makefile | 1 +
drivers/vhost/Makefile | 1 +
drivers/virtio/Makefile | 3 +++
net/9p/Makefile | 1 +
net/packet/Makefile | 1 +
net/vmw_vsock/Makefile | 2 ++
20 files changed, 50 insertions(+), 25 deletions(-)

--
MST

2016-12-06 15:40:44

by Michael S. Tsirkin

Subject: [PATCH 01/10] virtio_console: drop unused config fields

struct ports_device includes a config field including the whole
virtio_console_config, but only max_nr_ports in there is ever updated or
used. The rest is unused and in fact does not even mirror the
device config. Drop everything except max_nr_ports,
saving some memory.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5649234..8b00e79 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -152,8 +152,8 @@ struct ports_device {
spinlock_t c_ivq_lock;
spinlock_t c_ovq_lock;

- /* The current config space is stored here */
- struct virtio_console_config config;
+ /* max. number of ports this device can hold */
+ u32 max_nr_ports;

/* The virtio device we're associated with */
struct virtio_device *vdev;
@@ -1649,11 +1649,11 @@ static void handle_control_message(struct virtio_device *vdev,
break;
}
if (virtio32_to_cpu(vdev, cpkt->id) >=
- portdev->config.max_nr_ports) {
+ portdev->max_nr_ports) {
dev_warn(&portdev->vdev->dev,
"Request for adding port with "
"out-of-bound id %u, max. supported id: %u\n",
- cpkt->id, portdev->config.max_nr_ports - 1);
+ cpkt->id, portdev->max_nr_ports - 1);
break;
}
add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
@@ -1894,7 +1894,7 @@ static int init_vqs(struct ports_device *portdev)
u32 i, j, nr_ports, nr_queues;
int err;

- nr_ports = portdev->config.max_nr_ports;
+ nr_ports = portdev->max_nr_ports;
nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;

vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
@@ -2047,13 +2047,13 @@ static int virtcons_probe(struct virtio_device *vdev)
}

multiport = false;
- portdev->config.max_nr_ports = 1;
+ portdev->max_nr_ports = 1;

/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
if (!is_rproc_serial(vdev) &&
virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
struct virtio_console_config, max_nr_ports,
- &portdev->config.max_nr_ports) == 0) {
+ &portdev->max_nr_ports) == 0) {
multiport = true;
}

--
MST

2016-12-06 15:40:50

by Michael S. Tsirkin

Subject: [PATCH 03/10] drm/virtio: fix lock context imbalance

When virtio_gpu_free_vbufs exits due to list empty, it does not
drop the free_vbufs lock that it took.
list empty is not expected to happen anyway, but it can't hurt to fix
this and drop the lock.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_vq.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 5a0f8a7..2f0c2f9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -109,8 +109,10 @@ void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev)

spin_lock(&vgdev->free_vbufs_lock);
for (i = 0; i < count; i++) {
- if (WARN_ON(list_empty(&vgdev->free_vbufs)))
+ if (WARN_ON(list_empty(&vgdev->free_vbufs))) {
+ spin_unlock(&vgdev->free_vbufs_lock);
return;
+ }
vbuf = list_first_entry(&vgdev->free_vbufs,
struct virtio_gpu_vbuffer, list);
list_del(&vbuf->list);
--
MST

2016-12-06 15:40:58

by Michael S. Tsirkin

Subject: [PATCH 04/10] drm/virtio: annotate virtio_gpu_queue_ctrl_buffer_locked

virtio_gpu_queue_ctrl_buffer_locked is called with ctrlq.qlock taken, it
releases and acquires this lock. This causes a sparse warning. Add
appropriate annotations for sparse context checking.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 2f0c2f9..a6e2ce4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -297,6 +297,8 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)

static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
struct virtio_gpu_vbuffer *vbuf)
+ __releases(&vgdev->ctrlq.qlock)
+ __acquires(&vgdev->ctrlq.qlock)
{
struct virtqueue *vq = vgdev->ctrlq.vq;
struct scatterlist *sgs[3], vcmd, vout, vresp;
--
MST

2016-12-06 15:41:07

by Michael S. Tsirkin

Subject: [PATCH 08/10] vsock/virtio: mark an internal function static

virtio_transport_alloc_pkt is only used locally, make it static.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index a53b3a1..6120384 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -32,7 +32,7 @@ static const struct virtio_transport *virtio_transport_get_ops(void)
return container_of(t, struct virtio_transport, transport);
}

-struct virtio_vsock_pkt *
+static struct virtio_vsock_pkt *
virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
size_t len,
u32 src_cid,
--
MST

2016-12-06 15:41:22

by Michael S. Tsirkin

Subject: [PATCH 09/10] vsock/virtio: fix src/dst cid format

These fields are 64 bit, using le32_to_cpu and friends
on these will not do the right thing.
Fix this up.

Cc: [email protected]
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 6120384..22e99c4 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -606,9 +606,9 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
return 0;

pkt = virtio_transport_alloc_pkt(&info, 0,
- le32_to_cpu(pkt->hdr.dst_cid),
+ le64_to_cpu(pkt->hdr.dst_cid),
le32_to_cpu(pkt->hdr.dst_port),
- le32_to_cpu(pkt->hdr.src_cid),
+ le64_to_cpu(pkt->hdr.src_cid),
le32_to_cpu(pkt->hdr.src_port));
if (!pkt)
return -ENOMEM;
@@ -823,7 +823,7 @@ virtio_transport_send_response(struct vsock_sock *vsk,
struct virtio_vsock_pkt_info info = {
.op = VIRTIO_VSOCK_OP_RESPONSE,
.type = VIRTIO_VSOCK_TYPE_STREAM,
- .remote_cid = le32_to_cpu(pkt->hdr.src_cid),
+ .remote_cid = le64_to_cpu(pkt->hdr.src_cid),
.remote_port = le32_to_cpu(pkt->hdr.src_port),
.reply = true,
};
@@ -863,9 +863,9 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt)
child->sk_state = SS_CONNECTED;

vchild = vsock_sk(child);
- vsock_addr_init(&vchild->local_addr, le32_to_cpu(pkt->hdr.dst_cid),
+ vsock_addr_init(&vchild->local_addr, le64_to_cpu(pkt->hdr.dst_cid),
le32_to_cpu(pkt->hdr.dst_port));
- vsock_addr_init(&vchild->remote_addr, le32_to_cpu(pkt->hdr.src_cid),
+ vsock_addr_init(&vchild->remote_addr, le64_to_cpu(pkt->hdr.src_cid),
le32_to_cpu(pkt->hdr.src_port));

vsock_insert_connected(vchild);
@@ -904,9 +904,9 @@ void virtio_transport_recv_pkt(struct virtio_vsock_pkt *pkt)
struct sock *sk;
bool space_available;

- vsock_addr_init(&src, le32_to_cpu(pkt->hdr.src_cid),
+ vsock_addr_init(&src, le64_to_cpu(pkt->hdr.src_cid),
le32_to_cpu(pkt->hdr.src_port));
- vsock_addr_init(&dst, le32_to_cpu(pkt->hdr.dst_cid),
+ vsock_addr_init(&dst, le64_to_cpu(pkt->hdr.dst_cid),
le32_to_cpu(pkt->hdr.dst_port));

trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
--
MST

2016-12-06 15:41:18

by Michael S. Tsirkin

Subject: [PATCH 10/10] virtio: enable endian checks for sparse builds

__CHECK_ENDIAN__ isn't on by default presumably because
it triggers too many sparse warnings for correct code.
But virtio is now clean of these warnings, and
we want to keep it this way - enable this for
sparse builds.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---

It seems that there should be a better way to do it,
but this works too.

drivers/block/Makefile | 1 +
drivers/char/Makefile | 1 +
drivers/char/hw_random/Makefile | 2 ++
drivers/gpu/drm/virtio/Makefile | 1 +
drivers/net/Makefile | 3 +++
drivers/net/caif/Makefile | 1 +
drivers/rpmsg/Makefile | 1 +
drivers/s390/virtio/Makefile | 2 ++
drivers/scsi/Makefile | 1 +
drivers/vhost/Makefile | 1 +
drivers/virtio/Makefile | 3 +++
net/9p/Makefile | 1 +
net/packet/Makefile | 1 +
net/vmw_vsock/Makefile | 2 ++
14 files changed, 21 insertions(+)

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 1e9661e..597481c 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_BLK_DEV_OSD) += osdblk.o
obj-$(CONFIG_BLK_DEV_UMEM) += umem.o
obj-$(CONFIG_BLK_DEV_NBD) += nbd.o
obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
+CFLAGS_virtio_blk.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o

obj-$(CONFIG_BLK_DEV_SX8) += sx8.o
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 6e6c244..a99467d 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -6,6 +6,7 @@ obj-y += mem.o random.o
obj-$(CONFIG_TTY_PRINTK) += ttyprintk.o
obj-y += misc.o
obj-$(CONFIG_ATARI_DSP56K) += dsp56k.o
+CFLAGS_virtio_console.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_VIRTIO_CONSOLE) += virtio_console.o
obj-$(CONFIG_RAW_DRIVER) += raw.o
obj-$(CONFIG_SGI_SNSC) += snsc.o snsc_event.o
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e..a2b0931 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -17,6 +17,8 @@ obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
obj-$(CONFIG_HW_RANDOM_PASEMI) += pasemi-rng.o
+CFLAGS_virtio_transport.o += -D__CHECK_ENDIAN__
+CFLAGS_virtio-rng.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_HW_RANDOM_VIRTIO) += virtio-rng.o
obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index 3fb8eac..1162366 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -3,6 +3,7 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

ccflags-y := -Iinclude/drm
+ccflags-y += -D__CHECK_ENDIAN__

virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_drm_bus.o virtgpu_gem.o \
virtgpu_fb.o virtgpu_display.o virtgpu_vq.o virtgpu_ttm.o \
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 7336cbd..3f587de 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EQUALIZER) += eql.o
obj-$(CONFIG_IFB) += ifb.o
obj-$(CONFIG_MACSEC) += macsec.o
obj-$(CONFIG_MACVLAN) += macvlan.o
+CFLAGS_macvtap.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_MACVTAP) += macvtap.o
obj-$(CONFIG_MII) += mii.o
obj-$(CONFIG_MDIO) += mdio.o
@@ -20,8 +21,10 @@ obj-$(CONFIG_NETCONSOLE) += netconsole.o
obj-$(CONFIG_PHYLIB) += phy/
obj-$(CONFIG_RIONET) += rionet.o
obj-$(CONFIG_NET_TEAM) += team/
+CFLAGS_tun.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_TUN) += tun.o
obj-$(CONFIG_VETH) += veth.o
+CFLAGS_virtio_net.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
obj-$(CONFIG_VXLAN) += vxlan.o
obj-$(CONFIG_GENEVE) += geneve.o
diff --git a/drivers/net/caif/Makefile b/drivers/net/caif/Makefile
index 9bbd453..d1a922c 100644
--- a/drivers/net/caif/Makefile
+++ b/drivers/net/caif/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_CAIF_HSI) += caif_hsi.o

# Virtio interface
obj-$(CONFIG_CAIF_VIRTIO) += caif_virtio.o
+CFLAGS_caif_virtio.o += -D__CHECK_ENDIAN__
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index ae9c913..23c8b66 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_RPMSG) += rpmsg_core.o
obj-$(CONFIG_RPMSG_QCOM_SMD) += qcom_smd.o
obj-$(CONFIG_RPMSG_VIRTIO) += virtio_rpmsg_bus.o
+CFLAGS_virtio_rpmsg_bus.o += -D__CHECK_ENDIAN__
diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
index df40692..270ada5 100644
--- a/drivers/s390/virtio/Makefile
+++ b/drivers/s390/virtio/Makefile
@@ -6,6 +6,8 @@
# it under the terms of the GNU General Public License (version 2 only)
# as published by the Free Software Foundation.

+CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
+CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__
s390-virtio-objs := virtio_ccw.o
ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
s390-virtio-objs += kvm_virtio.o
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 38d938d..9f70d46 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -135,6 +135,7 @@ obj-$(CONFIG_SCSI_BNX2_ISCSI) += libiscsi.o bnx2i/
obj-$(CONFIG_BE2ISCSI) += libiscsi.o be2iscsi/
obj-$(CONFIG_SCSI_ESAS2R) += esas2r/
obj-$(CONFIG_SCSI_PMCRAID) += pmcraid.o
+CFLAGS_virtio_scsi.o += -D__CHECK_ENDIAN__
obj-$(CONFIG_SCSI_VIRTIO) += virtio_scsi.o
obj-$(CONFIG_VMWARE_PVSCSI) += vmw_pvscsi.o
obj-$(CONFIG_XEN_SCSI_FRONTEND) += xen-scsifront.o
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
index 6b012b9..619e2cd 100644
--- a/drivers/vhost/Makefile
+++ b/drivers/vhost/Makefile
@@ -1,3 +1,4 @@
+ccflags-y := -D__CHECK_ENDIAN__
obj-$(CONFIG_VHOST_NET) += vhost_net.o
vhost_net-y := net.o

diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 41e30e3..d331f19 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,3 +1,6 @@
+#virtio must be kept clean wrt endian tags,
+#otherwise we'll get to maintain broken host/guest ABIs
+ccflags-y := -D__CHECK_ENDIAN__
obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
diff --git a/net/9p/Makefile b/net/9p/Makefile
index a0874cc..acf1225 100644
--- a/net/9p/Makefile
+++ b/net/9p/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
trans_fd.o \
trans_common.o \

+CFLAGS_trans_virtio.o += -D__CHECK_ENDIAN__
9pnet_virtio-objs := \
trans_virtio.o \

diff --git a/net/packet/Makefile b/net/packet/Makefile
index 9df6134..a13bcb3 100644
--- a/net/packet/Makefile
+++ b/net/packet/Makefile
@@ -2,6 +2,7 @@
# Makefile for the packet AF.
#

+ccflags-y := -D__CHECK_ENDIAN__
obj-$(CONFIG_PACKET) += af_packet.o
obj-$(CONFIG_PACKET_DIAG) += af_packet_diag.o
af_packet_diag-y += diag.o
diff --git a/net/vmw_vsock/Makefile b/net/vmw_vsock/Makefile
index bc27c70..a61eccb 100644
--- a/net/vmw_vsock/Makefile
+++ b/net/vmw_vsock/Makefile
@@ -8,6 +8,8 @@ vsock-y += af_vsock.o vsock_addr.o
vmw_vsock_vmci_transport-y += vmci_transport.o vmci_transport_notify.o \
vmci_transport_notify_qstate.o

+CFLAGS_virtio_transport.o += -D__CHECK_ENDIAN__
+CFLAGS_virtio_transport_common.o += -D__CHECK_ENDIAN__
vmw_vsock_virtio_transport-y += virtio_transport.o

vmw_vsock_virtio_transport_common-y += virtio_transport_common.o
--
MST

2016-12-06 15:41:02

by Michael S. Tsirkin

Subject: [PATCH 06/10] vhost: add missing __user annotations

Several vhost functions were missing __user annotations
on pointers, causing sparse warnings. Fix this up.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/vhost/vhost.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 7331ef3..ba7db68 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -719,7 +719,7 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
struct iovec iov[], int iov_size, int access);

-static int vhost_copy_to_user(struct vhost_virtqueue *vq, void *to,
+static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
const void *from, unsigned size)
{
int ret;
@@ -749,7 +749,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void *to,
}

static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
- void *from, unsigned size)
+ void __user *from, unsigned size)
{
int ret;

@@ -783,7 +783,7 @@ static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
}

static void __user *__vhost_get_user(struct vhost_virtqueue *vq,
- void *addr, unsigned size)
+ void __user *addr, unsigned size)
{
int ret;

@@ -934,8 +934,8 @@ static int umem_access_ok(u64 uaddr, u64 size, int access)
return 0;
}

-int vhost_process_iotlb_msg(struct vhost_dev *dev,
- struct vhost_iotlb_msg *msg)
+static int vhost_process_iotlb_msg(struct vhost_dev *dev,
+ struct vhost_iotlb_msg *msg)
{
int ret = 0;

--
MST

2016-12-06 15:42:21

by Michael S. Tsirkin

Subject: [PATCH 07/10] vsock/virtio: add a missing __le annotation

guest cid is read from config space, therefore it's in little endian
format and is treated as such, annotate it accordingly.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 936d7ee..90096b9 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -336,7 +336,7 @@ static void virtio_vsock_reset_sock(struct sock *sk)
static void virtio_vsock_update_guest_cid(struct virtio_vsock *vsock)
{
struct virtio_device *vdev = vsock->vdev;
- u64 guest_cid;
+ __le64 guest_cid;

vdev->config->get(vdev, offsetof(struct virtio_vsock_config, guest_cid),
&guest_cid, sizeof(guest_cid));
--
MST

2016-12-06 15:42:59

by Michael S. Tsirkin

Subject: [PATCH 05/10] vhost: make interval tree static inline

vhost_umem_interval_tree is only used locally within vhost.c, mark it
static. As some functions generated go unused, this triggers warnings
unless we also mark it inline.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c6f2d89..7331ef3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -49,7 +49,7 @@ enum {

INTERVAL_TREE_DEFINE(struct vhost_umem_node,
rb, __u64, __subtree_last,
- START, LAST, , vhost_umem_interval_tree);
+ START, LAST, static inline, vhost_umem_interval_tree);

#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
static void vhost_disable_cross_endian(struct vhost_virtqueue *vq)
--
MST

2016-12-06 15:40:48

by Michael S. Tsirkin

Subject: [PATCH 02/10] drm/virtio: fix endianness in primary_plane_update

virtio_gpu_cmd_transfer_to_host_2d expects x and y
parameters in LE, but virtio_gpu_primary_plane_update
passes in the CPU format instead.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index ba28c0f..1fda965 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -88,8 +88,8 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
(vgdev, handle, 0,
cpu_to_le32(plane->state->src_w >> 16),
cpu_to_le32(plane->state->src_h >> 16),
- plane->state->src_x >> 16,
- plane->state->src_y >> 16, NULL);
+ cpu_to_le32(plane->state->src_x >> 16),
+ cpu_to_le32(plane->state->src_y >> 16), NULL);
}
} else {
handle = 0;
--
MST

2016-12-07 04:13:57

by Jason Wang

Subject: Re: [PATCH 03/10] drm/virtio: fix lock context imbalance

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> When virtio_gpu_free_vbufs exits due to list empty, it does not
> drop the free_vbufs lock that it took.
> list empty is not expected to happen anyway, but it can't hurt to fix
> this and drop the lock.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_vq.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Jason Wang <[email protected]>

>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 5a0f8a7..2f0c2f9 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -109,8 +109,10 @@ void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev)
>
> spin_lock(&vgdev->free_vbufs_lock);
> for (i = 0; i < count; i++) {
> - if (WARN_ON(list_empty(&vgdev->free_vbufs)))
> + if (WARN_ON(list_empty(&vgdev->free_vbufs))) {
> + spin_unlock(&vgdev->free_vbufs_lock);
> return;
> + }
> vbuf = list_first_entry(&vgdev->free_vbufs,
> struct virtio_gpu_vbuffer, list);
> list_del(&vbuf->list);

2016-12-07 04:13:56

by Jason Wang

Subject: Re: [PATCH 02/10] drm/virtio: fix endianness in primary_plane_update

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> virtio_gpu_cmd_transfer_to_host_2d expects x and y
> parameters in LE, but virtio_gpu_primary_plane_update
> passes in the CPU format instead.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Jason Wang <[email protected]>

>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index ba28c0f..1fda965 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -88,8 +88,8 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
> (vgdev, handle, 0,
> cpu_to_le32(plane->state->src_w >> 16),
> cpu_to_le32(plane->state->src_h >> 16),
> - plane->state->src_x >> 16,
> - plane->state->src_y >> 16, NULL);
> + cpu_to_le32(plane->state->src_x >> 16),
> + cpu_to_le32(plane->state->src_y >> 16), NULL);
> }
> } else {
> handle = 0;

2016-12-07 04:13:53

by Jason Wang

Subject: Re: [PATCH 01/10] virtio_console: drop unused config fields

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> struct ports_device includes a config field including the whole
> virtio_console_config, but only max_nr_ports in there is ever updated or
> used. The rest is unused and in fact does not even mirror the
> device config. Drop everything except max_nr_ports,
> saving some memory.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/char/virtio_console.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Jason Wang <[email protected]>

>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 5649234..8b00e79 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -152,8 +152,8 @@ struct ports_device {
> spinlock_t c_ivq_lock;
> spinlock_t c_ovq_lock;
>
> - /* The current config space is stored here */
> - struct virtio_console_config config;
> + /* max. number of ports this device can hold */
> + u32 max_nr_ports;
>
> /* The virtio device we're associated with */
> struct virtio_device *vdev;
> @@ -1649,11 +1649,11 @@ static void handle_control_message(struct virtio_device *vdev,
> break;
> }
> if (virtio32_to_cpu(vdev, cpkt->id) >=
> - portdev->config.max_nr_ports) {
> + portdev->max_nr_ports) {
> dev_warn(&portdev->vdev->dev,
> "Request for adding port with "
> "out-of-bound id %u, max. supported id: %u\n",
> - cpkt->id, portdev->config.max_nr_ports - 1);
> + cpkt->id, portdev->max_nr_ports - 1);
> break;
> }
> add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
> @@ -1894,7 +1894,7 @@ static int init_vqs(struct ports_device *portdev)
> u32 i, j, nr_ports, nr_queues;
> int err;
>
> - nr_ports = portdev->config.max_nr_ports;
> + nr_ports = portdev->max_nr_ports;
> nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>
> vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
> @@ -2047,13 +2047,13 @@ static int virtcons_probe(struct virtio_device *vdev)
> }
>
> multiport = false;
> - portdev->config.max_nr_ports = 1;
> + portdev->max_nr_ports = 1;
>
> /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
> if (!is_rproc_serial(vdev) &&
> virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> struct virtio_console_config, max_nr_ports,
> - &portdev->config.max_nr_ports) == 0) {
> + &portdev->max_nr_ports) == 0) {
> multiport = true;
> }
>

2016-12-07 04:16:25

by Jason Wang

Subject: Re: [PATCH 05/10] vhost: make interval tree static inline

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> vhost_umem_interval_tree is only used locally within vhost.c, mark it
> static. As some functions generated go unused, this triggers warnings
> unless we also mark it inline.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index c6f2d89..7331ef3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -49,7 +49,7 @@ enum {
>
> INTERVAL_TREE_DEFINE(struct vhost_umem_node,
> rb, __u64, __subtree_last,
> - START, LAST, , vhost_umem_interval_tree);
> + START, LAST, static inline, vhost_umem_interval_tree);
>
> #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> static void vhost_disable_cross_endian(struct vhost_virtqueue *vq)

Reviewed-by: Jason Wang <[email protected]>

2016-12-07 04:16:24

by Jason Wang

Subject: Re: [PATCH 04/10] drm/virtio: annotate virtio_gpu_queue_ctrl_buffer_locked

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> virtio_gpu_queue_ctrl_buffer_locked is called with ctrlq.qlock taken, it
> releases and acquires this lock. This causes a sparse warning. Add
> appropriate annotations for sparse context checking.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 2f0c2f9..a6e2ce4 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -297,6 +297,8 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
>
> static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_vbuffer *vbuf)
> + __releases(&vgdev->ctrlq.qlock)
> + __acquires(&vgdev->ctrlq.qlock)
> {
> struct virtqueue *vq = vgdev->ctrlq.vq;
> struct scatterlist *sgs[3], vcmd, vout, vresp;

Reviewed-by: Jason Wang <[email protected]>

2016-12-07 04:17:34

by Jason Wang

Subject: Re: [PATCH 06/10] vhost: add missing __user annotations

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> Several vhost functions were missing __user annotations
> on pointers, causing sparse warnings. Fix this up.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/vhost/vhost.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 7331ef3..ba7db68 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -719,7 +719,7 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
> struct iovec iov[], int iov_size, int access);
>
> -static int vhost_copy_to_user(struct vhost_virtqueue *vq, void *to,
> +static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
> const void *from, unsigned size)
> {
> int ret;
> @@ -749,7 +749,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void *to,
> }
>
> static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
> - void *from, unsigned size)
> + void __user *from, unsigned size)
> {
> int ret;
>
> @@ -783,7 +783,7 @@ static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
> }
>
> static void __user *__vhost_get_user(struct vhost_virtqueue *vq,
> - void *addr, unsigned size)
> + void __user *addr, unsigned size)
> {
> int ret;
>
> @@ -934,8 +934,8 @@ static int umem_access_ok(u64 uaddr, u64 size, int access)
> return 0;
> }
>
> -int vhost_process_iotlb_msg(struct vhost_dev *dev,
> - struct vhost_iotlb_msg *msg)
> +static int vhost_process_iotlb_msg(struct vhost_dev *dev,
> + struct vhost_iotlb_msg *msg)
> {
> int ret = 0;
>

Patch looks good but this looks like another static conversion not
__user annotations.

2016-12-07 04:19:21

by Jason Wang

Subject: Re: [PATCH 07/10] vsock/virtio: add a missing __le annotation

On 2016年12月06日 23:40, Michael S. Tsirkin wrote:
> guest cid is read from config space, therefore it's in little endian
> format and is treated as such, annotate it accordingly.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 936d7ee..90096b9 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -336,7 +336,7 @@ static void virtio_vsock_reset_sock(struct sock *sk)
> static void virtio_vsock_update_guest_cid(struct virtio_vsock *vsock)
> {
> struct virtio_device *vdev = vsock->vdev;
> - u64 guest_cid;
> + __le64 guest_cid;
>
> vdev->config->get(vdev, offsetof(struct virtio_vsock_config, guest_cid),
> &guest_cid, sizeof(guest_cid));

Reviewed-by: Jason Wang <[email protected]>

2016-12-07 04:21:29

by Jason Wang

Subject: Re: [PATCH 08/10] vsock/virtio: mark an internal function static

On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
> virtio_transport_alloc_pkt is only used locally, make it static.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index a53b3a1..6120384 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -32,7 +32,7 @@ static const struct virtio_transport *virtio_transport_get_ops(void)
> return container_of(t, struct virtio_transport, transport);
> }
>
> -struct virtio_vsock_pkt *
> +static struct virtio_vsock_pkt *
> virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
> size_t len,
> u32 src_cid,

Git grep shows it was used by tracing.

2016-12-07 04:32:04

by Jason Wang

Subject: Re: [PATCH 09/10] vsock/virtio: fix src/dst cid format

On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
> These fields are 64 bit, using le32_to_cpu and friends
> on these will not do the right thing.
> Fix this up.
>
> Cc: [email protected]
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport_common.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 6120384..22e99c4 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -606,9 +606,9 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> return 0;
>
> pkt = virtio_transport_alloc_pkt(&info, 0,
> - le32_to_cpu(pkt->hdr.dst_cid),
> + le64_to_cpu(pkt->hdr.dst_cid),
> le32_to_cpu(pkt->hdr.dst_port),
> - le32_to_cpu(pkt->hdr.src_cid),
> + le64_to_cpu(pkt->hdr.src_cid),
> le32_to_cpu(pkt->hdr.src_port));

Looking at sockaddr_vm, svm_cid is "unsigned int", do we really want 64
bit here?

> if (!pkt)
> return -ENOMEM;
> @@ -823,7 +823,7 @@ virtio_transport_send_response(struct vsock_sock *vsk,
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_RESPONSE,
> .type = VIRTIO_VSOCK_TYPE_STREAM,
> - .remote_cid = le32_to_cpu(pkt->hdr.src_cid),
> + .remote_cid = le64_to_cpu(pkt->hdr.src_cid),
> .remote_port = le32_to_cpu(pkt->hdr.src_port),
> .reply = true,
> };
> @@ -863,9 +863,9 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt)
> child->sk_state = SS_CONNECTED;
>
> vchild = vsock_sk(child);
> - vsock_addr_init(&vchild->local_addr, le32_to_cpu(pkt->hdr.dst_cid),
> + vsock_addr_init(&vchild->local_addr, le64_to_cpu(pkt->hdr.dst_cid),
> le32_to_cpu(pkt->hdr.dst_port));
> - vsock_addr_init(&vchild->remote_addr, le32_to_cpu(pkt->hdr.src_cid),
> + vsock_addr_init(&vchild->remote_addr, le64_to_cpu(pkt->hdr.src_cid),
> le32_to_cpu(pkt->hdr.src_port));
>
> vsock_insert_connected(vchild);
> @@ -904,9 +904,9 @@ void virtio_transport_recv_pkt(struct virtio_vsock_pkt *pkt)
> struct sock *sk;
> bool space_available;
>
> - vsock_addr_init(&src, le32_to_cpu(pkt->hdr.src_cid),
> + vsock_addr_init(&src, le64_to_cpu(pkt->hdr.src_cid),
> le32_to_cpu(pkt->hdr.src_port));
> - vsock_addr_init(&dst, le32_to_cpu(pkt->hdr.dst_cid),
> + vsock_addr_init(&dst, le64_to_cpu(pkt->hdr.dst_cid),
> le32_to_cpu(pkt->hdr.dst_port));
>
> trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,

2016-12-07 05:27:57

by Jason Wang

Subject: Re: [PATCH 10/10] virtio: enable endian checks for sparse builds

On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
> __CHECK_ENDIAN__ isn't on by default presumably because
> it triggers too many sparse warnings for correct code.
> But virtio is now clean of these warnings, and
> we want to keep it this way - enable this for
> sparse builds.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
>
> It seems that there should be a better way to do it,
> but this works too.

Reviewed-by: Jason Wang <[email protected]>

>
> drivers/block/Makefile | 1 +
> drivers/char/Makefile | 1 +
> drivers/char/hw_random/Makefile | 2 ++
> drivers/gpu/drm/virtio/Makefile | 1 +
> drivers/net/Makefile | 3 +++
> drivers/net/caif/Makefile | 1 +
> drivers/rpmsg/Makefile | 1 +
> drivers/s390/virtio/Makefile | 2 ++
> drivers/scsi/Makefile | 1 +
> drivers/vhost/Makefile | 1 +
> drivers/virtio/Makefile | 3 +++
> net/9p/Makefile | 1 +
> net/packet/Makefile | 1 +
> net/vmw_vsock/Makefile | 2 ++
> 14 files changed, 21 insertions(+)
>
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index 1e9661e..597481c 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_BLK_DEV_OSD) += osdblk.o
> obj-$(CONFIG_BLK_DEV_UMEM) += umem.o
> obj-$(CONFIG_BLK_DEV_NBD) += nbd.o
> obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
> +CFLAGS_virtio_blk.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
>
> obj-$(CONFIG_BLK_DEV_SX8) += sx8.o
> diff --git a/drivers/char/Makefile b/drivers/char/Makefile
> index 6e6c244..a99467d 100644
> --- a/drivers/char/Makefile
> +++ b/drivers/char/Makefile
> @@ -6,6 +6,7 @@ obj-y += mem.o random.o
> obj-$(CONFIG_TTY_PRINTK) += ttyprintk.o
> obj-y += misc.o
> obj-$(CONFIG_ATARI_DSP56K) += dsp56k.o
> +CFLAGS_virtio_console.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_VIRTIO_CONSOLE) += virtio_console.o
> obj-$(CONFIG_RAW_DRIVER) += raw.o
> obj-$(CONFIG_SGI_SNSC) += snsc.o snsc_event.o
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 5f52b1e..a2b0931 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -17,6 +17,8 @@ obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> obj-$(CONFIG_HW_RANDOM_PASEMI) += pasemi-rng.o
> +CFLAGS_virtio_transport.o += -D__CHECK_ENDIAN__
> +CFLAGS_virtio-rng.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_HW_RANDOM_VIRTIO) += virtio-rng.o
> obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
> obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
> diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
> index 3fb8eac..1162366 100644
> --- a/drivers/gpu/drm/virtio/Makefile
> +++ b/drivers/gpu/drm/virtio/Makefile
> @@ -3,6 +3,7 @@
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
> ccflags-y := -Iinclude/drm
> +ccflags-y += -D__CHECK_ENDIAN__
>
> virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_drm_bus.o virtgpu_gem.o \
> virtgpu_fb.o virtgpu_display.o virtgpu_vq.o virtgpu_ttm.o \
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 7336cbd..3f587de 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_EQUALIZER) += eql.o
> obj-$(CONFIG_IFB) += ifb.o
> obj-$(CONFIG_MACSEC) += macsec.o
> obj-$(CONFIG_MACVLAN) += macvlan.o
> +CFLAGS_macvtap.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_MACVTAP) += macvtap.o
> obj-$(CONFIG_MII) += mii.o
> obj-$(CONFIG_MDIO) += mdio.o
> @@ -20,8 +21,10 @@ obj-$(CONFIG_NETCONSOLE) += netconsole.o
> obj-$(CONFIG_PHYLIB) += phy/
> obj-$(CONFIG_RIONET) += rionet.o
> obj-$(CONFIG_NET_TEAM) += team/
> +CFLAGS_tun.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_TUN) += tun.o
> obj-$(CONFIG_VETH) += veth.o
> +CFLAGS_virtio_net.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
> obj-$(CONFIG_VXLAN) += vxlan.o
> obj-$(CONFIG_GENEVE) += geneve.o
> diff --git a/drivers/net/caif/Makefile b/drivers/net/caif/Makefile
> index 9bbd453..d1a922c 100644
> --- a/drivers/net/caif/Makefile
> +++ b/drivers/net/caif/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_CAIF_HSI) += caif_hsi.o
>
> # Virtio interface
> obj-$(CONFIG_CAIF_VIRTIO) += caif_virtio.o
> +CFLAGS_caif_virtio.o += -D__CHECK_ENDIAN__
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index ae9c913..23c8b66 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_RPMSG) += rpmsg_core.o
> obj-$(CONFIG_RPMSG_QCOM_SMD) += qcom_smd.o
> obj-$(CONFIG_RPMSG_VIRTIO) += virtio_rpmsg_bus.o
> +CFLAGS_virtio_rpmsg_bus.o += -D__CHECK_ENDIAN__
> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
> index df40692..270ada5 100644
> --- a/drivers/s390/virtio/Makefile
> +++ b/drivers/s390/virtio/Makefile
> @@ -6,6 +6,8 @@
> # it under the terms of the GNU General Public License (version 2 only)
> # as published by the Free Software Foundation.
>
> +CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
> +CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__
> s390-virtio-objs := virtio_ccw.o
> ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> s390-virtio-objs += kvm_virtio.o
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 38d938d..9f70d46 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -135,6 +135,7 @@ obj-$(CONFIG_SCSI_BNX2_ISCSI) += libiscsi.o bnx2i/
> obj-$(CONFIG_BE2ISCSI) += libiscsi.o be2iscsi/
> obj-$(CONFIG_SCSI_ESAS2R) += esas2r/
> obj-$(CONFIG_SCSI_PMCRAID) += pmcraid.o
> +CFLAGS_virtio_scsi.o += -D__CHECK_ENDIAN__
> obj-$(CONFIG_SCSI_VIRTIO) += virtio_scsi.o
> obj-$(CONFIG_VMWARE_PVSCSI) += vmw_pvscsi.o
> obj-$(CONFIG_XEN_SCSI_FRONTEND) += xen-scsifront.o
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index 6b012b9..619e2cd 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y := -D__CHECK_ENDIAN__
> obj-$(CONFIG_VHOST_NET) += vhost_net.o
> vhost_net-y := net.o
>
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3..d331f19 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -1,3 +1,6 @@
> +#virtio must be kept clean wrt endian tags,
> +#otherwise we'll get to maintain broken host/guest ABIs
> +ccflags-y := -D__CHECK_ENDIAN__
> obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> diff --git a/net/9p/Makefile b/net/9p/Makefile
> index a0874cc..acf1225 100644
> --- a/net/9p/Makefile
> +++ b/net/9p/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
> trans_fd.o \
> trans_common.o \
>
> +CFLAGS_trans_virtio.o += -D__CHECK_ENDIAN__
> 9pnet_virtio-objs := \
> trans_virtio.o \
>
> diff --git a/net/packet/Makefile b/net/packet/Makefile
> index 9df6134..a13bcb3 100644
> --- a/net/packet/Makefile
> +++ b/net/packet/Makefile
> @@ -2,6 +2,7 @@
> # Makefile for the packet AF.
> #
>
> +ccflags-y := -D__CHECK_ENDIAN__
> obj-$(CONFIG_PACKET) += af_packet.o
> obj-$(CONFIG_PACKET_DIAG) += af_packet_diag.o
> af_packet_diag-y += diag.o
> diff --git a/net/vmw_vsock/Makefile b/net/vmw_vsock/Makefile
> index bc27c70..a61eccb 100644
> --- a/net/vmw_vsock/Makefile
> +++ b/net/vmw_vsock/Makefile
> @@ -8,6 +8,8 @@ vsock-y += af_vsock.o vsock_addr.o
> vmw_vsock_vmci_transport-y += vmci_transport.o vmci_transport_notify.o \
> vmci_transport_notify_qstate.o
>
> +CFLAGS_virtio_transport.o += -D__CHECK_ENDIAN__
> +CFLAGS_virtio_transport_common.o += -D__CHECK_ENDIAN__
> vmw_vsock_virtio_transport-y += virtio_transport.o
>
> vmw_vsock_virtio_transport_common-y += virtio_transport_common.o

2016-12-07 06:26:41

by Johannes Berg

Subject: Re: [PATCH 10/10] virtio: enable endian checks for sparse builds

On Tue, 2016-12-06 at 17:41 +0200, Michael S. Tsirkin wrote:

> It seems that there should be a better way to do it,
> but this works too.

In some cases there might be:

> --- a/drivers/s390/virtio/Makefile
> +++ b/drivers/s390/virtio/Makefile
> @@ -6,6 +6,8 @@
>  # it under the terms of the GNU General Public License (version 2
> only)
>  # as published by the Free Software Foundation.
>  
> +CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
> +CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__
>  s390-virtio-objs := virtio_ccw.o
>  ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
>  s390-virtio-objs += kvm_virtio.o

Here you could use

ccflags-y += -D__CHECK_ENDIAN__

for example, or even

subdir-ccflags-y += -D__CHECK_ENDIAN__

(in case any subdirs ever get added here)

> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y := -D__CHECK_ENDIAN__

Looks like you did that here and in some other places though - so
perhaps the s390 one was intentionally different?

> --- a/net/packet/Makefile
> +++ b/net/packet/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for the packet AF.
>  #
>  
> +ccflags-y := -D__CHECK_ENDIAN__

Technically this is slightly more than advertised, but I guess that
still makes sense if it's clean now.

johannes

2016-12-07 07:30:27

by Christoph Hellwig

Subject: Re: [PATCH 10/10] virtio: enable endian checks for sparse builds

On Tue, Dec 06, 2016 at 05:41:05PM +0200, Michael S. Tsirkin wrote:
> __CHECK_ENDIAN__ isn't on by default presumably because
> it triggers too many sparse warnings for correct code.
> But virtio is now clean of these warnings, and
> we want to keep it this way - enable this for
> sparse builds.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>

Nah. Please just enable it globally when using sparse. I actually
had a chat with Linus about that a while ago and he seemed generally
fine with it, I just didn't manage to actually do it..

2016-12-07 13:22:35

by Michael S. Tsirkin

Subject: Re: [PATCH 10/10] virtio: enable endian checks for sparse builds

On Wed, Dec 07, 2016 at 07:25:51AM +0100, Johannes Berg wrote:
> On Tue, 2016-12-06 at 17:41 +0200, Michael S. Tsirkin wrote:
>
> > It seems that there should be a better way to do it,
> > but this works too.
>
> In some cases there might be:
>
> > --- a/drivers/s390/virtio/Makefile
> > +++ b/drivers/s390/virtio/Makefile
> > @@ -6,6 +6,8 @@
> > ?# it under the terms of the GNU General Public License (version 2
> > only)
> > ?# as published by the Free Software Foundation.
> > ?
> > +CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
> > +CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__
> > ?s390-virtio-objs := virtio_ccw.o
> > ?ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> > ?s390-virtio-objs += kvm_virtio.o
>
> Here you could use
>
> ccflags-y += -D__CHECK_ENDIAN__
>
> for example, or even
>
> subdir-ccflags-y += -D__CHECK_ENDIAN__
>
> (in case any subdirs ever get added here)

Oh right. I forgot this directory only has virtio stuff.

> > --- a/drivers/vhost/Makefile
> > +++ b/drivers/vhost/Makefile
> > @@ -1,3 +1,4 @@
> > +ccflags-y := -D__CHECK_ENDIAN__
>
> Looks like you did that here and in some other places though - so
> perhaps the s390 one was intentionally different?
>
> > --- a/net/packet/Makefile
> > +++ b/net/packet/Makefile
> > @@ -2,6 +2,7 @@
> > ?# Makefile for the packet AF.
> > ?#
> > ?
> > +ccflags-y := -D__CHECK_ENDIAN__
>
> Technically this is slightly more than advertised, but I guess that
> still makes sense if it's clean now.
>
> johannes

2016-12-07 14:09:54

by Stefan Hajnoczi

Subject: Re: [PATCH 09/10] vsock/virtio: fix src/dst cid format

On Tue, Dec 06, 2016 at 05:41:02PM +0200, Michael S. Tsirkin wrote:
> These fields are 64 bit, using le32_to_cpu and friends
> on these will not do the right thing.
> Fix this up.
>
> Cc: [email protected]
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport_common.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <[email protected]>

Attachments:

2016-12-07 14:09:52

by Stefan Hajnoczi

Subject: Re: [PATCH 10/10] virtio: enable endian checks for sparse builds

On Tue, Dec 06, 2016 at 05:41:05PM +0200, Michael S. Tsirkin wrote:
> __CHECK_ENDIAN__ isn't on by default presumably because
> it triggers too many sparse warnings for correct code.
> But virtio is now clean of these warnings, and
> we want to keep it this way - enable this for
> sparse builds.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
>
> It seems that there should be a better way to do it,
> but this works too.
>
> drivers/block/Makefile | 1 +
> drivers/char/Makefile | 1 +
> drivers/char/hw_random/Makefile | 2 ++
> drivers/gpu/drm/virtio/Makefile | 1 +
> drivers/net/Makefile | 3 +++
> drivers/net/caif/Makefile | 1 +
> drivers/rpmsg/Makefile | 1 +
> drivers/s390/virtio/Makefile | 2 ++
> drivers/scsi/Makefile | 1 +
> drivers/vhost/Makefile | 1 +
> drivers/virtio/Makefile | 3 +++
> net/9p/Makefile | 1 +
> net/packet/Makefile | 1 +
> net/vmw_vsock/Makefile | 2 ++
> 14 files changed, 21 insertions(+)

Reviewed-by: Stefan Hajnoczi <[email protected]>

Attachments:

2016-12-07 14:09:51

by Stefan Hajnoczi

Subject: Re: [PATCH 08/10] vsock/virtio: mark an internal function static

On Tue, Dec 06, 2016 at 05:41:00PM +0200, Michael S. Tsirkin wrote:
> virtio_transport_alloc_pkt is only used locally, make it static.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <[email protected]>

Attachments:

2016-12-07 14:09:49

by Stefan Hajnoczi

Subject: Re: [PATCH 07/10] vsock/virtio: add a missing __le annotation

On Tue, Dec 06, 2016 at 05:40:57PM +0200, Michael S. Tsirkin wrote:
> guest cid is read from config space, therefore it's in little endian
> format and is treated as such, annotate it accordingly.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> net/vmw_vsock/virtio_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <[email protected]>

Attachments:

2016-12-08 14:26:32

by Michael S. Tsirkin

Subject: Re: [PATCH 08/10] vsock/virtio: mark an internal function static

On Wed, Dec 07, 2016 at 12:21:22PM +0800, Jason Wang wrote:
>
>
> On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
> > virtio_transport_alloc_pkt is only used locally, make it static.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > net/vmw_vsock/virtio_transport_common.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index a53b3a1..6120384 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -32,7 +32,7 @@ static const struct virtio_transport *virtio_transport_get_ops(void)
> > return container_of(t, struct virtio_transport, transport);
> > }
> > -struct virtio_vsock_pkt *
> > +static struct virtio_vsock_pkt *
> > virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
> > size_t len,
> > u32 src_cid,
>
> Git grep shows it was used by tracing.

True but trace_virtio_transport_alloc_pkt is also local to
virtio_transport_common.c

--
MST

2016-12-11 02:54:53

by Michael S. Tsirkin

Subject: Re: [PATCH 09/10] vsock/virtio: fix src/dst cid format

On Wed, Dec 07, 2016 at 12:31:56PM +0800, Jason Wang wrote:
>
>
> On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
> > These fields are 64 bit, using le32_to_cpu and friends
> > on these will not do the right thing.
> > Fix this up.
> >
> > Cc: [email protected]
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > net/vmw_vsock/virtio_transport_common.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index 6120384..22e99c4 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -606,9 +606,9 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> > return 0;
> > pkt = virtio_transport_alloc_pkt(&info, 0,
> > - le32_to_cpu(pkt->hdr.dst_cid),
> > + le64_to_cpu(pkt->hdr.dst_cid),
> > le32_to_cpu(pkt->hdr.dst_port),
> > - le32_to_cpu(pkt->hdr.src_cid),
> > + le64_to_cpu(pkt->hdr.src_cid),
> > le32_to_cpu(pkt->hdr.src_port));
>
> Looking at sockaddr_vm, svm_cid is "unsigned int", do we really want 64 bit
> here?

Can't change the protocol at this point.


> > if (!pkt)
> > return -ENOMEM;
> > @@ -823,7 +823,7 @@ virtio_transport_send_response(struct vsock_sock *vsk,
> > struct virtio_vsock_pkt_info info = {
> > .op = VIRTIO_VSOCK_OP_RESPONSE,
> > .type = VIRTIO_VSOCK_TYPE_STREAM,
> > - .remote_cid = le32_to_cpu(pkt->hdr.src_cid),
> > + .remote_cid = le64_to_cpu(pkt->hdr.src_cid),
> > .remote_port = le32_to_cpu(pkt->hdr.src_port),
> > .reply = true,
> > };
> > @@ -863,9 +863,9 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt)
> > child->sk_state = SS_CONNECTED;
> > vchild = vsock_sk(child);
> > - vsock_addr_init(&vchild->local_addr, le32_to_cpu(pkt->hdr.dst_cid),
> > + vsock_addr_init(&vchild->local_addr, le64_to_cpu(pkt->hdr.dst_cid),
> > le32_to_cpu(pkt->hdr.dst_port));
> > - vsock_addr_init(&vchild->remote_addr, le32_to_cpu(pkt->hdr.src_cid),
> > + vsock_addr_init(&vchild->remote_addr, le64_to_cpu(pkt->hdr.src_cid),
> > le32_to_cpu(pkt->hdr.src_port));
> > vsock_insert_connected(vchild);
> > @@ -904,9 +904,9 @@ void virtio_transport_recv_pkt(struct virtio_vsock_pkt *pkt)
> > struct sock *sk;
> > bool space_available;
> > - vsock_addr_init(&src, le32_to_cpu(pkt->hdr.src_cid),
> > + vsock_addr_init(&src, le64_to_cpu(pkt->hdr.src_cid),
> > le32_to_cpu(pkt->hdr.src_port));
> > - vsock_addr_init(&dst, le32_to_cpu(pkt->hdr.dst_cid),
> > + vsock_addr_init(&dst, le64_to_cpu(pkt->hdr.dst_cid),
> > le32_to_cpu(pkt->hdr.dst_port));
> > trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,

2016-12-12 01:56:51

by Jason Wang

Subject: Re: [PATCH 08/10] vsock/virtio: mark an internal function static

On 2016年12月08日 22:25, Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2016 at 12:21:22PM +0800, Jason Wang wrote:
>>
>> On 2016年12月06日 23:41, Michael S. Tsirkin wrote:
>>> virtio_transport_alloc_pkt is only used locally, make it static.
>>>
>>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index a53b3a1..6120384 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -32,7 +32,7 @@ static const struct virtio_transport *virtio_transport_get_ops(void)
>>> return container_of(t, struct virtio_transport, transport);
>>> }
>>> -struct virtio_vsock_pkt *
>>> +static struct virtio_vsock_pkt *
>>> virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
>>> size_t len,
>>> u32 src_cid,
>> Git grep shows it was used by tracing.
> True but trace_virtio_transport_alloc_pkt is also local to
> virtio_transport_common.c
>

I see, so let's remove the EXPORT_SYMBOL_GPL() too?