[PATCH 0/3] feature related cleanups

[PATCH 0/3] feature related cleanups

[Michael S. Tsirkin]

A minor cleanup around handling of feature bits: this fixes
up terminology and adds build checks.

Lightly tested.

Michael S. Tsirkin (3):
  virtio: dwords->qwords
  virtio: words->dwords
  vhost: use checked versions of VIRTIO_BIT

 drivers/vhost/net.c                    | 14 +++++------
 drivers/virtio/virtio.c                |  8 +++----
 drivers/virtio/virtio_debug.c          |  2 +-
 drivers/virtio/virtio_pci_modern_dev.c |  6 ++---
 include/linux/virtio.h                 |  2 +-
 include/linux/virtio_features.h        | 33 ++++++++++++++++----------
 include/linux/virtio_pci_modern.h      |  8 +++----
 scripts/lib/kdoc/kdoc_parser.py        |  2 +-
 8 files changed, 42 insertions(+), 33 deletions(-)

-- 
MST

[PATCH 1/3] virtio: dwords->qwords

[Michael S. Tsirkin]

A "word" is 16 bit. 64 bit integers like virtio uses are not dwords,
they are actually qwords. Fix up macro names accordingly.

Fixes: e7d4c1c5a546 ("virtio: introduce extended features")
Cc: "Paolo Abeni" <pabeni@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/net.c               | 10 +++++-----
 drivers/virtio/virtio.c           |  8 ++++----
 drivers/virtio/virtio_debug.c     |  2 +-
 include/linux/virtio.h            |  2 +-
 include/linux/virtio_features.h   | 24 ++++++++++++------------
 include/linux/virtio_pci_modern.h |  8 ++++----
 scripts/lib/kdoc/kdoc_parser.py   |  2 +-
 7 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 35ded4330431..43d51fb1f8ea 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -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_QWORDS] = {
 	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_QWORDS];
 	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, VIRTIO_FEATURES_QWORDS);
 		if (copy_to_user(argp, vhost_net_features,
 				 copied * sizeof(u64)))
 			return -EFAULT;
@@ -1767,7 +1767,7 @@ 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, VIRTIO_FEATURES_QWORDS);
 		if (copy_from_user(all_features, argp, copied * sizeof(u64)))
 			return -EFAULT;
 
@@ -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_QWORDS; i++)
 			if (all_features[i] & ~vhost_net_features[i])
 				return -EOPNOTSUPP;
 
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a09eb4d62f82..08f8357cdd39 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -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_QWORDS];
+	u64 driver_features[VIRTIO_FEATURES_QWORDS];
 	u64 driver_features_legacy;
 
 	/* We have a driver! */
@@ -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_QWORDS; ++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_QWORDS];
 
 		virtio_features_copy(features, dev->features_array);
 		err = drv->validate(dev);
diff --git a/drivers/virtio/virtio_debug.c b/drivers/virtio/virtio_debug.c
index d58713ddf2e5..40f6b815caef 100644
--- a/drivers/virtio/virtio_debug.c
+++ b/drivers/virtio/virtio_debug.c
@@ -8,7 +8,7 @@ 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_QWORDS];
 	struct virtio_device *dev = s->private;
 	unsigned int i;
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 96c66126c074..5b258451dc0e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -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_QWORDS];
 #endif
 };
 
diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
index f748f2f87de8..bf41d8ec50ef 100644
--- a/include/linux/virtio_features.h
+++ b/include/linux/virtio_features.h
@@ -4,15 +4,15 @@
 
 #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_QWORDS	2
+#define VIRTIO_FEATURES_MAX	(VIRTIO_FEATURES_QWORDS * 64)
+#define VIRTIO_FEATURES_WORDS	(VIRTIO_FEATURES_QWORDS * 2)
 #define VIRTIO_BIT(b)		BIT_ULL((b) & 0x3f)
-#define VIRTIO_DWORD(b)		((b) >> 6)
+#define VIRTIO_QWORD(b)		((b) >> 6)
 #define VIRTIO_DECLARE_FEATURES(name)			\
 	union {						\
 		u64 name;				\
-		u64 name##_array[VIRTIO_FEATURES_DWORDS];\
+		u64 name##_array[VIRTIO_FEATURES_QWORDS];\
 	}
 
 static inline bool virtio_features_chk_bit(unsigned int bit)
