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

[Nikolay Shirokovskiy]

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