5:31 a.m.
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
5:31 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
This interface can be used for example by firmware to print
debug messages. Here is domain xml example:
<serial type='pty'>
<target type='isa-serial' port='0'>
<model name='isa-debugcon'/>
</target>
<address type='isa' iobase='0x402'/>
</serial>
Add checks to make sure this new serial type won't be reported
as back-compat console.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)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 | 2 ++
src/qemu/qemu_domain.c | 3 ++
tests/qemuxml2argvdata/isa-serial-debugcon.xml | 36 ++++++++++++++++++++
tests/qemuxml2xmloutdata/isa-serial-debugcon.xml | 43 ++++++++++++++++++++++++
tests/qemuxml2xmltest.c | 2 ++
9 files changed, 94 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.xml
create mode 100644 tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7f07bb7..857c2af 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7089,7 +7089,8 @@ qemu-kvm -net nic,model=? /dev/null
<span class="since">Since 3.10.0</span>, the
<code>target</code>
element can have an optional <code>model</code> subelement;
valid values for its <code>name</code> attribute are:
- <code>isa-serial</code> (usable with the
<code>isa-serial</code> target
+ <code>isa-serial</code> and <span class="since">since
5.1.0</span>,
+ <code>isa-debugcon</code>(usable with the
<code>isa-serial</code> target
type); <code>usb-serial</code> (usable with the
<code>usb-serial</code>
target type); <code>pci-serial</code>
(usable with the <code>pci-serial</code> target type);
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index ba80440..8fbcae2 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3763,6 +3763,7 @@
<value>16550a</value>
<value>sclpconsole</value>
<value>sclplmconsole</value>
+ <value>isa-debugcon</value>
</choice>
</attribute>
</element>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1fc4c8a..628f5cc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -516,6 +516,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel,
"sclpconsole",
"sclplmconsole",
"16550a",
+ "isa-debugcon",
);
VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST,
@@ -4392,6 +4393,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def)
switch ((virDomainChrSerialTargetType) def->serials[0]->targetType) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+ if (def->serials[0]->targetModel ==
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON)
+ break;
+
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2bc3f87..8d066e0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1134,6 +1134,7 @@ typedef enum {
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE,
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE,
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A,
+ VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON,
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST
} virDomainChrSerialTargetModel;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 374836a..aeab619 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9364,6 +9364,7 @@ qemuChrSerialTargetModelToCaps(virDomainChrSerialTargetModel
targetModel)
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
return QEMU_CAPS_DEVICE_PL011;
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
+ case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
break;
@@ -10795,6 +10796,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
+ case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON:
caps = qemuChrSerialTargetModelToCaps(serial->targetModel);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b6c1a0e..90bc6de 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4376,6 +4376,8 @@ qemuDomainChrSerialTargetModelToTargetType(int targetModel)
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
+ case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON:
+ return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
break;
@@ -4438,6 +4440,7 @@ qemuDomainChrTargetDefValidate(const virDomainChrDef *chr)
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
+ case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON:
expected = qemuDomainChrSerialTargetModelToTargetType(chr->targetModel);
diff --git a/tests/qemuxml2argvdata/isa-serial-debugcon.xml
b/tests/qemuxml2argvdata/isa-serial-debugcon.xml
new file mode 100644
index 0000000..1c2e1c3
--- /dev/null
+++ b/tests/qemuxml2argvdata/isa-serial-debugcon.xml
@@ -0,0 +1,36 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='pipe'>
+ <source path='/tmp/debugcon'/>
+ <target type='isa-serial' port='0'>
+ <model name='isa-debugcon'/>
+ </target>
+ <address type='isa' iobase='0x402'/>
+ </serial>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
b/tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
new file mode 100644
index 0000000..29ead19
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/isa-serial-debugcon.xml
@@ -0,0 +1,43 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='pipe'>
+ <source path='/tmp/debugcon'/>
+ <target type='isa-serial' port='0'>
+ <model name='isa-debugcon'/>
+ </target>
+ <address type='isa' iobase='0x402'/>
+ </serial>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 85261cb..768f7f2 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -491,6 +491,8 @@ mymain(void)
DO_TEST("pci-rom-disabled-invalid", NONE);
DO_TEST("pci-serial-dev-chardev", NONE);
+ DO_TEST("isa-serial-debugcon", NONE);
+
DO_TEST("encrypted-disk", QEMU_CAPS_QCOW2_LUKS);
DO_TEST("encrypted-disk-usage", QEMU_CAPS_QCOW2_LUKS);
DO_TEST("luks-disks", NONE);
--
1.8.3.1
8:34 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
[...]
@@ -4392,6 +4393,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr
def)
switch ((virDomainChrSerialTargetType) def->serials[0]->targetType) {
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+ if (def->serials[0]->targetModel ==
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON)
+ break;
This doesn't compile:
conf/domain_conf.c: In function 'virDomainDefAddConsoleCompat':
conf/domain_conf.c:4396:16: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (def->serials[0]->targetModel ==
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON)
^
conf/domain_conf.c:4399:9: note: here
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
^~~~
You need to add ATTRIBUTE_FALLBACK after the break to make the
compiler happy. A short comment explaining why you're skipping
isa-debugcon would definitely be appreciated as well.
Even with that fixed, while your code prevents a <console> element
associated to the isa-debugcon to be automatically created, it
doesn't prevent something like
<console type='pty'/>
<serial type='file'>
<source path='...'/>
<target type='isa-serial'>
<model name='isa-debugcon'/>
</target>
</serial>
to result in the same problematic configuration, while the user
clearly wanted to have both a regular serial console *and* the
isa-debugcon.
I'm actually not sure we can make something very reasonable like
the above work... Perhaps we'll be forced to use <channel> after
all.
Adding Dan and Pavel so they can weigh in too.
[...]
+++ b/tests/qemuxml2argvdata/isa-serial-debugcon.xml
@@ -0,0 +1,36 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
You can trim this input file significantly, and by doing so you'll
end up with something that clearly highlights what feature it's
supposed to test:
<domain type='qemu'>
<name>QEMUGuest1</name>
<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
<memory unit='KiB'>219100</memory>
<vcpu placement='static'>1</vcpu>
<os>
<type arch='i686' machine='pc'>hvm</type>
</os>
<devices>
<emulator>/usr/bin/qemu-system-i686</emulator>
<controller type='usb' model='none'/>
<serial type='pipe'>
<source path='/tmp/debugcon'/>
<target type='isa-serial' port='0'>
<model name='isa-debugcon'/>
</target>
<address type='isa' iobase='0x402'/>
</serial>
<memballoon model='none'/>
</devices>
</domain>
This is all you need for the purpose of testing isa-debugcon.
+ <serial type='pipe'>
+ <source path='/tmp/debugcon'/>
+ <target type='isa-serial' port='0'>
+ <model name='isa-debugcon'/>
+ </target>
+ <address type='isa' iobase='0x402'/>
+ </serial>
So IIUC iobase=0x402 is the de-facto standard for isa-debugcon,
right?
Assuming that's indeed the case, I would expect libvirt to fill in
that value automatically unless the user has provided an iobase
explicitly themselves.
(Incidentally, and pre-existing, but shouldn't something like that
happen for isa-serial as well?)
--
Andrea Bolognani / Red Hat / Virtualization
2:14 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
On 08.02.2019 17:34, Andrea Bolognani wrote:
On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
[...]
> @@ -4392,6 +4393,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def)
>
> switch ((virDomainChrSerialTargetType) def->serials[0]->targetType) {
> case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
> + if (def->serials[0]->targetModel ==
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON)
> + break;
This doesn't compile:
conf/domain_conf.c: In function 'virDomainDefAddConsoleCompat':
conf/domain_conf.c:4396:16: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (def->serials[0]->targetModel ==
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON)
^
conf/domain_conf.c:4399:9: note: here
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
^~~~
You need to add ATTRIBUTE_FALLBACK after the break to make the
compiler happy. A short comment explaining why you're skipping
isa-debugcon would definitely be appreciated as well.
Even with that fixed, while your code prevents a <console> element
associated to the isa-debugcon to be automatically created, it
doesn't prevent something like
<console type='pty'/>
<serial type='file'>
<source path='...'/>
<target type='isa-serial'>
<model name='isa-debugcon'/>
</target>
</serial>
to result in the same problematic configuration, while the user
clearly wanted to have both a regular serial console *and* the
isa-debugcon.
Yeah I noticed that too but I though this is like case of usb-serial
for example. We do not add missing console in that case but allow
existing console to be alias of usb-serial. Can usb-serial actually be console?
I'm actually not sure we can make something very reasonable like
the above work... Perhaps we'll be forced to use <channel> after
all.
Adding Dan and Pavel so they can weigh in too.
[...]
> +++ b/tests/qemuxml2argvdata/isa-serial-debugcon.xml
> @@ -0,0 +1,36 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'
cpuset='1-4,8-20,525'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'/>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0'
model='pci-root'/>
You can trim this input file significantly, and by doing so you'll
end up with something that clearly highlights what feature it's
supposed to test:
<domain type='qemu'>
<name>QEMUGuest1</name>
<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
<memory unit='KiB'>219100</memory>
<vcpu placement='static'>1</vcpu>
<os>
<type arch='i686' machine='pc'>hvm</type>
</os>
<devices>
<emulator>/usr/bin/qemu-system-i686</emulator>
<controller type='usb' model='none'/>
<serial type='pipe'>
<source path='/tmp/debugcon'/>
<target type='isa-serial' port='0'>
<model name='isa-debugcon'/>
</target>
<address type='isa' iobase='0x402'/>
</serial>
<memballoon model='none'/>
</devices>
</domain>
This is all you need for the purpose of testing isa-debugcon.
> + <serial type='pipe'>
> + <source path='/tmp/debugcon'/>
> + <target type='isa-serial' port='0'>
> + <model name='isa-debugcon'/>
> + </target>
> + <address type='isa' iobase='0x402'/>
> + </serial>
So IIUC iobase=0x402 is the de-facto standard for isa-debugcon,
right?
Assuming that's indeed the case, I would expect libvirt to fill in
that value automatically unless the user has provided an iobase
explicitly themselves.
I wonder then why qemu uses a different value - 0xe9?
Nikolay
(Incidentally, and pre-existing, but shouldn't something like that
happen for isa-serial as well?)
7:40 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
On Mon, 2019-02-11 at 08:14 +0000, Nikolay Shirokovskiy wrote:
On 08.02.2019 17:34, Andrea Bolognani wrote:
> Even with that fixed, while your code prevents a <console> element
> associated to the isa-debugcon to be automatically created, it
> doesn't prevent something like
>
> <console type='pty'/>
> <serial type='file'>
> <source path='...'/>
> <target type='isa-serial'>
> <model name='isa-debugcon'/>
> </target>
> </serial>
>
> to result in the same problematic configuration, while the user
> clearly wanted to have both a regular serial console *and* the
> isa-debugcon.
Yeah I noticed that too but I though this is like case of usb-serial
for example. We do not add missing console in that case but allow
existing console to be alias of usb-serial.
Hm, that looks like a bug. I think we should be consistent about
this: either the aliasing between <serial/> and <console/> works
both ways, or it should not happen at all IMHO.
Can usb-serial actually be console?
No idea :)
> > + <serial type='pipe'>
> > + <source path='/tmp/debugcon'/>
> > + <target type='isa-serial' port='0'>
> > + <model name='isa-debugcon'/>
> > + </target>
> > + <address type='isa' iobase='0x402'/>
> > + </serial>
>
> So IIUC iobase=0x402 is the de-facto standard for isa-debugcon,
> right?
>
> Assuming that's indeed the case, I would expect libvirt to fill in
> that value automatically unless the user has provided an iobase
> explicitly themselves.
I wonder then why qemu uses a different value - 0xe9?
If that's the default value for isa-debugcon.iobase as far as QEMU
is concerned, then we should use that value in libvirt too. But I
thought you said 0x402 was the default?
--
Andrea Bolognani / Red Hat / Virtualization
7:44 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
On Mon, Feb 11, 2019 at 02:40:55PM +0100, Andrea Bolognani wrote:
On Mon, 2019-02-11 at 08:14 +0000, Nikolay Shirokovskiy wrote:
> On 08.02.2019 17:34, Andrea Bolognani wrote:
> > Even with that fixed, while your code prevents a <console> element
> > associated to the isa-debugcon to be automatically created, it
> > doesn't prevent something like
> >
> > <console type='pty'/>
> > <serial type='file'>
> > <source path='...'/>
> > <target type='isa-serial'>
> > <model name='isa-debugcon'/>
> > </target>
> > </serial>
> >
> > to result in the same problematic configuration, while the user
> > clearly wanted to have both a regular serial console *and* the
> > isa-debugcon.
>
> Yeah I noticed that too but I though this is like case of usb-serial
> for example. We do not add missing console in that case but allow
> existing console to be alias of usb-serial.
Hm, that looks like a bug. I think we should be consistent about
this: either the aliasing between <serial/> and <console/> works
both ways, or it should not happen at all IMHO.
I think that's fine in the case of usb-serial, as that's still an
admin interactive console channel, which is what our duplication
aims to represent.
It is not so good for isa-debugcon as that's is a special purpose
device.
> Can usb-serial actually be console?
No idea :)
Yes, that's fine IMHO.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:30 a.m.
New subject: [libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface
On 11.02.2019 16:40, Andrea Bolognani wrote:
On Mon, 2019-02-11 at 08:14 +0000, Nikolay Shirokovskiy wrote:
> On 08.02.2019 17:34, Andrea Bolognani wrote:
>> Even with that fixed, while your code prevents a <console> element
>> associated to the isa-debugcon to be automatically created, it
>> doesn't prevent something like
>>
>> <console type='pty'/>
>> <serial type='file'>
>> <source path='...'/>
>> <target type='isa-serial'>
>> <model name='isa-debugcon'/>
>> </target>
>> </serial>
>>
>> to result in the same problematic configuration, while the user
>> clearly wanted to have both a regular serial console *and* the
>> isa-debugcon.
>
> Yeah I noticed that too but I though this is like case of usb-serial
> for example. We do not add missing console in that case but allow
> existing console to be alias of usb-serial.
Hm, that looks like a bug. I think we should be consistent about
this: either the aliasing between <serial/> and <console/> works
both ways, or it should not happen at all IMHO.
> Can usb-serial actually be console?
No idea :)
>>> + <serial type='pipe'>
>>> + <source path='/tmp/debugcon'/>
>>> + <target type='isa-serial' port='0'>
>>> + <model name='isa-debugcon'/>
>>> + </target>
>>> + <address type='isa' iobase='0x402'/>
>>> + </serial>
>>
>> So IIUC iobase=0x402 is the de-facto standard for isa-debugcon,
>> right?
>>
>> Assuming that's indeed the case, I would expect libvirt to fill in
>> that value automatically unless the user has provided an iobase
>> explicitly themselves.
>
> I wonder then why qemu uses a different value - 0xe9?
If that's the default value for isa-debugcon.iobase as far as QEMU
is concerned, then we should use that value in libvirt too. But I
thought you said 0x402 was the default?
AFAIK different firmwares have diferent iobase:
OMVF - 0x402
Seabios - 0x402
IPXE - 0xe9
So QEMU just takes iobase of IPXE. May be should just make iobase mandatory instead
of taking value of arbitrary firmware?
Nikolay
5:31 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_command.c | 8 ++++---
tests/qemuxml2argvdata/isa-serial-debugcon.args | 29 +++++++++++++++++++++++++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 35 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/isa-serial-debugcon.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aeab619..862bb2e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
info->addr.ccw.ssid,
info->addr.ccw.devno);
} else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
- virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
- info->addr.isa.iobase,
- info->addr.isa.irq);
+ if (info->addr.isa.iobase)
+ virBufferAsprintf(buf, ",iobase=0x%x", info->addr.isa.iobase);
+
+ if (info->addr.isa.irq)
+ virBufferAsprintf(buf, ",irq=0x%x", info->addr.isa.irq);
}
ret = 0;
diff --git a/tests/qemuxml2argvdata/isa-serial-debugcon.args
b/tests/qemuxml2argvdata/isa-serial-debugcon.args
new file mode 100644
index 0000000..c7faab4
--- /dev/null
+++ b/tests/qemuxml2argvdata/isa-serial-debugcon.args
@@ -0,0 +1,29 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
+bootindex=1 \
+-chardev pipe,id=charserial0,path=/tmp/debugcon \
+-device isa-debugcon,chardev=charserial0,id=serial0,iobase=0x402 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6dc05c3..3569391 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1490,6 +1490,7 @@ mymain(void)
QEMU_CAPS_DEVICE_ISA_SERIAL);
DO_TEST("pci-serial-dev-chardev",
QEMU_CAPS_DEVICE_PCI_SERIAL);
+ DO_TEST("isa-serial-debugcon", NONE);
DO_TEST("channel-guestfwd", NONE);
DO_TEST_CAPS_VER("channel-unix-guestfwd", "2.5.0");
--
1.8.3.1
9:02 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
[...]
@@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
info->addr.ccw.ssid,
info->addr.ccw.devno);
} else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
- virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
- info->addr.isa.iobase,
- info->addr.isa.irq);
+ if (info->addr.isa.iobase)
+ virBufferAsprintf(buf, ",iobase=0x%x", info->addr.isa.iobase);
+
+ if (info->addr.isa.irq)
+ virBufferAsprintf(buf, ",irq=0x%x", info->addr.isa.irq);
It's entirely unclear to me why you're doing this. Can you please
provide some explanation?
Also note that this won't just affect isa-debugcon but also any
other device with an ISA address, so I really don't think you can
just go ahead and change it without breaking existing guests.
[...]
+++ b/tests/qemuxml2argvtest.c
@@ -1490,6 +1490,7 @@ mymain(void)
QEMU_CAPS_DEVICE_ISA_SERIAL);
DO_TEST("pci-serial-dev-chardev",
QEMU_CAPS_DEVICE_PCI_SERIAL);
+ DO_TEST("isa-serial-debugcon", NONE);
DO_TEST("channel-guestfwd", NONE);
DO_TEST_CAPS_VER("channel-unix-guestfwd", "2.5.0");
If you had included this hunk in the previous commit, then the
QEMU command line would have been generated right away... I don't
think this belongs in a separate commit.
--
Andrea Bolognani / Red Hat / Virtualization
2:53 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On 08.02.2019 18:02, Andrea Bolognani wrote:
On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
[...]
> @@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
> info->addr.ccw.ssid,
> info->addr.ccw.devno);
> } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
> - virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
> - info->addr.isa.iobase,
> - info->addr.isa.irq);
> + if (info->addr.isa.iobase)
> + virBufferAsprintf(buf, ",iobase=0x%x",
info->addr.isa.iobase);
> +
> + if (info->addr.isa.irq)
> + virBufferAsprintf(buf, ",irq=0x%x", info->addr.isa.irq);
It's entirely unclear to me why you're doing this. Can you please
provide some explanation?
Both irq and iobase are optional and value reserved for "not specified" is 0.
However we pass this 0 value as it was set explicitly to qemu. This is odd.
For example if we have 2 isa-serials with addresses without irq then both
have irq=0. Is this meaningful configuration?
Specifying irq for debugcon just does not work:
error: internal error: qemu unexpectedly closed the monitor: 2019-02-11T08:33:00.460078Z
qemu-kvm: -device isa-debugcon,chardev=charserial0,id=serial0,iobase=0x402,irq=0x0:
Property '.irq' not found
Nikolay
Also note that this won't just affect isa-debugcon but also any
other device with an ISA address, so I really don't think you can
just go ahead and change it without breaking existing guests.
[...]
> +++ b/tests/qemuxml2argvtest.c
> @@ -1490,6 +1490,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_ISA_SERIAL);
> DO_TEST("pci-serial-dev-chardev",
> QEMU_CAPS_DEVICE_PCI_SERIAL);
> + DO_TEST("isa-serial-debugcon", NONE);
>
> DO_TEST("channel-guestfwd", NONE);
> DO_TEST_CAPS_VER("channel-unix-guestfwd", "2.5.0");
If you had included this hunk in the previous commit, then the
QEMU command line would have been generated right away... I don't
think this belongs in a separate commit.
11:23 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On Mon, 2019-02-11 at 08:53 +0000, Nikolay Shirokovskiy wrote:
On 08.02.2019 18:02, Andrea Bolognani wrote:
> On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
> [...]
> > @@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
> > info->addr.ccw.ssid,
> > info->addr.ccw.devno);
> > } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
> > - virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
> > - info->addr.isa.iobase,
> > - info->addr.isa.irq);
> > + if (info->addr.isa.iobase)
> > + virBufferAsprintf(buf, ",iobase=0x%x",
info->addr.isa.iobase);
> > +
> > + if (info->addr.isa.irq)
> > + virBufferAsprintf(buf, ",irq=0x%x",
info->addr.isa.irq);
>
> It's entirely unclear to me why you're doing this. Can you please
> provide some explanation?
Both irq and iobase are optional and value reserved for "not specified" is 0.
However we pass this 0 value as it was set explicitly to qemu. This is odd.
For example if we have 2 isa-serials with addresses without irq then both
have irq=0. Is this meaningful configuration?
Specifying irq for debugcon just does not work:
error: internal error: qemu unexpectedly closed the monitor: 2019-02-11T08:33:00.460078Z
qemu-kvm: -device isa-debugcon,chardev=charserial0,id=serial0,iobase=0x402,irq=0x0:
Property '.irq' not found
Okay, I see now why you'd want to be able to print one and not the
other.
However, right now if you have an address such as
<address type='isa' iobase='0x402'/>
it will be formatted as
iobase=0x402,irq=0x0
and I don't quite see how we could stop formatting the unassigned
attribute without breaking compatibility with that behavior. Unless
of course we can prove that QEMU always defaults to 0x0 for both
when not specified...
Another interesting fact I've noticed is that, unlike what happens
for usb-serial, pci-serial and spapr-vio-serial, if you have
<serial type='pty'>
<target type='isa-serial'/>
</serial>
an appropriate <address> element will *not* be added by libvirt.
That also seems wrong, but again unless QEMU defaults to 0x0 for
those options I don't think we can quite fix it :(
--
Andrea Bolognani / Red Hat / Virtualization
8:15 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On 11.02.2019 20:23, Andrea Bolognani wrote:
On Mon, 2019-02-11 at 08:53 +0000, Nikolay Shirokovskiy wrote:
> On 08.02.2019 18:02, Andrea Bolognani wrote:
>> On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
>> [...]
>>> @@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>>> info->addr.ccw.ssid,
>>> info->addr.ccw.devno);
>>> } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
>>> - virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
>>> - info->addr.isa.iobase,
>>> - info->addr.isa.irq);
>>> + if (info->addr.isa.iobase)
>>> + virBufferAsprintf(buf, ",iobase=0x%x",
info->addr.isa.iobase);
>>> +
>>> + if (info->addr.isa.irq)
>>> + virBufferAsprintf(buf, ",irq=0x%x",
info->addr.isa.irq);
>>
>> It's entirely unclear to me why you're doing this. Can you please
>> provide some explanation?
>
> Both irq and iobase are optional and value reserved for "not specified" is
0.
> However we pass this 0 value as it was set explicitly to qemu. This is odd.
> For example if we have 2 isa-serials with addresses without irq then both
> have irq=0. Is this meaningful configuration?
>
> Specifying irq for debugcon just does not work:
>
> error: internal error: qemu unexpectedly closed the monitor:
2019-02-11T08:33:00.460078Z qemu-kvm: -device
isa-debugcon,chardev=charserial0,id=serial0,iobase=0x402,irq=0x0: Property '.irq'
not found
Okay, I see now why you'd want to be able to print one and not the
other.
However, right now if you have an address such as
<address type='isa' iobase='0x402'/>
it will be formatted as
iobase=0x402,irq=0x0
and I don't quite see how we could stop formatting the unassigned
attribute without breaking compatibility with that behavior. Unless
of course we can prove that QEMU always defaults to 0x0 for both
when not specified...
QEMU defaults to *not* 0x0:
# git grep '"irq"' | grep DEFINE_PROP_UINT32
hw/audio/cs4231a.c: DEFINE_PROP_UINT32 ("irq", CSState, irq, 9),
hw/audio/gus.c: DEFINE_PROP_UINT32 ("irq", GUSState, emu.gusirq, 7),
hw/audio/sb16.c: DEFINE_PROP_UINT32 ("irq", SB16State, irq, 5),
hw/block/fdc.c: DEFINE_PROP_UINT32("irq", FDCtrlISABus, irq, 6),
hw/char/parallel.c: DEFINE_PROP_UINT32("irq", ISAParallelState, isairq,
7),
hw/char/serial-isa.c: DEFINE_PROP_UINT32("irq", ISASerialState, isairq,
-1),
hw/ide/isa.c: DEFINE_PROP_UINT32("irq", ISAIDEState, isairq, 14),
hw/net/ne2000-isa.c: DEFINE_PROP_UINT32("irq", ISANE2000State, isairq, 9),
hw/tpm/tpm_tis.c: DEFINE_PROP_UINT32("irq", TPMState, irq_num, TPM_TIS_IRQ),
# git grep '"iobase"' | grep DEFINE_PROP_UINT32
hw/audio/adlib.c: DEFINE_PROP_UINT32 ("iobase", AdlibState, port, 0x220),
hw/audio/cs4231a.c: DEFINE_PROP_UINT32 ("iobase", CSState, port, 0x534),
hw/audio/gus.c: DEFINE_PROP_UINT32 ("iobase", GUSState, port,
0x240),
hw/audio/pcspk.c: DEFINE_PROP_UINT32("iobase", PCSpkState, iobase, -1),
hw/audio/sb16.c: DEFINE_PROP_UINT32 ("iobase", SB16State, port, 0x220),
hw/block/fdc.c: DEFINE_PROP_UINT32("iobase", FDCtrlISABus, iobase, 0x3f0),
hw/char/debugcon.c: DEFINE_PROP_UINT32("iobase", ISADebugconState, iobase,
0xe9),
hw/char/parallel.c: DEFINE_PROP_UINT32("iobase", ISAParallelState, iobase,
-1),
hw/char/serial-isa.c: DEFINE_PROP_UINT32("iobase", ISASerialState, iobase,
-1),
hw/dma/i82374.c: DEFINE_PROP_UINT32("iobase", I82374State, iobase, 0x400),
hw/i386/kvm/i8254.c: DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,
-1),
hw/ide/isa.c: DEFINE_PROP_UINT32("iobase", ISAIDEState, iobase, 0x1f0),
hw/intc/i8259_common.c: DEFINE_PROP_UINT32("iobase", PICCommonState, iobase,
-1),
hw/misc/debugexit.c: DEFINE_PROP_UINT32("iobase", ISADebugExitState, iobase,
0x501),
hw/net/ne2000-isa.c: DEFINE_PROP_UINT32("iobase", ISANE2000State, iobase,
0x300),
hw/timer/i8254.c: DEFINE_PROP_UINT32("iobase", PITCommonState, iobase, -1),
hw/timer/m48t59-isa.c: DEFINE_PROP_UINT32("iobase", M48txxISAState, io_base,
0x74),
I guess most users live with this defaults and don't use <address>.
I can not prove that of course. But if they do and use address without either irq or
iobase
then they will have conflicting 0 settings which is invalid config I guess.
Another interesting fact I've noticed is that, unlike what happens
for usb-serial, pci-serial and spapr-vio-serial, if you have
<serial type='pty'>
<target type='isa-serial'/>
</serial>
an appropriate <address> element will *not* be added by libvirt.
That also seems wrong, but again unless QEMU defaults to 0x0 for
those options I don't think we can quite fix it :(
Probably auto assigning addresses for isa is not trivial task or even
assigning arbitrary values won't work. I don't have knowledge in this area
unfortunately.
So I belive to some extent that nobody uses <address> without explicitly
setting both irq and iobase. Then we take the discussed hunk. Then one can
start omitting irq or iobase. Also we would like then to use different value
for "not specified" rather then 0 as irq = 0 is a valid value.
Nikolay
8:29 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On Mon, Feb 11, 2019 at 06:23:32PM +0100, Andrea Bolognani wrote:
On Mon, 2019-02-11 at 08:53 +0000, Nikolay Shirokovskiy wrote:
> On 08.02.2019 18:02, Andrea Bolognani wrote:
> > On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
> > [...]
> > > @@ -393,9 +393,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
> > > info->addr.ccw.ssid,
> > > info->addr.ccw.devno);
> > > } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) {
> > > - virBufferAsprintf(buf, ",iobase=0x%x,irq=0x%x",
> > > - info->addr.isa.iobase,
> > > - info->addr.isa.irq);
> > > + if (info->addr.isa.iobase)
> > > + virBufferAsprintf(buf, ",iobase=0x%x",
info->addr.isa.iobase);
> > > +
> > > + if (info->addr.isa.irq)
> > > + virBufferAsprintf(buf, ",irq=0x%x",
info->addr.isa.irq);
> >
> > It's entirely unclear to me why you're doing this. Can you please
> > provide some explanation?
>
> Both irq and iobase are optional and value reserved for "not specified" is
0.
> However we pass this 0 value as it was set explicitly to qemu. This is odd.
> For example if we have 2 isa-serials with addresses without irq then both
> have irq=0. Is this meaningful configuration?
>
> Specifying irq for debugcon just does not work:
>
> error: internal error: qemu unexpectedly closed the monitor:
2019-02-11T08:33:00.460078Z qemu-kvm: -device
isa-debugcon,chardev=charserial0,id=serial0,iobase=0x402,irq=0x0: Property '.irq'
not found
Okay, I see now why you'd want to be able to print one and not the
other.
However, right now if you have an address such as
<address type='isa' iobase='0x402'/>
it will be formatted as
iobase=0x402,irq=0x0
and I don't quite see how we could stop formatting the unassigned
attribute without breaking compatibility with that behavior. Unless
of course we can prove that QEMU always defaults to 0x0 for both
when not specified...
Setting IRQ to 0 is nonsense for ISA devices, why would we need to stay compatible with
that?
Another interesting fact I've noticed is that, unlike what happens
for usb-serial, pci-serial and spapr-vio-serial, if you have
<serial type='pty'>
<target type='isa-serial'/>
</serial>
an appropriate <address> element will *not* be added by libvirt.
That also seems wrong, but again unless QEMU defaults to 0x0 for
those options I don't think we can quite fix it :(
IMO if the user does not fill it, they are okay with the default. And we
should reflect the default in the XML (IOW fill it with what QEMU fills in
right now).
Jano
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:31 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev
On Fri, Feb 08, 2019 at 04:02:38PM +0100, Andrea Bolognani wrote:
On Thu, 2019-02-07 at 14:31 +0300, Nikolay Shirokovskiy wrote:
> +++ b/tests/qemuxml2argvtest.c
> @@ -1490,6 +1490,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_ISA_SERIAL);
> DO_TEST("pci-serial-dev-chardev",
> QEMU_CAPS_DEVICE_PCI_SERIAL);
> + DO_TEST("isa-serial-debugcon", NONE);
>
> DO_TEST("channel-guestfwd", NONE);
> DO_TEST_CAPS_VER("channel-unix-guestfwd", "2.5.0");
If you had included this hunk in the previous commit, then the
QEMU command line would have been generated right away... I don't
think this belongs in a separate commit.
That's a consequence of the compiler guarding the switches with all the
enums. The XML parser and QEMU command line formatter would ideally be
in separate commits, but due to the -Wswitch-enum option a complete
separation would fail.
Jano
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:37 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:44 a.m.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
9:07 a.m.
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
10:21 a.m.
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
3:03 a.m.
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
3:59 a.m.
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
6:29 a.m.
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
10:18 a.m.
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
5:33 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:28 a.m.
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
2073
days inactive
2080
days old
23 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Nikolay Shirokovskiy