@@ -34,26 +34,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_QWORD(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_QWORD(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_QWORD(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_QWORDS);
 }
 
 static inline void virtio_features_from_u64(u64 *features, u64 from)
@@ -66,7 +66,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_QWORDS; ++i)
 		if (f1[i] != f2[i])
 			return false;
 	return true;
@@ -74,14 +74,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_QWORDS);
 }
 
 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_QWORDS; i++)
 		to[i] = f1[i] & ~f2[i];
 }
 
diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
index 48bc12d1045b..15818bd04716 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -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_QWORDS];
 
 	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_QWORDS];
 	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_QWORDS; ++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_QWORDS];
 
 	virtio_features_from_u64(features_array, features);
 	vp_modern_set_extended_features(mdev, features_array);
diff --git a/scripts/lib/kdoc/kdoc_parser.py b/scripts/lib/kdoc/kdoc_parser.py
index fe730099eca8..5d629aebc8f0 100644
--- a/scripts/lib/kdoc/kdoc_parser.py
+++ b/scripts/lib/kdoc/kdoc_parser.py
@@ -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
-- 
MST

Re: [PATCH 1/3] virtio: dwords->qwords

[Andrew Lunn]

On Thu, Oct 09, 2025 at 07:24:08AM -0400, Michael S. Tsirkin wrote:
> A "word" is 16 bit. 64 bit integers like virtio uses are not dwords,
> they are actually qwords.

I'm having trouble with this....

This bit makes sense. 4x 16bits = 64 bits.

