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

[Farhan Ali]

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