Virtualization - Feb 2013 - [RFC PATCH] virt_mmio: fix signature checking for BE guests

Using readl() to read the magic value and then memcmp() to check it
fails on BE, as bytes will be the other way around (by virtue of
the registers to follow the endianess of the guest).

Fix it by encoding the magic as an integer instead of a string.

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: Pawel Moll <pawel.moll at arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
---
So I'm not completely sure this is the right fix, and I can imagine
other ways to cure the problem:
- Reading the MAGIC register byte by byte. Is that allowed? The spec
  only says it is 32bit wide.
- Using __raw_readl() instead. Is that a generic enough API?

 drivers/virtio/virtio_mmio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 31f966f..3811e94 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -470,7 +470,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 
 	/* Check magic value */
 	magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
-	if (memcmp(&magic, "virt", 4) != 0) {
+	if (magic != ('v' | 'i' << 8 | 'r' << 16 |
't' << 24)) {
 		dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
 		return -ENODEV;
 	}
-- 
1.8.1.2

On Wed, 2013-02-13 at 14:25 +0000, Marc Zyngier wrote:
> Using readl() to read the magic value and then memcmp() to check it
> fails on BE, as bytes will be the other way around (by virtue of
> the registers to follow the endianess of the guest).
Hm. Interesting. I missed the fact that readl() as a "PCI operation"
will always assume LE values...
> Fix it by encoding the magic as an integer instead of a string.
> So I'm not completely sure this is the right fix, 
It seems right, however...
> - Using __raw_readl() instead. Is that a generic enough API?
> 
... this implies that either the spec is wrong (as it should say: the
device registers are always LE, in the PCI spirit) or all readl()s & co.
should be replaced with __raw equivalents.

Having said that, does the change make everything else work with a BE
guest? (I assume we're talking about the guest being BE, right? ;-) If
so it means that the host is not following the current spec and it
treats all the registers as LE.
> - Reading the MAGIC register byte by byte. Is that allowed? The spec
>   only says it is 32bit wide.
And the spirit of the spec was: _exactly 32bit wide_. It's just simpler
to implement one access width on the host side.

Pawe?