Linux Virtualization - Aug 2020 - [PATCH v2 22/24] vdpa_sim: fix endian-ness of config space

Config space endian-ness is currently a mess: fields are
not tagged with the correct endian-ness so it's easy
to make mistakes like instanciating config space in
native endian-ness.

The following patches adding sparse tagging are currently in my tree.
Lightly tested.

As a follow-up, I plan to add new APIs that handle modern config space
in a more efficient way (bypassing the version check).

That is still TBD.

I also start with a version using gcc extensions, then switch
to _Generic. This is helpful for backports to older kernels/older
distros: _Generic patch can just be skipped.

Michael S. Tsirkin (24):
  virtio_balloon: fix sparse warning
  virtio_ring: sparse warning fixup
  virtio: allow __virtioXX, __leXX in config space
  virtio_9p: correct tags for config space fields
  virtio_balloon: correct tags for config space fields
  virtio_blk: correct tags for config space fields
  virtio_console: correct tags for config space fields
  virtio_crypto: correct tags for config space fields
  virtio_fs: correct tags for config space fields
  virtio_gpu: correct tags for config space fields
  virtio_input: correct tags for config space fields
  virtio_iommu: correct tags for config space fields
  virtio_mem: correct tags for config space fields
  virtio_net: correct tags for config space fields
  virtio_pmem: correct tags for config space fields
  virtio_scsi: correct tags for config space fields
  virtio_config: disallow native type fields
  mlxbf-tmfifo: sparse tags for config access
  vdpa: make sure set_features in invoked for legacy
  vhost/vdpa: switch to new helpers
  virtio_vdpa: legacy features handling
  vdpa_sim: fix endian-ness of config space
  virtio_config: cread/write cleanup
  virtio_config: rewrite using _Generic

 drivers/platform/mellanox/mlxbf-tmfifo.c |  13 +-
 drivers/scsi/virtio_scsi.c               |   4 +-
 drivers/vdpa/vdpa.c                      |   1 +
 drivers/vdpa/vdpa_sim/vdpa_sim.c         |  31 ++++-
 drivers/vhost/vdpa.c                     |   8 +-
 drivers/virtio/virtio_balloon.c          |   2 +-
 drivers/virtio/virtio_vdpa.c             |   9 +-
 include/linux/vdpa.h                     |  34 +++++
 include/linux/virtio_config.h            | 159 ++++++++++++++---------
 include/linux/virtio_ring.h              |  19 ++-
 include/uapi/linux/virtio_9p.h           |   4 +-
 include/uapi/linux/virtio_balloon.h      |  10 +-
 include/uapi/linux/virtio_blk.h          |  26 ++--
 include/uapi/linux/virtio_console.h      |   8 +-
 include/uapi/linux/virtio_crypto.h       |  26 ++--
 include/uapi/linux/virtio_fs.h           |   2 +-
 include/uapi/linux/virtio_gpu.h          |   8 +-
 include/uapi/linux/virtio_input.h        |  18 +--
 include/uapi/linux/virtio_iommu.h        |  12 +-
 include/uapi/linux/virtio_mem.h          |  14 +-
 include/uapi/linux/virtio_net.h          |   8 +-
 include/uapi/linux/virtio_pmem.h         |   4 +-
 include/uapi/linux/virtio_scsi.h         |  20 +--
 23 files changed, 270 insertions(+), 170 deletions(-)

-- 
MST

balloon uses virtio32_to_cpu instead of cpu_to_virtio32
to convert a native endian number to virtio.
No practical difference but makes sparse warn.
Fix it up.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/virtio/virtio_balloon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 54fd989f9353..8bc1704ffdf3 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -600,7 +600,7 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
 	while (virtqueue_get_buf(vq, &unused))
 		;
 
-	vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
+	vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
 					virtio_balloon_cmd_id_received(vb));
 	sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
 	err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active,
GFP_KERNEL);
-- 
MST

virtio_store_mb was built with split ring in mind so it accepts
__virtio16 arguments. Packed ring uses __le16 values, so sparse
complains.  It's just a store with some barriers so let's convert it to
a macro, we don't loose too much type safety by doing that.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio_ring.h | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 3dc70adfe5f5..b485b13fa50b 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -46,16 +46,15 @@ static inline void virtio_wmb(bool weak_barriers)
 		dma_wmb();
 }
 
-static inline void virtio_store_mb(bool weak_barriers,
-				   __virtio16 *p, __virtio16 v)
-{
-	if (weak_barriers) {
-		virt_store_mb(*p, v);
-	} else {
-		WRITE_ONCE(*p, v);
-		mb();
-	}
-}
+#define virtio_store_mb(weak_barriers, p, v) \
+do { \
+	if (weak_barriers) { \
+		virt_store_mb(*p, v); \
+	} else { \
+		WRITE_ONCE(*p, v); \
+		mb(); \
+	} \
+} while (0) \
 
 struct virtio_device;
 struct virtqueue;
-- 
MST

Currently all config space fields are of the type __uXX.
This confuses people and some drivers (notably vdpa)
access them using CPU endian-ness - which only
works well for legacy or LE platforms.

Update virtio_cread/virtio_cwrite macros to allow __virtioXX
and __leXX field types. Follow-up patches will convert
config space to use these types.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio_config.h | 50 +++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 3b4eae5ac5e3..64da491936f7 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -6,6 +6,7 @@
 #include <linux/bug.h>
 #include <linux/virtio.h>
 #include <linux/virtio_byteorder.h>
+#include <linux/compiler_types.h>
 #include <uapi/linux/virtio_config.h>
 
 struct irq_affinity;
@@ -287,12 +288,57 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
 	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
 }
 
+/*
+ * Only the checker differentiates between __virtioXX and __uXX types. But we
+ * try to share as much code as we can with the regular GCC build.
+ */
+#if !defined(CONFIG_CC_IS_GCC) && !defined(__CHECKER__)
+
+/* Not a checker - we can keep things simple */
+#define __virtio_native_typeof(x) typeof(x)
+
+#else
+
+/*
+ * We build this out of a couple of helper macros in a vain attempt to
+ * help you keep your lunch down while reading it.
+ */
+#define __virtio_pick_value(x, type, then, otherwise)			\
+	__builtin_choose_expr(__same_type(x, type), then, otherwise)
+
+#define __virtio_pick_type(x, type, then, otherwise)			\
+	__virtio_pick_value(x, type, (then)0, otherwise)
+
+#define __virtio_pick_endian(x, x16, x32, x64, otherwise)			\
+	__virtio_pick_type(x, x16, __u16,					\
+		__virtio_pick_type(x, x32, __u32,				\
+			__virtio_pick_type(x, x64, __u64,			\
+				otherwise)))
+
+#define __virtio_native_typeof(x) typeof(					\
+	__virtio_pick_type(x, __u8, __u8,					\
+		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
+			__virtio_pick_endian(x, __le16, __le32, __le64,		\
+				__virtio_pick_endian(x, __u16, __u32, __u64,	\
+					/* No other type allowed */		\
+					(void)0)))))
+
+#endif
+
+#define __virtio_native_type(structname, member) \
+	__virtio_native_typeof(((structname*)0)->member)
+
+#define __virtio_typecheck(structname, member, val) \
+		/* Must match the member's type, and be integer */ \
+		typecheck(__virtio_native_type(structname, member), (val))
+
+
 /* Config space accessors. */
 #define virtio_cread(vdev, structname, member, ptr)			\
 	do {								\
 		might_sleep();						\
 		/* Must match the member's type, and be integer */	\
-		if (!typecheck(typeof((((structname*)0)->member)), *(ptr))) \
+		if (!__virtio_typecheck(structname, member, *(ptr)))	\
 			(*ptr) = 1;					\
 									\
 		switch (sizeof(*ptr)) {					\
@@ -322,7 +368,7 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
 	do {								\
 		might_sleep();						\
 		/* Must match the member's type, and be integer */	\
-		if (!typecheck(typeof((((structname*)0)->member)), *(ptr))) \
+		if (!__virtio_typecheck(structname, member, *(ptr)))	\
 			BUG_ON((*ptr) == 1);				\
 									\
 		switch (sizeof(*ptr)) {					\
-- 
MST

Tag config space fields as having virtio endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_9p.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/virtio_9p.h b/include/uapi/linux/virtio_9p.h
index 277c4ad44e84..441047432258 100644
--- a/include/uapi/linux/virtio_9p.h
+++ b/include/uapi/linux/virtio_9p.h
@@ -25,7 +25,7 @@
  * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE. */
-#include <linux/types.h>
+#include <linux/virtio_types.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 
@@ -36,7 +36,7 @@
 
 struct virtio_9p_config {
 	/* length of the tag name */
-	__u16 tag_len;
+	__virtio16 tag_len;
 	/* non-NULL terminated tag name */
 	__u8 tag[0];
 } __attribute__((packed));
-- 
MST

Tag config space fields as having little endian-ness.
Note that balloon is special: LE even when using
the legacy interface.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_balloon.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index dc3e656470dd..ddaa45e723c4 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -45,20 +45,20 @@
 #define VIRTIO_BALLOON_CMD_ID_DONE	1
 struct virtio_balloon_config {
 	/* Number of pages host wants Guest to give up. */
-	__u32 num_pages;
+	__le32 num_pages;
 	/* Number of pages we've actually got in balloon. */
-	__u32 actual;
+	__le32 actual;
 	/*
 	 * Free page hint command id, readonly by guest.
 	 * Was previously named free_page_report_cmd_id so we
 	 * need to carry that name for legacy support.
 	 */
 	union {
-		__u32 free_page_hint_cmd_id;
-		__u32 free_page_report_cmd_id;	/* deprecated */
+		__le32 free_page_hint_cmd_id;
+		__le32 free_page_report_cmd_id;	/* deprecated */
 	};
 	/* Stores PAGE_POISON if page poisoning is in use */
-	__u32 poison_val;
+	__le32 poison_val;
 };
 
 #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
-- 
MST

Tag config space fields as having virtio endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_blk.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 0f99d7b49ede..d888f013d9ff 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -57,20 +57,20 @@
 
 struct virtio_blk_config {
 	/* The capacity (in 512-byte sectors). */
-	__u64 capacity;
+	__virtio64 capacity;
 	/* The maximum segment size (if VIRTIO_BLK_F_SIZE_MAX) */
-	__u32 size_max;
+	__virtio32 size_max;
 	/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
-	__u32 seg_max;
+	__virtio32 seg_max;
 	/* geometry of the device (if VIRTIO_BLK_F_GEOMETRY) */
 	struct virtio_blk_geometry {
-		__u16 cylinders;
+		__virtio16 cylinders;
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
 
 	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
-	__u32 blk_size;
+	__virtio32 blk_size;
 
 	/* the next 4 entries are guarded by VIRTIO_BLK_F_TOPOLOGY  */
 	/* exponent for physical block per logical block. */
@@ -78,42 +78,42 @@ struct virtio_blk_config {
 	/* alignment offset in logical blocks. */
 	__u8 alignment_offset;
 	/* minimum I/O size without performance penalty in logical blocks. */
-	__u16 min_io_size;
+	__virtio16 min_io_size;
 	/* optimal sustained I/O size in logical blocks. */
-	__u32 opt_io_size;
+	__virtio32 opt_io_size;
 
 	/* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
 	__u8 wce;
 	__u8 unused;
 
 	/* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
-	__u16 num_queues;
+	__virtio16 num_queues;
 
 	/* the next 3 entries are guarded by VIRTIO_BLK_F_DISCARD */
 	/*
 	 * The maximum discard sectors (in 512-byte sectors) for
 	 * one segment.
 	 */
-	__u32 max_discard_sectors;
+	__virtio32 max_discard_sectors;
 	/*
 	 * The maximum number of discard segments in a
 	 * discard command.
 	 */
-	__u32 max_discard_seg;
+	__virtio32 max_discard_seg;
 	/* Discard commands must be aligned to this number of sectors. */
-	__u32 discard_sector_alignment;
+	__virtio32 discard_sector_alignment;
 
 	/* the next 3 entries are guarded by VIRTIO_BLK_F_WRITE_ZEROES */
 	/*
 	 * The maximum number of write zeroes sectors (in 512-byte sectors) in
 	 * one segment.
 	 */
-	__u32 max_write_zeroes_sectors;
+	__virtio32 max_write_zeroes_sectors;
 	/*
 	 * The maximum number of segments in a write zeroes
 	 * command.
 	 */
-	__u32 max_write_zeroes_seg;
+	__virtio32 max_write_zeroes_seg;
 	/*
 	 * Set if a VIRTIO_BLK_T_WRITE_ZEROES request may result in the
 	 * deallocation of one or more of the sectors.
-- 
MST

Tag config space fields as having virtio endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_console.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/virtio_console.h
b/include/uapi/linux/virtio_console.h
index b7fb108c9310..7e6ec2ff0560 100644
--- a/include/uapi/linux/virtio_console.h
+++ b/include/uapi/linux/virtio_console.h
@@ -45,13 +45,13 @@
 
 struct virtio_console_config {
 	/* colums of the screens */
-	__u16 cols;
+	__virtio16 cols;
 	/* rows of the screens */
-	__u16 rows;
+	__virtio16 rows;
 	/* max. number of ports this device can hold */
-	__u32 max_nr_ports;
+	__virtio32 max_nr_ports;
 	/* emergency write register */
-	__u32 emerg_wr;
+	__virtio32 emerg_wr;
 } __attribute__((packed));
 
 /*
-- 
MST

Since crypto is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_crypto.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/virtio_crypto.h
b/include/uapi/linux/virtio_crypto.h
index 50cdc8aebfcf..a03932f10565 100644
--- a/include/uapi/linux/virtio_crypto.h
+++ b/include/uapi/linux/virtio_crypto.h
@@ -414,33 +414,33 @@ struct virtio_crypto_op_data_req {
 
 struct virtio_crypto_config {
 	/* See VIRTIO_CRYPTO_OP_* above */
-	__u32  status;
+	__le32  status;
 
 	/*
 	 * Maximum number of data queue
 	 */
-	__u32  max_dataqueues;
+	__le32  max_dataqueues;
 
 	/*
 	 * Specifies the services mask which the device support,
 	 * see VIRTIO_CRYPTO_SERVICE_* above
 	 */
-	__u32 crypto_services;
+	__le32 crypto_services;
 
 	/* Detailed algorithms mask */
-	__u32 cipher_algo_l;
-	__u32 cipher_algo_h;
-	__u32 hash_algo;
-	__u32 mac_algo_l;
-	__u32 mac_algo_h;
-	__u32 aead_algo;
+	__le32 cipher_algo_l;
+	__le32 cipher_algo_h;
+	__le32 hash_algo;
+	__le32 mac_algo_l;
+	__le32 mac_algo_h;
+	__le32 aead_algo;
 	/* Maximum length of cipher key */
-	__u32 max_cipher_key_len;
+	__le32 max_cipher_key_len;
 	/* Maximum length of authenticated key */
-	__u32 max_auth_key_len;
-	__u32 reserve;
+	__le32 max_auth_key_len;
+	__le32 reserve;
 	/* Maximum size of each crypto request's content */
-	__u64 max_size;
+	__le64 max_size;
 };
 
 struct virtio_crypto_inhdr {
-- 
MST

Since fs is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_fs.h b/include/uapi/linux/virtio_fs.h
index b02eb2ac3d99..3056b6e9f8ce 100644
--- a/include/uapi/linux/virtio_fs.h
+++ b/include/uapi/linux/virtio_fs.h
@@ -13,7 +13,7 @@ struct virtio_fs_config {
 	__u8 tag[36];
 
 	/* Number of request queues */
-	__u32 num_request_queues;
+	__le32 num_request_queues;
 } __attribute__((packed));
 
 #endif /* _UAPI_LINUX_VIRTIO_FS_H */
-- 
MST

Since gpu is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_gpu.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 0c85914d9369..ccbd174ef321 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -320,10 +320,10 @@ struct virtio_gpu_resp_edid {
 #define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
 
 struct virtio_gpu_config {
-	__u32 events_read;
-	__u32 events_clear;
-	__u32 num_scanouts;
-	__u32 num_capsets;
+	__le32 events_read;
+	__le32 events_clear;
+	__le32 num_scanouts;
+	__le32 num_capsets;
 };
 
 /* simple formats for fbcon/X use */
-- 
MST

Since this is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_input.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/virtio_input.h
b/include/uapi/linux/virtio_input.h
index a7fe5c8fb135..52084b1fb965 100644
--- a/include/uapi/linux/virtio_input.h
+++ b/include/uapi/linux/virtio_input.h
@@ -40,18 +40,18 @@ enum virtio_input_config_select {
 };
 
 struct virtio_input_absinfo {
-	__u32 min;
-	__u32 max;
-	__u32 fuzz;
-	__u32 flat;
-	__u32 res;
+	__le32 min;
+	__le32 max;
+	__le32 fuzz;
+	__le32 flat;
+	__le32 res;
 };
 
 struct virtio_input_devids {
-	__u16 bustype;
-	__u16 vendor;
-	__u16 product;
-	__u16 version;
+	__le16 bustype;
+	__le16 vendor;
+	__le16 product;
+	__le16 version;
 };
 
 struct virtio_input_config {
-- 
MST

Since this is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_iommu.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/virtio_iommu.h
b/include/uapi/linux/virtio_iommu.h
index 48e3c29223b5..237e36a280cb 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -18,24 +18,24 @@
 #define VIRTIO_IOMMU_F_MMIO			5
 
 struct virtio_iommu_range_64 {
-	__u64					start;
-	__u64					end;
+	__le64					start;
+	__le64					end;
 };
 
 struct virtio_iommu_range_32 {
-	__u32					start;
-	__u32					end;
+	__le32					start;
+	__le32					end;
 };
 
 struct virtio_iommu_config {
 	/* Supported page sizes */
-	__u64					page_size_mask;
+	__le64					page_size_mask;
 	/* Supported IOVA range */
 	struct virtio_iommu_range_64		input_range;
 	/* Max domain ID size */
 	struct virtio_iommu_range_32		domain_range;
 	/* Probe buffer size */
-	__u32					probe_size;
+	__le32					probe_size;
 };
 
 /* Request types */
-- 
MST

Since this is a modern-only device,
tag config space fields as having little endian-ness.

TODO: check other uses of __virtioXX types in this header,
should probably be __leXX.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_mem.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
index a9ffe041843c..70e01c687d5e 100644
--- a/include/uapi/linux/virtio_mem.h
+++ b/include/uapi/linux/virtio_mem.h
@@ -185,27 +185,27 @@ struct virtio_mem_resp {
 
 struct virtio_mem_config {
 	/* Block size and alignment. Cannot change. */
-	__u64 block_size;
+	__le64 block_size;
 	/* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
-	__u16 node_id;
+	__le16 node_id;
 	__u8 padding[6];
 	/* Start address of the memory region. Cannot change. */
-	__u64 addr;
+	__le64 addr;
 	/* Region size (maximum). Cannot change. */
-	__u64 region_size;
+	__le64 region_size;
 	/*
 	 * Currently usable region size. Can grow up to region_size. Can
 	 * shrink due to VIRTIO_MEM_REQ_UNPLUG_ALL (in which case no config
 	 * update will be sent).
 	 */
-	__u64 usable_region_size;
+	__le64 usable_region_size;
 	/*
 	 * Currently used size. Changes due to plug/unplug requests, but no
 	 * config updates will be sent.
 	 */
-	__u64 plugged_size;
+	__le64 plugged_size;
 	/* Requested size. New plug requests cannot exceed it. Can change. */
-	__u64 requested_size;
+	__le64 requested_size;
 };
 
 #endif /* _LINUX_VIRTIO_MEM_H */
-- 
MST

Tag config space fields as having virtio endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_net.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 19d23e5baa4e..27d996f29dd1 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -87,19 +87,19 @@ struct virtio_net_config {
 	/* The config defining mac address (if VIRTIO_NET_F_MAC) */
 	__u8 mac[ETH_ALEN];
 	/* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
-	__u16 status;
+	__virtio16 status;
 	/* Maximum number of each of transmit and receive queues;
 	 * see VIRTIO_NET_F_MQ and VIRTIO_NET_CTRL_MQ.
 	 * Legal values are between 1 and 0x8000
 	 */
-	__u16 max_virtqueue_pairs;
+	__virtio16 max_virtqueue_pairs;
 	/* Default maximum transmit unit advice */
-	__u16 mtu;
+	__virtio16 mtu;
 	/*
 	 * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
 	 * Any other value stands for unknown.
 	 */
-	__u32 speed;
+	__virtio32 speed;
 	/*
 	 * 0x00 - half duplex
 	 * 0x01 - full duplex
-- 
MST

Since this is a modern-only device,
tag config space fields as having little endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/uapi/linux/virtio_pmem.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/virtio_pmem.h b/include/uapi/linux/virtio_pmem.h
index b022787ffb94..d676b3620383 100644
--- a/include/uapi/linux/virtio_pmem.h
+++ b/include/uapi/linux/virtio_pmem.h
@@ -15,8 +15,8 @@
 #include <linux/virtio_config.h>
 
 struct virtio_pmem_config {
-	__u64 start;
-	__u64 size;
+	__le64 start;
+	__le64 size;
 };
 
 #define VIRTIO_PMEM_REQ_TYPE_FLUSH      0
-- 
MST

Tag config space fields as having virtio endian-ness.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/scsi/virtio_scsi.c       |  4 ++--
 include/uapi/linux/virtio_scsi.h | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0e0910c5b942..c36aeb9a1330 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -746,14 +746,14 @@ static struct scsi_host_template virtscsi_host_template =
{
 
 #define virtscsi_config_get(vdev, fld) \
 	({ \
-		typeof(((struct virtio_scsi_config *)0)->fld) __val; \
+		__virtio_native_type(struct virtio_scsi_config, fld) __val; \
 		virtio_cread(vdev, struct virtio_scsi_config, fld, &__val); \
 		__val; \
 	})
 
 #define virtscsi_config_set(vdev, fld, val) \
 	do { \
-		typeof(((struct virtio_scsi_config *)0)->fld) __val = (val); \
+		__virtio_native_type(struct virtio_scsi_config, fld) __val = (val); \
 		virtio_cwrite(vdev, struct virtio_scsi_config, fld, &__val); \
 	} while(0)
 
diff --git a/include/uapi/linux/virtio_scsi.h b/include/uapi/linux/virtio_scsi.h
index cc18ef8825c0..0abaae4027c0 100644
--- a/include/uapi/linux/virtio_scsi.h
+++ b/include/uapi/linux/virtio_scsi.h
@@ -103,16 +103,16 @@ struct virtio_scsi_event {
 } __attribute__((packed));
 
 struct virtio_scsi_config {
-	__u32 num_queues;
-	__u32 seg_max;
-	__u32 max_sectors;
-	__u32 cmd_per_lun;
-	__u32 event_info_size;
-	__u32 sense_size;
-	__u32 cdb_size;
-	__u16 max_channel;
-	__u16 max_target;
-	__u32 max_lun;
+	__virtio32 num_queues;
+	__virtio32 seg_max;
+	__virtio32 max_sectors;
+	__virtio32 cmd_per_lun;
+	__virtio32 event_info_size;
+	__virtio32 sense_size;
+	__virtio32 cdb_size;
+	__virtio16 max_channel;
+	__virtio16 max_target;
+	__virtio32 max_lun;
 } __attribute__((packed));
 
 /* Feature Bits */
-- 
MST

Transitional devices should all use __virtioXX types.
Modern ones should use __leXX.
_uXX type would be a bug.
Let's prevent that.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio_config.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 64da491936f7..c68f58f3bf34 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -319,9 +319,8 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
 	__virtio_pick_type(x, __u8, __u8,					\
 		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
 			__virtio_pick_endian(x, __le16, __le32, __le64,		\
-				__virtio_pick_endian(x, __u16, __u32, __u64,	\
-					/* No other type allowed */		\
-					(void)0)))))
+				/* No other type allowed */			\
+				(void)0))))
 
 #endif
 
-- 
MST

mlxbf-tmfifo accesses config space using native types -
which works for it since the legacy virtio native types.

This will break if it ever needs to support modern virtio,
so with new tags previously introduced for virtio net config,
sparse now warns for this in drivers.

Since this is a legacy only device, fix it up using
virtio_legacy_is_little_endian for now.

No functional changes.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
b/drivers/platform/mellanox/mlxbf-tmfifo.c
index 5739a9669b29..bbc4e71a16ff 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -625,7 +625,10 @@ static void mlxbf_tmfifo_rxtx_header(struct
mlxbf_tmfifo_vring *vring,
 			vdev_id = VIRTIO_ID_NET;
 			hdr_len = sizeof(struct virtio_net_hdr);
 			config = &fifo->vdev[vdev_id]->config.net;
-			if (ntohs(hdr.len) > config->mtu +
+			/* A legacy-only interface for now. */
+			if (ntohs(hdr.len) >
+			    __virtio16_to_cpu(virtio_legacy_is_little_endian(),
+					      config->mtu) +
 			    MLXBF_TMFIFO_NET_L2_OVERHEAD)
 				return;
 		} else {
@@ -1231,8 +1234,12 @@ static int mlxbf_tmfifo_probe(struct platform_device
*pdev)
 
 	/* Create the network vdev. */
 	memset(&net_config, 0, sizeof(net_config));
-	net_config.mtu = ETH_DATA_LEN;
-	net_config.status = VIRTIO_NET_S_LINK_UP;
+
+	/* A legacy-only interface for now. */
+	net_config.mtu = __cpu_to_virtio16(virtio_legacy_is_little_endian(),
+					   ETH_DATA_LEN);
+	net_config.status = __cpu_to_virtio16(virtio_legacy_is_little_endian(),
+					      VIRTIO_NET_S_LINK_UP);
 	mlxbf_tmfifo_get_cfg_mac(net_config.mac);
 	rc = mlxbf_tmfifo_create_vdev(dev, fifo, VIRTIO_ID_NET,
 				      MLXBF_TMFIFO_NET_FEATURES, &net_config,
-- 
MST

Some legacy guests just assume features are 0 after reset.
We detect that config space is accessed before features are
set and set features to 0 automatically.
Note: some legacy guests might not even access config space, if this is
reported in the field we might need to catch a kick to handle these.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/vdpa/vdpa.c  |  1 +
 include/linux/vdpa.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index de211ef3738c..7105265e4793 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -96,6 +96,7 @@ struct vdpa_device *__vdpa_alloc_device(struct device *parent,
 	vdev->dev.release = vdpa_release_dev;
 	vdev->index = err;
 	vdev->config = config;
+	vdev->features_valid = false;
 
 	err = dev_set_name(&vdev->dev, "vdpa%u", vdev->index);
 	if (err)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 239db794357c..29b8296f1414 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -33,12 +33,14 @@ struct vdpa_notification_area {
  * @dma_dev: the actual device that is performing DMA
  * @config: the configuration ops for this device.
  * @index: device index
+ * @features_valid: were features initialized? for legacy guests
  */
 struct vdpa_device {
 	struct device dev;
 	struct device *dma_dev;
 	const struct vdpa_config_ops *config;
 	unsigned int index;
+	bool features_valid;
 };
 
 /**
@@ -266,4 +268,36 @@ static inline struct device *vdpa_get_dma_dev(struct
vdpa_device *vdev)
 {
 	return vdev->dma_dev;
 }
+
+static inline void vdpa_reset(struct vdpa_device *vdev)
+{
+        const struct vdpa_config_ops *ops = vdev->config;
+
+	vdev->features_valid = false;
+        ops->set_status(vdev, 0);
+}
+
+static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+{
+        const struct vdpa_config_ops *ops = vdev->config;
+
+	vdev->features_valid = true;
+        return ops->set_features(vdev, features);
+}
+
+
+static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
+				   void *buf, unsigned int len)
+{
+        const struct vdpa_config_ops *ops = vdev->config;
+
+	/*
+	 * Config accesses aren't supposed to trigger before features are set.
+	 * If it does happen we assume a legacy guest.
+	 */
+	if (!vdev->features_valid)
+		vdpa_set_features(vdev, 0);
+	ops->get_config(vdev, offset, buf, len);
+}
+
 #endif /* _LINUX_VDPA_H */
-- 
MST

For new helpers handling legacy features to be effective,
vhost needs to invoke them. Tie them in.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/vhost/vdpa.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 18869a35d408..3674404688f5 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -118,9 +118,8 @@ static irqreturn_t vhost_vdpa_config_cb(void *private)
 static void vhost_vdpa_reset(struct vhost_vdpa *v)
 {
 	struct vdpa_device *vdpa = v->vdpa;
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	ops->set_status(vdpa, 0);
+	vdpa_reset(vdpa);
 }
 
 static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
@@ -196,7 +195,6 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 				  struct vhost_vdpa_config __user *c)
 {
 	struct vdpa_device *vdpa = v->vdpa;
-	const struct vdpa_config_ops *ops = vdpa->config;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	u8 *buf;
@@ -209,7 +207,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	if (!buf)
 		return -ENOMEM;
 
-	ops->get_config(vdpa, config.off, buf, config.len);
+	vdpa_get_config(vdpa, config.off, buf, config.len);
 
 	if (copy_to_user(c->buf, buf, config.len)) {
 		kvfree(buf);
@@ -282,7 +280,7 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v,
u64 __user *featurep)
 	if (features & ~vhost_vdpa_features[v->virtio_id])
 		return -EINVAL;
 
-	if (ops->set_features(vdpa, features))
+	if (vdpa_set_features(vdpa, features))
 		return -EINVAL;
 
 	return 0;
-- 
MST

We normally expect vdpa to use the modern interface.
However for consistency, let's use same APIs as vhost
for legacy guests.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/virtio/virtio_vdpa.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index c30eb55030be..4a9ddb44b2a7 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -57,9 +57,8 @@ static void virtio_vdpa_get(struct virtio_device *vdev,
unsigned offset,
 			    void *buf, unsigned len)
 {
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	ops->get_config(vdpa, offset, buf, len);
+	vdpa_get_config(vdpa, offset, buf, len);
 }
 
 static void virtio_vdpa_set(struct virtio_device *vdev, unsigned offset,
@@ -101,9 +100,8 @@ static void virtio_vdpa_set_status(struct virtio_device
*vdev, u8 status)
 static void virtio_vdpa_reset(struct virtio_device *vdev)
 {
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	return ops->set_status(vdpa, 0);
+	vdpa_reset(vdpa);
 }
 
 static bool virtio_vdpa_notify(struct virtqueue *vq)
@@ -294,12 +292,11 @@ static u64 virtio_vdpa_get_features(struct virtio_device
*vdev)
 static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
 {
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	return ops->set_features(vdpa, vdev->features);
+	return vdpa_set_features(vdpa, vdev->features);
 }
 
 static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)
-- 
MST

VDPA sim accesses config space as native endian - this is
wrong since it's a modern device and actually uses LE.

It only supports modern guests so we could punt and
just force LE, but let's use the full virtio APIs since people
tend to copy/paste code, and this is not data path anyway.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index a9bc5e0fb353..fa05e065ff69 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -24,6 +24,7 @@
 #include <linux/etherdevice.h>
 #include <linux/vringh.h>
 #include <linux/vdpa.h>
+#include <linux/virtio_byteorder.h>
 #include <linux/vhost_iotlb.h>
 #include <uapi/linux/virtio_config.h>
 #include <uapi/linux/virtio_net.h>
@@ -72,6 +73,23 @@ struct vdpasim {
 	u64 features;
 };
 
+/* TODO: cross-endian support */
+static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
+{
+	return virtio_legacy_is_little_endian() ||
+		(vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+}
+
+static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
+{
+	return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
+}
+
+static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16 val)
+{
+	return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
+}
+
 static struct vdpasim *vdpasim_dev;
 
 static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
@@ -306,7 +324,6 @@ static const struct vdpa_config_ops vdpasim_net_config_ops;
 
 static struct vdpasim *vdpasim_create(void)
 {
-	struct virtio_net_config *config;
 	struct vdpasim *vdpasim;
 	struct device *dev;
 	int ret = -ENOMEM;
@@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
 	if (!vdpasim->buffer)
 		goto err_iommu;
 
-	config = &vdpasim->config;
-	config->mtu = 1500;
-	config->status = VIRTIO_NET_S_LINK_UP;
-	eth_random_addr(config->mac);
+	eth_random_addr(vdpasim->config.mac);
 
 	vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
 	vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
@@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
 static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+	struct virtio_net_config *config = &vdpasim->config;
 
 	/* DMA mapping must be done by driver */
 	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device *vdpa,
u64 features)
 
 	vdpasim->features = features & vdpasim_features;
 
+	/* We only know whether guest is using the legacy interface here, so
+	 * that's the earliest we can set config fields.
+	 */
+
+	config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
+	config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
 	return 0;
 }
 
-- 
MST

Use vars of the correct type instead of casting.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio_config.h | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index c68f58f3bf34..5c3b02245ecd 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -444,53 +444,60 @@ static inline void virtio_cwrite8(struct virtio_device
*vdev,
 static inline u16 virtio_cread16(struct virtio_device *vdev,
 				 unsigned int offset)
 {
-	u16 ret;
+	__virtio16 ret;
 
 	might_sleep();
 	vdev->config->get(vdev, offset, &ret, sizeof(ret));
-	return virtio16_to_cpu(vdev, (__force __virtio16)ret);
+	return virtio16_to_cpu(vdev, ret);
 }
 
 static inline void virtio_cwrite16(struct virtio_device *vdev,
 				   unsigned int offset, u16 val)
 {
+	__virtio16 v;
+
 	might_sleep();
-	val = (__force u16)cpu_to_virtio16(vdev, val);
-	vdev->config->set(vdev, offset, &val, sizeof(val));
+	v = cpu_to_virtio16(vdev, val);
+	vdev->config->set(vdev, offset, &v, sizeof(v));
 }
 
 static inline u32 virtio_cread32(struct virtio_device *vdev,
 				 unsigned int offset)
 {
-	u32 ret;
+	__virtio32 ret;
 
 	might_sleep();
 	vdev->config->get(vdev, offset, &ret, sizeof(ret));
-	return virtio32_to_cpu(vdev, (__force __virtio32)ret);
+	return virtio32_to_cpu(vdev, ret);
 }
 
 static inline void virtio_cwrite32(struct virtio_device *vdev,
 				   unsigned int offset, u32 val)
 {
+	__virtio32 v;
+
 	might_sleep();
-	val = (__force u32)cpu_to_virtio32(vdev, val);
-	vdev->config->set(vdev, offset, &val, sizeof(val));
+	v = cpu_to_virtio32(vdev, val);
+	vdev->config->set(vdev, offset, &v, sizeof(v));
 }
 
 static inline u64 virtio_cread64(struct virtio_device *vdev,
 				 unsigned int offset)
 {
-	u64 ret;
+	__virtio64 ret;
+
 	__virtio_cread_many(vdev, offset, &ret, 1, sizeof(ret));
-	return virtio64_to_cpu(vdev, (__force __virtio64)ret);
+	return virtio64_to_cpu(vdev, ret);
 }
 
 static inline void virtio_cwrite64(struct virtio_device *vdev,
 				   unsigned int offset, u64 val)
 {
+	__virtio64 v;
+
 	might_sleep();
-	val = (__force u64)cpu_to_virtio64(vdev, val);
-	vdev->config->set(vdev, offset, &val, sizeof(val));
+	v = cpu_to_virtio64(vdev, val);
+	vdev->config->set(vdev, offset, &v, sizeof(v));
 }
 
 /* Conditional config space accessors. */
-- 
MST

Min compiler version has been raised, so that's ok now.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio_config.h | 163 ++++++++++++++++------------------
 1 file changed, 77 insertions(+), 86 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 5c3b02245ecd..21c7098595ad 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -288,112 +288,103 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
 	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
 }
 
-/*
- * Only the checker differentiates between __virtioXX and __uXX types. But we
- * try to share as much code as we can with the regular GCC build.
- */
-#if !defined(CONFIG_CC_IS_GCC) && !defined(__CHECKER__)
+#define virtio_to_cpu(vdev, x) \
+	_Generic((x), \
+		__u8: (x), \
+		__virtio16: virtio16_to_cpu((vdev), (x)), \
+		__virtio32: virtio32_to_cpu((vdev), (x)), \
+		__virtio64: virtio64_to_cpu((vdev), (x)), \
+		/*
+		 * Why define a default? checker can distinguish between
+		 * e.g. __u16, __le16 and __virtio16, but GCC can't so
+		 * attempts to define variants for both look like a duplicate
+		 * variant to it.
+		 */ \
+		default: _Generic((x), \
+				 __u8: (x), \
+				 __le16: virtio16_to_cpu((vdev), (__force __virtio16)(x)), \
+				 __le32: virtio32_to_cpu((vdev), (__force __virtio32)(x)), \
+				 __le64: virtio64_to_cpu((vdev), (__force __virtio64)(x)), \
+				 default: _Generic((x), \
+						  __u8: (x), \
+						  __u16: virtio16_to_cpu((vdev), (__force __virtio16)(x)), \
+						  __u32: virtio32_to_cpu((vdev), (__force __virtio32)(x)), \
+						  __u64: virtio64_to_cpu((vdev), (__force __virtio64)(x)) \
+						  ) \
+				 ) \
+		)
 
-/* Not a checker - we can keep things simple */
-#define __virtio_native_typeof(x) typeof(x)
-
-#else
-
-/*
- * We build this out of a couple of helper macros in a vain attempt to
- * help you keep your lunch down while reading it.
- */
-#define __virtio_pick_value(x, type, then, otherwise)			\
-	__builtin_choose_expr(__same_type(x, type), then, otherwise)
-
-#define __virtio_pick_type(x, type, then, otherwise)			\
-	__virtio_pick_value(x, type, (then)0, otherwise)
-
-#define __virtio_pick_endian(x, x16, x32, x64, otherwise)			\
-	__virtio_pick_type(x, x16, __u16,					\
-		__virtio_pick_type(x, x32, __u32,				\
-			__virtio_pick_type(x, x64, __u64,			\
-				otherwise)))
-
-#define __virtio_native_typeof(x) typeof(					\
-	__virtio_pick_type(x, __u8, __u8,					\
-		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
-			__virtio_pick_endian(x, __le16, __le32, __le64,		\
-				/* No other type allowed */			\
-				(void)0))))
-
-#endif
+#define cpu_to_virtio(vdev, x, m) \
+	_Generic((m), \
+		__u8: (x), \
+		__virtio16: cpu_to_virtio16((vdev), (x)), \
+		__virtio32: cpu_to_virtio32((vdev), (x)), \
+		__virtio64: cpu_to_virtio64((vdev), (x)), \
+		/*
+		 * Why define a default? checker can distinguish between
+		 * e.g. __u16, __le16 and __virtio16, but GCC can't so
+		 * attempts to define variants for both look like a duplicate
+		 * variant to it.
+		 */ \
+		default: _Generic((m), \
+				 __u8: (x), \
+				 __le16: (__force __le16)cpu_to_virtio16((vdev), (x)), \
+				 __le32: (__force __le32)cpu_to_virtio32((vdev), (x)), \
+				 __le64: (__force __le64)cpu_to_virtio64((vdev), (x)), \
+				 default: _Generic((m), \
+						  __u8: (x), \
+						  __u16: (__force __u16)cpu_to_virtio16((vdev), (x)), \
+						  __u32: (__force __u32)cpu_to_virtio32((vdev), (x)), \
+						  __u64: (__force __u64)cpu_to_virtio64((vdev), (x)) \
+						  ) \
+				 ) \
+		)
 
 #define __virtio_native_type(structname, member) \
-	__virtio_native_typeof(((structname*)0)->member)
-
-#define __virtio_typecheck(structname, member, val) \
-		/* Must match the member's type, and be integer */ \
-		typecheck(__virtio_native_type(structname, member), (val))
-
+	typeof(virtio_to_cpu(NULL, ((structname*)0)->member))
 
 /* Config space accessors. */
 #define virtio_cread(vdev, structname, member, ptr)			\
 	do {								\
-		might_sleep();						\
-		/* Must match the member's type, and be integer */	\
-		if (!__virtio_typecheck(structname, member, *(ptr)))	\
-			(*ptr) = 1;					\
+		typeof(((structname*)0)->member) virtio_cread_v;	\
 									\
-		switch (sizeof(*ptr)) {					\
+		might_sleep();						\
+		/* Sanity check: must match the member's type */	\
+		/*typecheck(typeof(virtio_to_cpu((vdev), virtio_cread_v)), *(ptr)); */\
+									\
+		switch (sizeof(virtio_cread_v)) {			\
 		case 1:							\
-			*(ptr) = virtio_cread8(vdev,			\
-					       offsetof(structname, member)); \
-			break;						\
 		case 2:							\
-			*(ptr) = virtio_cread16(vdev,			\
-						offsetof(structname, member)); \
-			break;						\
 		case 4:							\
-			*(ptr) = virtio_cread32(vdev,			\
-						offsetof(structname, member)); \
-			break;						\
-		case 8:							\
-			*(ptr) = virtio_cread64(vdev,			\
-						offsetof(structname, member)); \
+			vdev->config->get((vdev), 			\
+					  offsetof(structname, member), \
+					  &virtio_cread_v,		\
+					  sizeof(virtio_cread_v));	\
 			break;						\
 		default:						\
-			BUG();						\
+			__virtio_cread_many((vdev), 			\
+					  offsetof(structname, member), \
+					  &virtio_cread_v,		\
+					  1,				\
+					  sizeof(virtio_cread_v));	\
+			break;						\
 		}							\
+		*(ptr) = virtio_to_cpu(vdev, virtio_cread_v);		\
 	} while(0)
 
 /* Config space accessors. */
 #define virtio_cwrite(vdev, structname, member, ptr)			\
 	do {								\
-		might_sleep();						\
-		/* Must match the member's type, and be integer */	\
-		if (!__virtio_typecheck(structname, member, *(ptr)))	\
-			BUG_ON((*ptr) == 1);				\
+		typeof(((structname*)0)->member) virtio_cwrite_v =	\
+			cpu_to_virtio(vdev, *(ptr), ((structname*)0)->member); \
 									\
-		switch (sizeof(*ptr)) {					\
-		case 1:							\
-			virtio_cwrite8(vdev,				\
-				       offsetof(structname, member),	\
-				       *(ptr));				\
-			break;						\
-		case 2:							\
-			virtio_cwrite16(vdev,				\
-					offsetof(structname, member),	\
-					*(ptr));			\
-			break;						\
-		case 4:							\
-			virtio_cwrite32(vdev,				\
-					offsetof(structname, member),	\
-					*(ptr));			\
-			break;						\
-		case 8:							\
-			virtio_cwrite64(vdev,				\
-					offsetof(structname, member),	\
-					*(ptr));			\
-			break;						\
-		default:						\
-			BUG();						\
-		}							\
+		might_sleep();						\
+		/* Sanity check: must match the member's type */	\
+		typecheck(typeof(virtio_to_cpu((vdev), virtio_cwrite_v)), *(ptr)); \
+									\
+		vdev->config->set((vdev), offsetof(structname, member),	\
+				  &virtio_cwrite_v,			\
+				  sizeof(virtio_cwrite_v));		\
 	} while(0)
 
 /* Read @count fields, @bytes each. */
-- 
MST

> Am 03.08.2020 um 22:59 schrieb Michael S. Tsirkin <mst at
redhat.com>:
> 
> ?Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> TODO: check other uses of __virtioXX types in this header,
> should probably be __leXX.
Doesn?t matter in practice IIRC, like this change. But we could do it (the spec
documents everything as __le) in case it makes things clearer.

Acked-by: David Hildenbrand <david at redhat.com>
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
> include/uapi/linux/virtio_mem.h | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_mem.h
b/include/uapi/linux/virtio_mem.h
> index a9ffe041843c..70e01c687d5e 100644
> --- a/include/uapi/linux/virtio_mem.h
> +++ b/include/uapi/linux/virtio_mem.h
> @@ -185,27 +185,27 @@ struct virtio_mem_resp {
> 
> struct virtio_mem_config {
>    /* Block size and alignment. Cannot change. */
> -    __u64 block_size;
> +    __le64 block_size;
>    /* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
> -    __u16 node_id;
> +    __le16 node_id;
>    __u8 padding[6];
>    /* Start address of the memory region. Cannot change. */
> -    __u64 addr;
> +    __le64 addr;
>    /* Region size (maximum). Cannot change. */
> -    __u64 region_size;
> +    __le64 region_size;
>    /*
>     * Currently usable region size. Can grow up to region_size. Can
>     * shrink due to VIRTIO_MEM_REQ_UNPLUG_ALL (in which case no config
>     * update will be sent).
>     */
> -    __u64 usable_region_size;
> +    __le64 usable_region_size;
>    /*
>     * Currently used size. Changes due to plug/unplug requests, but no
>     * config updates will be sent.
>     */
> -    __u64 plugged_size;
> +    __le64 plugged_size;
>    /* Requested size. New plug requests cannot exceed it. Can change. */
> -    __u64 requested_size;
> +    __le64 requested_size;
> };
> 
> #endif /* _LINUX_VIRTIO_MEM_H */
> -- 
> MST
>

> Am 03.08.2020 um 22:58 schrieb Michael S. Tsirkin <mst at
redhat.com>:
> 
> ?balloon uses virtio32_to_cpu instead of cpu_to_virtio32
> to convert a native endian number to virtio.
> No practical difference but makes sparse warn.
> Fix it up.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
I think I acked this one already.

Acked-by: David Hildenbrand <david at redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 54fd989f9353..8bc1704ffdf3 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -600,7 +600,7 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
>    while (virtqueue_get_buf(vq, &unused))
>        ;
> 
> -    vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> +    vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
>                    virtio_balloon_cmd_id_received(vb));
>    sg_init_one(&sg, &vb->cmd_id_active,
sizeof(vb->cmd_id_active));
>    err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active,
GFP_KERNEL);
> -- 
> MST
>

> Am 03.08.2020 um 22:59 schrieb Michael S. Tsirkin <mst at
redhat.com>:
> 
> ?Tag config space fields as having little endian-ness.
> Note that balloon is special: LE even when using
> the legacy interface.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Acked-by: David Hildenbrand <david at redhat.com>
> ---
> include/uapi/linux/virtio_balloon.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
> index dc3e656470dd..ddaa45e723c4 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -45,20 +45,20 @@
> #define VIRTIO_BALLOON_CMD_ID_DONE    1
> struct virtio_balloon_config {
>    /* Number of pages host wants Guest to give up. */
> -    __u32 num_pages;
> +    __le32 num_pages;
>    /* Number of pages we've actually got in balloon. */
> -    __u32 actual;
> +    __le32 actual;
>    /*
>     * Free page hint command id, readonly by guest.
>     * Was previously named free_page_report_cmd_id so we
>     * need to carry that name for legacy support.
>     */
>    union {
> -        __u32 free_page_hint_cmd_id;
> -        __u32 free_page_report_cmd_id;    /* deprecated */
> +        __le32 free_page_hint_cmd_id;
> +        __le32 free_page_report_cmd_id;    /* deprecated */
>    };
>    /* Stores PAGE_POISON if page poisoning is in use */
> -    __u32 poison_val;
> +    __le32 poison_val;
> };
> 
> #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
> -- 
> MST
>

On Mon, Aug 03, 2020 at 04:59:23PM -0400, Michael S. Tsirkin
wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_input.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_input.h
b/include/uapi/linux/virtio_input.h
> index a7fe5c8fb135..52084b1fb965 100644
> --- a/include/uapi/linux/virtio_input.h
> +++ b/include/uapi/linux/virtio_input.h
> @@ -40,18 +40,18 @@ enum virtio_input_config_select {
>  };
>  
>  struct virtio_input_absinfo {
> -	__u32 min;
> -	__u32 max;
> -	__u32 fuzz;
> -	__u32 flat;
> -	__u32 res;
> +	__le32 min;
> +	__le32 max;
> +	__le32 fuzz;
> +	__le32 flat;
> +	__le32 res;
>  };
>  
>  struct virtio_input_devids {
> -	__u16 bustype;
> -	__u16 vendor;
> -	__u16 product;
> -	__u16 version;
> +	__le16 bustype;
> +	__le16 vendor;
> +	__le16 product;
> +	__le16 version;
>  };
>  
>  struct virtio_input_config {
> -- 
> MST
Reviewed-by: Gerd Hoffmann <kraxel at redhat.com>

On Mon, Aug 03, 2020 at 04:59:27PM -0400, Michael S. Tsirkin
wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe at linaro.org>

And tested with the latest sparse
> ---
>  include/uapi/linux/virtio_iommu.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_iommu.h
b/include/uapi/linux/virtio_iommu.h
> index 48e3c29223b5..237e36a280cb 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -18,24 +18,24 @@
>  #define VIRTIO_IOMMU_F_MMIO			5
>  
>  struct virtio_iommu_range_64 {
> -	__u64					start;
> -	__u64					end;
> +	__le64					start;
> +	__le64					end;
>  };
>  
>  struct virtio_iommu_range_32 {
> -	__u32					start;
> -	__u32					end;
> +	__le32					start;
> +	__le32					end;
>  };
>  
>  struct virtio_iommu_config {
>  	/* Supported page sizes */
> -	__u64					page_size_mask;
> +	__le64					page_size_mask;
>  	/* Supported IOVA range */
>  	struct virtio_iommu_range_64		input_range;
>  	/* Max domain ID size */
>  	struct virtio_iommu_range_32		domain_range;
>  	/* Probe buffer size */
> -	__u32					probe_size;
> +	__le32					probe_size;
>  };
>  
>  /* Request types */
> -- 
> MST
>

On Mon, Aug 03, 2020 at 04:59:13PM -0400, Michael S. Tsirkin
wrote:
> Since fs is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
virtio spec does list this field as "le32".

Acked-by: Vivek Goyal <vgoyal at redhat.com>

Vivek
> ---
>  include/uapi/linux/virtio_fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/virtio_fs.h
b/include/uapi/linux/virtio_fs.h
> index b02eb2ac3d99..3056b6e9f8ce 100644
> --- a/include/uapi/linux/virtio_fs.h
> +++ b/include/uapi/linux/virtio_fs.h
> @@ -13,7 +13,7 @@ struct virtio_fs_config {
>  	__u8 tag[36];
>  
>  	/* Number of request queues */
> -	__u32 num_request_queues;
> +	__le32 num_request_queues;
>  } __attribute__((packed));
>  
>  #endif /* _UAPI_LINUX_VIRTIO_FS_H */
> -- 
> MST
>

On Mon, 3 Aug 2020 16:58:38 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> balloon uses virtio32_to_cpu instead of cpu_to_virtio32
> to convert a native endian number to virtio.
> No practical difference but makes sparse warn.
> Fix it up.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:58:42 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> virtio_store_mb was built with split ring in mind so it accepts
> __virtio16 arguments. Packed ring uses __le16 values, so sparse
> complains.  It's just a store with some barriers so let's convert
it to
> a macro, we don't loose too much type safety by doing that.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/linux/virtio_ring.h | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
Acked-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:58:46 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Currently all config space fields are of the type __uXX.
> This confuses people and some drivers (notably vdpa)
> access them using CPU endian-ness - which only
> works well for legacy or LE platforms.
> 
> Update virtio_cread/virtio_cwrite macros to allow __virtioXX
> and __leXX field types. Follow-up patches will convert
> config space to use these types.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/linux/virtio_config.h | 50 +++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
(...)
> @@ -287,12 +288,57 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
>  	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
>  }
>  
> +/*
> + * Only the checker differentiates between __virtioXX and __uXX types. But
we
> + * try to share as much code as we can with the regular GCC build.
> + */
> +#if !defined(CONFIG_CC_IS_GCC) && !defined(__CHECKER__)
> +
> +/* Not a checker - we can keep things simple */
> +#define __virtio_native_typeof(x) typeof(x)
> +
> +#else
> +
> +/*
> + * We build this out of a couple of helper macros in a vain attempt to
> + * help you keep your lunch down while reading it.
> + */
It might help with the lunch, but it still gives a slight queasiness.
No ideas for a better version, though.
> +#define __virtio_pick_value(x, type, then, otherwise)			\
> +	__builtin_choose_expr(__same_type(x, type), then, otherwise)
> +
> +#define __virtio_pick_type(x, type, then, otherwise)			\
> +	__virtio_pick_value(x, type, (then)0, otherwise)
> +
> +#define __virtio_pick_endian(x, x16, x32, x64, otherwise)			\
> +	__virtio_pick_type(x, x16, __u16,					\
> +		__virtio_pick_type(x, x32, __u32,				\
> +			__virtio_pick_type(x, x64, __u64,			\
> +				otherwise)))
> +
> +#define __virtio_native_typeof(x) typeof(					\
> +	__virtio_pick_type(x, __u8, __u8,					\
> +		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
> +			__virtio_pick_endian(x, __le16, __le32, __le64,		\
> +				__virtio_pick_endian(x, __u16, __u32, __u64,	\
> +					/* No other type allowed */		\
> +					(void)0)))))
> +
> +#endif
> +
> +#define __virtio_native_type(structname, member) \
> +	__virtio_native_typeof(((structname*)0)->member)
> +
> +#define __virtio_typecheck(structname, member, val) \
> +		/* Must match the member's type, and be integer */ \
> +		typecheck(__virtio_native_type(structname, member), (val))
> +
> +
Acked-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:58:50 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having virtio endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_9p.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:58:55 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having little endian-ness.
> Note that balloon is special: LE even when using
> the legacy interface.
At least that is clearer now.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_balloon.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:58:59 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having virtio endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_blk.h | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:04 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having virtio endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_console.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:09 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since crypto is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_crypto.h | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:13 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since fs is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:18 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since gpu is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_gpu.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:23 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_input.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:27 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_iommu.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:32 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> TODO: check other uses of __virtioXX types in this header,
> should probably be __leXX.
Yes, I think so.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_mem.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:37 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having virtio endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_net.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_net.h
b/include/uapi/linux/virtio_net.h
> index 19d23e5baa4e..27d996f29dd1 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -87,19 +87,19 @@ struct virtio_net_config {
>  	/* The config defining mac address (if VIRTIO_NET_F_MAC) */
>  	__u8 mac[ETH_ALEN];
>  	/* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
> -	__u16 status;
> +	__virtio16 status;
>  	/* Maximum number of each of transmit and receive queues;
>  	 * see VIRTIO_NET_F_MQ and VIRTIO_NET_CTRL_MQ.
>  	 * Legal values are between 1 and 0x8000
>  	 */
> -	__u16 max_virtqueue_pairs;
> +	__virtio16 max_virtqueue_pairs;
>  	/* Default maximum transmit unit advice */
> -	__u16 mtu;
> +	__virtio16 mtu;
>  	/*
>  	 * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
>  	 * Any other value stands for unknown.
>  	 */
> -	__u32 speed;
> +	__virtio32 speed;
Hm... VIRTIO_NET_F_SPEED_DUPLEX can only be negotiated if VERSION_1 has
also been negotiated; I think this should be __le32?
>  	/*
>  	 * 0x00 - half duplex
>  	 * 0x01 - full duplex

On Mon, 3 Aug 2020 16:59:41 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/uapi/linux/virtio_pmem.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:45 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Tag config space fields as having virtio endian-ness.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/scsi/virtio_scsi.c       |  4 ++--
>  include/uapi/linux/virtio_scsi.h | 20 ++++++++++----------
>  2 files changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Cornelia Huck <cohuck at redhat.com>

On Mon, 3 Aug 2020 16:59:57 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Transitional devices should all use __virtioXX types.
I think they should use __leXX for those fields that are not present
with legacy devices?
> Modern ones should use __leXX.
> _uXX type would be a bug.
> Let's prevent that.
That sounds right, though.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  include/linux/virtio_config.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 64da491936f7..c68f58f3bf34 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -319,9 +319,8 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
>  	__virtio_pick_type(x, __u8, __u8,					\
>  		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
>  			__virtio_pick_endian(x, __le16, __le32, __le64,		\
> -				__virtio_pick_endian(x, __u16, __u32, __u64,	\
> -					/* No other type allowed */		\
> -					(void)0)))))
> +				/* No other type allowed */			\
> +				(void)0))))
>  
>  #endif
>

On Mon, 3 Aug 2020 17:00:01 -0400
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> mlxbf-tmfifo accesses config space using native types -
> which works for it since the legacy virtio native types.
> 
> This will break if it ever needs to support modern virtio,
> so with new tags previously introduced for virtio net config,
> sparse now warns for this in drivers.
> 
> Since this is a legacy only device, fix it up using
> virtio_legacy_is_little_endian for now.
I'm wondering if the driver should make this more explicit?

No issues with the patch, though.
> 
> No functional changes.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
Acked-by: Cornelia Huck <cohuck at redhat.com>

On 2020/8/4 ??5:00, Michael S. Tsirkin wrote:
> Some legacy guests just assume features are 0 after reset.
> We detect that config space is accessed before features are
> set and set features to 0 automatically.
> Note: some legacy guests might not even access config space, if this is
> reported in the field we might need to catch a kick to handle these.

I wonder whether it's easier to just support modern device?

Thanks

>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>   drivers/vdpa/vdpa.c  |  1 +
>   include/linux/vdpa.h | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index de211ef3738c..7105265e4793 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -96,6 +96,7 @@ struct vdpa_device *__vdpa_alloc_device(struct device
*parent,
>   	vdev->dev.release = vdpa_release_dev;
>   	vdev->index = err;
>   	vdev->config = config;
> +	vdev->features_valid = false;
>   
>   	err = dev_set_name(&vdev->dev, "vdpa%u",
vdev->index);
>   	if (err)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 239db794357c..29b8296f1414 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -33,12 +33,14 @@ struct vdpa_notification_area {
>    * @dma_dev: the actual device that is performing DMA
>    * @config: the configuration ops for this device.
>    * @index: device index
> + * @features_valid: were features initialized? for legacy guests
>    */
>   struct vdpa_device {
>   	struct device dev;
>   	struct device *dma_dev;
>   	const struct vdpa_config_ops *config;
>   	unsigned int index;
> +	bool features_valid;
>   };
>   
>   /**
> @@ -266,4 +268,36 @@ static inline struct device *vdpa_get_dma_dev(struct
vdpa_device *vdev)
>   {
>   	return vdev->dma_dev;
>   }
> +
> +static inline void vdpa_reset(struct vdpa_device *vdev)
> +{
> +        const struct vdpa_config_ops *ops = vdev->config;
> +
> +	vdev->features_valid = false;
> +        ops->set_status(vdev, 0);
> +}
> +
> +static inline int vdpa_set_features(struct vdpa_device *vdev, u64
features)
> +{
> +        const struct vdpa_config_ops *ops = vdev->config;
> +
> +	vdev->features_valid = true;
> +        return ops->set_features(vdev, features);
> +}
> +
> +
> +static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned
offset,
> +				   void *buf, unsigned int len)
> +{
> +        const struct vdpa_config_ops *ops = vdev->config;
> +
> +	/*
> +	 * Config accesses aren't supposed to trigger before features are
set.
> +	 * If it does happen we assume a legacy guest.
> +	 */
> +	if (!vdev->features_valid)
> +		vdpa_set_features(vdev, 0);
> +	ops->get_config(vdev, offset, buf, len);
> +}
> +
>   #endif /* _LINUX_VDPA_H */

On 2020/8/4 ??5:00, Michael S. Tsirkin wrote:
> VDPA sim accesses config space as native endian - this is
> wrong since it's a modern device and actually uses LE.
>
> It only supports modern guests so we could punt and
> just force LE, but let's use the full virtio APIs since people
> tend to copy/paste code, and this is not data path anyway.
>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>   drivers/vdpa/vdpa_sim/vdpa_sim.c | 31 ++++++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c
b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index a9bc5e0fb353..fa05e065ff69 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -24,6 +24,7 @@
>   #include <linux/etherdevice.h>
>   #include <linux/vringh.h>
>   #include <linux/vdpa.h>
> +#include <linux/virtio_byteorder.h>
>   #include <linux/vhost_iotlb.h>
>   #include <uapi/linux/virtio_config.h>
>   #include <uapi/linux/virtio_net.h>
> @@ -72,6 +73,23 @@ struct vdpasim {
>   	u64 features;
>   };
>   
> +/* TODO: cross-endian support */
> +static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
> +{
> +	return virtio_legacy_is_little_endian() ||
> +		(vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
> +}
> +
> +static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16
val)
> +{
> +	return __virtio16_to_cpu(vdpasim_is_little_endian(vdpasim), val);
> +}
> +
> +static inline __virtio16 cpu_to_vdpasim16(struct vdpasim *vdpasim, u16
val)
> +{
> +	return __cpu_to_virtio16(vdpasim_is_little_endian(vdpasim), val);
> +}
> +
>   static struct vdpasim *vdpasim_dev;
>   
>   static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
> @@ -306,7 +324,6 @@ static const struct vdpa_config_ops
vdpasim_net_config_ops;
>   
>   static struct vdpasim *vdpasim_create(void)
>   {
> -	struct virtio_net_config *config;
>   	struct vdpasim *vdpasim;
>   	struct device *dev;
>   	int ret = -ENOMEM;
> @@ -331,10 +348,7 @@ static struct vdpasim *vdpasim_create(void)
>   	if (!vdpasim->buffer)
>   		goto err_iommu;
>   
> -	config = &vdpasim->config;
> -	config->mtu = 1500;
> -	config->status = VIRTIO_NET_S_LINK_UP;
> -	eth_random_addr(config->mac);
> +	eth_random_addr(vdpasim->config.mac);
>   
>   	vringh_set_iotlb(&vdpasim->vqs[0].vring, vdpasim->iommu);
>   	vringh_set_iotlb(&vdpasim->vqs[1].vring, vdpasim->iommu);
> @@ -448,6 +462,7 @@ static u64 vdpasim_get_features(struct vdpa_device
*vdpa)
>   static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
>   {
>   	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> +	struct virtio_net_config *config = &vdpasim->config;
>   
>   	/* DMA mapping must be done by driver */
>   	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> @@ -455,6 +470,12 @@ static int vdpasim_set_features(struct vdpa_device
*vdpa, u64 features)
>   
>   	vdpasim->features = features & vdpasim_features;
>   
> +	/* We only know whether guest is using the legacy interface here, so
> +	 * that's the earliest we can set config fields.
> +	 */

We check whether or not ACCESS_PLATFORM is set before which is probably 
a hint that only modern device is supported. So I wonder just force LE 
and fail if VERSION_1 is not set is better?

Thanks

> +
> +	config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> +	config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
>   	return 0;
>   }
>

On 2020/8/4 ??4:58, Michael S. Tsirkin wrote:
> Currently all config space fields are of the type __uXX.
> This confuses people and some drivers (notably vdpa)
> access them using CPU endian-ness - which only
> works well for legacy or LE platforms.
>
> Update virtio_cread/virtio_cwrite macros to allow __virtioXX
> and __leXX field types. Follow-up patches will convert
> config space to use these types.
>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>   include/linux/virtio_config.h | 50 +++++++++++++++++++++++++++++++++--
>   1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 3b4eae5ac5e3..64da491936f7 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -6,6 +6,7 @@
>   #include <linux/bug.h>
>   #include <linux/virtio.h>
>   #include <linux/virtio_byteorder.h>
> +#include <linux/compiler_types.h>
>   #include <uapi/linux/virtio_config.h>
>   
>   struct irq_affinity;
> @@ -287,12 +288,57 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
>   	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
>   }
>   
> +/*
> + * Only the checker differentiates between __virtioXX and __uXX types. But
we
> + * try to share as much code as we can with the regular GCC build.
> + */
> +#if !defined(CONFIG_CC_IS_GCC) && !defined(__CHECKER__)
> +
> +/* Not a checker - we can keep things simple */
> +#define __virtio_native_typeof(x) typeof(x)
> +
> +#else
> +
> +/*
> + * We build this out of a couple of helper macros in a vain attempt to
> + * help you keep your lunch down while reading it.
> + */
> +#define __virtio_pick_value(x, type, then, otherwise)			\
> +	__builtin_choose_expr(__same_type(x, type), then, otherwise)
> +
> +#define __virtio_pick_type(x, type, then, otherwise)			\
> +	__virtio_pick_value(x, type, (then)0, otherwise)
> +
> +#define __virtio_pick_endian(x, x16, x32, x64, otherwise)			\
> +	__virtio_pick_type(x, x16, __u16,					\
> +		__virtio_pick_type(x, x32, __u32,				\
> +			__virtio_pick_type(x, x64, __u64,			\
> +				otherwise)))
> +
> +#define __virtio_native_typeof(x) typeof(					\
> +	__virtio_pick_type(x, __u8, __u8,					\
> +		__virtio_pick_endian(x, __virtio16, __virtio32, __virtio64,	\
> +			__virtio_pick_endian(x, __le16, __le32, __le64,		\
> +				__virtio_pick_endian(x, __u16, __u32, __u64,	\
> +					/* No other type allowed */		\
> +					(void)0)))))
> +
> +#endif
> +
> +#define __virtio_native_type(structname, member) \
> +	__virtio_native_typeof(((structname*)0)->member)
> +
> +#define __virtio_typecheck(structname, member, val) \
> +		/* Must match the member's type, and be integer */ \
> +		typecheck(__virtio_native_type(structname, member), (val))
> +
> +
>   /* Config space accessors. */
>   #define virtio_cread(vdev, structname, member, ptr)			\
>   	do {								\
>   		might_sleep();						\
>   		/* Must match the member's type, and be integer */	\
> -		if (!typecheck(typeof((((structname*)0)->member)), *(ptr))) \
> +		if (!__virtio_typecheck(structname, member, *(ptr)))	\
>   			(*ptr) = 1;					\

A silly question,? compare to using set()/get() directly, what's the 
value of the accessors macro here?

Thanks

>   									\
>   		switch (sizeof(*ptr)) {					\
> @@ -322,7 +368,7 @@ static inline __virtio64 cpu_to_virtio64(struct
virtio_device *vdev, u64 val)
>   	do {								\
>   		might_sleep();						\
>   		/* Must match the member's type, and be integer */	\
> -		if (!typecheck(typeof((((structname*)0)->member)), *(ptr))) \
> +		if (!__virtio_typecheck(structname, member, *(ptr)))	\
>   			BUG_ON((*ptr) == 1);				\
>   									\
>   		switch (sizeof(*ptr)) {					\