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

[Andrea Bolognani]

On Wed, 2019-02-13 at 13:29 +0100, Ján Tomko wrote:
> 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:
> > > 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"

I enthusiastically backed a proposal for improvement literally in the
paragraph you're replying to :)

> >  <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)

There are no "generic" devices, only hypervisor-specific devices that
present a similar interface to the guest but are completely separate
and not interchangeable from the hypervisor's point of view.

For controllers, we could have decided to use "intel-pcie-root-port"
instead of "ioh3420" and then translate back and forth, but I guess
at the time it was considered to be not worth doing, or perhaps the
lack of redundancy resulted in the issue not being raised at all.

> > 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?

Yes, thus matching how controllers (and possibly other devices?)
work. When I introduced the <model> element for serial devices,
that's what I based the idea on, so it makes sense to me that we'll
keep following the same semantics.

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

Clearly an oversight: if you look through the patches, you'll see
that there is no 'debucon-isa' anywhere in the code.

> > and its implementation
> > is very simple;
> 
> Ah, the beauty of using virDomainChrSerialTargetModelTypeToString
> instead of qemuDomainChrSerialTargetModelTypeToString

What about the beauty of not having to implement
qemuDomainChrSerialTargetModelTypeToString() in the first place? :)

> > 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.

I don't believe avoiding those four extra bytes is worth the extra
code complexity and deviating from well-established semantics.

-- 
Andrea Bolognani / Red Hat / Virtualization