Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
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.
> > 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.
<controller type='pci' model='pcie-root-port'>
<model name='pcie-root-port'/>
</controller>
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 and its implementation
is very simple; 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.
--
Andrea Bolognani / Red Hat / Virtualization