Re: [Qemu-devel] [PATCH] virtio-pci: replace byte swap hack

[Blue Swirl]

On Sun, Jan 6, 2013 at 1:17 PM, Alexander Graf <address@hidden> wrote:
>
> On 30.12.2012, at 13:55, Blue Swirl wrote:
>
>> Remove byte swaps by declaring the config space
>> as native endian.
>
> This is wrong. Virtio-pci config space is split into 2 regions. One with 
> native endianness, the other one with little endian.

True. I must say that this is poor architectural design in multiple
ways. The endianness problem is aggravated by device being also
instantiated by other devices, so we can't (at least easily) pass an
endianness flag from above, or make two devices that are selected by
the board. Another problem is that the offset changes dynamically
depending on if MSIX is enabled or not. The design is probably also
fixed by the guest driver assumptions and can't ever be improved.
Paravirtual devices should have cleaner design than what for example
original PC devices have, not more insane. :-(

Anyway, the real fix is that the memory region should contain two
subregions, native and LE. The offset of the second region should be
adjusted if MSIX bit is toggled. This may need some kind of
(nontrivial) callback system.

The original design could be improved to avoid the checks and calls to
virtio_is_big_endian() from the config handler by introducing two sets
of config functions (LE+LE, BE+LE) and selecting the correct one at
init time (based on result of virtio_is_big_endian() which does not
change). This would not help the real fix.

Both need more thought and hopefully there are even better options, so
I'll revert the commit.

>
> Alex
>
>>
>> Signed-off-by: Blue Swirl <address@hidden>
>> ---
>> exec.c          |   18 ------------------
>> hw/virtio-pci.c |   17 +----------------
>> 2 files changed, 1 insertions(+), 34 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index a6923ad..140eb56 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2587,24 +2587,6 @@ int cpu_memory_rw_debug(CPUArchState *env, 
>> target_ulong addr,
>> }
>> #endif
>>
>> -#if !defined(CONFIG_USER_ONLY)
>> -
>> -/*
>> - * A helper function for the _utterly broken_ virtio device model to find 
>> out if
>> - * it's running on a big endian machine. Don't do this at home kids!
>> - */
>> -bool virtio_is_big_endian(void);
>> -bool virtio_is_big_endian(void)
>> -{
>> -#if defined(TARGET_WORDS_BIGENDIAN)
>> -    return true;
>> -#else
>> -    return false;
>> -#endif
>> -}
>> -
>> -#endif
>> -
>> #ifndef CONFIG_USER_ONLY
>> bool cpu_physical_memory_is_io(hwaddr phys_addr)
>> {
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index d2d2454..df78a3b 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -92,9 +92,6 @@
>>  */
>> #define wmb() do { } while (0)
>>
>> -/* HACK for virtio to determine if it's running a big endian guest */
>> -bool virtio_is_big_endian(void);
>> -
>> /* virtio device */
>>
>> static void virtio_pci_notify(void *opaque, uint16_t vector)
>> @@ -390,15 +387,9 @@ static uint64_t virtio_pci_config_read(void *opaque, 
>> hwaddr addr,
>>         break;
>>     case 2:
>>         val = virtio_config_readw(proxy->vdev, addr);
>> -        if (virtio_is_big_endian()) {
>> -            val = bswap16(val);
>> -        }
>>         break;
>>     case 4:
>>         val = virtio_config_readl(proxy->vdev, addr);
>> -        if (virtio_is_big_endian()) {
>> -            val = bswap32(val);
>> -        }
>>         break;
>>     }
>>     return val;
>> @@ -423,15 +414,9 @@ static void virtio_pci_config_write(void *opaque, 
>> hwaddr addr,
>>         virtio_config_writeb(proxy->vdev, addr, val);
>>         break;
>>     case 2:
>> -        if (virtio_is_big_endian()) {
>> -            val = bswap16(val);
>> -        }
>>         virtio_config_writew(proxy->vdev, addr, val);
>>         break;
>>     case 4:
>> -        if (virtio_is_big_endian()) {
>> -            val = bswap32(val);
>> -        }
>>         virtio_config_writel(proxy->vdev, addr, val);
>>         break;
>>     }
>> @@ -444,7 +429,7 @@ static const MemoryRegionOps virtio_pci_config_ops = {
>>         .min_access_size = 1,
>>         .max_access_size = 4,
>>     },
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> };
>>
>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>> --
>> 1.7.2.5
>>
>>
>