[Spice-devel] [libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver

[Martin Kletzander]

On Tue, Jul 21, 2015 at 08:41:33AM -0400, Frediano Ziglio wrote:
>>
>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>> >On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>> >> I spend all morning fixing this to be installed properly in the
>> >> system.  Anyway, I finally managed to make this work and found out the
>> >> guest I used for it is not ready to have multiple monitors.  Anyway,
>> >> looking at everything else this definitely works, so ACK, I'll push it
>> >> in a while.
>> >
>> >I'm still a bit worried that all existing guests will have heads='1' in
>> >their XML as libvirt is adding that by default, but I don't really see a
>> >way around it :-/ We should make sure libvirt stops adding it from now
>> >on though ;)
>> >
>>
>> Well, how would you then limit the outputs to 1?  Having said that, I
>> have no idea why we started adding heads="1" automatically and playing
>> with different changes, we have another bug in the parsing/formatting
>> code.  You'll also won't be able to migrate from older libvirt with
>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
>> reverting the change in libvirt or even using heads > 1.  Although
>> that won't migrate either.  So the only other thing that we can do is
>> to introduce new parameter, like maxHeads.  The sound you just heard
>> is me slapping my face because again there will multiple parameters
>> meaning similar things...
>>
>
>heads specify the number of heads the emulated virtual card has. I think the problem was specify the number for QXL which never honored this setting.
>I don't think you can't migrate, just after migration the machine won't allow more than 1 head.
>Unfortunately the XML file does not have any field to specify the libvirt version was meant for. It would be useful to add so if you see the version it means that heads==1 means 1 if not it means is not defined.
>I'm against a new parameter which specify the same meaning of another parameter. I would instead something like validHeads or headsSet with a false as default.
>

[Would you mind convincing your mail client to wrap long lines?]

Well, libvirt is, unfortunately, by definition, backward compatible.
So we guarantee, that you don't have to change this stuff and you can
migrate to newer ones.  The problem here with the migration is that
the guests will be binary incompatible, the amount of memory you need
to transfer from source will not fit the destination.

Until we reach a decision, I would revert the patch.  We need to
decide how to handle this.

About the new name.  I take it as 'heads' defines how many monitors
there are and 'maxHeads' would define the maximum of them to have.
The difference would be there of course only for qxl.  I don't think
cirrus, for example, can dynamically change number of monitors
connected, can it?  We had similar problem with the ram/vram/vgaram
attributes and we ended up with all of them and bunch of lines dealing
with just the update and migration compatibility.

Having headsSet is pointless.  How would you decide whether heads is
actually 1 or not if you were to parse an XML with heads='1'?
Dropping heads='1' is not an option as well since that would make
other video models (that support heads=) change their settings.  And
nobody could set heads='1' any more without also setting another
parameter.  The problem is we screwed up when we defaulted heads to a
number even though there are devices that do not handle that
properly.  That's the past, we can't do much about that due to
back-compat (Aargh!), but that's how it is, unfortunately).

That are my thoughts on why maxHeads should be a new parameter that
would set max_outputs for the qxl device.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150721/dfdb02a4/attachment.sig>