Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Alexander Graf <agraf@suse.de>]

On 13.06.14 14:14, Greg Kurz wrote:
> On Fri, 13 Jun 2014 13:46:39 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 13.06.14 13:23, Greg Kurz wrote:
>>> Some CPU families can dynamically change their endianness. This means we
>>> can have little endian ppc or big endian arm guests for example. This has
>>> an impact on legacy virtio data structures since they are target endian.
>>> We hence introduce a new property to track the endianness of each virtio
>>> device. It is reasonnably assumed that endianness won't change while the
>>> device is in use : we hence capture the device endianness when it gets
>>> reset.
>>>
>>> We migrate this property in a subsection, after the device descriptor. This
>>> means the load code must not rely on it until it is restored. As a consequence,
>>> the vring sanity checks had to be moved after the call to vmstate_load_state().
>>> We enforce paranoia by poisoning the property at the begining of virtio_load().
>>>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>>
>>>    Changes since last version (virtio migration RFC):
>>>    - endianness cached at device reset time
>>>    - reworked migration support
>>>
>>>    hw/virtio/virtio-pci.c     |    8 ++--
>>>    hw/virtio/virtio.c         |  100 +++++++++++++++++++++++++++++++++++++++-----
>>>    include/hw/virtio/virtio.h |   12 ++++-
>>>    3 files changed, 102 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>> index 390c8d2..2ffceb8 100644
>>> --- a/hw/virtio/virtio-pci.c
>>> +++ b/hw/virtio/virtio-pci.c
>>> @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>>>            break;
>>>        case 2:
>>>            val = virtio_config_readw(vdev, addr);
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap16(val);
>>>            }
>>>            break;
>>>        case 4:
>>>            val = virtio_config_readl(vdev, addr);
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap32(val);
>>>            }
>>>            break;
>>> @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
>>>            virtio_config_writeb(vdev, addr, val);
>>>            break;
>>>        case 2:
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap16(val);
>>>            }
>>>            virtio_config_writew(vdev, addr, val);
>>>            break;
>>>        case 4:
>>> -        if (virtio_is_big_endian()) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>>                val = bswap32(val);
>>>            }
>>>            virtio_config_writel(vdev, addr, val);
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 16b73d9..6235df8 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>>        vdev->status = val;
>>>    }
>>>    
>>> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
>>> +{
>>> +#ifdef TARGET_WORDS_BIGENDIAN
>>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
>>> +#else
>>> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +#endif
>>> +}
>>> +
>>> +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
>>> +{
>>> +    if (cpu_virtio_is_big_endian(cpu)) {
>>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
>>> +    } else {
>>> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +    }
>>> +}
>>> +
>>>    void virtio_reset(void *opaque)
>>>    {
>>>        VirtIODevice *vdev = opaque;
>>> @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
>>>        int i;
>>>    
>>>        virtio_set_status(vdev, 0);
>>> +    if (current_cpu) {
>>> +        /* Guest initiated reset */
>>> +        virtio_set_endian_cpu(vdev, current_cpu);
>>> +    } else {
>>> +        /* System reset */
>>> +        virtio_set_endian_target_default(vdev);
>>> +    }
>>>    
>>>        if (k->reset) {
>>>            k->reset(vdev);
>>> @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
>>>        virtio_notify_vector(vdev, vdev->config_vector);
>>>    }
>>>    
>>> +static bool virtio_device_endian_needed(void *opaque)
>>> +{
>>> +    VirtIODevice *vdev = opaque;
>>> +
>>> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
>>> +#ifdef TARGET_WORDS_BIGENDIAN
>>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>>> +#else
>>> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>>> +#endif
>> if (target_words_bigendian()) {
>>       return virtio_is_big_endian();
>> } else {
>>       return !virtio_is_big_endian();
>> }
>>
>> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a
>> step towards compiling virtio once ;).
>>
>>
>> Alex
>>
> Heh ! It used to be target_words_bigendian() in the first place and I turned
> it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common
> why don't you sort this out at compile time?" remarks. :)
>
> Since we are definitely not on a fastpath here, I'll gladly fix this.

Yeah, just combine it with virtio_set_endian_target_default.

static enum foo virtio_default_endian(vdev)
{
     if (target_words_bigendian()) {
         return VIRTIO_DEVICE_ENDIAN_BIG;
     ....
}

Instead of calling virtio_set_endian_target_default, you do

     vdev->device_endian = virtio_default_endian(vdev);

and

static bool virtio_biendian_section_needed(vdev)
{
     assert(unknown);
     return vdev->device_endian != virtio_default_endian(vdev);
}


Alex