feature related cleanups

A "word" is 16 bit. 64 bit integers like virtio uses are not dwords, they are actually qwords.

Quoted 2 lines · Michael S. Tsirkin

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.

drivers/vhost/net.c MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;" · line 72

Quoted 2 lines

drivers/vhost/net.c +1 -1

-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

Quoted 6 lines · Andrew Lunn

Quoted 2 lines · Michael S. Tsirkin

A "word" is 16 bit. 64 bit integers like virtio uses are not dwords, they are actually qwords.

I'm having trouble with this....

Quoted 8 lines

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

drivers/vhost/net.c MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;" · line 72

Quoted 2 lines

drivers/vhost/net.c +1 -1

-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.

Quoted 22 lines

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.

Quoted 1 line

Andrew

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

Quoted 45 lines · Michael S. Tsirkin

Quoted 6 lines · Andrew Lunn

Quoted 2 lines · Michael S. Tsirkin

A "word" is 16 bit. 64 bit integers like virtio uses are not dwords, they are actually qwords.

I'm having trouble with this....

Quoted 8 lines

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

drivers/vhost/net.c MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;" · line 72

Quoted 2 lines

drivers/vhost/net.c +1 -1

-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.

Quoted 22 lines

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.

Quoted 4 lines

Quoted 1 line

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

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

Quoted 1 line

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

Quoted 9 lines

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

Quoted 7 lines

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.

Quoted 27 lines

Quoted 9 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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?

Quoted 8 lines

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.

Quoted 6 lines

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

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" <[email protected]>

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

---
 drivers/vhost/net.c             | 4 ++--
 include/linux/virtio_features.h | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

drivers/vhost/net.c +0 -0

index 43d51fb1f8ea..8b98e1a8baaa 100644

Quoted 11 lines · 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" <[email protected]>

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

---
 drivers/vhost/net.c             | 4 ++--
 include/linux/virtio_features.h | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

drivers/vhost/net.c static const u64 vhost_net_features[VIRTIO_FEATURES_QWORDS] = { · line 80

Quoted 12 lines

drivers/vhost/net.c +2 -2

index 43d51fb1f8ea..8b98e1a8baaa 100644
@@ -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