[libvirt] [PATCH v2 0/2] add debugcon-isa chardev guest interface
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
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@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
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
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?)
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
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 :|
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
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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
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
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.
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
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
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
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
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
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
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
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
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Nikolay Shirokovskiy