[Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Frediano Ziglio]

This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.

libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).

This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html.

Main question and stop over are parameter name. Consider that this
parameter is actually more a hint to drivers. I'm looking anyway to
a way to enforce this in spice-server.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 hw/display/qxl.c | 6 ++++++
 hw/display/qxl.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b220e2d..e9ccd30 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -327,6 +327,11 @@ static void init_qxl_rom(PCIQXLDevice *d)
     rom->log_level     = cpu_to_le32(d->guestdebug);
     rom->modes_offset  = cpu_to_le32(sizeof(QXLRom));
 
+    if (d->max_heads && d->max_heads <= 64) {
+        rom->flags         = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
+        rom->max_heads     = cpu_to_le16(d->max_heads);
+    }
+
     rom->slot_gen_bits = MEMSLOT_GENERATION_BITS;
     rom->slot_id_bits  = MEMSLOT_SLOT_BITS;
     rom->slots_start   = 1;
@@ -2278,6 +2283,7 @@ static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
         DEFINE_PROP_UINT32("vgamem_mb", PCIQXLDevice, vgamem_size_mb, 16),
         DEFINE_PROP_INT32("surfaces", PCIQXLDevice, ssd.num_surfaces, 1024),
+        DEFINE_PROP_UINT16("max_heads", PCIQXLDevice, max_heads, 0),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..d785761 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,7 @@ typedef struct PCIQXLDevice {
     QXLModes           *modes;
     uint32_t           rom_size;
     MemoryRegion       rom_bar;
+    uint16_t           max_heads;
 
     /* vram pci bar */
     uint32_t           vram_size;
-- 
2.1.0

Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Gerd Hoffmann]

On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
> This patch allow to limit number of heads using qxl driver. By default
> qxl driver is not limited on any kind on head use so can decide to use
> as much heads.

There is a hard limit of 16 monitors in the qxl device ...

> Main question and stop over are parameter name.

max_outputs please, to be consistent with upcoming virtio-vga.

> +    if (d->max_heads && d->max_heads <= 64) {
> +        rom->flags         = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
> +        rom->max_heads     = cpu_to_le16(d->max_heads);
> +    }

rom does already carry information about the heads.  How about simply
applying the limit when filling rom->client_monitors_config in
interface_client_monitors_config()?

No device changes, no driver changes, alot less trouble.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Frediano Ziglio]

2015-06-09 10:12 GMT+01:00 Gerd Hoffmann <kraxel@redhat.com>:
> On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
>> This patch allow to limit number of heads using qxl driver. By default
>> qxl driver is not limited on any kind on head use so can decide to use
>> as much heads.
>
> There is a hard limit of 16 monitors in the qxl device ...
>

Ok, didn't see it.

>> Main question and stop over are parameter name.
>
> max_outputs please, to be consistent with upcoming virtio-vga.
>

I'll do.

>> +    if (d->max_heads && d->max_heads <= 64) {
>> +        rom->flags         = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
>> +        rom->max_heads     = cpu_to_le16(d->max_heads);
>> +    }
>
> rom does already carry information about the heads.  How about simply
> applying the limit when filling rom->client_monitors_config in
> interface_client_monitors_config()?
>
> No device changes, no driver changes, alot less trouble.
>
> cheers,
>   Gerd
>

So what you are proposing is client should limit heads based on
spice-server limit and also spice-server should limit monitor received
from client. I'll try it. So this would require only Qemu + libvirt
change. This is slightly change anyway as guest will see more heads
without monitor connected instead of limiting heads. But anyway is a
way to enforce heads.

Frediano

Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Gerd Hoffmann]

On Di, 2015-06-09 at 10:26 +0100, Frediano Ziglio wrote:
> 2015-06-09 10:12 GMT+01:00 Gerd Hoffmann <kraxel@redhat.com>:
> > On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
> >> This patch allow to limit number of heads using qxl driver. By default
> >> qxl driver is not limited on any kind on head use so can decide to use
> >> as much heads.
> >
> > There is a hard limit of 16 monitors in the qxl device ...
> >
> 
> Ok, didn't see it.

The rom->client_monitors_config heads array has 16 entries, and I
suspect the same limit could be in more places ...

> >> +    if (d->max_heads && d->max_heads <= 64) {
> >> +        rom->flags         = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
> >> +        rom->max_heads     = cpu_to_le16(d->max_heads);
> >> +    }
> >
> > rom does already carry information about the heads.  How about simply
> > applying the limit when filling rom->client_monitors_config in
> > interface_client_monitors_config()?
> >
> > No device changes, no driver changes, alot less trouble.
> >
> > cheers,
> >   Gerd
> >
> 
> So what you are proposing is client should limit heads based on
> spice-server limit and also spice-server should limit monitor received
> from client.

No.  You'll add a new property like you did.  In
interface_client_monitors_config(), where qemu already checks it
wouldn't overflow the 16 entries in the array it has, you additionally
check for the configured limit.

Which will filter the monitors_config sent from spice client to the
guest to not carry more than $limit entries.  So, if you configured two
heads, and spice client says "fyi, I have three heads here ...", qemu
will forward only two entries guests to the guest.  Therefore the guest
will use two heads them only, leaving the third unused.

In theory spice-server could do the filtering too, but doing it in qemu
is easier as you can easily pass in the limit as device property.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Daniel P. Berrange]