> -static const u64 vhost_net_features[VIRTIO_FEATURES_DWORDS] = {
> +static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {

If this was u16, and VIRTIO_FEATURES_QWORDS was 4, which the Q would
imply, than i would agree with what you are saying. But this is a u64
type.  It is already a QWORD, and this is an array of two of them.

I think the real issue here is not D vs Q, but WORD. We have a default
meaning of a u16 for a word, especially in C. But that is not the
actual definition of a word a computer scientist would use. Wikipedia
has:

  In computing, a word is any processor design's natural unit of
  data. A word is a fixed-sized datum handled as a unit by the
  instruction set or the hardware of the processor.

A word can be any size. In this context, virtio is not referring to
the instruction set, but a protocol. Are all fields in this protocol
u64? Hence word is u64? And this is an array of two words? That would
make DWORD correct, it is two words.

If you want to change anything here, i would actually change WORD to
something else, maybe FIELD?

And i could be wrong here, i've not looked at the actual protocol, so
i've no idea if all fields in the protocol are u64. There are
protocols like this, IPv6 uses u32, not octets, and the length field
in the headers refer to the number of u32s in the header.

	Andrew

Re: [PATCH 1/3] virtio: dwords->qwords

[Michael S. Tsirkin]

On Thu, Oct 09, 2025 at 02:31:04PM +0200, Andrew Lunn wrote:
> On Thu, Oct 09, 2025 at 07:24:08AM -0400, Michael S. Tsirkin wrote:
> > A "word" is 16 bit. 64 bit integers like virtio uses are not dwords,
> > they are actually qwords.
> 
> I'm having trouble with this....
> 
> This bit makes sense. 4x 16bits = 64 bits.
> 
> > -static const u64 vhost_net_features[VIRTIO_FEATURES_DWORDS] = {
> > +static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
> 
> If this was u16, and VIRTIO_FEATURES_QWORDS was 4, which the Q would
> imply, than i would agree with what you are saying. But this is a u64
> type.  It is already a QWORD, and this is an array of two of them.

I don't get what you are saying here.
It's an array of qwords and VIRTIO_FEATURES_QWORDS tells you
how many QWORDS are needed to fit all of them.

This is how C arrays are declared.


> I think the real issue here is not D vs Q, but WORD. We have a default
> meaning of a u16 for a word, especially in C. But that is not the
> actual definition of a word a computer scientist would use. Wikipedia
> has:
> 
>   In computing, a word is any processor design's natural unit of
>   data. A word is a fixed-sized datum handled as a unit by the
>   instruction set or the hardware of the processor.
> 
> A word can be any size. In this context, virtio is not referring to
> the instruction set, but a protocol. Are all fields in this protocol
> u64? Hence word is u64? And this is an array of two words? That would
> make DWORD correct, it is two words.
> 
> If you want to change anything here, i would actually change WORD to
> something else, maybe FIELD?
> 
> And i could be wrong here, i've not looked at the actual protocol, so
> i've no idea if all fields in the protocol are u64. There are
> protocols like this, IPv6 uses u32, not octets, and the length field
> in the headers refer to the number of u32s in the header.
> 
> 	Andrew


Virtio uses "dword" to mean "32 bits" in several places:



device-types/i2c/description.tex:The \field{padding} is used to pad to full dword.

pads to 32 bit


transport-pci.tex:        u8 padding[2];  /* Pad to full dword. */

same




Under pci, the meaning is also generally as I use it here.
E.g.:

Documentation/PCI/pci.rst:You can use `pci_(read|write)_config_(byte|word|dword)` to access the config






-- 
MST

Re: [PATCH 1/3] virtio: dwords->qwords

[Andrew Lunn]

On Thu, Oct 09, 2025 at 09:37:20AM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 09, 2025 at 02:31:04PM +0200, Andrew Lunn wrote:
> > On Thu, Oct 09, 2025 at 07:24:08AM -0400, Michael S. Tsirkin wrote:
> > > A "word" is 16 bit. 64 bit integers like virtio uses are not dwords,
> > > they are actually qwords.
> > 
> > I'm having trouble with this....
> > 
> > This bit makes sense. 4x 16bits = 64 bits.
> > 
> > > -static const u64 vhost_net_features[VIRTIO_FEATURES_DWORDS] = {
> > > +static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
> > 
> > If this was u16, and VIRTIO_FEATURES_QWORDS was 4, which the Q would
> > imply, than i would agree with what you are saying. But this is a u64
> > type.  It is already a QWORD, and this is an array of two of them.
> 
> I don't get what you are saying here.
> It's an array of qwords and VIRTIO_FEATURES_QWORDS tells you
> how many QWORDS are needed to fit all of them.
> 
> This is how C arrays are declared.
> 
> 
> > I think the real issue here is not D vs Q, but WORD. We have a default
> > meaning of a u16 for a word, especially in C. But that is not the
> > actual definition of a word a computer scientist would use. Wikipedia
> > has:
> > 
> >   In computing, a word is any processor design's natural unit of
> >   data. A word is a fixed-sized datum handled as a unit by the
> >   instruction set or the hardware of the processor.
> > 
> > A word can be any size. In this context, virtio is not referring to
> > the instruction set, but a protocol. Are all fields in this protocol
> > u64? Hence word is u64? And this is an array of two words? That would
> > make DWORD correct, it is two words.
> > 
> > If you want to change anything here, i would actually change WORD to
> > something else, maybe FIELD?
> > 
> > And i could be wrong here, i've not looked at the actual protocol, so
> > i've no idea if all fields in the protocol are u64. There are
> > protocols like this, IPv6 uses u32, not octets, and the length field
> > in the headers refer to the number of u32s in the header.
> > 
> > 	Andrew
> 
> 
> Virtio uses "dword" to mean "32 bits" in several places:

It also uses WORD to represent 32 bits:

void
vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,
				       u64 *features)
{
	struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
	int i;

	virtio_features_zero(features);
	for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
		u64 cur;

		vp_iowrite32(i, &cfg->guest_feature_select);
		cur = vp_ioread32(&cfg->guest_feature);
		features[i >> 1] |= cur << (32 * (i & 1));
	}
}

And this is a function dealing features. So this seems to suggest a
WORD is a u32, when dealing with features. A DWORD would thus be a
u64, making the current code correct.

As i said, the problem here is WORD. It means different things to
different people. And it even has different means to different parts
of the virtio code, as you pointed out.

If we want to change anything here, i suggest we change WORD to
something else, to try to avoid the problem that word could be a u16,
u32, or even a u42, depending on where it is used.

	Andrew

Re: [PATCH 1/3] virtio: dwords->qwords

[Michael S. Tsirkin]

On Sat, Oct 11, 2025 at 07:25:55PM +0200, Andrew Lunn wrote:
> On Thu, Oct 09, 2025 at 09:37:20AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Oct 09, 2025 at 02:31:04PM +0200, Andrew Lunn wrote:
> > > On Thu, Oct 09, 2025 at 07:24:08AM -0400, Michael S. Tsirkin wrote:
> > > > A "word" is 16 bit. 64 bit integers like virtio uses are not dwords,
> > > > they are actually qwords.
> > > 
> > > I'm having trouble with this....
> > > 
> > > This bit makes sense. 4x 16bits = 64 bits.
> > > 
> > > > -static const u64 vhost_net_features[VIRTIO_FEATURES_DWORDS] = {
> > > > +static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
> > > 
> > > If this was u16, and VIRTIO_FEATURES_QWORDS was 4, which the Q would
> > > imply, than i would agree with what you are saying. But this is a u64
> > > type.  It is already a QWORD, and this is an array of two of them.
> > 
> > I don't get what you are saying here.
> > It's an array of qwords and VIRTIO_FEATURES_QWORDS tells you
> > how many QWORDS are needed to fit all of them.
> > 
> > This is how C arrays are declared.
> > 
> > 
> > > I think the real issue here is not D vs Q, but WORD. We have a default
> > > meaning of a u16 for a word, especially in C. But that is not the
> > > actual definition of a word a computer scientist would use. Wikipedia
> > > has:
> > > 
> > >   In computing, a word is any processor design's natural unit of
> > >   data. A word is a fixed-sized datum handled as a unit by the
> > >   instruction set or the hardware of the processor.
> > > 
> > > A word can be any size. In this context, virtio is not referring to
> > > the instruction set, but a protocol. Are all fields in this protocol
> > > u64? Hence word is u64? And this is an array of two words? That would
> > > make DWORD correct, it is two words.
> > > 
> > > If you want to change anything here, i would actually change WORD to
> > > something else, maybe FIELD?
> > > 
> > > And i could be wrong here, i've not looked at the actual protocol, so
> > > i've no idea if all fields in the protocol are u64. There are
> > > protocols like this, IPv6 uses u32, not octets, and the length field
> > > in the headers refer to the number of u32s in the header.
> > > 
> > > 	Andrew
> > 
> > 
> > Virtio uses "dword" to mean "32 bits" in several places:
> 
> It also uses WORD to represent 32 bits:


That's not spec, that's linux driver. The spec is the source of truth.


> void
> vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,
> 				       u64 *features)
> {
> 	struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> 	int i;
> 
> 	virtio_features_zero(features);
> 	for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
> 		u64 cur;
> 
> 		vp_iowrite32(i, &cfg->guest_feature_select);
> 		cur = vp_ioread32(&cfg->guest_feature);
> 		features[i >> 1] |= cur << (32 * (i & 1));
> 	}
> }
> 
> And this is a function dealing features. So this seems to suggest a
> WORD is a u32, when dealing with features.

This is very recent with Paolo's patches/
That's exactly why my patches fix it.

> A DWORD would thus be a
> u64, making the current code correct.
> 
> As i said, the problem here is WORD. It means different things to
> different people. And it even has different means to different parts
> of the virtio code, as you pointed out.
>
>
> 
> If we want to change anything here, i suggest we change WORD to
> something else, to try to avoid the problem that word could be a u16,
> u32, or even a u42, depending on where it is used.
> 
> 	Andrew

Re: [PATCH 1/3] virtio: dwords->qwords

[Andrew Lunn]

> That's not spec, that's linux driver. The spec is the source of truth.

Right, lets follow this.

I'm looking at

https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html

Is that correct?

That document does not have a definition of word. However, what is
interesting is section "4.2.2 MMIO Device Register Layout"

DeviceFeaturesSel 0x014

Device (host) features word selection.
Writing to this register selects a set of 32 device feature bits accessible by reading from DeviceFeatures.

and

DriverFeaturesSel 0x024

Activated (guest) features word selection
Writing to this register selects a set of 32 activated feature bits accessible by writing to DriverFeatures.

I would interpret this as meaning a feature word is a u32. Hence a
DWORD is a u64, as the current code uses.

	Andrew

Re: [PATCH 1/3] virtio: dwords->qwords

[Michael S. Tsirkin]

On Sat, Oct 11, 2025 at 08:52:18PM +0200, Andrew Lunn wrote:
> > That's not spec, that's linux driver. The spec is the source of truth.
> 
> Right, lets follow this.
> 
> I'm looking at
> 
> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html
> 
> Is that correct?
> 
> That document does not have a definition of word. However, what is
> interesting is section "4.2.2 MMIO Device Register Layout"
> 
> DeviceFeaturesSel 0x014
> 
> Device (host) features word selection.
> Writing to this register selects a set of 32 device feature bits accessible by reading from DeviceFeatures.
> 
> and
> 
> DriverFeaturesSel 0x024
> 
> Activated (guest) features word selection
> Writing to this register selects a set of 32 activated feature bits accessible by writing to DriverFeatures.
> 
> I would interpret this as meaning a feature word is a u32. Hence a
> DWORD is a u64, as the current code uses.
> 
> 	Andrew


Hmm indeed.
At the same time, pci transport has:

         u8 padding[2];  /* Pad to full dword. */

and i2c has:

The \field{padding} is used to pad to full dword.

both of which use dword to mean 32 bit.

This comes from PCI which also does not define word but uses it
to mean 16 bit.




I don't have the problem changing everything to some other
wording completely but "chunk" is uninformative, and
more importantly does not give a clean way to refer to
2 chunks and 4 chunks.
Similarly, if we use "word" to mean 32 bit there is n clean
way to refer to 16 bits which we use a lot.


using word as 16 bit has the advantage that you
can say byte/word/dword/qword and these do not
cause too much confusion.


So I am still inclined to align everything on pci terminology
but interested to hear what alternative you suggest.


-- 
MST

Re: [PATCH 1/3] virtio: dwords->qwords

[Andrew Lunn]

> > DeviceFeaturesSel 0x014
> > 
> > Device (host) features word selection.
> > Writing to this register selects a set of 32 device feature bits accessible by reading from DeviceFeatures.
> > 
> > and
> > 
> > DriverFeaturesSel 0x024
> > 
> > Activated (guest) features word selection
> > Writing to this register selects a set of 32 activated feature bits accessible by writing to DriverFeatures.
> > 
> > I would interpret this as meaning a feature word is a u32. Hence a
> > DWORD is a u64, as the current code uses.
> > 
> > 	Andrew
> 
> 
> Hmm indeed.
> At the same time, pci transport has:
> 
>          u8 padding[2];  /* Pad to full dword. */
> 
> and i2c has:
> 
> The \field{padding} is used to pad to full dword.
> 
> both of which use dword to mean 32 bit.
> 
> This comes from PCI which also does not define word but uses it
> to mean 16 bit.

Yes, reading the spec, you need to consider the context 'word' is used
in. Maybe this is something which can be cleaned up, made uniform
across the whole document?

> I don't have the problem changing everything to some other
> wording completely but "chunk" is uninformative, and
> more importantly does not give a clean way to refer to
> 2 chunks and 4 chunks.
> Similarly, if we use "word" to mean 32 bit there is n clean
> way to refer to 16 bits which we use a lot.
> 
> 
> using word as 16 bit has the advantage that you
> can say byte/word/dword/qword and these do not
> cause too much confusion.

> So I am still inclined to align everything on pci terminology
> but interested to hear what alternative you suggest.

How about something simple:

#define VIRTIO_FEATURES_DU32WORDS	2
#define VIRTIO_FEATURES_U32WORDS	(VIRTIO_FEATURES_D32WORDS * 2)

or, if the spec moves away from using 'word':

#define VIRTIO_FEATURES_U64S	2
#define VIRTIO_FEATURES_U32S	(VIRTIO_FEATURES_U32S * 2)

The coding style says not to use Hungarian notation, but here it
actually make sense, and avoids the ambiguity in the spec.

	Andrew

Re: [PATCH 1/3] virtio: dwords->qwords

[Michael S. Tsirkin]

On Sun, Oct 12, 2025 at 04:39:06PM +0200, Andrew Lunn wrote:
> > > DeviceFeaturesSel 0x014
> > > 
> > > Device (host) features word selection.
> > > Writing to this register selects a set of 32 device feature bits accessible by reading from DeviceFeatures.
> > > 
> > > and
> > > 
> > > DriverFeaturesSel 0x024
> > > 
> > > Activated (guest) features word selection
> > > Writing to this register selects a set of 32 activated feature bits accessible by writing to DriverFeatures.
> > > 
> > > I would interpret this as meaning a feature word is a u32. Hence a
> > > DWORD is a u64, as the current code uses.
> > > 
> > > 	Andrew
> > 
> > 
> > Hmm indeed.
> > At the same time, pci transport has:
> > 
> >          u8 padding[2];  /* Pad to full dword. */
> > 
> > and i2c has:
> > 
> > The \field{padding} is used to pad to full dword.
> > 
> > both of which use dword to mean 32 bit.
> > 
> > This comes from PCI which also does not define word but uses it
> > to mean 16 bit.
> 
> Yes, reading the spec, you need to consider the context 'word' is used
> in. Maybe this is something which can be cleaned up, made uniform
> across the whole document?

Yes and thanks for bringing this to my attention.
So MMIO can be "features set selection"

And pci can be "pad to 32 bit".


Less work than defining "word".

> > I don't have the problem changing everything to some other
> > wording completely but "chunk" is uninformative, and
> > more importantly does not give a clean way to refer to
> > 2 chunks and 4 chunks.
> > Similarly, if we use "word" to mean 32 bit there is n clean
> > way to refer to 16 bits which we use a lot.
> > 
> > 
> > using word as 16 bit has the advantage that you
> > can say byte/word/dword/qword and these do not
> > cause too much confusion.
> 
> > So I am still inclined to align everything on pci terminology
> > but interested to hear what alternative you suggest.
> 
> How about something simple:
> 
> #define VIRTIO_FEATURES_DU32WORDS	2
> #define VIRTIO_FEATURES_U32WORDS	(VIRTIO_FEATURES_D32WORDS * 2)
> 
> or, if the spec moves away from using 'word':
> 
> #define VIRTIO_FEATURES_U64S	2
> #define VIRTIO_FEATURES_U32S	(VIRTIO_FEATURES_U32S * 2)
> 
> The coding style says not to use Hungarian notation, but here it
> actually make sense, and avoids the ambiguity in the spec.
> 
> 	Andrew

I mean we can just define number of bits. Open-code / 32 or / 64
as needed. Hmm?


-- 
MST

[PATCH 2/3] virtio: words->dwords

[Michael S. Tsirkin]

A "word" is 16 bit. 32 bit integers like virtio pci uses are not words,
they are actually dwords. Fix up the macro name accordingly.

Fixes: e7d4c1c5a546 ("virtio: introduce extended features")
Cc: "Paolo Abeni" <pabeni@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 6 +++---
 include/linux/virtio_features.h        | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index 9e503b7a58d8..4b619cc91bc7 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -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_DWORDS; 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_DWORDS; 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_DWORDS; i++) {
 		u32 cur = features[i >> 1] >> (32 * (i & 1));
 
 		vp_iowrite32(i, &cfg->guest_feature_select);
diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
index bf41d8ec50ef..f41acb035af9 100644
--- a/include/linux/virtio_features.h
+++ b/include/linux/virtio_features.h
@@ -6,7 +6,7 @@
 
 #define VIRTIO_FEATURES_QWORDS	2
 #define VIRTIO_FEATURES_MAX	(VIRTIO_FEATURES_QWORDS * 64)
-#define VIRTIO_FEATURES_WORDS	(VIRTIO_FEATURES_QWORDS * 2)
+#define VIRTIO_FEATURES_DWORDS	(VIRTIO_FEATURES_QWORDS * 2)
 #define VIRTIO_BIT(b)		BIT_ULL((b) & 0x3f)
 #define VIRTIO_QWORD(b)		((b) >> 6)
 #define VIRTIO_DECLARE_FEATURES(name)			\
-- 
MST

[PATCH 3/3] vhost: use checked versions of VIRTIO_BIT

[Michael S. Tsirkin]

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.

Cc: "Paolo Abeni" <pabeni@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/net.c             | 4 ++--
 include/linux/virtio_features.h | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 43d51fb1f8ea..8b98e1a8baaa 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -76,8 +76,8 @@ static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
 	(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 {
diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
index f41acb035af9..466f7d8ed5ba 100644
--- a/include/linux/virtio_features.h
+++ b/include/linux/virtio_features.h
@@ -9,6 +9,15 @@
 #define VIRTIO_FEATURES_DWORDS	(VIRTIO_FEATURES_QWORDS * 2)
 #define VIRTIO_BIT(b)		BIT_ULL((b) & 0x3f)
 #define VIRTIO_QWORD(b)		((b) >> 6)
+
+/* Get a given feature bit in a given qword. */
+#define VIRTIO_BIT_QWORD(bit, qword) \
+	(BUILD_BUG_ON_ZERO(const_true(VIRTIO_QWORD(bit) != (qword))) + \
+	 BIT_ULL((bit) - 64 * (qword)))
+
+#define VIRTIO_BIT_LO(b) VIRTIO_BIT_QWORD(b, 0)
+#define VIRTIO_BIT_HI(b) VIRTIO_BIT_QWORD(b, 1)
+
 #define VIRTIO_DECLARE_FEATURES(name)			\
 	union {						\
 		u64 name;				\
-- 
MST

Re: [PATCH 3/3] vhost: use checked versions of VIRTIO_BIT

[Andrew Lunn]

On Thu, Oct 09, 2025 at 07:24:16AM -0400, Michael S. Tsirkin wrote:
> 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.
> 
> Cc: "Paolo Abeni" <pabeni@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/vhost/net.c             | 4 ++--
>  include/linux/virtio_features.h | 9 +++++++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 43d51fb1f8ea..8b98e1a8baaa 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -76,8 +76,8 @@ static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
>  	(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),

How any bits in vhost_net_features are currently in use? How likely is
it to go from 2x 64bit words to 3x 64 bit words? Rather than _LO, _HI,
would _1ST, _2ND be better leaving it open for _3RD?

I would also be tempted to rename these macros to include _LO_ and
_HI_ in them. VIRTIO_BIT_HI(VIRTIO_LO_F_IN_ORDER) is more likely to be
spotted as wrong this way.

An alternative would be to convert to a linux bitmap, which is
arbitrary length so you just use bit number and leave the
implementation to map that to the correct offset in the underlying
data structure.

	Andrew

Re: [PATCH 3/3] vhost: use checked versions of VIRTIO_BIT

[Michael S. Tsirkin]

On Thu, Oct 09, 2025 at 02:47:53PM +0200, Andrew Lunn wrote:
> On Thu, Oct 09, 2025 at 07:24:16AM -0400, Michael S. Tsirkin wrote:
> > 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.
> > 
> > Cc: "Paolo Abeni" <pabeni@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/vhost/net.c             | 4 ++--
> >  include/linux/virtio_features.h | 9 +++++++++
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 43d51fb1f8ea..8b98e1a8baaa 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -76,8 +76,8 @@ static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = {
> >  	(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),
> 
> How any bits in vhost_net_features are currently in use?

68

> How likely is
> it to go from 2x 64bit words to 3x 64 bit words?

Maybe.

> Rather than _LO, _HI,
> would _1ST, _2ND be better leaving it open for _3RD?

I can just open-code

	VIRTIO_BIT_QWORD(VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO, 1)


> I would also be tempted to rename these macros to include _LO_ and
> _HI_ in them. VIRTIO_BIT_HI(VIRTIO_LO_F_IN_ORDER) is more likely to be
> spotted as wrong this way.

Hmm but with my macros compiler will warn so why uglify then?


> An alternative would be to convert to a linux bitmap, which is
> arbitrary length so you just use bit number and leave the
> implementation to map that to the correct offset in the underlying
> data structure.
> 
> 	Andrew

Right but it's a bit more work as we then have to change
all drivers. Not ruling this out, but this patchset
is not aiming that high.

-- 
MST