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