[libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface

[Ján Tomko]

On Wed, Feb 13, 2019 at 10:59:28AM +0100, Andrea Bolognani wrote:
>On Wed, 2019-02-13 at 10:03 +0100, Ján Tomko wrote:
>> On Tue, Feb 12, 2019 at 05:21:56PM +0100, Andrea Bolognani wrote:
>> > On Tue, 2019-02-12 at 16:07 +0100, Ján Tomko wrote:
>> > > > > There should be no pressure to maintain the 1:1 mapping.
>> > > > > For QEMU, the devices need to be represented in single namespace, so
>> > > > > they have to include the bus. In libvirt, we already have the serial
>> > > > > type and the <address> element. It does not have to be duplicated in the
>> > > > > model name as well.
>> >
>> > Note that the <address> element is not automatically added for
>> > ISA devices, so that specific duplication is not present.
>>
>> The other duplication in serial type is more worrying.
>
>Back when I last touched that code, part of the duplication was
>already in place and neither me nor the reviewer were able to come
>up with a less redundant representation that still maintained some
>amount of logic and consistency between serial console types. So
>yeah, it's far from being an optimal solution, but that's kinda what
>happens when your XML design grows organically over 10+ years :)
>
>> Also, the device will be given an iobase by QEMU, we should represent
>> that in the XML and fill in that default.
>
>If we can manage to implement it in a way that's both reliable and
>backwards compatible, then I'm absolutely in favor of doing that! The
>current behavior is clearly not great.
>

We cannot if any proposal for improvement is met with "it's already
ugly so we might keep doing it that way"

>> > > My point is that the internal implementation is not relevant here
>> > > (we do map XML attributes to QEMU devices elsewhere, see
>> > > qemuDeviceVideo), it's the XML that matters.
>> > >
>> > > The 'usb-serial', 'pci-serial', 'isa-serial' models are all a generic
>> > > repetition of the target type, all of those are IMO better than
>> > > <model name='serial'/> or <model name='generic'/>
>> > >
>> > > However
>> > > <target type='isa-serial'>
>> > >   <model name='debugcon'/>
>> > > </target>
>> > > looks better to me than
>> > > <target type='isa-serial'>
>> > >   <model name='isa-debugcon'/>
>> > > </target>
>> >
>> > We are consistently using the QEMU device name as the model
>> > attribute for <serial> devices,
>>
>> Consistently mapping QEMU device names to libvirt attributes is
>> explicitly a non-goal of libvirt. The reason for the XML should
>> be: "it represents the device well", not "we won't need to add another
>> enum". See "virtio-scsi"
>
>There is precedent for using the model attribute as a way to store
>the hypervisor-specific device name, eg.
>

Ah, the copy & paste argument.

>  <controller type='pci' model='pcie-root-port'>
>    <model name='pcie-root-port'/>
>  </controller>
>

While having a model attribute and a model element is ugly, despite
exaclty matching QEMU's device, this is just a different way of saying
a generic device (e.g. isa-serial in my example above)

>vs
>
>  <controller type='pci' model='pcie-root-port'>
>    <model name='ioh3420'/>
>  </controller>
>
>Either way, my point is that while the current serial device XML is a
>bit redundant, at least it's mostly consistent

consistent meaning it matches the QEMU devices?

BTW if you look at the title of this thread, it says 'debugcon-isa', while
the QEMU device is named 'isa-debugcon'.

>and its implementation
>is very simple;

Ah, the beauty of using virDomainChrSerialTargetModelTypeToString
instead of qemuDomainChrSerialTargetModelTypeToString

>what you suggest doing would compromise both of those
>positive facts without allowing us to remove any redundancy from the
>existing scenarios, so I think it would overall represent a net
>negative.

It allows us not to introduce more redundancy.

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190213/488e9ca5/attachment-0001.sig>