Re: [Qemu-devel] [PATCH 5/6] prep: add pc87312 Super I/O emulation

["Hervé Poussineau" <hpoussin@reactos.org>]

Andreas Färber a écrit :
> Am 17.03.2012 15:39, schrieb Hervé Poussineau:
>> This provides floppy and IDE controllers as well as serial and parallel ports.
>> However, dynamic configuration of devices is not yet supported.
>>
>> Cc: Andreas Färber <andreas.faerber@web.de>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  Makefile.objs |    1 +
>>  hw/pc87312.c  |  425 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 426 insertions(+), 0 deletions(-)
>>  create mode 100644 hw/pc87312.c
> 
>> diff --git a/hw/pc87312.c b/hw/pc87312.c
>> new file mode 100644
>> index 0000000..1e28dbd
>> --- /dev/null
>> +++ b/hw/pc87312.c
>> @@ -0,0 +1,425 @@
>> +/*
>> + * QEMU National Semiconductor PC87312 (Super I/O)
>> + *
>> + * Copyright (c) 2010-2012 Herve Poussineau
> 
> FWIW mind to add
> 
> Copyright (c) 2011 Andreas Färber
> 
> ?

Yes, of course. Sorry about that.

>> +
>> +    chr = s->parallel.chr;
>> +    if (s->parallel.chr != NULL && is_parallel_enabled(s)) {
> 
> This logic still seems to be flawed: it should not depend on
> s->parallel.chr. If that is NULL we need to create a null char device to
> match the device that's present in hardware according to
> is_parallel_enabled(s).

Ok, will remove the 'chr != NULL' check.

> 
>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK */
> 
> HACK alarm in [PATCH]: What for?

The problem is composition. Main board contains a pc87312, which 
contains a isa-parallel. Main board doesn't know the isa-parallel device.

isa-parallel device must have a chardev (set by a property). pc87312 
creates the isa-parallel device, so it must know the chardev. I did it 
also with a chardev property.
Unfortunately, a chardev can only be used once, and I have not found a 
better way to give chardev from main board to superI/O to parallel.
Do you have any better idea?


>> +    for (i = 0; i < 2; i++) {
>> +        chr = s->uart[i].chr;
> 
>> +        if (chr != NULL && is_uart_enabled(s, i)) {
>> +            qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK */
> 
> 2x ditto.

Same answer :)

>> +
>> +static void pc87312_class_initfn(ObjectClass *klass, void *data)
> 
> I always thought initfn was used for instances...

Ok, will change to pc87312_class_init

>> +
>> +type_init(pc87312_register_types)
>> +
> 
> Trailing empty line.

Ok, will remove.

> 
> So what about the ugly ISA hot-plug issue that originally stalled our
> two series? I'm missing a Change Log about that. You changed the initial
> configuration to the one used by 40P firmware IIRC and stopped
> supporting the chipset's reconfiguration? Either way any limitation of
> this implementation should be prominently documented please.

Yes, chipset reconfiguration required ISA hot-plug issue, so I didn't 
want to wait for this serie. In commit message, I wrote "However, 
dynamic configuration of devices is not yet supported.", and you get an 
error message at each reconfiguration:

+static void reconfigure_devices(PC87312State *s)
+{
+    error_report("pc87312: unsupported device reconfiguration (%02x 
%02x %02x)",
+                 s->FER, s->FAR, s->PTR);
+}

What else should I add?
Anyway, if/when dynamic reconfiguration will be implemented, only this 
method will be changed.


About the initial configuration, it is controlled by "config" property.
In patch 6/6, when I use the Super I/O chip in 'prep' machine, I set 
this property to 13, which means fdc + serial0 + serial1 + parallel0.
For IBM 40p, the initial configuration should be 15 (fdc + ide + serial0 
+ parallel0).
So I don't think the Super I/O is exclusively tied to IBM 40p.


> 
> Thanks for your work on this,

Thanks

Hervé