On Tue, Jun 09, 2015 at 09:49:44AM +0100, Frediano Ziglio wrote:
> This patch allow to limit number of heads using qxl driver. By default
> qxl driver is not limited on any kind on head use so can decide to use
> as much heads.
> 
> libvirt has this as a video card parameter (actually set to 1 but not
> used). This parameter will allow to limit setting a use can do (which
> could be confusing).
> 
> This patch rely on some change in spice-protocol which are not still
> accepted. See
> http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html.
> 
> Main question and stop over are parameter name. Consider that this
> parameter is actually more a hint to drivers. I'm looking anyway to
> a way to enforce this in spice-server.

What is the actual benefit of being able to limit the number of
heads in QXL / SPICE ?

I know libvirt has the 'heads' attribute for specifying the number
of video outputs. This is used in hypervisors which only support a
fixed number of outputs and need up-front configuration. Since
QXL/SPICE can dynamically enable/disable heads, there is no need to
support this libvirt configuration attribute - it is better from a
usability POV to have the head count totally dynamic, than to add
a fixed limit.

IOW, unless there's some need to use this limit in order to go
above the 16 head maximum the code currently has, I think adding
this manual limit to QXL/SPICE is a step backwards really. It
feels like a feature in search of a purpose IMHO.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [Spice-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Christophe Fergeau]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

On Tue, Jun 09, 2015 at 10:50:48AM +0100, Daniel P. Berrange wrote:
> On Tue, Jun 09, 2015 at 09:49:44AM +0100, Frediano Ziglio wrote:
> > This patch allow to limit number of heads using qxl driver. By default
> > qxl driver is not limited on any kind on head use so can decide to use
> > as much heads.
> > 
> > libvirt has this as a video card parameter (actually set to 1 but not
> > used). This parameter will allow to limit setting a use can do (which
> > could be confusing).
> > 
> > This patch rely on some change in spice-protocol which are not still
> > accepted. See
> > http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html.
> > 
> > Main question and stop over are parameter name. Consider that this
> > parameter is actually more a hint to drivers. I'm looking anyway to
> > a way to enforce this in spice-server.
> 
> What is the actual benefit of being able to limit the number of
> heads in QXL / SPICE ?
> 
> I know libvirt has the 'heads' attribute for specifying the number
> of video outputs. This is used in hypervisors which only support a
> fixed number of outputs and need up-front configuration. Since
> QXL/SPICE can dynamically enable/disable heads, there is no need to
> support this libvirt configuration attribute - it is better from a
> usability POV to have the head count totally dynamic, than to add
> a fixed limit.
> 
> IOW, unless there's some need to use this limit in order to go
> above the 16 head maximum the code currently has, I think adding
> this manual limit to QXL/SPICE is a step backwards really. It
> feels like a feature in search of a purpose IMHO.

The head count is indeed dynamic with SPICE, but the available video RAM
is fixed after QEMU startup, and if you start trying to fit 4 full HD
heads in 16MB of VRAM, you'll get weird failures. If you know you are
only going to use one screen, it's nice to be able to allocate a small
amount of VRAM and to enforce the maximum number of heads for the amount
of VRAM you have assigned to the VM.
remote-viewer also gets a 'display' menu with the number of available
displays listed in it. I can imagine some people/admins wanted to
mandate that only N displays are going to be shown here to avoid user
confusion.

So while we should keep defaulting to the current dynamic behaviour,
a way to optionally limit the number of available heads is useful in
some situations

Christophe

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

[Frediano Ziglio]

2015-06-09 10:43 GMT+01:00 Gerd Hoffmann <kraxel@redhat.com>:
> On Di, 2015-06-09 at 10:26 +0100, Frediano Ziglio wrote:
>> 2015-06-09 10:12 GMT+01:00 Gerd Hoffmann <kraxel@redhat.com>:
>> > On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
>> >> This patch allow to limit number of heads using qxl driver. By default
>> >> qxl driver is not limited on any kind on head use so can decide to use
>> >> as much heads.
>> >
>> > There is a hard limit of 16 monitors in the qxl device ...
>> >
>>
>> Ok, didn't see it.
>
> The rom->client_monitors_config heads array has 16 entries, and I
> suspect the same limit could be in more places ...
>

In my header is 64 entries. Anyway, it has an hard limit.

>> >> +    if (d->max_heads && d->max_heads <= 64) {
>> >> +        rom->flags         = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
>> >> +        rom->max_heads     = cpu_to_le16(d->max_heads);
>> >> +    }
>> >
>> > rom does already carry information about the heads.  How about simply
>> > applying the limit when filling rom->client_monitors_config in
>> > interface_client_monitors_config()?
>> >
>> > No device changes, no driver changes, alot less trouble.
>> >
>> > cheers,
>> >   Gerd
>> >
>>
>> So what you are proposing is client should limit heads based on
>> spice-server limit and also spice-server should limit monitor received
>> from client.
>
> No.  You'll add a new property like you did.  In
> interface_client_monitors_config(), where qemu already checks it
> wouldn't overflow the 16 entries in the array it has, you additionally
> check for the configured limit.
>
> Which will filter the monitors_config sent from spice client to the
> guest to not carry more than $limit entries.  So, if you configured two
> heads, and spice client says "fyi, I have three heads here ...", qemu
> will forward only two entries guests to the guest.  Therefore the guest
> will use two heads them only, leaving the third unused.
>
> In theory spice-server could do the filtering too, but doing it in qemu
> is easier as you can easily pass in the limit as device property.
>
> cheers,
>   Gerd
>
>

I did change in interface_client_monitors_config and also in
spice-server adding a new spice function to cap the monitor numbers
from guest to client. This is required as client base the
configuration it send based on the number of head available on the
guest. So faking the guest in spice-server in both directions make
client and guest happy. Yes, this does not require driver changes. I
added a function to spice-server so at least a bump of minor ABI
version is needed. I don't know if there are a function to set some
configuration that does not require a new ABI.

Frediano