[libvirt] [PATCH v2 1/2] conf: add debugcon chardev guest interface

[Nikolay Shirokovskiy]

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