Re: [Qemu-devel] [PATCH v2 1/2] virtio-gpu: Handle endian conversion

[Farhan Ali <alifm@linux.vnet.ibm.com>]

On 09/15/2017 03:31 AM, Gerd Hoffmann wrote:
>   Hi,
>
>> +static void
>> +virtio_gpu_ctrl_hdr_bswap_cpu(struct virtio_gpu_ctrl_hdr *hdr)
>> +{
>> +    hdr->type =  le32_to_cpu(hdr->type);
>> +    hdr->flags = le32_to_cpu(hdr->flags);
>> +    hdr->fence_id = le64_to_cpu(hdr->fence_id);
>> +    hdr->ctx_id = le32_to_cpu(hdr->ctx_id);
>> +    hdr->padding = le32_to_cpu(hdr->padding);
>> +}
>> +
>> +static void
>> +virtio_gpu_ctrl_hdr_bswap_le(struct virtio_gpu_ctrl_hdr *hdr)
>> +{
>> +    hdr->type =  cpu_to_le32(hdr->type);
>> +    hdr->flags = cpu_to_le32(hdr->flags);
>> +    hdr->fence_id = cpu_to_le64(hdr->fence_id);
>> +    hdr->ctx_id = cpu_to_le32(hdr->ctx_id);
>> +    hdr->padding = cpu_to_le32(hdr->padding);
>> +}
>
> These two functions do exactly the same thing because cpu_to_le32 and
> le32_to_cpu are the same operation.  The two variants only exist to
> make the code more readable.  So this isn't needed twice.  I'd suggest
> to just call the functions "*_bswap" and drop any _le or _cpu postfix.
> Note that there are also variants for in-place swapping.
>
> Have a look at the comments in include/qemu/bswap.h
>
Thanks for pointing that out. I will re-spin another version.

>> +
>> +static void virtio_gpu_bswap_cpu_32(void *ptr,
>> +                                    size_t size)
>> +{
>
> #ifdef HOST_WORDS_BIGENDIAN
>
>> +    size_t i;
>> +    struct virtio_gpu_ctrl_hdr *hdr = (struct virtio_gpu_ctrl_hdr *)
>> ptr;
>> +
>> +    virtio_gpu_ctrl_hdr_bswap_cpu(hdr);
>> +
>> +    i = sizeof(struct virtio_gpu_ctrl_hdr);
>> +    while (i < size) {
>> +        *(uint32_t *)(ptr + i) = le32_to_cpu(*(uint32_t *)(ptr +
>> i));
>> +        i = i + sizeof(uint32_t);
>> +    }
>
> #endif
>
> I suspect the compiler isn't clever enough to figure the whole function
> is a nop on little endian hosts.

This makes sense.

Thanks
Farhan
>
> cheers,
>   Gerd
>