[Qemu-devel] Re: [PATCH] spice: add qxl device

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 17, 2010 at 02:28:21PM +0100, Gerd Hoffmann wrote:
> On 11/16/10 18:43, Michael S. Tsirkin wrote:
> >On Tue, Nov 02, 2010 at 02:34:58PM +0100, Gerd Hoffmann wrote:
> >>+        if (ram_size<  32 * 1024 * 1024)
> >>+            ram_size = 32 * 1024 * 1024;
> >>+        vga_common_init(vga, ram_size);
> >>+        vga_init(vga);
> >>+        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
> >>+
> >>+        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> >>+                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> >>+        qxl->ssd.ds = vga->ds;
> >>+        qxl->ssd.bufsize = (16 * 1024 * 1024);
> >>+        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
> >>+
> >>+        qxl0 = qxl;
> >
> >What happens when this device is then removed?
> 
> Better don't try ...

Better prevent it then?

> The primary vga can't be hot-unplugged in qemu.  Not only because
> the qxl0 pointer would point into nowhere in this case, but also
> because you can't unregister the graphic console.

But do we really have to make this part of qxl design, with global
vars and stuff? Even if we have a limitation in qemu, qxl should be able
to keep all its data in device state I think ...

>  Also having non-pci ressources (legacy vga I/O ports) is a problem.

I'm not sure why is this a problem. It shouldn't be.

> >>+        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
> >>+    } else {
> >>+        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);
> >
> >So 1st device has device id different from the rest?
> 
> Yes.
> 
> >Why?
> 
> Because the first one actually *is* different. It is the only one
> which is vga compatible.  It serves as primary display.  You'll see
> the boot messages there.

I thought it's up to the guest where to send boot messages.
Modern BIOSes have options to control this behaviour.
I think it's a mistake to make things such as device class
depend on the order devices are created in.
It would be better to make it a separate property, or give it a
different name.

> >>+    device_id++;
> >
> >what happens when this wraps around?
> >Since it's an int probably undefined behaviour ...
> 
> I doubt you'll see it wrap in any real world scenario.  Even with one
> hotplug + unplug cycle per second you'll need a bunch of years to
> see it wrap.  Beside that at least in windows the device can't be
> unplugged in the first place, windows will veto the unplug request.
> 
> cheers,
>   Gerd

Yes but we are splitting the unplug part from the guest eject, so user
will in the future be able to perform surprise removal just like with
real hardware. And with this in place, it need not take even a
millisecond to unplug a device.

-- 
MST