Skip to contentSkip to conversation

virtio_pci: fix macro exported in uapi

PATCH Superseded Michael S. Tsirkin 3 participants 5 messages May 16, 2013 – May 20, 2013

Subsystems (2)
KVM virtio core

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

PATCH 1/1 virtio_pci: fix macro exported in uapi

macro VIRTIO_PCI_CONFIG except in the unlikely event userspace actually has a structure with a field named msix_enabled. Get the msix_enabled by value instead, to make it useful for userspace.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/virtio/virtio_pci.c     | 8 ++++----
 include/uapi/linux/virtio_pci.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)
drivers/virtio/virtio_pci.c +4 -4
index a7ce730..1bb6ff3 100644
@@ -132,8 +132,8 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr + offset +
+				VIRTIO_PCI_CONFIG(vp_dev->msix_enabled);
 	u8 *ptr = buf;
 	int i;
 

@@ -147,8 +147,8 @@ static void vp_set(struct virtio_device *vdev, unsigned offset,
 		   const void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void __iomem *ioaddr = vp_dev->ioaddr +
-				VIRTIO_PCI_CONFIG(vp_dev) + offset;
+	void __iomem *ioaddr = vp_dev->ioaddr + offset +
+				VIRTIO_PCI_CONFIG(vp_dev->msix_enabled);
 	const u8 *ptr = buf;
 	int i;
include/uapi/linux/virtio_pci.h +1 -1
index ea66f3f..610c6c6 100644
@@ -80,7 +80,7 @@
 
 /* The remaining space is defined by each driver as the per-driver
  * configuration space */
-#define VIRTIO_PCI_CONFIG(dev)		((dev)->msix_enabled ? 24 : 20)
+#define VIRTIO_PCI_CONFIG(msix_enabled)	((msix_enabled) ? 24 : 20)
 
 /* Virtio ABI version, this must match exactly */
 #define VIRTIO_PCI_ABI_VERSION		0
Signature · 1 line
MST
#1 lore raw
Quoted 6 lines · Michael S. Tsirkin

macro VIRTIO_PCI_CONFIG except in the unlikely event userspace actually has a structure with a field named msix_enabled. Get the msix_enabled by value instead, to make it useful for userspace.

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

Macro still isn't usable, because userspace can't know whether it's the new or old.

We need to either remove it from UAPI, or rename it to VIRTIO_PCI_CONFIG_OFF.

Cheers,
Rusty.

#2 lore raw
Quoted 5 lines · Rusty Russell

Macro still isn't usable, because userspace can't know whether it's the new or old.

We need to either remove it from UAPI, or rename it to VIRTIO_PCI_CONFIG_OFF.

Surely, if userspace is using it as is, you can't remove it, rename it or alter it?

David

#3 lore raw
Quoted 10 lines · David Howells
Quoted 5 lines · Rusty Russell

Macro still isn't usable, because userspace can't know whether it's the new or old.

We need to either remove it from UAPI, or rename it to VIRTIO_PCI_CONFIG_OFF.

Surely, if userspace is using it as is, you can't remove it, rename it or alter it?

The point of the patch is that it's unusable:

#define VIRTIO_PCI_CONFIG(dev) ((dev)->msix_enabled ? 24 : 20)

ie. it's accessing a member of the kernel's virtio_pci_dev structure.

In theory, userspace could have a structure with the same field and be using it now, but that's unlikely (qemu certainly doesn't). No harm no foul.

In theory it's actually a useful macro, so we could expose it to userspace, but we'd need to have a new name so userspace can #ifdef for older headers...

Hope that clarifies,
Rusty.

#4 lore raw
Quoted 5 lines · Rusty Russell

The point of the patch is that it's unusable:

#define VIRTIO_PCI_CONFIG(dev) ((dev)->msix_enabled ? 24 : 20)

ie. it's accessing a member of the kernel's virtio_pci_dev structure.

Ah, okay. In that case, zap it and see if anyone complains.

David