virtio-mmio registers are always little endian even on machines
where peripheral registers are normally big endian.
At least on nommu, non-coldfire, m68k (see: arch/m68k/include/asm/io_no.h)
readl() and friends are defined to the raw versions. So the value from
the bus is returned as is.
Since virtio-mmio is a bit special and will always be little endian
even if that is different to the machine's convention use the raw
accessors and do the endian swap, where needed, in house.
There are some places in the code where the little endian values are
actually wanted and we currently swap those back again. If we don't
swap them in the first place maybe that saves some unneeded swapping?
Signed-off-by: Daniel Palmer <daniel@thingy.jp>
---
drivers/virtio/virtio_mmio.c | 120 +++++++++++++++++++----------------
1 file changed, 64 insertions(+), 56 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index dda915b2ac9f..4050533cc976 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -70,7 +70,14 @@
#include <uapi/linux/virtio_mmio.h>
#include <linux/virtio_ring.h>
-
+/*
+ * The virtio registers are always little endian, even on machines
+ * where everything else is big endian. readl()/write() could use the
+ * value from the bus as-is or endian swap it from little endian.
+ * To avoid confusion use the raw accessors and do the swap here.
+ */
+#define virtio_mmio_readl(addr) le32_to_cpu(__raw_readl(addr))
+#define virtio_mmio_writel(value, addr) __raw_writel(cpu_to_le32(value), addr)
/* The alignment to use between consumer and producer parts of vring.
* Currently hardcoded to the page size. */
@@ -96,12 +103,12 @@ static u64 vm_get_features(struct virtio_device *vdev)
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
u64 features;
- writel(1, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
- features = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
+ virtio_mmio_writel(1, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
features <<= 32;
- writel(0, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
- features |= readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features |= virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
return features;
}
@@ -120,12 +127,12 @@ static int vm_finalize_features(struct virtio_device *vdev)
return -EINVAL;
}
- writel(1, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
- writel((u32)(vdev->features >> 32),
+ virtio_mmio_writel(1, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ virtio_mmio_writel((u32)(vdev->features >> 32),
vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
- writel(0, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
- writel((u32)vdev->features,
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ virtio_mmio_writel((u32)vdev->features,
vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
return 0;
@@ -155,17 +162,17 @@ static void vm_get(struct virtio_device *vdev, unsigned int offset,
memcpy(buf, &b, sizeof(b));
break;
case 2:
- w = cpu_to_le16(readw(base + offset));
+ w = __raw_readw(base + offset);
memcpy(buf, &w, sizeof(w));
break;
case 4:
- l = cpu_to_le32(readl(base + offset));
+ l = __raw_readl(base + offset);
memcpy(buf, &l, sizeof(l));
break;
case 8:
- l = cpu_to_le32(readl(base + offset));
+ l = __raw_readl(base + offset);
memcpy(buf, &l, sizeof(l));
- l = cpu_to_le32(ioread32(base + offset + sizeof(l)));
+ l = __raw_readl(base + offset + sizeof(l));
memcpy(buf + sizeof(l), &l, sizeof(l));
break;
default:
@@ -199,17 +206,17 @@ static void vm_set(struct virtio_device *vdev, unsigned int offset,
break;
case 2:
memcpy(&w, buf, sizeof(w));
- writew(le16_to_cpu(w), base + offset);
+ __raw_writew(w, base + offset);
break;
case 4:
memcpy(&l, buf, sizeof(l));
- writel(le32_to_cpu(l), base + offset);
+ __raw_writel(l, base + offset);
break;
case 8:
memcpy(&l, buf, sizeof(l));
- writel(le32_to_cpu(l), base + offset);
+ __raw_writel(l, base + offset);
memcpy(&l, buf + sizeof(l), sizeof(l));
- writel(le32_to_cpu(l), base + offset + sizeof(l));
+ __raw_writel(l, base + offset + sizeof(l));
break;
default:
BUG();
@@ -223,14 +230,14 @@ static u32 vm_generation(struct virtio_device *vdev)
if (vm_dev->version == 1)
return 0;
else
- return readl(vm_dev->base + VIRTIO_MMIO_CONFIG_GENERATION);
+ return virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_CONFIG_GENERATION);
}
static u8 vm_get_status(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
- return readl(vm_dev->base + VIRTIO_MMIO_STATUS) & 0xff;
+ return virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_STATUS) & 0xff;
}
static void vm_set_status(struct virtio_device *vdev, u8 status)
@@ -245,7 +252,7 @@ static void vm_set_status(struct virtio_device *vdev, u8 status)
* that the cache coherent memory writes have completed
* before writing to the MMIO region.
*/
- writel(status, vm_dev->base + VIRTIO_MMIO_STATUS);
+ virtio_mmio_writel(status, vm_dev->base + VIRTIO_MMIO_STATUS);
}
static void vm_reset(struct virtio_device *vdev)
@@ -253,7 +260,7 @@ static void vm_reset(struct virtio_device *vdev)
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
/* 0 status means a reset. */
- writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
}
@@ -267,7 +274,7 @@ static bool vm_notify(struct virtqueue *vq)
/* We write the queue's selector into the notification register to
* signal the other end */
- writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+ virtio_mmio_writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
return true;
}
@@ -276,7 +283,7 @@ static bool vm_notify_with_data(struct virtqueue *vq)
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
u32 data = vring_notification_data(vq);
- writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+ virtio_mmio_writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
return true;
}
@@ -290,8 +297,8 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
irqreturn_t ret = IRQ_NONE;
/* Read and acknowledge interrupts */
- status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
- writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);
+ status = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
+ virtio_mmio_writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);
if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
virtio_config_changed(&vm_dev->vdev);
@@ -314,12 +321,12 @@ static void vm_del_vq(struct virtqueue *vq)
unsigned int index = vq->index;
/* Select and deactivate the queue */
- writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+ virtio_mmio_writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
if (vm_dev->version == 1) {
- writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
} else {
- writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
- WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+ WARN_ON(virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
}
vring_del_virtqueue(vq);
@@ -362,16 +369,17 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
return NULL;
/* Select the queue we're interested in */
- writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+ virtio_mmio_writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
/* Queue shouldn't already be set up. */
- if (readl(vm_dev->base + (vm_dev->version == 1 ?
+ if (virtio_mmio_readl(vm_dev->base + (vm_dev->version == 1 ?
VIRTIO_MMIO_QUEUE_PFN : VIRTIO_MMIO_QUEUE_READY))) {
err = -ENOENT;
goto error_available;
}
- num = readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);
+ num = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);
+
if (num == 0) {
err = -ENOENT;
goto error_new_virtqueue;
@@ -388,7 +396,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
vq->num_max = num;
/* Activate the queue */
- writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
+ virtio_mmio_writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
if (vm_dev->version == 1) {
u64 q_pfn = virtqueue_get_desc_addr(vq) >> PAGE_SHIFT;
@@ -405,27 +413,27 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
goto error_bad_pfn;
}
- writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
- writel(q_pfn, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+ virtio_mmio_writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
+ virtio_mmio_writel(q_pfn, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
} else {
u64 addr;
addr = virtqueue_get_desc_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
- writel((u32)(addr >> 32),
+ virtio_mmio_writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
+ virtio_mmio_writel((u32)(addr >> 32),
vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
addr = virtqueue_get_avail_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
- writel((u32)(addr >> 32),
+ virtio_mmio_writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
+ virtio_mmio_writel((u32)(addr >> 32),
vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
addr = virtqueue_get_used_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
- writel((u32)(addr >> 32),
+ virtio_mmio_writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
+ virtio_mmio_writel((u32)(addr >> 32),
vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
- writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+ virtio_mmio_writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
}
return vq;
@@ -434,10 +442,10 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
vring_del_virtqueue(vq);
error_new_virtqueue:
if (vm_dev->version == 1) {
- writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
} else {
- writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
- WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
+ virtio_mmio_writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+ WARN_ON(virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
}
error_available:
return ERR_PTR(err);
@@ -496,11 +504,11 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
u64 len, addr;
/* Select the region we're interested in */
- writel(id, vm_dev->base + VIRTIO_MMIO_SHM_SEL);
+ virtio_mmio_writel(id, vm_dev->base + VIRTIO_MMIO_SHM_SEL);
/* Read the region size */
- len = (u64) readl(vm_dev->base + VIRTIO_MMIO_SHM_LEN_LOW);
- len |= (u64) readl(vm_dev->base + VIRTIO_MMIO_SHM_LEN_HIGH) << 32;
+ len = (u64) virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_SHM_LEN_LOW);
+ len |= (u64) virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_SHM_LEN_HIGH) << 32;
region->len = len;
@@ -511,8 +519,8 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
return false;
/* Read the region base address */
- addr = (u64) readl(vm_dev->base + VIRTIO_MMIO_SHM_BASE_LOW);
- addr |= (u64) readl(vm_dev->base + VIRTIO_MMIO_SHM_BASE_HIGH) << 32;
+ addr = (u64) virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_SHM_BASE_LOW);
+ addr |= (u64) virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_SHM_BASE_HIGH) << 32;
region->addr = addr;
@@ -548,7 +556,7 @@ static int virtio_mmio_restore(struct device *dev)
struct virtio_mmio_device *vm_dev = dev_get_drvdata(dev);
if (vm_dev->version == 1)
- writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+ virtio_mmio_writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
return virtio_device_restore(&vm_dev->vdev);
}
@@ -591,7 +599,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
}
/* Check magic value */
- magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
+ magic = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
rc = -ENODEV;
@@ -599,7 +607,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
}
/* Check device version */
- vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
+ vm_dev->version = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_VERSION);
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
@@ -607,7 +615,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
goto free_vm_dev;
}
- vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
+ vm_dev->vdev.id.device = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
if (vm_dev->vdev.id.device == 0) {
/*
* virtio-mmio device with an ID 0 is a (dummy) placeholder
@@ -616,10 +624,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
rc = -ENODEV;
goto free_vm_dev;
}
- vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
+ vm_dev->vdev.id.vendor = virtio_mmio_readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
if (vm_dev->version == 1) {
- writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+ virtio_mmio_writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
rc = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
/*
--
2.51.0On Sat, Mar 14, 2026, at 04:06, Daniel Palmer wrote: > virtio-mmio registers are always little endian even on machines > where peripheral registers are normally big endian. > > At least on nommu, non-coldfire, m68k (see: arch/m68k/include/asm/io_no.h) > readl() and friends are defined to the raw versions. So the value from > the bus is returned as is. > > Since virtio-mmio is a bit special and will always be little endian > even if that is different to the machine's convention use the raw > accessors and do the endian swap, where needed, in house. > > There are some places in the code where the little endian values are > actually wanted and we currently swap those back again. If we don't > swap them in the first place maybe that saves some unneeded swapping? > > Signed-off-by: Daniel Palmer <daniel@thingy.jp> I don't think this is safe to do in portable code > - > +/* > + * The virtio registers are always little endian, even on machines > + * where everything else is big endian. readl()/write() could use the > + * value from the bus as-is or endian swap it from little endian. > + * To avoid confusion use the raw accessors and do the swap here. > + */ > +#define virtio_mmio_readl(addr) le32_to_cpu(__raw_readl(addr)) > +#define virtio_mmio_writel(value, addr) __raw_writel(cpu_to_le32(value), addr) The __raw accessors are not well-defined across architectures and don't technically guarantee anything about the access beyond being usable as part of a memory access to byte data behind a bridge. In particular using these for MMIO registers (real or emulated) misses the bits about - atomicity of the access - emulating them from virtualization host - actual endianess - ordering against memory or MMIO accesses I think a better approach would be to change the dragonball specific device drivers to just use iowrite32_be/ioread32_be instead of writel/readl, and make the behavior consistent with the rest of Linux (including io_mm.h and coldfire), using the generic definition: diff --git a/arch/m68k/include/asm/io_no.h b/arch/m68k/include/asm/io_no.h index 516371d5587a..c39db8798ef2 100644 --- a/arch/m68k/include/asm/io_no.h +++ b/arch/m68k/include/asm/io_no.h @@ -96,15 +96,6 @@ static inline void writel(u32 value, volatile void __iomem *addr) __raw_writel(swab32(value), addr); } -#else - -#define readb __raw_readb -#define readw __raw_readw -#define readl __raw_readl -#define writeb __raw_writeb -#define writew __raw_writew -#define writel __raw_writel - #endif /* IOMEMBASE */ #if defined(CONFIG_PCI) Can you try if this works? Arnd
Hi Arnd, On Sat, 14 Mar 2026 at 16:42, Arnd Bergmann <arnd@kernel.org> wrote: > I don't think this is safe to do in portable code :( > diff --git a/arch/m68k/include/asm/io_no.h b/arch/m68k/include/asm/io_no.h > index 516371d5587a..c39db8798ef2 100644 > --- a/arch/m68k/include/asm/io_no.h > +++ b/arch/m68k/include/asm/io_no.h > @@ -96,15 +96,6 @@ static inline void writel(u32 value, volatile void __iomem *addr) > __raw_writel(swab32(value), addr); > } > > -#else > - > -#define readb __raw_readb > -#define readw __raw_readw > -#define readl __raw_readl > -#define writeb __raw_writeb > -#define writew __raw_writew > -#define writel __raw_writel > - > #endif /* IOMEMBASE */ > > #if defined(CONFIG_PCI) > > Can you try if this works? This builds but doesn't boot on my devicetree'd dragonball branch. I don't think it's possible to boot the stuff that is in mainline (and you wouldn't know if it booted or not due to the lack of a serial driver). I think I can fix my stuff so it works with the changes in readl() etc but I think there are a few other people that have their own 68000 trees for weird custom boards and I guess this might break their stuff. Not saying we should force a change onto virtio-mmio because of weird hobby projects. Not sure what to do. Thanks, Daniel
© 2016 - 2026 Red Hat, Inc.