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

[BALATON Zoltan]

On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
> no -bios is given -- to configure the functions accordingly.

Pegasos2 emulates firmware when no -bios is given, this was explained in 
previos commit so maybe not needed to be explained it here again so you 
could drop the comment between -- -- but I don't mind.

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 90 insertions(+), 31 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 9c2333a277..be202d23cf 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"
> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>
> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>
> +static void vt82c686b_superio_update(ViaSuperIOState *s)
> +{
> +    isa_parallel_set_enabled(s->superio.parallel[0],
> +                             (s->regs[0xe2] & 0x3) != 3);
> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
> +
> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
> +}

I wonder if some code duplication could be saved by adding a shared 
via_superio_update() for this further up in the abstract via-superio class 
instead of this method and vt8231_superio_update() below. This common 
method in abstract class would need to handle the differences which seem 
to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. 
These could either be handled by adding function parameters or fields to 
ViaSuperIOState for this that the subclasses can set and the method check. 
(Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or 
similar for how many ports are there then can have a for loop for those 
that would only run once for vt8231).

> +static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
> +{
> +    ViaSuperIOState *s = opaque;
> +
> +    vt82c686b_superio_update(s);
> +
> +    return 0;

You could lose some blank lines here. You seem to love them, half of your 
cover letter is just blank lines :-) but I'm the opposite and like more 
code to fit in one screen even on todays displays that are wider than 
tall. So this funciton would take less space without blank lines. (Even 
the local variable may not be necessary as you don't access any fields 
within and void * should just cast without a warning but for spelling out 
the desired type as a reminder I'm ok with leaving that but no excessive 
blank lines please if you don't mind that much.)

Regards,
BALATON Zoltan

> +}
> +
> +static const VMStateDescription vmstate_vt82c686b_superio = {
> +    .name = "vt82c686b_superio",
> +    .version_id = 1,
> +    .post_load = vmstate_vt82c686b_superio_post_load,
> +};
> +
> static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>                                         uint64_t data, unsigned size)
> {
> @@ -368,7 +400,11 @@ 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 ... 0xe3:
> +    case 0xe6 ... 0xe8:
> +        sc->regs[idx] = data;
> +        vt82c686b_superio_update(sc);
> +        return;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>
>     memset(s->regs, 0, sizeof(s->regs));
>     /* Device ID */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
> -    /* Function select - all disabled */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
> +    s->regs[0xe0] = 0x3c;
> +    /*
> +     * 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.
> +     */
> +    s->regs[0xe2] = 0x0f;
>     /* Floppy ctrl base addr 0x3f0-7 */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
> +    s->regs[0xe3] = 0xfc;
>     /* Parallel port base addr 0x378-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
> +    s->regs[0xe6] = 0xde;
>     /* Serial port 1 base addr 0x3f8-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
> +    s->regs[0xe7] = 0xfe;
>     /* Serial port 2 base addr 0x2f8-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
> +    s->regs[0xe8] = 0xbe;
>
> -    vt82c686b_superio_cfg_write(s, 0, 0, 1);
> +    vt82c686b_superio_update(s);
> }
>
> static void vt82c686b_superio_init(Object *obj)
> @@ -429,6 +464,7 @@ static void vt82c686b_superio_class_init(ObjectClass 
> *klass, void *data)
>     sc->parallel.count = 1;
>     sc->ide.count = 0; /* emulated by via-ide */
>     sc->floppy.count = 1;
> +    dc->vmsd = &vmstate_vt82c686b_superio;
> }
>
> static const TypeInfo vt82c686b_superio_info = {
> @@ -443,6 +479,33 @@ static const TypeInfo vt82c686b_superio_info = {
>
> #define TYPE_VT8231_SUPERIO "vt8231-superio"
>
> +static void vt8231_superio_update(ViaSuperIOState *s)
> +{
> +    isa_parallel_set_enabled(s->superio.parallel[0],
> +                             (s->regs[0xf2] & 0x3) != 3);
> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xf2] & BIT(2));
> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xf2] & BIT(4));
> +
> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xf4] & 0xfe) << 2);
> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xf6] << 2);
> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xf7] & 0xfc) << 2);
> +}
> +
> +static int vmstate_vt8231_superio_post_load(void *opaque, int version_id)
> +{
> +    ViaSuperIOState *s = opaque;
> +
> +    vt8231_superio_update(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_vt8231_superio = {
> +    .name = "vt8231_superio",
> +    .version_id = 1,
> +    .post_load = vmstate_vt8231_superio_post_load,
> +};
> +
> static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>                                      uint64_t data, unsigned size)
> {
> @@ -465,6 +528,12 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr 
> addr,
>     case 0xfd:
>         /* ignore write to read only registers */
>         return;
> +    case 0xf2:
> +    case 0xf4:
> +    case 0xf6 ... 0xf7:
> +        sc->regs[idx] = data;
> +        vt8231_superio_update(sc);
> +        return;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -493,19 +562,15 @@ static void vt8231_superio_reset(DeviceState *dev)
>     /* Device revision */
>     s->regs[0xf1] = 0x01;
>     /* Function select - all disabled */
> -    vt8231_superio_cfg_write(s, 0, 0xf2, 1);
> -    vt8231_superio_cfg_write(s, 1, 0x03, 1);
> +    s->regs[0xf2] = 0x03;
>     /* Serial port base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf4, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xfe, 1);
> +    s->regs[0xf4] = 0xfe;
>     /* Parallel port base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf6, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xde, 1);
> +    s->regs[0xf6] = 0xde;
>     /* Floppy ctrl base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf7, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xfc, 1);
> +    s->regs[0xf7] = 0xfc;
>
> -    vt8231_superio_cfg_write(s, 0, 0, 1);
> +    vt8231_superio_update(s);
> }
>
> static void vt8231_superio_init(Object *obj)
> @@ -513,12 +578,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,10 +585,10 @@ 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;
> +    dc->vmsd = &vmstate_vt8231_superio;
> }
>
> static const TypeInfo vt8231_superio_info = {