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

[Andrea Bolognani]

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