virtio: feature related cleanups
Subsystems (6)
Michael S. Tsirkin
maintainerv2 cover virtio: feature related cleanups
A minor cleanup around handling of feature bits: this fixes up terminology and adds build checks.
Lightly tested.
changes from v1:
dropped using "word" completely
Michael S. Tsirkin (2):
virtio: clean up features qword/dword terms
vhost: use checked versions of VIRTIO_BIT
drivers/vhost/net.c | 16 +++++------ drivers/virtio/virtio.c | 12 ++++----- drivers/virtio/virtio_debug.c | 10 +++---- drivers/virtio/virtio_pci_modern_dev.c | 6 ++--- include/linux/virtio.h | 2 +- include/linux/virtio_config.h | 2 +- include/linux/virtio_features.h | 37 ++++++++++++++++---------- include/linux/virtio_pci_modern.h | 8 +++--- scripts/lib/kdoc/kdoc_parser.py | 2 +- 9 files changed, 52 insertions(+), 43 deletions(-)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 1/2 virtio: clean up features qword/dword terms
virtio pci uses word to mean "16 bits". mmio uses it to mean "32 bits".
To avoid confusion, let's avoid the term in core virtio altogether. Just say U64 to mean "64 bit".
--- drivers/vhost/net.c | 12 +++++------ drivers/virtio/virtio.c | 12 +++++------ drivers/virtio/virtio_debug.c | 10 ++++----- drivers/virtio/virtio_pci_modern_dev.c | 6 +++--- include/linux/virtio.h | 2 +- include/linux/virtio_config.h | 2 +- include/linux/virtio_features.h | 29 +++++++++++++------------- include/linux/virtio_pci_modern.h | 8 +++---- scripts/lib/kdoc/kdoc_parser.py | 2 +- 9 files changed, 42 insertions(+), 41 deletions(-)
drivers/vhost/net.c +6 -6
@@ -69,7 +69,7 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"#define VHOST_DMA_IS_DONE (len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN) -static const u64 vhost_net_features[VIRTIO_FEATURES_DWORDS ] = { +static const u64 vhost_net_features[VIRTIO_FEATURES_ARRAY_SIZE ] = {VHOST_FEATURES | (1ULL <<VHOST_NET_F_VIRTIO_NET_HDR ) | (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
@@ -1720,7 +1720,7 @@ static long vhost_net_set_owner(struct vhost_net *n)static long vhost_net_ioctl (struct file *f,unsigned int ioctl,unsigned long arg) { -u64 all_features[VIRTIO_FEATURES_DWORDS ]; +u64 all_features[VIRTIO_FEATURES_ARRAY_SIZE ];struct vhost_net *n = f->private_data;void __user *argp = (void __user *)arg;u64 __user *featurep = argp;
@@ -1752,7 +1752,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,/* Copy the net features, up to the user-provided buffer size */ argp +=sizeof (u64); - copied =min (count,VIRTIO_FEATURES_DWORDS ); + copied =min (count, (u64 )VIRTIO_FEATURES_ARRAY_SIZE );if (copy_to_user (argp, vhost_net_features, copied *sizeof (u64)))return -EFAULT ;
@@ -1767,13 +1767,13 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,virtio_features_zero (all_features); argp +=sizeof (u64); - copied =min (count,VIRTIO_FEATURES_DWORDS ); + copied =min (count, (u64 )VIRTIO_FEATURES_ARRAY_SIZE );if (copy_from_user (all_features, argp, copied *sizeof (u64)))return -EFAULT ;/* * Any feature specified by user-space above -* VIRTIO_FEATURES_MAX is not supported by definition. +* VIRTIO_FEATURES_BITS is not supported by definition. */ for (i = copied; i < count; ++i) { if (copy_from_user(&features, featurep + 1 + i,
@@ -1783,7 +1783,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,return -EOPNOTSUPP ; } -for (i =0 ; i <VIRTIO_FEATURES_DWORDS ; i++) +for (i =0 ; i <VIRTIO_FEATURES_ARRAY_SIZE ; i++)if (all_features[i] & ~vhost_net_features[i])return -EOPNOTSUPP ;
drivers/virtio/virtio.c +6 -6
@@ -53,7 +53,7 @@ static ssize_t features_show(struct device *_d,/* We actually represent this as a bitstring, as it could be * arbitrary length in future. */ -for (i =0 ; i <VIRTIO_FEATURES_MAX ; i++) +for (i =0 ; i <VIRTIO_FEATURES_BITS ; i++) len +=sysfs_emit_at (buf, len,"%c" ,__virtio_test_bit (dev, i) ?'1' :'0' ); len +=sysfs_emit_at (buf, len,"\n" );
@@ -272,8 +272,8 @@ static int virtio_dev_probe(struct device *_d)int err, i;struct virtio_device *dev =dev_to_virtio (_d);struct virtio_driver *drv =drv_to_virtio (dev->dev.driver); -u64 device_features[VIRTIO_FEATURES_DWORDS ]; -u64 driver_features[VIRTIO_FEATURES_DWORDS ]; +u64 device_features[VIRTIO_FEATURES_ARRAY_SIZE ]; +u64 driver_features[VIRTIO_FEATURES_ARRAY_SIZE ];u64 driver_features_legacy;/* We have a driver! */
@@ -286,7 +286,7 @@ static int virtio_dev_probe(struct device *_d)virtio_features_zero (driver_features);for (i =0 ; i < drv->feature_table_size; i++) {unsigned int f = drv->feature_table[i]; -if (!WARN_ON_ONCE (f >=VIRTIO_FEATURES_MAX )) +if (!WARN_ON_ONCE (f >=VIRTIO_FEATURES_BITS ))virtio_features_set_bit (driver_features, f); }
@@ -303,7 +303,7 @@ static int virtio_dev_probe(struct device *_d) }if (virtio_features_test_bit (device_features,VIRTIO_F_VERSION_1 )) { -for (i =0 ; i <VIRTIO_FEATURES_DWORDS ; ++i) +for (i =0 ; i <VIRTIO_FEATURES_ARRAY_SIZE ; ++i) dev->features_array[i] = driver_features[i] & device_features[i]; }else {
@@ -325,7 +325,7 @@ static int virtio_dev_probe(struct device *_d) goto err;if (drv->validate) { -u64 features[VIRTIO_FEATURES_DWORDS ]; +u64 features[VIRTIO_FEATURES_ARRAY_SIZE ];virtio_features_copy (features, dev->features_array); err = drv->validate (dev);
drivers/virtio/virtio_debug.c +5 -5
@@ -8,12 +8,12 @@ static struct dentry *virtio_debugfs_dir;static int virtio_debug_device_features_show (struct seq_file *s,void *data) { -u64 device_features[VIRTIO_FEATURES_DWORDS ]; +u64 device_features[VIRTIO_FEATURES_ARRAY_SIZE ];struct virtio_device *dev = s->private;unsigned int i;virtio_get_features (dev, device_features); -for (i =0 ; i <VIRTIO_FEATURES_MAX ; i++) { +for (i =0 ; i <VIRTIO_FEATURES_BITS ; i++) {if (virtio_features_test_bit (device_features, i))seq_printf (s,"%u\n" , i); }
@@ -26,7 +26,7 @@ static int virtio_debug_filter_features_show(struct seq_file *s, void *data)struct virtio_device *dev = s->private;unsigned int i; -for (i =0 ; i <VIRTIO_FEATURES_MAX ; i++) { +for (i =0 ; i <VIRTIO_FEATURES_BITS ; i++) {if (virtio_features_test_bit (dev->debugfs_filter_features, i))seq_printf (s,"%u\n" , i); }
@@ -50,7 +50,7 @@ static int virtio_debug_filter_feature_add(void *data, u64 val) {struct virtio_device *dev = data; -if (val >=VIRTIO_FEATURES_MAX ) +if (val >=VIRTIO_FEATURES_BITS )return -EINVAL ;virtio_features_set_bit (dev->debugfs_filter_features, val);
@@ -64,7 +64,7 @@ static int virtio_debug_filter_feature_del(void *data, u64 val) {struct virtio_device *dev = data; -if (val >=VIRTIO_FEATURES_MAX ) +if (val >=VIRTIO_FEATURES_BITS )return -EINVAL ;virtio_features_clear_bit (dev->debugfs_filter_features, val);
drivers/virtio/virtio_pci_modern_dev.c +3 -3
@@ -401,7 +401,7 @@ void vp_modern_get_extended_features(struct virtio_pci_modern_device *mdev,int i;virtio_features_zero (features); -for (i =0 ; i <VIRTIO_FEATURES_WORDS ; i++) { +for (i =0 ; i <VIRTIO_FEATURES_BITS /32 ; i++) {u64 cur;vp_iowrite32 (i, &cfg->device_feature_select);
@@ -427,7 +427,7 @@ vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,int i;virtio_features_zero (features); -for (i =0 ; i <VIRTIO_FEATURES_WORDS ; i++) { +for (i =0 ; i <VIRTIO_FEATURES_BITS /32 ; i++) {u64 cur;vp_iowrite32 (i, &cfg->guest_feature_select);
@@ -448,7 +448,7 @@ void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev,struct virtio_pci_common_cfg __iomem *cfg = mdev->common;int i; -for (i =0 ; i <VIRTIO_FEATURES_WORDS ; i++) { +for (i =0 ; i <VIRTIO_FEATURES_BITS /32 ; i++) {u32 cur = features[i >>1 ] >> (32 * (i &1 ));vp_iowrite32 (i, &cfg->guest_feature_select);
include/linux/virtio.h +1 -1
@@ -177,7 +177,7 @@ struct virtio_device {union virtio_map vmap;#ifdef CONFIG_VIRTIO_DEBUG struct dentry *debugfs_dir; -u64 debugfs_filter_features[VIRTIO_FEATURES_DWORDS ]; +u64 debugfs_filter_features[VIRTIO_FEATURES_ARRAY_SIZE ];#endif };
include/linux/virtio_config.h +1 -1
@@ -80,7 +80,7 @@ struct virtqueue_info {* Returns the first 64 feature bits. * @get_extended_features: * vdev: the virtio_device -* Returns the first VIRTIO_FEATURES_MAX feature bits (all we currently +* Returns the first VIRTIO_FEATURES_BITS feature bits (all we currently * need). * @finalize_features: confirm what device features we'll be using. * vdev: the virtio_device
include/linux/virtio_features.h +15 -14
@@ -4,15 +4,16 @@#include <linux/bits.h> -#define VIRTIO_FEATURES_DWORDS 2 -#define VIRTIO_FEATURES_MAX (VIRTIO_FEATURES_DWORDS * 64) -#define VIRTIO_FEATURES_WORDS (VIRTIO_FEATURES_DWORDS * 2) +#define VIRTIO_FEATURES_BITS (128)#define VIRTIO_BIT (b) BIT_ULL((b) & 0x3f) -#define VIRTIO_DWORD (b) ((b) >> 6) +#define VIRTIO_U64 (b) ((b) >> 6) + +#define VIRTIO_FEATURES_ARRAY_SIZE VIRTIO_U64(VIRTIO_FEATURES_BITS) +#define VIRTIO_DECLARE_FEATURES (name) \ union { \ u64 name; \ - u64 name##_array[VIRTIO_FEATURES_DWORDS];\ + u64 name##_array[VIRTIO_FEATURES_ARRAY_SIZE];\ }static inline bool virtio_features_chk_bit (unsigned int bit)
@@ -22,9 +23,9 @@ static inline bool virtio_features_chk_bit(unsigned int bit)* Don't care returning the correct value: the build * will fail before any bad features access */ -BUILD_BUG_ON (bit >=VIRTIO_FEATURES_MAX ); +BUILD_BUG_ON (bit >=VIRTIO_FEATURES_BITS ); }else { -if (WARN_ON_ONCE (bit >=VIRTIO_FEATURES_MAX )) +if (WARN_ON_ONCE (bit >=VIRTIO_FEATURES_BITS ))return false; }return true;
@@ -34,26 +35,26 @@ static inline bool virtio_features_test_bit(const u64 *features, unsigned int bit) {return virtio_features_chk_bit (bit) && - !!(features[VIRTIO_DWORD (bit)] &VIRTIO_BIT (bit)); + !!(features[VIRTIO_U64 (bit)] &VIRTIO_BIT (bit)); }static inline void virtio_features_set_bit (u64 *features,unsigned int bit) {if (virtio_features_chk_bit (bit)) - features[VIRTIO_DWORD (bit)] |=VIRTIO_BIT (bit); + features[VIRTIO_U64 (bit)] |=VIRTIO_BIT (bit); }static inline void virtio_features_clear_bit (u64 *features,unsigned int bit) {if (virtio_features_chk_bit (bit)) - features[VIRTIO_DWORD (bit)] &= ~VIRTIO_BIT (bit); + features[VIRTIO_U64 (bit)] &= ~VIRTIO_BIT (bit); }static inline void virtio_features_zero (u64 *features) { -memset (features,0 ,sizeof (features[0 ]) *VIRTIO_FEATURES_DWORDS ); +memset (features,0 ,sizeof (features[0 ]) *VIRTIO_FEATURES_ARRAY_SIZE ); }static inline void virtio_features_from_u64 (u64 *features,u64 from)
@@ -66,7 +67,7 @@ static inline bool virtio_features_equal(const u64 *f1, const u64 *f2) {int i; -for (i =0 ; i <VIRTIO_FEATURES_DWORDS ; ++i) +for (i =0 ; i <VIRTIO_FEATURES_ARRAY_SIZE ; ++i)if (f1[i] != f2[i])return false;return true;
@@ -74,14 +75,14 @@ static inline bool virtio_features_equal(const u64 *f1, const u64 *f2)static inline void virtio_features_copy (u64 *to,const u64 *from) { -memcpy (to, from,sizeof (to[0 ]) *VIRTIO_FEATURES_DWORDS ); +memcpy (to, from,sizeof (to[0 ]) *VIRTIO_FEATURES_ARRAY_SIZE ); }static inline void virtio_features_andnot (u64 *to,const u64 *f1,const u64 *f2) {int i; -for (i =0 ; i <VIRTIO_FEATURES_DWORDS ; i++) +for (i =0 ; i <VIRTIO_FEATURES_ARRAY_SIZE ; i++) to[i] = f1[i] & ~f2[i]; }
include/linux/virtio_pci_modern.h +4 -4
@@ -107,7 +107,7 @@ void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev,static inline u64 vp_modern_get_features (struct virtio_pci_modern_device *mdev) { -u64 features_array[VIRTIO_FEATURES_DWORDS ]; +u64 features_array[VIRTIO_FEATURES_ARRAY_SIZE ];vp_modern_get_extended_features (mdev, features_array);return features_array[0 ];
@@ -116,11 +116,11 @@ vp_modern_get_features(struct virtio_pci_modern_device *mdev)static inline u64 vp_modern_get_driver_features (struct virtio_pci_modern_device *mdev) { -u64 features_array[VIRTIO_FEATURES_DWORDS ]; +u64 features_array[VIRTIO_FEATURES_ARRAY_SIZE ];int i;vp_modern_get_driver_extended_features (mdev, features_array); -for (i =1 ; i <VIRTIO_FEATURES_DWORDS ; ++i) +for (i =1 ; i <VIRTIO_FEATURES_ARRAY_SIZE ; ++i)WARN_ON_ONCE (features_array[i]);return features_array[0 ]; }
@@ -128,7 +128,7 @@ vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev)static inline void vp_modern_set_features (struct virtio_pci_modern_device *mdev,u64 features) { -u64 features_array[VIRTIO_FEATURES_DWORDS ]; +u64 features_array[VIRTIO_FEATURES_ARRAY_SIZE ];virtio_features_from_u64 (features_array, features);vp_modern_set_extended_features (mdev, features_array);
scripts/lib/kdoc/kdoc_parser.py +1 -1
@@ -638,7 +638,7 @@ class KernelDoc: (KernRe(r'(?:__)?DECLARE_FLEX_ARRAY\s*\(' + args_pattern + r',\s*' + args_pattern + r'\)', re.S), r'\1 \2[]'), (KernRe(r'DEFINE_DMA_UNMAP_ADDR\s*\(' + args_pattern + r'\)', re.S), r'dma_addr_t \1'), (KernRe(r'DEFINE_DMA_UNMAP_LEN\s*\(' + args_pattern + r'\)', re.S), r'__u32 \1'), - (KernRe(r'VIRTIO_DECLARE_FEATURES\s*\(' + args_pattern + r'\)', re.S), r'u64 \1; u64 \1_array[VIRTIO_FEATURES_DWORDS]'), + (KernRe(r'VIRTIO_DECLARE_FEATURES\s*\(' + args_pattern + r'\)', re.S), r'u64 \1; u64 \1_array[VIRTIO_FEATURES_QWORDS]'), ] # Regexes here are guaranteed to have the end limiter matching
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 2/2 vhost: use checked versions of VIRTIO_BIT
This adds compile-time checked versions of VIRTIO_BIT that set bits in low and high qword, respectively. Will prevent confusion when people set bits in the wrong qword.
--- drivers/vhost/net.c | 4 ++-- include/linux/virtio_features.h | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-)
drivers/vhost/net.c +2 -2
@@ -76,8 +76,8 @@ static const u64 vhost_net_features[VIRTIO_FEATURES_ARRAY_SIZE] = { (1ULL <<VIRTIO_F_ACCESS_PLATFORM ) | (1ULL <<VIRTIO_F_RING_RESET ) | (1ULL <<VIRTIO_F_IN_ORDER ), -VIRTIO_BIT (VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO ) | - VIRTIO_BIT(VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO), +VIRTIO_BIT_HI (VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO ) | + VIRTIO_BIT_HI(VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO), }; enum {
include/linux/virtio_features.h +8 -0
@@ -8,6 +8,14 @@#define VIRTIO_BIT (b) BIT_ULL((b) & 0x3f)#define VIRTIO_U64 (b) ((b) >> 6) +/* Get a given feature bit in a given u64 entry. */ +#define VIRTIO_BIT_U64 (bit, entry) \ + (BUILD_BUG_ON_ZERO(const_true(VIRTIO_U64(bit) != (qword))) + \ + BIT_ULL((bit) - 64 * (entry))) + +#define VIRTIO_BIT_LO (b) VIRTIO_BIT_U64(b, 0) +#define VIRTIO_BIT_HI (b) VIRTIO_BIT_U64(b, 1) +#define VIRTIO_FEATURES_ARRAY_SIZE VIRTIO_U64(VIRTIO_FEATURES_BITS)#define VIRTIO_DECLARE_FEATURES (name) \
Signature · 1 line
MST
Re: [PATCH v2 1/2] virtio: clean up features qword/dword terms
In reply to Michael S. Tsirkindrivers/vhost/net.c static long vhost_net_ioctl(struct file *f, unsigned int ioctl, · line 1755
Quoted 6 lines
drivers/vhost/net.c +1
-1@@ -1752,7 +1752,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,/* Copy the net features, up to the user-provided buffer size */ argp +=sizeof (u64); - copied =min (count,VIRTIO_FEATURES_DWORDS ); + copied =min (count, (u64 )VIRTIO_FEATURES_ARRAY_SIZE );
Why is the cast needed? Why was 2 O.K, but (128 >> 6) needs a cast?
include/linux/virtio_features.h line 11
Quoted 9 lines
include/linux/virtio_features.h +4
-4-#define VIRTIO_FEATURES_DWORDS 2 -#define VIRTIO_FEATURES_MAX (VIRTIO_FEATURES_DWORDS * 64) -#define VIRTIO_FEATURES_WORDS (VIRTIO_FEATURES_DWORDS * 2) +#define VIRTIO_FEATURES_BITS (128)#define VIRTIO_BIT (b) BIT_ULL((b) & 0x3f) -#define VIRTIO_DWORD (b) ((b) >> 6) +#define VIRTIO_U64 (b) ((b) >> 6) + +#define VIRTIO_FEATURES_ARRAY_SIZE VIRTIO_U64(VIRTIO_FEATURES_BITS)
Andrew
Michael S. Tsirkin
maintainerRe: [PATCH v2 1/2] virtio: clean up features qword/dword terms
Quoted 8 lines · Andrew Lunn
drivers/vhost/net.c static long vhost_net_ioctl(struct file *f, unsigned int ioctl, · line 1755
Quoted 6 lines
drivers/vhost/net.c +1
-1@@ -1752,7 +1752,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,/* Copy the net features, up to the user-provided buffer size */ argp +=sizeof (u64); - copied =min (count,VIRTIO_FEATURES_DWORDS ); + copied =min (count, (u64 )VIRTIO_FEATURES_ARRAY_SIZE );Why is the cast needed? Why was 2 O.K, but (128 >> 6) needs a cast?
It's not - a leftover from one of the approaches I tried, thanks!
Quoted 11 lines
include/linux/virtio_features.h line 11
Quoted 9 lines
include/linux/virtio_features.h +4
-4-#define VIRTIO_FEATURES_DWORDS 2 -#define VIRTIO_FEATURES_MAX (VIRTIO_FEATURES_DWORDS * 64) -#define VIRTIO_FEATURES_WORDS (VIRTIO_FEATURES_DWORDS * 2) +#define VIRTIO_FEATURES_BITS (128)#define VIRTIO_BIT (b) BIT_ULL((b) & 0x3f) -#define VIRTIO_DWORD (b) ((b) >> 6) +#define VIRTIO_U64 (b) ((b) >> 6) + +#define VIRTIO_FEATURES_ARRAY_SIZE VIRTIO_U64(VIRTIO_FEATURES_BITS)Andrew