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

[Gerd Hoffmann]

  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

> +
> +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.

cheers,
  Gerd