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

[Alexander Graf]

On 10.01.2012, at 23:02, Anthony Liguori wrote:

> On 01/10/2012 05:35 AM, 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.
>> 
>> 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
> 
> I think this is the only reasonable way to do it, but I'd suggest adding 
> target_is_bigendian() to arch_init.c that way we wouldn't have to move the 
> object from libhw.

No. Libhw shouldn't be able to know anything about target endianness. If a 
device is as brokenly spec'ed as virtio and is coupled to the "main CPU 
endianness", it clearly belongs with the CPU, not into libhw.


Alex