Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
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