Re: [PATCH v4 11/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[BALATON Zoltan]

On Mon, 8 Jan 2024, Bernhard Beschow wrote:
> Am 7. Januar 2024 13:59:44 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Sat, 6 Jan 2024, Bernhard Beschow wrote:
> > > The VIA south bridges are able to relocate and toggle (enable or disable) their
> > > SuperI/O functions. So far this is hardcoded such that all functions are always
> > > enabled and are located at fixed addresses.
> > >
> > > Some PC BIOSes seem to probe for I/O occupancy before activating such a function
> > > and issue an error in case of a conflict. Since the functions are currently
> > > enabled on reset, conflicts are always detected. Prevent that by implementing
> > > relocation and toggling of the SuperI/O functions.
> > >
> > > Note that all SuperI/O functions are now deactivated upon reset (except for
> > > VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
> > > enabled by default). Rely on firmware to configure the functions accordingly.
> > >
> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > > hw/isa/vt82c686.c | 66 ++++++++++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 56 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > index d3e0f6d01f..9f62fb5964 100644
> > > --- a/hw/isa/vt82c686.c
> > > +++ b/hw/isa/vt82c686.c
> > > @@ -15,6 +15,9 @@
> > >
> > > #include "qemu/osdep.h"
> > > #include "hw/isa/vt82c686.h"
> > > +#include "hw/block/fdc.h"
> > > +#include "hw/char/parallel-isa.h"
> > > +#include "hw/char/serial.h"
> > > #include "hw/pci/pci.h"
> > > #include "hw/qdev-properties.h"
> > > #include "hw/ide/pci.h"
> > > @@ -323,6 +326,18 @@ static uint64_t via_superio_cfg_read(void *opaque, hwaddr 
> > > addr, unsigned size)
> > >     return val;
> > > }
> > >
> > > +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
> > > +{
> > > +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
> > > +    size_t i;
> >
> > The expected value for i is 0 or 1 (maybe up to 3 sometimes it there are more 
> > serial ports in a chip). so why use such big type?
>
> serial.count is of type size_t, that's why I chose it. Let me know if you still 
> want an int, otherwise I'd leave it as is.

I think int would suffice here but it's not a big deal. The count is 
indeed size_t, not sure why. How many components were expected? But the 
practical value is just a few so larger type seems to be an overkill.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
> > This should just be int. Newly it's also allowed to declare it within the for 
> > so if you want that you could do so but I have no preference on that and 
> > declaring it here is also OK. Otherwise:
> >
> > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > > +
> > > +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
> > > +    for (i = 0; i < ic->serial.count; i++) {
> > > +        isa_serial_set_enabled(s->superio.serial[i], data & BIT(i + 2));
> > > +    }
> > > +    isa_fdc_set_enabled(s->superio.floppy, data & BIT(4));
> > > +}
> > > +
> > > static void via_superio_class_init(ObjectClass *klass, void *data)
> > > {
> > >     DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -368,7 +383,25 @@ static void vt82c686b_superio_cfg_write(void *opaque, 
> > > hwaddr addr,
> > >     case 0xfd ... 0xff:
> > >         /* ignore write to read only registers */
> > >         return;
> > > -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
> > > +    case 0xe2:
> > > +        data &= 0x1f;
> > > +        via_superio_devices_enable(sc, data);
> > > +        break;
> > > +    case 0xe3:
> > > +        data &= 0xfc;
> > > +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
> > > +        break;
> > > +    case 0xe6:
> > > +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
> > > +        break;
> > > +    case 0xe7:
> > > +        data &= 0xfe;
> > > +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
> > > +        break;
> > > +    case 0xe8:
> > > +        data &= 0xfe;
> > > +        isa_serial_set_iobase(sc->superio.serial[1], data << 2);
> > > +        break;
> > >     default:
> > >         qemu_log_mask(LOG_UNIMP,
> > >                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> > > @@ -395,9 +428,14 @@ static void vt82c686b_superio_reset(DeviceState *dev)
> > >     /* Device ID */
> > >     vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
> > >     vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
> > > -    /* Function select - all disabled */
> > > +    /*
> > > +     * Function select - only serial enabled
> > > +     * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it. 
> > > This
> > > +     * suggests that the serial ports are enabled by default, so override the
> > > +     * datasheet.
> > > +     */
> > >     vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
> > > -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
> > > +    vt82c686b_superio_cfg_write(s, 1, 0x0f, 1);
> > >     /* Floppy ctrl base addr 0x3f0-7 */
> > >     vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
> > >     vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
> > > @@ -465,6 +503,21 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr 
> > > addr,
> > >     case 0xfd:
> > >         /* ignore write to read only registers */
> > >         return;
> > > +    case 0xf2:
> > > +        data &= 0x17;
> > > +        via_superio_devices_enable(sc, data);
> > > +        break;
> > > +    case 0xf4:
> > > +        data &= 0xfe;
> > > +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
> > > +        break;
> > > +    case 0xf6:
> > > +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
> > > +        break;
> > > +    case 0xf7:
> > > +        data &= 0xfc;
> > > +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
> > > +        break;
> > >     default:
> > >         qemu_log_mask(LOG_UNIMP,
> > >                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> > > @@ -513,12 +566,6 @@ static void vt8231_superio_init(Object *obj)
> > >     VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
> > > }
> > >
> > > -static uint16_t vt8231_superio_serial_iobase(ISASuperIODevice *sio,
> > > -                                             uint8_t index)
> > > -{
> > > -        return 0x2f8; /* FIXME: This should be settable via registers f2-f4 */
> > > -}
> > > -
> > > static void vt8231_superio_class_init(ObjectClass *klass, void *data)
> > > {
> > >     DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -526,7 +573,6 @@ static void vt8231_superio_class_init(ObjectClass *klass, 
> > > void *data)
> > >
> > >     dc->reset = vt8231_superio_reset;
> > >     sc->serial.count = 1;
> > > -    sc->serial.get_iobase = vt8231_superio_serial_iobase;
> > >     sc->parallel.count = 1;
> > >     sc->ide.count = 0; /* emulated by via-ide */
> > >     sc->floppy.count = 1;