Virtio GPU for S390
v2 cover Virtio GPU for S390
These patches wire up the virtio-gpu device for CCW bus for S390.
For the S390 architecture which does not natively support any graphics device, virtio gpu in 2D mode could be used to emulate a simple graphics card and use VNC as the display.
eg: qemu-system-s390x ... -device virtio-gpu-ccw,devno=fe.0.0101
-vnc host_ip_addr:5900
Note, to actually see any display content the guest kernel needs to support DRM layer, Virtio GPU driver, the Virtual Terminal layer etc.
I would appreciate any feedback on these patches, specially the first patch.
ChangeLog
--------
v1 -> v2
- remove error_propagate (patch 2)
- Add byteswap functions (patch 1)
- Handle endian conversion for virtio_gpu_ctrl_response (patch 1)
Thank you
Farhan
Farhan Ali (2):
virtio-gpu: Handle endian conversion
virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
hw/display/virtio-gpu.c | 75 +++++++++++++++++++++++++++++++++++++++++++------ hw/s390x/virtio-ccw.c | 49 ++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 +++++++ 3 files changed, 126 insertions(+), 8 deletions(-)
Signature · 1 line
1.9.1
PATCH v2 1/2 virtio-gpu: Handle endian conversion
Virtio GPU code currently only supports litte endian format, and so using the Virtio GPU device on a big endian machine does not work.
Let's fix it by supporting the correct host cpu byte order.
--- hw/display/virtio-gpu.c | 75 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 8 deletions(-)
hw/display/virtio-gpu.c +67 -8
@@ -30,6 +30,54 @@ virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);static void virtio_gpu_cleanup_mapping (struct virtio_gpu_simple_resource *res); +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); +} + +static void virtio_gpu_bswap_cpu_32 (void *ptr, +size_t size) +{ +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 ); + } +} + +static void +virtio_gpu_t2d_bswap_cpu (struct virtio_gpu_transfer_to_host_2d *t2d) +{ +virtio_gpu_ctrl_hdr_bswap_cpu (&t2d->hdr); + t2d->r.x =le32_to_cpu (t2d->r.x); + t2d->r.y =le32_to_cpu (t2d->r.y); + t2d->r.width =le32_to_cpu (t2d->r.width); + t2d->r.height =le32_to_cpu (t2d->r.height); + t2d->offset =le64_to_cpu (t2d->offset); + t2d->resource_id =le32_to_cpu (t2d->resource_id); + t2d->padding =le32_to_cpu (t2d->padding); +} +#ifdef CONFIG_VIRGL #include <virglrenderer.h> #define VIRGL (_g, _virgl, _simple, ...) \
@@ -205,6 +253,7 @@ void virtio_gpu_ctrl_response(VirtIOGPU *g, resp->fence_id = cmd->cmd_hdr.fence_id; resp->ctx_id = cmd->cmd_hdr.ctx_id; } +virtio_gpu_ctrl_hdr_bswap_le (resp); s =iov_from_buf (cmd->elem.in_sg, cmd->elem.in_num,0 , resp, resp_len);if (s != resp_len) { qemu_log_mask(LOG_GUEST_ERROR,
@@ -236,8 +285,8 @@ virtio_gpu_fill_display_info(VirtIOGPU *g,for (i =0 ; i < g->conf.max_outputs; i++) {if (g->enabled_output_bitmask & (1 << i)) { dpy_info->pmodes[i].enabled =1 ; - dpy_info->pmodes[i].r.width = g->req_state[i].width; - dpy_info->pmodes[i].r.height = g->req_state[i].height; + dpy_info->pmodes[i].r.width =cpu_to_le32 (g->req_state[i].width); + dpy_info->pmodes[i].r.height =cpu_to_le32 (g->req_state[i].height); } } }
@@ -287,6 +336,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,struct virtio_gpu_resource_create_2d c2d;VIRTIO_GPU_FILL_CMD (c2d); +virtio_gpu_bswap_cpu_32 (&c2d,sizeof (c2d));trace_virtio_gpu_cmd_res_create_2d (c2d.resource_id, c2d.format, c2d.width, c2d.height);
@@ -360,6 +410,7 @@ static void virtio_gpu_resource_unref(VirtIOGPU *g,struct virtio_gpu_resource_unref unref;VIRTIO_GPU_FILL_CMD (unref); +virtio_gpu_bswap_cpu_32 (&unref,sizeof (unref));trace_virtio_gpu_cmd_res_unref (unref.resource_id); res =virtio_gpu_find_resource (g, unref.resource_id);
@@ -383,6 +434,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,struct virtio_gpu_transfer_to_host_2d t2d;VIRTIO_GPU_FILL_CMD (t2d); +virtio_gpu_t2d_bswap_cpu (&t2d);trace_virtio_gpu_cmd_res_xfer_toh_2d (t2d.resource_id); res =virtio_gpu_find_resource (g, t2d.resource_id);
@@ -439,6 +491,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,int i;VIRTIO_GPU_FILL_CMD (rf); +virtio_gpu_bswap_cpu_32 (&rf,sizeof (rf));trace_virtio_gpu_cmd_res_flush (rf.resource_id, rf.r.width, rf.r.height, rf.r.x, rf.r.y);
@@ -511,6 +564,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,struct virtio_gpu_set_scanout ss;VIRTIO_GPU_FILL_CMD (ss); +virtio_gpu_bswap_cpu_32 (&ss,sizeof (ss));trace_virtio_gpu_cmd_set_scanout (ss.scanout_id, ss.resource_id, ss.r.width, ss.r.height, ss.r.x, ss.r.y);
@@ -633,13 +687,15 @@ int virtio_gpu_create_mapping_iov(struct virtio_gpu_resource_attach_backing *ab, *addr =g_malloc0 (sizeof (uint64_t ) * ab->nr_entries); }for (i =0 ; i < ab->nr_entries; i++) { -hwaddr len = ents[i].length; - (*iov)[i].iov_len = ents[i].length; - (*iov)[i].iov_base =cpu_physical_memory_map (ents[i].addr, &len,1 ); +uint64_t a =le64_to_cpu (ents[i].addr); +uint32_t l =le32_to_cpu (ents[i].length); +hwaddr len = l; + (*iov)[i].iov_len = l; + (*iov)[i].iov_base =cpu_physical_memory_map (a, &len,1 );if (addr) { - (*addr)[i] = ents[i].addr; + (*addr)[i] = a; } -if (!(*iov)[i].iov_base || len != ents[i].length) { +if (!(*iov)[i].iov_base || len != l) {qemu_log_mask (LOG_GUEST_ERROR ,"%s: failed to map MMIO memory for" " resource %d element %d\n" , __func__, ab->resource_id, i);
@@ -686,6 +742,7 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,int ret;VIRTIO_GPU_FILL_CMD (ab); +virtio_gpu_bswap_cpu_32 (&ab,sizeof (ab));trace_virtio_gpu_cmd_res_back_attach (ab.resource_id); res =virtio_gpu_find_resource (g, ab.resource_id);
@@ -718,6 +775,7 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,struct virtio_gpu_resource_detach_backing detach;VIRTIO_GPU_FILL_CMD (detach); +virtio_gpu_bswap_cpu_32 (&detach,sizeof (detach));trace_virtio_gpu_cmd_res_back_detach (detach.resource_id); res =virtio_gpu_find_resource (g, detach.resource_id);
@@ -734,6 +792,7 @@ static void virtio_gpu_simple_process_cmd(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) {VIRTIO_GPU_FILL_CMD (cmd->cmd_hdr); +virtio_gpu_ctrl_hdr_bswap_cpu (&cmd->cmd_hdr);switch (cmd->cmd_hdr.type) {case VIRTIO_GPU_CMD_GET_DISPLAY_INFO :
@@ -1135,7 +1194,7 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) } g->config_size =sizeof (struct virtio_gpu_config ); - g->virtio_config.num_scanouts = g->conf.max_outputs; + g->virtio_config.num_scanouts =cpu_to_le32 (g->conf.max_outputs);virtio_init (VIRTIO_DEVICE (g),"virtio-gpu" ,VIRTIO_ID_GPU , g->config_size);
Signature · 1 line
1.9.1
PATCH v2 2/2 virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
In reply to Farhan AliWire up the virtio-gpu device for the CCW bus. The virtio-gpu is a virtio-1 device, so disable revision 0.
--- hw/s390x/virtio-ccw.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++ 2 files changed, 59 insertions(+)
hw/s390x/virtio-ccw.c +49 -0
@@ -1007,6 +1007,15 @@ static void virtio_ccw_crypto_realize(VirtioCcwDevice *ccw_dev, Error **errp) NULL); } +static void virtio_ccw_gpu_realize (VirtioCcwDevice *ccw_dev,Error **errp) +{ +VirtIOGPUCcw *dev =VIRTIO_GPU_CCW (ccw_dev); +DeviceState *vdev =DEVICE (&dev->vdev); + +qdev_set_parent_bus (vdev,BUS (&ccw_dev->bus)); +object_property_set_bool (OBJECT (vdev), true,"realized" , errp); +} +/* DeviceState to VirtioCcwDevice. Note: used on datapath, * be careful and test performance if you change this. */
@@ -1616,6 +1625,45 @@ static const TypeInfo virtio_ccw_crypto = { .class_init = virtio_ccw_crypto_class_init, }; +static Property virtio_ccw_gpu_properties[] = { +DEFINE_PROP_BIT ("ioeventfd" , VirtioCcwDevice, flags, +VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT , true), +DEFINE_PROP_UINT32 ("max_revision" , VirtioCcwDevice, max_rev, +VIRTIO_CCW_MAX_REV ), +DEFINE_PROP_END_OF_LIST (), +}; + +static void virtio_ccw_gpu_instance_init (Object *obj) +{ +VirtIOGPUCcw *dev =VIRTIO_GPU_CCW (obj); +VirtioCcwDevice *ccw_dev =VIRTIO_CCW_DEVICE (obj); + + ccw_dev->force_revision_1 = true; +virtio_instance_init_common (obj, &dev->vdev,sizeof (dev->vdev), +TYPE_VIRTIO_GPU ); +} + +static void virtio_ccw_gpu_class_init (ObjectClass *klass,void *data) +{ +DeviceClass *dc =DEVICE_CLASS (klass); +VirtIOCCWDeviceClass *k =VIRTIO_CCW_DEVICE_CLASS (klass); + + k->realize = virtio_ccw_gpu_realize; + k->exit = virtio_ccw_exit; + dc->reset = virtio_ccw_reset; + dc->props = virtio_ccw_gpu_properties; + dc->hotpluggable = false; +set_bit (DEVICE_CATEGORY_DISPLAY , dc->categories); +} + +static const TypeInfo virtio_ccw_gpu = { + .name =TYPE_VIRTIO_GPU_CCW , + .parent =TYPE_VIRTIO_CCW_DEVICE , + .instance_size =sizeof (VirtIOGPUCcw), + .instance_init = virtio_ccw_gpu_instance_init, + .class_init = virtio_ccw_gpu_class_init, +}; +static void virtio_ccw_busdev_realize (DeviceState *dev,Error **errp) {VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
@@ -1815,6 +1863,7 @@ static void virtio_ccw_register(void)type_register_static (&vhost_vsock_ccw_info);#endif type_register_static (&virtio_ccw_crypto); +type_register_static (&virtio_ccw_gpu); }type_init (virtio_ccw_register)
hw/s390x/virtio-ccw.h +10 -0
@@ -27,6 +27,7 @@#ifdef CONFIG_VHOST_VSOCK #include "hw/virtio/vhost-vsock.h" #endif /* CONFIG_VHOST_VSOCK */ +#include "hw/virtio/virtio-gpu.h" #include "hw/s390x/s390_flic.h" #include "hw/s390x/css.h"
@@ -223,4 +224,13 @@ typedef struct VHostVSockCCWState {#endif /* CONFIG_VHOST_VSOCK */ +#define TYPE_VIRTIO_GPU_CCW "virtio-gpu-ccw" +#define VIRTIO_GPU_CCW (obj) \ + OBJECT_CHECK(VirtIOGPUCcw, (obj), TYPE_VIRTIO_GPU_CCW) + +typedef struct VirtIOGPUCcw { +VirtioCcwDevice parent_obj; +VirtIOGPU vdev; +}VirtIOGPUCcw ; +#endif
Signature · 1 line
1.9.1
Re: [Qemu-devel] [PATCH v2 1/2] virtio-gpu: Handle endian conversion
In reply to Farhan AliHi,
Quoted 9 lines
hw/display/virtio-gpu.c +9
-0+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); +}
hw/display/virtio-gpu.c virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id); · line 51
Quoted 10 lines
hw/display/virtio-gpu.c +10
-0+ +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
hw/display/virtio-gpu.c virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id); · line 55
Quoted 4 lines
hw/display/virtio-gpu.c +4
-0+ +static void virtio_gpu_bswap_cpu_32 (void *ptr, + size_t size) +{
#ifdef HOST_WORDS_BIGENDIAN
Quoted 9 lines
+ size_t i;
+ structvirtio_gpu_ctrl_hdr*hdr = (structvirtio_gpu_ctrl_hdr*) ptr;hw/display/virtio-gpu.c +6
-0+ + 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 +
Quoted 3 lines
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
Re: [Qemu-devel] [PATCH v2 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
In reply to Farhan AliQuoted 9 lines · Farhan Ali
Wire up the virtio-gpu device for the CCW bus. The virtio-gpu is a virtio-1 device, so disable revision 0.
Signed-off-by Farhan Ali <[email protected]>Reviewed-by Halil Pasic <[email protected]>--- hw/s390x/virtio-ccw.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++
Re: [Qemu-devel] [PATCH v2 1/2] virtio-gpu: Handle endian conversion
In reply to Gerd HoffmannQuoted 28 lines · Gerd Hoffmann
Hi,
Quoted 9 lines
hw/display/virtio-gpu.c +9
-0+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); +}hw/display/virtio-gpu.c virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id); · line 51
Quoted 10 lines
hw/display/virtio-gpu.c +10
-0+ +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_le32andle32_to_cpuare 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_leor_cpupostfix. Note that there are also variants for in-place swapping.
Quoted 2 lines
Have a look at the comments in
include/qemu/bswap.h
Thanks for pointing that out. I will re-spin another version.
Quoted 22 lines
hw/display/virtio-gpu.c virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id); · line 55
Quoted 4 lines
hw/display/virtio-gpu.c +4
-0+ +static void virtio_gpu_bswap_cpu_32 (void *ptr, +size_t size) +{#ifdef
HOST_WORDS_BIGENDIANReview
Quoted 12 lines
(quoted diff) +2
-0+ size_t i; + struct virtio_gpu_ctrl_hdr *hdr = (struct virtio_gpu_ctrl_hdr *)ptr;
hw/display/virtio-gpu.c +6
-0+ +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));
(quoted diff) +2
-0+ i = i + sizeof(uint32_t); + }#endif
Quoted 2 lines
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
Quoted 4 lines
cheers,
Gerd
Re: [Qemu-devel] [PATCH v2 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
In reply to Thomas HuthQuoted 4 lines · Thomas Huth
Quoted 3 lines · Farhan Ali
Wire up the virtio-gpu device for the CCW bus. The virtio-gpu is a virtio-1 device, so disable revision 0.
Quoted 9 lines
Quoted 6 lines
Signed-off-by Farhan Ali <[email protected]>Reviewed-by Halil Pasic <[email protected]>--- hw/s390x/virtio-ccw.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++Reviewed-by Thomas Huth <[email protected]>
Thanks for the review :)