Using "-device virtio-gpu,blob=true" currently does not work on big
endian hosts (like s390x). The guest kernel prints an error message
like:

 [drm:virtio_gpu_dequeue_ctrl_func [virtio_gpu]] *ERROR* response 0x1200 (command 0x10c)

and the display stays black. When running QEMU with "-d guest_errors",
it shows an error message like this:

 virtio_gpu_create_mapping_iov: nr_entries is too big (83886080 > 16384)

which indicates that this value has not been properly byte-swapped.
And indeed, the virtio_gpu_create_blob_bswap() function (that should
swap the fields in the related structure) fails to swap some of the
entries. After correctly swapping all missing values here, too, the
virtio-gpu device is now also working with blob=true on s390x hosts.

Fixes: e0933d91b1 ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2230469
Message-Id: <20230815122007.928049-1-thuth@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/virtio/virtio-gpu-bswap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/virtio/virtio-gpu-bswap.h b/include/hw/virtio/virtio-gpu-bswap.h
index 9124108485..637a0585d0 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -63,7 +63,10 @@ virtio_gpu_create_blob_bswap(struct virtio_gpu_resource_create_blob *cblob)
 {
     virtio_gpu_ctrl_hdr_bswap(&cblob->hdr);
     le32_to_cpus(&cblob->resource_id);
+    le32_to_cpus(&cblob->blob_mem);
     le32_to_cpus(&cblob->blob_flags);
+    le32_to_cpus(&cblob->nr_entries);
+    le64_to_cpus(&cblob->blob_id);
     le64_to_cpus(&cblob->size);
 }
 
-- 
2.39.3

23.08.2023 14:45, Thomas Huth wrpte:
> Using "-device virtio-gpu,blob=true" currently does not work on big
> endian hosts (like s390x). The guest kernel prints an error message
> like:
> 
>   [drm:virtio_gpu_dequeue_ctrl_func [virtio_gpu]] *ERROR* response 0x1200 (command 0x10c)
> 
> and the display stays black. When running QEMU with "-d guest_errors",
> it shows an error message like this:
> 
>   virtio_gpu_create_mapping_iov: nr_entries is too big (83886080 > 16384)
> 
> which indicates that this value has not been properly byte-swapped.
> And indeed, the virtio_gpu_create_blob_bswap() function (that should
> swap the fields in the related structure) fails to swap some of the
> entries. After correctly swapping all missing values here, too, the
> virtio-gpu device is now also working with blob=true on s390x hosts.
> 
> Fixes: e0933d91b1 ("virtio-gpu: Add virtio_gpu_resource_create_blob")
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2230469
> Message-Id: <20230815122007.928049-1-thuth@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

This smells like -stable material. Please let me know if it is not.
e0933d91b1 is 6.1.

Thanks,

/mjt

On 23/08/2023 16.00, Michael Tokarev wrote:
> 23.08.2023 14:45, Thomas Huth wrpte:
>> Using "-device virtio-gpu,blob=true" currently does not work on big
>> endian hosts (like s390x). The guest kernel prints an error message
>> like:
>>
>>   [drm:virtio_gpu_dequeue_ctrl_func [virtio_gpu]] *ERROR* response 0x1200 
>> (command 0x10c)
>>
>> and the display stays black. When running QEMU with "-d guest_errors",
>> it shows an error message like this:
>>
>>   virtio_gpu_create_mapping_iov: nr_entries is too big (83886080 > 16384)
>>
>> which indicates that this value has not been properly byte-swapped.
>> And indeed, the virtio_gpu_create_blob_bswap() function (that should
>> swap the fields in the related structure) fails to swap some of the
>> entries. After correctly swapping all missing values here, too, the
>> virtio-gpu device is now also working with blob=true on s390x hosts.
>>
>> Fixes: e0933d91b1 ("virtio-gpu: Add virtio_gpu_resource_create_blob")
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2230469
>> Message-Id: <20230815122007.928049-1-thuth@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> This smells like -stable material. Please let me know if it is not.
> e0933d91b1 is 6.1.

Yes, please include it in -stable.

  Thanks,
   Thomas

23.08.2023 17:00, Michael Tokarev wrote:
..
> This smells like -stable material. Please let me know if it is not.
> e0933d91b1 is 6.1.

Ah, it has already been Cc'd to stable, n/m.

/mjt