Virtio GPU for S390
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.
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 | 53 +++++++++++++++++++++++++++++++++++++++--------- hw/s390x/virtio-ccw.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 +++++++++ 3 files changed, 108 insertions(+), 9 deletions(-)
Signature · 1 line
1.9.1
PATCH 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 | 53 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 9 deletions(-)
hw/display/virtio-gpu.c +44 -9
@@ -236,8 +236,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 +287,12 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,struct virtio_gpu_resource_create_2d c2d;VIRTIO_GPU_FILL_CMD (c2d); + + c2d.resource_id =le32_to_cpu (c2d.resource_id); + c2d.format =le32_to_cpu (c2d.format); + c2d.width =le32_to_cpu (c2d.width); + c2d.height =le32_to_cpu (c2d.height); +trace_virtio_gpu_cmd_res_create_2d (c2d.resource_id, c2d.format, c2d.width, c2d.height);
@@ -383,6 +389,14 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,struct virtio_gpu_transfer_to_host_2d t2d;VIRTIO_GPU_FILL_CMD (t2d); + + 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.resource_id =le32_to_cpu (t2d.resource_id); + t2d.offset =le64_to_cpu (t2d.offset); +trace_virtio_gpu_cmd_res_xfer_toh_2d (t2d.resource_id); res =virtio_gpu_find_resource (g, t2d.resource_id);
@@ -439,6 +453,13 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,int i;VIRTIO_GPU_FILL_CMD (rf); + + rf.resource_id =le32_to_cpu (rf.resource_id); + rf.r.width =le32_to_cpu (rf.r.width); + rf.r.height =le32_to_cpu (rf.r.height); + rf.r.x =le32_to_cpu (rf.r.x); + rf.r.y =le32_to_cpu (rf.r.y); +trace_virtio_gpu_cmd_res_flush (rf.resource_id, rf.r.width, rf.r.height, rf.r.x, rf.r.y);
@@ -511,6 +532,14 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,struct virtio_gpu_set_scanout ss;VIRTIO_GPU_FILL_CMD (ss); + + ss.scanout_id =le32_to_cpu (ss.scanout_id); + ss.resource_id =le32_to_cpu (ss.resource_id); + ss.r.width =le32_to_cpu (ss.r.width); + ss.r.height =le32_to_cpu (ss.r.height); + ss.r.x =le32_to_cpu (ss.r.x); + ss.r.y =le32_to_cpu (ss.r.y); +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 +662,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 +717,10 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,int ret;VIRTIO_GPU_FILL_CMD (ab); + + ab.resource_id =le32_to_cpu (ab.resource_id); + ab.nr_entries =le32_to_cpu (ab.nr_entries); +trace_virtio_gpu_cmd_res_back_attach (ab.resource_id); res =virtio_gpu_find_resource (g, ab.resource_id);
@@ -735,7 +770,7 @@ static void virtio_gpu_simple_process_cmd(VirtIOGPU *g, {VIRTIO_GPU_FILL_CMD (cmd->cmd_hdr); -switch (cmd->cmd_hdr.type) { +switch (le32_to_cpu (cmd->cmd_hdr.type)) {case VIRTIO_GPU_CMD_GET_DISPLAY_INFO :virtio_gpu_get_display_info (g, cmd);break ;
@@ -1135,7 +1170,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
Re: [Qemu-devel] [PATCH v1 1/2] virtio_gpu: Handle endian conversion
Hi,
hw/display/virtio-gpu.c static void virtio_gpu_resource_create_2d(VirtIOGPU *g, · line 295
Quoted 11 lines
(quoted diff) +0
-0@@ -287,6 +287,12 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g, struct virtio_gpu_resource_create_2d c2d;
VIRTIO_GPU_FILL_CMD(c2d);hw/display/virtio-gpu.c +6
-0+ + c2d.resource_id = le32_to_cpu(c2d.resource_id); + c2d.format = le32_to_cpu(c2d.format); + c2d.width = le32_to_cpu(c2d.width); + c2d.height = le32_to_cpu(c2d.height); +
Please move this to a helper function, maybe by updating the VIRTIO_GPU_FILL_CMD macro.
The header fields should be byteswapped too. As most structs have 32bit fields only (with the exception of hdr.fence_id) you should be able to create a generic byteswap function which only needs the struct size as argument and handles all structs without addresses/offsets (which are 64bit fields).
The conversion looks incomplete, at least virtio_gpu_ctrl_response will need adaptions too. It probably works by luck because the guest driver uses fences only in virgl (3d) mode.
cheers,
Gerd
Re: [Qemu-devel] [PATCH v1 1/2] virtio_gpu: Handle endian conversion
Quoted 8 lines · Gerd Hoffmann
Please move this to a helper function, maybe by updating the
VIRTIO_GPU_FILL_CMDmacro.The header fields should be byteswapped too. As most structs have 32bit fields only (with the exception of hdr.fence_id) you should be able to create a generic byteswap function which only needs the struct size as argument and handles all structs without addresses/offsets (which are 64bit fields).
I am not sure if I understand what you mean here. Since struct virtio_gpu_ctrl_hdr is part of every struct, so any such function would also need to handle the case of hdr.fence_id, right?
Quoted 5 lines
The conversion looks incomplete, at least
virtio_gpu_ctrl_responsewill need adaptions too. It probably works by luck because the guest driver uses fences only in virgl (3d) mode.
Oh right, I need to handle the conversion there as well. Thanks for catching that.
Also I believe this conversion patch isn't comprehensive, it's mostly the changes I made to get a display working on S390. So I appreciate you reviewing the changes.
Quoted 2 lines
cheers,
Gerd
Thanks
Farhan
Re: [Qemu-devel] [PATCH v1 1/2] virtio_gpu: Handle endian conversion
Quoted 13 lines · Farhan Ali
Quoted 10 lines · Gerd Hoffmann
Please move this to a helper function, maybe by updating the
VIRTIO_GPU_FILL_CMDmacro.The header fields should be byteswapped too. As most structs have 32bit fields only (with the exception of hdr.fence_id) you should be
able to create a generic byteswap function which only needs the struct
size as argument and handles all structs without addresses/offsets (which are 64bit fields).
Quoted 3 lines
I am not sure if I understand what you mean here. Since struct
virtio_gpu_ctrl_hdris part of every struct, so any such function would also need to handle the case of hdr.fence_id, right?
Yes, but that is common in all structs. You can have one function to handle the header, one generic function (calls the header function for the header and just does 32bit byteswaps for everything else), one specific function for each operation which has a 64bit address somewhere in the struct (which again can use the header function for the header).
cheers,
Gerd
PATCH 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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++ 2 files changed, 64 insertions(+)
hw/s390x/virtio-ccw.c +54 -0
@@ -1007,6 +1007,20 @@ 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); +Error *err =NULL ; + +qdev_set_parent_bus (vdev,BUS (&ccw_dev->bus)); +object_property_set_bool (OBJECT (vdev), true,"realized" , &err); +if (err) { +error_propagate (errp, err); +return ; + } +} +/* DeviceState to VirtioCcwDevice. Note: used on datapath, * be careful and test performance if you change this. */
@@ -1616,6 +1630,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 +1868,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 v1 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
Quoted 20 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]>--- hw/s390x/virtio-ccw.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++ 2 files changed, 64 insertions(+)hw/s390x/virtio-ccw.c +2
-0@@ -1007,6 +1007,20 @@ static void virtio_ccw_crypto_realize(VirtioCcwDevice *ccw_dev, Error **errp) NULL); } +static void virtio_ccw_gpu_realize (VirtioCcwDevice *ccw_dev,Error **errp) +{
hw/s390x/virtio-ccw.c static void virtio_ccw_crypto_realize(VirtioCcwDevice *ccw_dev, Error **errp) · line 1021
Quoted 10 lines
hw/s390x/virtio-ccw.c +10
-0+VirtIOGPUCcw *dev =VIRTIO_GPU_CCW (ccw_dev); +DeviceState *vdev =DEVICE (&dev->vdev); +Error *err =NULL ; + +qdev_set_parent_bus (vdev,BUS (&ccw_dev->bus)); +object_property_set_bool (OBJECT (vdev), true,"realized" , &err); +if (err) { +error_propagate (errp, err); +return ; + }
You can call error_propagate() unconditionally, it can deal with !err.
Quoted 27 lines
hw/s390x/virtio-ccw.c +20
-0+} +/* DeviceState to VirtioCcwDevice. Note: used on datapath, * be careful and test performance if you change this. */ @@ -1616,6 +1630,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 ); +} +
hw/s390x/virtio-ccw.c static const TypeInfo virtio_ccw_crypto = { · line 1660
Quoted 10 lines
hw/s390x/virtio-ccw.c +10
-0+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;
Wonder if hotplug could work?
Signature · 3 lines
Thanks, David
Re: [Qemu-devel] [PATCH v1 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
Quoted 4 lines · David Hildenbrand
Quoted 2 lines
+
dc->props=virtio_ccw_gpu_properties;
+dc->hotpluggable= false;Wonder if hotplug could work?
No, ui/* doesn't support hotplug.
cheers,
Gerd
Re: [Qemu-devel] [PATCH v1 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
In reply to David HildenbrandQuoted 19 lines · David Hildenbrand
Quoted 18 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]>--- hw/s390x/virtio-ccw.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/virtio-ccw.h | 10 ++++++++++ 2 files changed, 64 insertions(+)hw/s390x/virtio-ccw.c +0
-0@@ -1007,6 +1007,20 @@ static void virtio_ccw_crypto_realize(VirtioCcwDevice *ccw_dev, Error **errp) NULL); }
Quoted 14 lines
hw/s390x/virtio-ccw.c static void virtio_ccw_crypto_realize(VirtioCcwDevice *ccw_dev, Error **errp) · line 1021
Quoted 12 lines
hw/s390x/virtio-ccw.c +12
-0+static void virtio_ccw_gpu_realize (VirtioCcwDevice *ccw_dev,Error **errp) +{ +VirtIOGPUCcw *dev =VIRTIO_GPU_CCW (ccw_dev); +DeviceState *vdev =DEVICE (&dev->vdev); +Error *err =NULL ; + +qdev_set_parent_bus (vdev,BUS (&ccw_dev->bus)); +object_property_set_bool (OBJECT (vdev), true,"realized" , &err); +if (err) { +error_propagate (errp, err); +return ; + }You can call
error_propagate()unconditionally, it can deal with !err.
I think you even do not need error_propagate here - just pass errp directly to object_property_set_bool. You exit the function afterwards anyway.
Thomas
Re: [Qemu-devel] [PATCH v1 2/2] virtio-gpu-ccw: Create a virtio gpu device for the ccw bus
In reply to Farhan AliQuoted 5 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]>
[..]
Quoted 7 lines · Farhan Ali
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.
Quoted 6 lines
eg: qemu-system-s390x ... -device virtio-gpu-ccw,devno=fe.0.0101
-vnc host_ip_addr:5900Note, to actually see any display content the guest kernel needs to support DRM layer, Virtio GPU driver, the Virtual Terminal layer etc.
Do you have a list of CONFIG options that need to be enabled there? Are there also any patches to the guest kernel driver required? Or did that work out of the box once you've enabled the right CONFIG options?
Quoted 2 lines
I would appreciate any feedback on these patches, specially the first patch.
Patches look good to me, but I'm not at all familiar with the virtio-gpu code, so that likely does not count...
Anyway, thanks a lot for tackling this! It's pretty cool to finally have a graphics card on s390x, too :-)
Thomas
Quoted 10 lines · Thomas Huth
Quoted 9 lines · Farhan Ali
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
Quoted 9 lines
Quoted 3 lines
Note, to actually see any display content the guest kernel needs to support DRM layer, Virtio GPU driver, the Virtual Terminal layer etc.
Do you have a list of CONFIG options that need to be enabled there? Are there also any patches to the guest kernel driver required? Or did that work out of the box once you've enabled the right CONFIG options?
It required some kernel hacking. You need to enable the VT layer for S390 to get any kind of graphics displayed.
I experimented on the guest side to enable the VT layer and run a framebuffer console and also the Xfce desktop :)
Anyway the CONFIG options I used are:
The DRM configs to enable the DRM layer and virtio-gpu. I went with the default options for DRM layer.
CONFIG_DRM
CONFIG_DRM_VIRTIO_GPU
We also need to enable configs for the VT layer
CONFIG_VT
CONFIG_DUMMY_CONSOLE
And to display a framebuffer console for the guest
CONFIG_FRAMEBUFFER_CONSOLE
Quoted 11 lines
Quoted 2 lines
I would appreciate any feedback on these patches, specially the first patch.
Patches look good to me, but I'm not at all familiar with the virtio-gpu code, so that likely does not count...
Anyway, thanks a lot for tackling this! It's pretty cool to finally have a graphics card on s390x, too :-)
Thomas
Hi,
Quoted 2 lines
Do you have a list of CONFIG options that need to be enabled there? Are there also any patches to the guest kernel driver required?
guest kernel driver should be fine, it works for ppc64 (big endian) guests.
cheers,
Gerd