[Qemu-devel] [post 1.1 PATCH] hw/qxl: disallow non sync io for revision >= 3

[Qemu-devel] [post 1.1 PATCH] hw/qxl: disallow non sync io for revision >= 3

[Alon Levy]

The guest drivers should already know how to use async for revision 3.
But since it's still possible to have an older driver with revision 3
that doesn't check for the revision, require a new parameter
"force_async", which we can later turn to 1 by default.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 6c11e70..451e8e5 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1316,6 +1316,23 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
         return;
     }
 
+    if (d->force_async && d->revision >= 3) {
+        switch (io_port) {
+        case QXL_IO_UPDATE_AREA:
+        case QXL_IO_MEMSLOT_ADD:
+        case QXL_IO_CREATE_PRIMARY:
+        case QXL_IO_DESTROY_PRIMARY:
+        case QXL_IO_DESTROY_SURFACE_WAIT:
+        case QXL_IO_DESTROY_ALL_SURFACES:
+            qxl_guest_bug(d, "sync io %d not supported for revision >= 3",
+                          io_port);
+            return;
+            break;
+        default:
+            break;
+        }
+    }
+
     /* we change the io_port to avoid ifdeffery in the main switch */
     orig_io_port = io_port;
     switch (io_port) {
@@ -2036,6 +2053,7 @@ static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("ram_size_mb",  PCIQXLDevice, ram_size_mb, -1),
         DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, -1),
         DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
+        DEFINE_PROP_UINT32("force_async", PCIQXLDevice, force_async, 0),
         DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.7.10.1

Re: [Qemu-devel] [post 1.1 PATCH] hw/qxl: disallow non sync io for revision >= 3

[Gerd Hoffmann]

On 05/09/12 13:26, Alon Levy wrote:
> The guest drivers should already know how to use async for revision 3.
> But since it's still possible to have an older driver with revision 3
> that doesn't check for the revision, require a new parameter
> "force_async", which we can later turn to 1 by default.

Hmm.  PCI revisions are supposed to be backward compatible, I don't
think this is a good idea.  We might want to log a warning or inform via
'info spice' in case we find a guest using the old, sync commands, so
management can figure and warn, but I wouldn't do more.

We can think about defining a qxl2 pci device, with a different pci id,
where we can throw away old stuff.  For such a new & incompatible device
I'd like to change a few more things while being at it ...

(1) go for a single 32bit mmio register set similar to real hardware.
That should replace the rom bar (device info), the fields in the ram
area (where interrupt mask etc live) and the io command bar.

(2) consider virtio for the rings.  Not sure this is worth it as that
wouldn't make qxl bus-agnostic, for vga compatibility and device memory
we'll still have a dependency on pci ...

(3) renumber the remaining bars so we can make the vram bar 64bit,
without a 32bit compatibility alias needed.

(4) Drop QXL_IO_SET_MODE support and the mode list.

Possibly more ...

cheers,
  Gerd

Re: [Qemu-devel] [post 1.1 PATCH] hw/qxl: disallow non sync io for revision >= 3

[Alon Levy]

On Wed, May 09, 2012 at 02:51:01PM +0200, Gerd Hoffmann wrote:
> On 05/09/12 13:26, Alon Levy wrote:
> > The guest drivers should already know how to use async for revision 3.
> > But since it's still possible to have an older driver with revision 3
> > that doesn't check for the revision, require a new parameter
> > "force_async", which we can later turn to 1 by default.
> 
> Hmm.  PCI revisions are supposed to be backward compatible, I don't
> think this is a good idea.  We might want to log a warning or inform via
> 'info spice' in case we find a guest using the old, sync commands, so
> management can figure and warn, but I wouldn't do more.
> 
> We can think about defining a qxl2 pci device, with a different pci id,
> where we can throw away old stuff.  For such a new & incompatible device
> I'd like to change a few more things while being at it ...
> 
> (1) go for a single 32bit mmio register set similar to real hardware.
> That should replace the rom bar (device info), the fields in the ram
> area (where interrupt mask etc live) and the io command bar.
> 
> (2) consider virtio for the rings.  Not sure this is worth it as that
> wouldn't make qxl bus-agnostic, for vga compatibility and device memory
> we'll still have a dependency on pci ...
> 
> (3) renumber the remaining bars so we can make the vram bar 64bit,
> without a 32bit compatibility alias needed.
> 
> (4) Drop QXL_IO_SET_MODE support and the mode list.
> 
> Possibly more ...

Sounds like a good idea. I'll try to do a patch with the sync warning
first - the reason for this patch was trying to prevent deadlocks with
boxes, the management in question.

The virtio-rings - I know someone is already working on this and is
pretty well advanced. Actually I think they are doing a whole new
device, but I haven't seen any code yet. Probably not pci. But just
replacing the rings should not be that hard (tm) :)

> 
> cheers,
>   Gerd