[libvirt] [PATCH v2 2/2] qemu: implement debugcon-isa chardev

[Nikolay Shirokovskiy]

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.
>