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

[Andreas Färber]

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 <address@hidden>
> Signed-off-by: Hervé Poussineau <address@hidden>
> ---
>  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

?

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */

> +//#define DEBUG_PC87312
> +
> +#ifdef DEBUG_PC87312
> +#define DPRINTF(fmt, ...) \
> +do { fprintf(stderr, "pc87312: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +do {} while (0)
> +#endif

Mid-term we should replace this through proper tracing.

> +static int pc87312_init(ISADevice *dev)
> +{
> +    PC87312State *s;
> +    ISADevice *isa;
> +    ISABus *bus;
> +    CharDriverState *chr;
> +    BlockDriverState *bs;
> +    int i;
> +
> +    s = DO_UPCAST(PC87312State, dev, dev);
> +    bus = isa_bus_from_device(dev);
> +    pc87312_hard_reset(s);
> +
> +    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).

> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK */

HACK alarm in [PATCH]: What for?

> +        isa = isa_create(bus, "isa-parallel");
> +        qdev_prop_set_uint32(&isa->qdev, "index", 0);
> +        qdev_prop_set_uint32(&isa->qdev, "iobase", get_parallel_iobase(s));
> +        qdev_prop_set_uint32(&isa->qdev, "irq", get_parallel_irq(s));
> +        qdev_prop_set_chr(&isa->qdev, "chardev", chr);
> +        qdev_init_nofail(&isa->qdev);
> +        s->parallel.dev = &isa->qdev;
> +        DPRINTF("parallel: base 0x%x, irq %u\n",
> +                get_parallel_iobase(s), get_parallel_irq(s));
> +    }
> +
> +    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.

> +            isa = isa_create(bus, "isa-serial");
> +            qdev_prop_set_uint32(&isa->qdev, "index", i);
> +            qdev_prop_set_uint32(&isa->qdev, "iobase", get_uart_iobase(s, 
> i));
> +            qdev_prop_set_uint32(&isa->qdev, "irq", get_uart_irq(s, i));
> +            qdev_prop_set_chr(&isa->qdev, "chardev", chr);
> +            qdev_init_nofail(&isa->qdev);
> +            s->uart[i].dev = &isa->qdev;
> +            DPRINTF("uart%d: base 0x%x, irq %u\n", i,
> +                    get_uart_iobase(s, i),
> +                    get_uart_irq(s, i));
> +        }
> +    }
> +
> +    if (is_fdc_enabled(s)) {
> +        isa = isa_create(bus, "isa-fdc");
> +        qdev_prop_set_uint32(&isa->qdev, "iobase", get_fdc_iobase(s));
> +        qdev_prop_set_uint32(&isa->qdev, "irq", 6);
> +        bs = s->fdc.drive[0];
> +        if (bs != NULL) {
> +            bdrv_detach_dev(bs, bdrv_get_attached_dev(bs)); /* HACK */
> +            qdev_prop_set_drive_nofail(&isa->qdev, "driveA", bs);
> +        }
> +        bs = s->fdc.drive[1];
> +        if (bs != NULL) {
> +            bdrv_detach_dev(bs, bdrv_get_attached_dev(bs)); /* HACK */
> +            qdev_prop_set_drive_nofail(&isa->qdev, "driveB", bs);
> +        }
> +        qdev_init_nofail(&isa->qdev);
> +        s->fdc.dev = &isa->qdev;
> +        DPRINTF("fdc: base 0x%x\n", get_fdc_iobase(s));
> +    }
> +
> +    if (is_ide_enabled(s)) {
> +        isa = isa_create(bus, "isa-ide");
> +        qdev_prop_set_uint32(&isa->qdev, "iobase", get_ide_iobase(s));
> +        qdev_prop_set_uint32(&isa->qdev, "iobase2", get_ide_iobase(s) + 
> 0x206);
> +        qdev_prop_set_uint32(&isa->qdev, "irq", 14);
> +        qdev_init_nofail(&isa->qdev);
> +        s->ide.dev = &isa->qdev;
> +        DPRINTF("ide: base 0x%x\n", get_ide_iobase(s));
> +    }
> +
> +    register_ioport_write(s->iobase, 2, 1, pc87312_ioport_write, s);
> +    register_ioport_read(s->iobase, 2, 1, pc87312_ioport_read, s);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_pc87312 = {
> +    .name = "pc87312",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = pc87312_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(read_id_step, PC87312State),
> +        VMSTATE_UINT8(selected_index, PC87312State),
> +        VMSTATE_UINT8_ARRAY(regs, PC87312State, 3),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property pc87312_properties[] = {
> +    DEFINE_PROP_HEX32("iobase", PC87312State, iobase, 0x398),
> +    DEFINE_PROP_UINT8("config", PC87312State, config, 1),
> +    DEFINE_PROP_CHR("parallel", PC87312State, parallel.chr),
> +    DEFINE_PROP_CHR("uart1", PC87312State, uart[0].chr),
> +    DEFINE_PROP_CHR("uart2", PC87312State, uart[1].chr),
> +    DEFINE_PROP_DRIVE("floppyA", PC87312State, fdc.drive[0]),
> +    DEFINE_PROP_DRIVE("floppyB", PC87312State, fdc.drive[1]),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void pc87312_class_initfn(ObjectClass *klass, void *data)

I always thought initfn was used for instances...

> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +
> +    ic->init = pc87312_init;
> +    dc->reset = pc87312_reset;
> +    dc->vmsd = &vmstate_pc87312;
> +    dc->props = pc87312_properties;
> +}
> +
> +static TypeInfo pc87312_info = {
> +    .name          = "pc87312",
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(PC87312State),
> +    .class_init    = pc87312_class_initfn,
> +};
> +
> +static void pc87312_register_types(void)
> +{
> +    type_register_static(&pc87312_info);
> +}
> +
> +type_init(pc87312_register_types)
> +

Trailing empty line.

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.

Thanks for your work on this,

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg