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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Tag config space fields as having virtio endian-ness.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
For new helpers handling legacy features to be effective,
vhost needs to invoke them. Tie them in.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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
Use vars of the correct type instead of casting.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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 <[email protected]>
---
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
Tag config space fields as having virtio endian-ness.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
> Am 03.08.2020 um 22:59 schrieb Michael S. Tsirkin <[email protected]>:
>
> 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 <[email protected]>
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> 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 <[email protected]>:
>
> 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 <[email protected]>
I think I acked this one already.
Acked-by: David Hildenbrand <[email protected]>
> ---
> 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 <[email protected]>:
>
> 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 <[email protected]>
Acked-by: David Hildenbrand <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
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 <[email protected]>
virtio spec does list this field as "le32".
Acked-by: Vivek Goyal <[email protected]>
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" <[email protected]> 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 <[email protected]>
> ---
> drivers/virtio/virtio_balloon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:58:42 -0400
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> include/linux/virtio_ring.h | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
Acked-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:58:46 -0400
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Mon, 3 Aug 2020 16:58:50 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Tag config space fields as having virtio endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_9p.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:58:55 -0400
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> include/uapi/linux/virtio_balloon.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:58:59 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Tag config space fields as having virtio endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_blk.h | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:04 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Tag config space fields as having virtio endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_console.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Tue, Aug 04, 2020 at 04:23:40PM +0200, Cornelia Huck wrote:
> On Mon, 3 Aug 2020 16:58:46 -0400
> "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> > ---
> > 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.
A later patch fixes this. Hopefully :)
> > +#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 <[email protected]>
On Mon, 3 Aug 2020 16:59:13 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Since fs is a modern-only device,
> tag config space fields as having little endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_fs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:23 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_input.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:18 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Since gpu is a modern-only device,
> tag config space fields as having little endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_gpu.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:27 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_iommu.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:32 -0400
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> include/uapi/linux/virtio_mem.h | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:37 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Tag config space fields as having virtio endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> 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" <[email protected]> wrote:
> Since this is a modern-only device,
> tag config space fields as having little endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/uapi/linux/virtio_pmem.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:45 -0400
"Michael S. Tsirkin" <[email protected]> wrote:
> Tag config space fields as having virtio endian-ness.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/scsi/virtio_scsi.c | 4 ++--
> include/uapi/linux/virtio_scsi.h | 20 ++++++++++----------
> 2 files changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 3 Aug 2020 16:59:57 -0400
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> 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" <[email protected]> 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 <[email protected]>
> ---
> drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
Acked-by: Cornelia Huck <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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)) { \
On Tue, Aug 04, 2020 at 04:44:44PM +0200, Cornelia Huck wrote:
> On Mon, 3 Aug 2020 16:59:37 -0400
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > Tag config space fields as having virtio endian-ness.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > 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?
OK APIs for this are in a separate patch.
I'll add conversion as a patch on top.
> > /*
> > * 0x00 - half duplex
> > * 0x01 - full duplex
On Tue, Aug 04, 2020 at 04:50:39PM +0200, Cornelia Huck wrote:
> On Mon, 3 Aug 2020 16:59:57 -0400
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > Transitional devices should all use __virtioXX types.
>
> I think they should use __leXX for those fields that are not present
> with legacy devices?
Will correct.
> > 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 <[email protected]>
> > ---
> > 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 Tue, Aug 04, 2020 at 04:56:34PM +0200, Cornelia Huck wrote:
> On Mon, 3 Aug 2020 17:00:01 -0400
> "Michael S. Tsirkin" <[email protected]> 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?
Not sure how though.
> No issues with the patch, though.
>
> >
> > No functional changes.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
>
> Acked-by: Cornelia Huck <[email protected]>
On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
>
> 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
Well hardware vendors are I think interested in supporting legacy
guests. Limiting vdpa to modern only would make it uncompetitive.
>
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > 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 Wed, Aug 05, 2020 at 02:28:23PM +0800, Jason Wang wrote:
>
> 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 <[email protected]>
> > ---
> > 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
get/set don't convert to the native endian, I guess that's why
drivers use cread/cwrite. It is also nice that there's type
safety, checking the correct integer width is used.
>
> > \
> > 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)) { \
On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
>
> 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 <[email protected]>
> > ---
> > 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
So how about I add a comment along the lines of
/*
* vdpasim ATM requires VIRTIO_F_ACCESS_PLATFORM, so we don't need to
* support legacy guests. Keep transitional device code around for
* the benefit of people who might copy-and-paste this into transitional
* device code.
*/
>
> > +
> > + config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> > + config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> > return 0;
> > }
On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
>
> 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 <[email protected]>
> > ---
> > 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
>
Hmm good point. As usual legacy does not have a clean way to fail though,
are we sure we don't ever want to support legacy guests?
> > +
> > + config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> > + config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> > return 0;
> > }
On 2020/8/5 下午7:40, Michael S. Tsirkin wrote:
> On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
>> 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
> Well hardware vendors are I think interested in supporting legacy
> guests. Limiting vdpa to modern only would make it uncompetitive.
My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA
to work. So it can only work for modern device ...
Thanks
>
>
>
On 2020/8/5 下午8:06, Michael S. Tsirkin wrote:
> On Wed, Aug 05, 2020 at 02:21:07PM +0800, Jason Wang wrote:
>> 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<[email protected]>
>>> ---
>>> 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
> So how about I add a comment along the lines of
>
> /*
> * vdpasim ATM requires VIRTIO_F_ACCESS_PLATFORM, so we don't need to
> * support legacy guests. Keep transitional device code around for
> * the benefit of people who might copy-and-paste this into transitional
> * device code.
> */
That's fine.
Thanks
>
>
On 2020/8/5 下午7:45, Michael S. Tsirkin wrote:
>>> #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
> get/set don't convert to the native endian, I guess that's why
> drivers use cread/cwrite. It is also nice that there's type
> safety, checking the correct integer width is used.
Yes, but this is simply because a macro is used here, how about just
doing things similar like virtio_cread_bytes():
static inline void virtio_cread(struct virtio_device *vdev,
unsigned int offset,
void *buf, size_t len)
And do the endian conversion inside?
Thanks
>
On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote:
>
> On 2020/8/5 下午7:40, Michael S. Tsirkin wrote:
> > On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
> > > 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
> > Well hardware vendors are I think interested in supporting legacy
> > guests. Limiting vdpa to modern only would make it uncompetitive.
>
>
> My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to
> work.
Hmm I don't really see why. Assume host maps guest memory properly,
VM does not have an IOMMU, legacy guest can just work.
Care explaining what's wrong with this picture?
> So it can only work for modern device ...
>
> Thanks
>
>
> >
> >
> >
On Thu, Aug 06, 2020 at 11:37:38AM +0800, Jason Wang wrote:
>
> On 2020/8/5 下午7:45, Michael S. Tsirkin wrote:
> > > > #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
> > get/set don't convert to the native endian, I guess that's why
> > drivers use cread/cwrite. It is also nice that there's type
> > safety, checking the correct integer width is used.
>
>
> Yes, but this is simply because a macro is used here, how about just doing
> things similar like virtio_cread_bytes():
>
> static inline void virtio_cread(struct virtio_device *vdev,
> unsigned int offset,
> void *buf, size_t len)
>
>
> And do the endian conversion inside?
>
> Thanks
>
Then you lose type safety. It's very easy to have an le32 field
and try to read it into a u16 by mistake.
These macros are all about preventing bugs: and the whole patchset
is about several bugs sparse found - that is what prompted me to make
type checks more strict.
> >
On 2020/8/6 下午1:53, Michael S. Tsirkin wrote:
> On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote:
>> On 2020/8/5 下午7:40, Michael S. Tsirkin wrote:
>>> On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
>>>> 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
>>> Well hardware vendors are I think interested in supporting legacy
>>> guests. Limiting vdpa to modern only would make it uncompetitive.
>>
>> My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to
>> work.
> Hmm I don't really see why. Assume host maps guest memory properly,
> VM does not have an IOMMU, legacy guest can just work.
Yes, guest may not set IOMMU_PLATFORM.
>
> Care explaining what's wrong with this picture?
The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can
not work if IOMMU is enabled.
Thanks
>
>
>> So it can only work for modern device ...
>>
>> Thanks
>>
>>
>>>
>>>
On Thu, Aug 06, 2020 at 03:27:38PM +0800, Jason Wang wrote:
>
> On 2020/8/6 下午1:53, Michael S. Tsirkin wrote:
> > On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote:
> > > On 2020/8/5 下午7:40, Michael S. Tsirkin wrote:
> > > > On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
> > > > > 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
> > > > Well hardware vendors are I think interested in supporting legacy
> > > > guests. Limiting vdpa to modern only would make it uncompetitive.
> > >
> > > My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to
> > > work.
> > Hmm I don't really see why. Assume host maps guest memory properly,
> > VM does not have an IOMMU, legacy guest can just work.
>
>
> Yes, guest may not set IOMMU_PLATFORM.
>
>
> >
> > Care explaining what's wrong with this picture?
>
>
> The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can not
> work if IOMMU is enabled.
>
> Thanks
So that's a virtio_vdpa limitation. In the same way, if a device
does not have an on-device iommu *and* is not behind an iommu,
then vdpa can't bind to it.
But this virtio_vdpa specific hack does not belong in a generic vdpa code.
>
> >
> >
> > > So it can only work for modern device ...
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >
On 2020/8/6 下午6:00, Michael S. Tsirkin wrote:
> On Thu, Aug 06, 2020 at 03:27:38PM +0800, Jason Wang wrote:
>> On 2020/8/6 下午1:53, Michael S. Tsirkin wrote:
>>> On Thu, Aug 06, 2020 at 11:23:05AM +0800, Jason Wang wrote:
>>>> On 2020/8/5 下午7:40, Michael S. Tsirkin wrote:
>>>>> On Wed, Aug 05, 2020 at 02:14:07PM +0800, Jason Wang wrote:
>>>>>> 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
>>>>> Well hardware vendors are I think interested in supporting legacy
>>>>> guests. Limiting vdpa to modern only would make it uncompetitive.
>>>> My understanding is that, IOMMU_PLATFORM is mandatory for hardware vDPA to
>>>> work.
>>> Hmm I don't really see why. Assume host maps guest memory properly,
>>> VM does not have an IOMMU, legacy guest can just work.
>>
>> Yes, guest may not set IOMMU_PLATFORM.
>>
>>
>>> Care explaining what's wrong with this picture?
>>
>> The problem is virtio_vdpa, without IOMMU_PLATFORM it uses PA which can not
>> work if IOMMU is enabled.
>>
>> Thanks
> So that's a virtio_vdpa limitation.
Probably not, I think this goes back to the long debate of whether to
use DMA API unconditionally. If we did that, we can support legacy
virtio driver.
The vDPA device needs to provide a DMA device and the virtio core and
perform DMA API with that device which should work for all of the cases.
But a big question is, do upstream care about out of tree virtio drivers?
Thanks
> In the same way, if a device
> does not have an on-device iommu *and* is not behind an iommu,
> then vdpa can't bind to it.
>
> But this virtio_vdpa specific hack does not belong in a generic vdpa code.
>
>>>
>>>> So it can only work for modern device ...
>>>>
>>>> Thanks
>>>>
>>>>
>>>>>
On 2020/8/6 下午1:58, Michael S. Tsirkin wrote:
> On Thu, Aug 06, 2020 at 11:37:38AM +0800, Jason Wang wrote:
>> On 2020/8/5 下午7:45, Michael S. Tsirkin wrote:
>>>>> #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
>>> get/set don't convert to the native endian, I guess that's why
>>> drivers use cread/cwrite. It is also nice that there's type
>>> safety, checking the correct integer width is used.
>>
>> Yes, but this is simply because a macro is used here, how about just doing
>> things similar like virtio_cread_bytes():
>>
>> static inline void virtio_cread(struct virtio_device *vdev,
>> unsigned int offset,
>> void *buf, size_t len)
>>
>>
>> And do the endian conversion inside?
>>
>> Thanks
>>
> Then you lose type safety. It's very easy to have an le32 field
> and try to read it into a u16 by mistake.
>
> These macros are all about preventing bugs: and the whole patchset
> is about several bugs sparse found - that is what prompted me to make
> type checks more strict.
Yes, but we need to do the macro with compiler extensions. I wonder
whether or not the kernel has already had something since this request
here is pretty common?
Thanks
>
>