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

Nikolay Shirokovskiy posted 2 patches 5 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1549539109-412590-1-git-send-email-nshirokovskiy@virtuozzo.com
docs/formatdomain.html.in                        |  3 +-
docs/schemas/domaincommon.rng                    |  1 +
src/conf/domain_conf.c                           |  4 +++
src/conf/domain_conf.h                           |  1 +
src/qemu/qemu_command.c                          | 10 ++++--
src/qemu/qemu_domain.c                           |  3 ++
tests/qemuxml2argvdata/isa-serial-debugcon.args  | 29 ++++++++++++++++
tests/qemuxml2argvdata/isa-serial-debugcon.xml   | 36 ++++++++++++++++++++
tests/qemuxml2argvtest.c                         |  1 +
tests/qemuxml2xmloutdata/isa-serial-debugcon.xml | 43 ++++++++++++++++++++++++
tests/qemuxml2xmltest.c                          |  2 ++
11 files changed, 129 insertions(+), 4 deletions(-)
create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.args
create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.xml
create mode 100644 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
[libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Nikolay Shirokovskiy 5 years, 1 month ago
Diff from v1 [1]:
=================
- expose the device as serial device instead of channel in config

I use isa-debugcon name becase libvirt passes these names to qemu as-is
so I don't want to make exception for this device.

First version discussion:
[1] https://www.redhat.com/archives/libvir-list/2019-January/msg00951.html

Nikolay Shirokovskiy (2):
  conf: add debugcon chardev guest interface
  qemu: implement debugcon-isa chardev

 docs/formatdomain.html.in                        |  3 +-
 docs/schemas/domaincommon.rng                    |  1 +
 src/conf/domain_conf.c                           |  4 +++
 src/conf/domain_conf.h                           |  1 +
 src/qemu/qemu_command.c                          | 10 ++++--
 src/qemu/qemu_domain.c                           |  3 ++
 tests/qemuxml2argvdata/isa-serial-debugcon.args  | 29 ++++++++++++++++
 tests/qemuxml2argvdata/isa-serial-debugcon.xml   | 36 ++++++++++++++++++++
 tests/qemuxml2argvtest.c                         |  1 +
 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml | 43 ++++++++++++++++++++++++
 tests/qemuxml2xmltest.c                          |  2 ++
 11 files changed, 129 insertions(+), 4 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.args
 create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.xml
 create mode 100644 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Ján Tomko 5 years, 1 month ago
On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote:
>Diff from v1 [1]:
>=================
>- expose the device as serial device instead of channel in config
>
>I use isa-debugcon name becase libvirt passes these names to qemu as-is
>so I don't want to make exception for this device.
>

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.

Jano

>First version discussion:
>[1] https://www.redhat.com/archives/libvir-list/2019-January/msg00951.html
>
>Nikolay Shirokovskiy (2):
>  conf: add debugcon chardev guest interface
>  qemu: implement debugcon-isa chardev
>
> docs/formatdomain.html.in                        |  3 +-
> docs/schemas/domaincommon.rng                    |  1 +
> src/conf/domain_conf.c                           |  4 +++
> src/conf/domain_conf.h                           |  1 +
> src/qemu/qemu_command.c                          | 10 ++++--
> src/qemu/qemu_domain.c                           |  3 ++
> tests/qemuxml2argvdata/isa-serial-debugcon.args  | 29 ++++++++++++++++
> tests/qemuxml2argvdata/isa-serial-debugcon.xml   | 36 ++++++++++++++++++++
> tests/qemuxml2argvtest.c                         |  1 +
> tests/qemuxml2xmloutdata/isa-serial-debugcon.xml | 43 ++++++++++++++++++++++++
> tests/qemuxml2xmltest.c                          |  2 ++
> 11 files changed, 129 insertions(+), 4 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.args
> create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.xml
> create mode 100644 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
>
>-- 
>1.8.3.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Nikolay Shirokovskiy 5 years, 1 month ago

On 12.02.2019 17:37, Ján Tomko wrote:
> On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote:
>> Diff from v1 [1]:
>> =================
>> - expose the device as serial device instead of channel in config
>>
>> I use isa-debugcon name becase libvirt passes these names to qemu as-is
>> so I don't want to make exception for this device.
>>
> 
> 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.
> 

Yeah. But we already have models like isa-serial, usb-serial etc. And thus
we don't need map libvirt models to qemu models i.e. internally
we use virDomainChrSerialTargetModelTypeToString to generates names for
qemu. It would be odd if I start to use map just for debugcon now.

Nikolay

> 
>> First version discussion:
>> [1] https://www.redhat.com/archives/libvir-list/2019-January/msg00951.html
>>
>> Nikolay Shirokovskiy (2):
>>  conf: add debugcon chardev guest interface
>>  qemu: implement debugcon-isa chardev
>>
>> docs/formatdomain.html.in                        |  3 +-
>> docs/schemas/domaincommon.rng                    |  1 +
>> src/conf/domain_conf.c                           |  4 +++
>> src/conf/domain_conf.h                           |  1 +
>> src/qemu/qemu_command.c                          | 10 ++++--
>> src/qemu/qemu_domain.c                           |  3 ++
>> tests/qemuxml2argvdata/isa-serial-debugcon.args  | 29 ++++++++++++++++
>> tests/qemuxml2argvdata/isa-serial-debugcon.xml   | 36 ++++++++++++++++++++
>> tests/qemuxml2argvtest.c                         |  1 +
>> tests/qemuxml2xmloutdata/isa-serial-debugcon.xml | 43 ++++++++++++++++++++++++
>> tests/qemuxml2xmltest.c                          |  2 ++
>> 11 files changed, 129 insertions(+), 4 deletions(-)
>> create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.args
>> create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.xml
>> create mode 100644 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
>>
>> -- 
>> 1.8.3.1
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Ján Tomko 5 years, 1 month ago
On Tue, Feb 12, 2019 at 02:44:05PM +0000, Nikolay Shirokovskiy wrote:
>
>
>On 12.02.2019 17:37, Ján Tomko wrote:
>> On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote:
>>> Diff from v1 [1]:
>>> =================
>>> - expose the device as serial device instead of channel in config
>>>
>>> I use isa-debugcon name becase libvirt passes these names to qemu as-is
>>> so I don't want to make exception for this device.
>>>
>>
>> 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.
>>
>
>Yeah. But we already have models like isa-serial, usb-serial etc. And thus
>we don't need map libvirt models to qemu models i.e. internally
>we use virDomainChrSerialTargetModelTypeToString to generates names for
>qemu. It would be odd if I start to use map just for debugcon now.
>

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>

Jano

>Nikolay
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Andrea Bolognani 5 years, 1 month ago
On Tue, 2019-02-12 at 16:07 +0100, Ján Tomko wrote:
> On Tue, Feb 12, 2019 at 02:44:05PM +0000, Nikolay Shirokovskiy wrote:
> > On 12.02.2019 17:37, Ján Tomko wrote:
> > > On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote:
> > > > Diff from v1 [1]:
> > > > =================
> > > > - expose the device as serial device instead of channel in config
> > > > 
> > > > I use isa-debugcon name becase libvirt passes these names to qemu as-is
> > > > so I don't want to make exception for this device.
> > > 
> > > 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.

> > Yeah. But we already have models like isa-serial, usb-serial etc. And thus
> > we don't need map libvirt models to qemu models i.e. internally
> > we use virDomainChrSerialTargetModelTypeToString to generates names for
> > qemu. It would be odd if I start to use map just for debugcon now.
> 
> 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, so I don't really see a compelling
reason to start adding inconsistencies now...

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Ján Tomko 5 years, 1 month ago
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:
>> On Tue, Feb 12, 2019 at 02:44:05PM +0000, Nikolay Shirokovskiy wrote:
>> > On 12.02.2019 17:37, Ján Tomko wrote:
>> > > On Thu, Feb 07, 2019 at 02:31:47PM +0300, Nikolay Shirokovskiy wrote:
>> > > > Diff from v1 [1]:
>> > > > =================
>> > > > - expose the device as serial device instead of channel in config
>> > > >
>> > > > I use isa-debugcon name becase libvirt passes these names to qemu as-is
>> > > > so I don't want to make exception for this device.
>> > >
>> > > 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.
Also, the device will be given an iobase by QEMU, we should represent
that in the XML and fill in that default.

>> > Yeah. But we already have models like isa-serial, usb-serial etc. And thus
>> > we don't need map libvirt models to qemu models i.e. internally
>> > we use virDomainChrSerialTargetModelTypeToString to generates names for
>> > qemu. It would be odd if I start to use map just for debugcon now.
>>
>> 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"

>so I don't really see a compelling
>reason to start adding inconsistencies now...
>

  Do you remember when you lost your passion for this work?
    -- Lisa Simpson

Jano

>-- 
>Andrea Bolognani / Red Hat / Virtualization
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Andrea Bolognani 5 years, 1 month ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Ján Tomko 5 years, 1 month ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Andrea Bolognani 5 years, 1 month ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Ján Tomko 5 years, 1 month ago
On Wed, Feb 13, 2019 at 05:18:55PM +0100, Andrea Bolognani wrote:
>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.
>

Nonsense.

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

Given that your only compelling argument is "it's extra work" I went
ahead and done the work:
https://www.redhat.com/archives/libvir-list/2019-February/msg00860.html

Jano

>> > 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
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
Posted by Andrea Bolognani 5 years, 1 month ago
On Thu, 2019-02-14 at 12:33 +0100, Ján Tomko wrote:
> Given that your only compelling argument is "it's extra work" I went
> ahead and done the work:
> https://www.redhat.com/archives/libvir-list/2019-February/msg00860.html

I won't NACK that series, even though I disagree with the approach
taken. But, just so we're clear, I'm definitely not going to review
it either.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list