Re: [Qemu-devel] [PATCH] virtio-pci: Fix endianness of virtio config

[Alexander Graf]

On 10.01.2012, at 12:35, Benjamin Herrenschmidt wrote:

> The virtio config area in PIO space is a bit special. The initial
> header is little endian but the rest (device specific) is guest
> native endian.
> 
> The PIO accessors for PCI on machines that don't have native IO ports
> assume that all PIO is little endian, which works fine for everything
> except the above.
> 
> A complicated way to fix it would be to split the BAR into two memory
> regions with different endianess settings, but this isn't practical
> to do, besides, the PIO code doesn't honor region endianness anyway
> (I have a patch for that too but it isn't necessary at this stage).
> 
> So I decided to go for the quick fix instead which consists of
> reverting the swap in virtio-pci in selected places, hoping that when
> we eventually do a "v2" of the virtio protocols, we sort that out once
> and for all using a fixed endian setting for everything.
> 
> Unfortunately, that mean moving virtio-pci from Makefile.objs to
> Makefile.target so we can use TARGET_WORDS_BIGENDIAN which would
> otherwise be poisoned.

I just tried to run -net nic,model=virtio with the bamboo target, after I 
verified that e1000 works. Without your patch, it works. With your patch, it 
also works.

What exactly is the effect you're seeing without the swap?

Does emulating an e1000 work for you? Maybe the RTAS callbacks really want you 
to return stuff in little endian?

---

Without your patch:

00:01.0 Class 0200: Device 1af4:1000
        Subsystem: Device 1af4:0001
        Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
        Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin A routed to IRQ 28
        Region 0: I/O ports at 1000 [size=32]
        [virtual] Expansion ROM at a0000000 [disabled] [size=64K]
        Kernel driver in use: virtio-pci
00: f4 1a 00 10 05 00 00 00 00 00 00 02 00 00 00 00
10: 01 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 f4 1a 01 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00

With your patch:

00:01.0 Class 0200: Device 1af4:1000
        Subsystem: Device 1af4:0001
        Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
        Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin A routed to IRQ 28
        Region 0: I/O ports at 1000 [size=32]
        [virtual] Expansion ROM at a0000000 [disabled] [size=64K]
        Kernel driver in use: virtio-pci
00: f4 1a 00 10 05 00 00 00 00 00 00 02 00 00 00 00
10: 01 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 f4 1a 01 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00


Alex

> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> ---
> Makefile.objs   |    1 -
> Makefile.target |    1 +
> hw/virtio-pci.c |   24 ++++++++++++++++++++++--
> 3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 4f6d26c..b721fca 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -186,7 +186,6 @@ hw-obj-y =
> hw-obj-y += vl.o loader.o
> hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
> hw-obj-y += usb-libhw.o
> -hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> hw-obj-y += fw_cfg.o
> hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
> hw-obj-$(CONFIG_PCI) += msix.o msi.o
> diff --git a/Makefile.target b/Makefile.target
> index ef6834b..03d44c3 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -191,6 +191,7 @@ obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o 
> balloon.o ioport.o
> # need to fix this properly
> obj-$(CONFIG_NO_PCI) += pci-stub.o
> obj-$(CONFIG_VIRTIO) += virtio.o virtio-blk.o virtio-balloon.o virtio-net.o 
> virtio-serial-bus.o
> +obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> obj-y += vhost_net.o
> obj-$(CONFIG_VHOST_NET) += vhost.o
> obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/virtio-9p-device.o
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 77b75bc..ca70e42 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -412,20 +412,34 @@ static uint32_t virtio_pci_config_readw(void *opaque, 
> uint32_t addr)
> {
>     VirtIOPCIProxy *proxy = opaque;
>     uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev);
> +    uint16_t val;
>     if (addr < config)
>         return virtio_ioport_read(proxy, addr);
>     addr -= config;
> -    return virtio_config_readw(proxy->vdev, addr);
> +    val = virtio_config_readw(proxy->vdev, addr);
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    /* virtio is odd, ioports are LE but config space is target native
> +     * endian. However, in qemu, all PIO is LE, so we need to re-swap
> +     * on BE targets
> +     */
> +    val = bswap16(val);
> +#endif
> +    return val;
> }
> 
> static uint32_t virtio_pci_config_readl(void *opaque, uint32_t addr)
> {
>     VirtIOPCIProxy *proxy = opaque;
>     uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev);
> +    uint32_t val;
>     if (addr < config)
>         return virtio_ioport_read(proxy, addr);
>     addr -= config;
> -    return virtio_config_readl(proxy->vdev, addr);
> +    val = virtio_config_readl(proxy->vdev, addr);
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif
> +    return val;
> }
> 
> static void virtio_pci_config_writeb(void *opaque, uint32_t addr, uint32_t 
> val)
> @@ -449,6 +463,9 @@ static void virtio_pci_config_writew(void *opaque, 
> uint32_t addr, uint32_t val)
>         return;
>     }
>     addr -= config;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap16(val);
> +#endif
>     virtio_config_writew(proxy->vdev, addr, val);
> }
> 
> @@ -461,6 +478,9 @@ static void virtio_pci_config_writel(void *opaque, 
> uint32_t addr, uint32_t val)
>         return;
>     }
>     addr -= config;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif
>     virtio_config_writel(proxy->vdev, addr, val);
> }
> 
> 
>