In VIA super south bridge the io ranges of superio components
(parallel and serial ports and FDC) can be controlled by superio
config registers to set their base address and enable/disable them.
This is not easy to implement in QEMU because ISA emulation is only
designed to set io base address once on creating the device and io
ranges are registered at creation and cannot easily be disabled or
moved later.

In this patch we hack around that but only for serial ports because
those have a single io range at port base that's relatively easy to
handle and it's what guests actually use and set address different
than the default.

We do not attempt to handle controlling the parallel and FDC regions
because those have multiple io ranges so handling them would be messy
and guests either don't change their deafult or don't care. We could
even get away with disabling and not emulating them, but since they
are already there, this patch leaves them mapped at their default
address just in case this could be useful for a guest in the future.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 05d084f698..a3353ec5db 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
     MemoryRegion io;
+    ISASuperIODevice *superio;
+    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
 } SuperIOConfig;
 
+static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
+                                    int offs)
+{
+    MemoryRegion *subregion, *mr = NULL;
+
+    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
+        if (subregion->addr == offs) {
+            mr = subregion;
+            break;
+        }
+    }
+    return mr;
+}
+
 static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
                               unsigned size)
 {
@@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     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;
+        if (data & BIT(2)) { /* Serial port 1 enable */
+            ISADevice *dev = sc->superio->serial[0];
+            if (!memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_add_subregion(isa_address_space_io(dev),
+                                            dev->ioport_id, sc->serial_io[0]);
+            }
+        } else {
+            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
+            if (memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_del_subregion(io, sc->serial_io[0]);
+            }
+        }
+        if (data & BIT(3)) { /* Serial port 2 enable */
+            ISADevice *dev = sc->superio->serial[1];
+            if (!memory_region_is_mapped(sc->serial_io[1])) {
+                memory_region_add_subregion(isa_address_space_io(dev),
+                                            dev->ioport_id, sc->serial_io[1]);
+            }
+        } else {
+            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
+            if (memory_region_is_mapped(sc->serial_io[1])) {
+                memory_region_del_subregion(io, sc->serial_io[1]);
+            }
+        }
+        break;
+    }
+    case 0xe7: /* Serial port 1 io base address */
+    {
+        data &= 0xfe;
+        sc->superio->serial[0]->ioport_id = data << 2;
+        if (memory_region_is_mapped(sc->serial_io[0])) {
+            memory_region_set_address(sc->serial_io[0], data << 2);
+        }
+        break;
+    }
+    case 0xe8: /* Serial port 2 io base address */
+    {
+        data &= 0xfe;
+        sc->superio->serial[1]->ioport_id = data << 2;
+        if (memory_region_is_mapped(sc->serial_io[1])) {
+            memory_region_set_address(sc->serial_io[1], data << 2);
+        }
+        break;
+    }
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
     qemu_irq *isa_irq;
+    ISASuperIOClass *ic;
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
@@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
+    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
+                                                      TYPE_VT82C686B_SUPERIO));
+    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
@@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
      */
     memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
                                 &s->superio_cfg.io);
+
+    /* Grab io regions of serial devices so we can control them */
+    for (i = 0; i < ic->serial.count; i++) {
+        ISADevice *sd = s->superio_cfg.superio->serial[i];
+        MemoryRegion *io = isa_address_space_io(sd);
+        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
+        if (!mr) {
+            error_setg(errp, "Could not get io region for serial %d", i);
+            return;
+        }
+        s->superio_cfg.serial_io[i] = mr;
+        if (memory_region_is_mapped(mr)) {
+            memory_region_del_subregion(io, mr);
+        }
+    }
 }
 
 static void via_class_init(ObjectClass *klass, void *data)
-- 
2.21.3

ping for review?

On 3/10/21 3:58 AM, BALATON Zoltan wrote:
> In VIA super south bridge the io ranges of superio components
> (parallel and serial ports and FDC) can be controlled by superio
> config registers to set their base address and enable/disable them.
> This is not easy to implement in QEMU because ISA emulation is only
> designed to set io base address once on creating the device and io
> ranges are registered at creation and cannot easily be disabled or
> moved later.
> 
> In this patch we hack around that but only for serial ports because
> those have a single io range at port base that's relatively easy to
> handle and it's what guests actually use and set address different
> than the default.
> 
> We do not attempt to handle controlling the parallel and FDC regions
> because those have multiple io ranges so handling them would be messy
> and guests either don't change their deafult or don't care. We could
> even get away with disabling and not emulating them, but since they
> are already there, this patch leaves them mapped at their default
> address just in case this could be useful for a guest in the future.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 05d084f698..a3353ec5db 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
>  typedef struct SuperIOConfig {
>      uint8_t regs[0x100];
>      MemoryRegion io;
> +    ISASuperIODevice *superio;
> +    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
>  } SuperIOConfig;
>  
> +static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
> +                                    int offs)
> +{
> +    MemoryRegion *subregion, *mr = NULL;
> +
> +    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
> +        if (subregion->addr == offs) {
> +            mr = subregion;
> +            break;
> +        }
> +    }
> +    return mr;
> +}
> +
>  static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>                                unsigned size)
>  {
> @@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>      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;
> +        if (data & BIT(2)) { /* Serial port 1 enable */
> +            ISADevice *dev = sc->superio->serial[0];
> +            if (!memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[0]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
> +            if (memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_del_subregion(io, sc->serial_io[0]);
> +            }
> +        }
> +        if (data & BIT(3)) { /* Serial port 2 enable */
> +            ISADevice *dev = sc->superio->serial[1];
> +            if (!memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[1]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
> +            if (memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_del_subregion(io, sc->serial_io[1]);
> +            }
> +        }
> +        break;
> +    }
> +    case 0xe7: /* Serial port 1 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[0]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[0])) {
> +            memory_region_set_address(sc->serial_io[0], data << 2);
> +        }
> +        break;
> +    }
> +    case 0xe8: /* Serial port 2 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[1]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[1])) {
> +            memory_region_set_address(sc->serial_io[1], data << 2);
> +        }
> +        break;
> +    }
>      default:
>          qemu_log_mask(LOG_UNIMP,
>                        "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      DeviceState *dev = DEVICE(d);
>      ISABus *isa_bus;
>      qemu_irq *isa_irq;
> +    ISASuperIOClass *ic;
>      int i;
>  
>      qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> @@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
>      i8254_pit_init(isa_bus, 0x40, 0, NULL);
>      i8257_dma_init(isa_bus, 0);
> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
> +                                                      TYPE_VT82C686B_SUPERIO));
> +    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
>      mc146818_rtc_init(isa_bus, 2000, NULL);
>  
>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> @@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>       */
>      memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
>                                  &s->superio_cfg.io);
> +
> +    /* Grab io regions of serial devices so we can control them */
> +    for (i = 0; i < ic->serial.count; i++) {
> +        ISADevice *sd = s->superio_cfg.superio->serial[i];
> +        MemoryRegion *io = isa_address_space_io(sd);
> +        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
> +        if (!mr) {
> +            error_setg(errp, "Could not get io region for serial %d", i);
> +            return;
> +        }
> +        s->superio_cfg.serial_io[i] = mr;
> +        if (memory_region_is_mapped(mr)) {
> +            memory_region_del_subregion(io, mr);
> +        }
> +    }
>  }
>  
>  static void via_class_init(ObjectClass *klass, void *data)
> 

On Fri, Mar 12, 2021 at 12:47:54AM +0100, Philippe Mathieu-Daudé wrote:
> ping for review?

I'm not sure who you're asking for a review from.

> 
> On 3/10/21 3:58 AM, BALATON Zoltan wrote:
> > In VIA super south bridge the io ranges of superio components
> > (parallel and serial ports and FDC) can be controlled by superio
> > config registers to set their base address and enable/disable them.
> > This is not easy to implement in QEMU because ISA emulation is only
> > designed to set io base address once on creating the device and io
> > ranges are registered at creation and cannot easily be disabled or
> > moved later.
> > 
> > In this patch we hack around that but only for serial ports because
> > those have a single io range at port base that's relatively easy to
> > handle and it's what guests actually use and set address different
> > than the default.
> > 
> > We do not attempt to handle controlling the parallel and FDC regions
> > because those have multiple io ranges so handling them would be messy
> > and guests either don't change their deafult or don't care. We could
> > even get away with disabling and not emulating them, but since they
> > are already there, this patch leaves them mapped at their default
> > address just in case this could be useful for a guest in the future.
> > 
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >  hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 82 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 05d084f698..a3353ec5db 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
> >  typedef struct SuperIOConfig {
> >      uint8_t regs[0x100];
> >      MemoryRegion io;
> > +    ISASuperIODevice *superio;
> > +    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
> >  } SuperIOConfig;
> >  
> > +static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
> > +                                    int offs)
> > +{
> > +    MemoryRegion *subregion, *mr = NULL;
> > +
> > +    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
> > +        if (subregion->addr == offs) {
> > +            mr = subregion;
> > +            break;
> > +        }
> > +    }
> > +    return mr;
> > +}
> > +
> >  static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
> >                                unsigned size)
> >  {
> > @@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
> >      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;
> > +        if (data & BIT(2)) { /* Serial port 1 enable */
> > +            ISADevice *dev = sc->superio->serial[0];
> > +            if (!memory_region_is_mapped(sc->serial_io[0])) {
> > +                memory_region_add_subregion(isa_address_space_io(dev),
> > +                                            dev->ioport_id, sc->serial_io[0]);
> > +            }
> > +        } else {
> > +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
> > +            if (memory_region_is_mapped(sc->serial_io[0])) {
> > +                memory_region_del_subregion(io, sc->serial_io[0]);
> > +            }
> > +        }
> > +        if (data & BIT(3)) { /* Serial port 2 enable */
> > +            ISADevice *dev = sc->superio->serial[1];
> > +            if (!memory_region_is_mapped(sc->serial_io[1])) {
> > +                memory_region_add_subregion(isa_address_space_io(dev),
> > +                                            dev->ioport_id, sc->serial_io[1]);
> > +            }
> > +        } else {
> > +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
> > +            if (memory_region_is_mapped(sc->serial_io[1])) {
> > +                memory_region_del_subregion(io, sc->serial_io[1]);
> > +            }
> > +        }
> > +        break;
> > +    }
> > +    case 0xe7: /* Serial port 1 io base address */
> > +    {
> > +        data &= 0xfe;
> > +        sc->superio->serial[0]->ioport_id = data << 2;
> > +        if (memory_region_is_mapped(sc->serial_io[0])) {
> > +            memory_region_set_address(sc->serial_io[0], data << 2);
> > +        }
> > +        break;
> > +    }
> > +    case 0xe8: /* Serial port 2 io base address */
> > +    {
> > +        data &= 0xfe;
> > +        sc->superio->serial[1]->ioport_id = data << 2;
> > +        if (memory_region_is_mapped(sc->serial_io[1])) {
> > +            memory_region_set_address(sc->serial_io[1], data << 2);
> > +        }
> > +        break;
> > +    }
> >      default:
> >          qemu_log_mask(LOG_UNIMP,
> >                        "via_superio_cfg: unimplemented register 0x%x\n", idx);
> > @@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
> >      DeviceState *dev = DEVICE(d);
> >      ISABus *isa_bus;
> >      qemu_irq *isa_irq;
> > +    ISASuperIOClass *ic;
> >      int i;
> >  
> >      qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> > @@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
> >      isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> >      i8254_pit_init(isa_bus, 0x40, 0, NULL);
> >      i8257_dma_init(isa_bus, 0);
> > -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> > +    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
> > +                                                      TYPE_VT82C686B_SUPERIO));
> > +    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
> >      mc146818_rtc_init(isa_bus, 2000, NULL);
> >  
> >      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> > @@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
> >       */
> >      memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
> >                                  &s->superio_cfg.io);
> > +
> > +    /* Grab io regions of serial devices so we can control them */
> > +    for (i = 0; i < ic->serial.count; i++) {
> > +        ISADevice *sd = s->superio_cfg.superio->serial[i];
> > +        MemoryRegion *io = isa_address_space_io(sd);
> > +        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
> > +        if (!mr) {
> > +            error_setg(errp, "Could not get io region for serial %d", i);
> > +            return;
> > +        }
> > +        s->superio_cfg.serial_io[i] = mr;
> > +        if (memory_region_is_mapped(mr)) {
> > +            memory_region_del_subregion(io, mr);
> > +        }
> > +    }
> >  }
> >  
> >  static void via_class_init(ObjectClass *klass, void *data)
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Wed, 10 Mar 2021, BALATON Zoltan wrote:
> In VIA super south bridge the io ranges of superio components
> (parallel and serial ports and FDC) can be controlled by superio
> config registers to set their base address and enable/disable them.
> This is not easy to implement in QEMU because ISA emulation is only
> designed to set io base address once on creating the device and io
> ranges are registered at creation and cannot easily be disabled or
> moved later.
>
> In this patch we hack around that but only for serial ports because
> those have a single io range at port base that's relatively easy to
> handle and it's what guests actually use and set address different
> than the default.
>
> We do not attempt to handle controlling the parallel and FDC regions
> because those have multiple io ranges so handling them would be messy
> and guests either don't change their deafult or don't care. We could
> even get away with disabling and not emulating them, but since they
> are already there, this patch leaves them mapped at their default
> address just in case this could be useful for a guest in the future.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Could this patch be reviewed now please? I've dropped it from later 
versions to avoid this holding back the series but now that it won't be in 
6.0 I'd like to go back to this. This is implementing the behaviour of the 
real hardware better than the unsettable default value we have as a 
replacement. That approach also works for the guests I've tried (MorphOS 
and Linux) but if we can do better than why not do it?

Regards,
BALATON Zoltan

> ---
> hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 05d084f698..a3353ec5db 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
> typedef struct SuperIOConfig {
>     uint8_t regs[0x100];
>     MemoryRegion io;
> +    ISASuperIODevice *superio;
> +    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
> } SuperIOConfig;
>
> +static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
> +                                    int offs)
> +{
> +    MemoryRegion *subregion, *mr = NULL;
> +
> +    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
> +        if (subregion->addr == offs) {
> +            mr = subregion;
> +            break;
> +        }
> +    }
> +    return mr;
> +}
> +
> static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>                               unsigned size)
> {
> @@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>     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;
> +        if (data & BIT(2)) { /* Serial port 1 enable */
> +            ISADevice *dev = sc->superio->serial[0];
> +            if (!memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[0]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
> +            if (memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_del_subregion(io, sc->serial_io[0]);
> +            }
> +        }
> +        if (data & BIT(3)) { /* Serial port 2 enable */
> +            ISADevice *dev = sc->superio->serial[1];
> +            if (!memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[1]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
> +            if (memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_del_subregion(io, sc->serial_io[1]);
> +            }
> +        }
> +        break;
> +    }
> +    case 0xe7: /* Serial port 1 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[0]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[0])) {
> +            memory_region_set_address(sc->serial_io[0], data << 2);
> +        }
> +        break;
> +    }
> +    case 0xe8: /* Serial port 2 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[1]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[1])) {
> +            memory_region_set_address(sc->serial_io[1], data << 2);
> +        }
> +        break;
> +    }
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>     DeviceState *dev = DEVICE(d);
>     ISABus *isa_bus;
>     qemu_irq *isa_irq;
> +    ISASuperIOClass *ic;
>     int i;
>
>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> @@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>     isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(isa_bus, 0);
> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
> +                                                      TYPE_VT82C686B_SUPERIO));
> +    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
>     mc146818_rtc_init(isa_bus, 2000, NULL);
>
>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> @@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      */
>     memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
>                                 &s->superio_cfg.io);
> +
> +    /* Grab io regions of serial devices so we can control them */
> +    for (i = 0; i < ic->serial.count; i++) {
> +        ISADevice *sd = s->superio_cfg.superio->serial[i];
> +        MemoryRegion *io = isa_address_space_io(sd);
> +        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
> +        if (!mr) {
> +            error_setg(errp, "Could not get io region for serial %d", i);
> +            return;
> +        }
> +        s->superio_cfg.serial_io[i] = mr;
> +        if (memory_region_is_mapped(mr)) {
> +            memory_region_del_subregion(io, mr);
> +        }
> +    }
> }
>
> static void via_class_init(ObjectClass *klass, void *data)
>

On 23/03/2021 12:54, BALATON Zoltan wrote:

> On Wed, 10 Mar 2021, BALATON Zoltan wrote:
>> In VIA super south bridge the io ranges of superio components
>> (parallel and serial ports and FDC) can be controlled by superio
>> config registers to set their base address and enable/disable them.
>> This is not easy to implement in QEMU because ISA emulation is only
>> designed to set io base address once on creating the device and io
>> ranges are registered at creation and cannot easily be disabled or
>> moved later.
>>
>> In this patch we hack around that but only for serial ports because
>> those have a single io range at port base that's relatively easy to
>> handle and it's what guests actually use and set address different
>> than the default.
>>
>> We do not attempt to handle controlling the parallel and FDC regions
>> because those have multiple io ranges so handling them would be messy
>> and guests either don't change their deafult or don't care. We could
>> even get away with disabling and not emulating them, but since they
>> are already there, this patch leaves them mapped at their default
>> address just in case this could be useful for a guest in the future.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> 
> Could this patch be reviewed now please? I've dropped it from later versions to avoid 
> this holding back the series but now that it won't be in 6.0 I'd like to go back to 
> this. This is implementing the behaviour of the real hardware better than the 
> unsettable default value we have as a replacement. That approach also works for the 
> guests I've tried (MorphOS and Linux) but if we can do better than why not do it?

Do bear in mind that several people have already looked at this patch and haven't 
felt comfortable enough to review it, and I've also said in a previous email that 
this isn't the right approach. Given that the ISA bus partly uses the ioport address 
to reference the device, manually moving the memory regions around for devices on the 
bus without the ISA bus seeing those changes is going to cause issues.

IIRC the ability to dynamically change the standard ISA addresses was present in 
several motherboards from that era, and given that this functionality hasn't been 
implemented in QEMU this already tells us that no firmware is currently is using it.

I don't understand why using the hard-coded addresses in the v10 is a problem here? 
The addresses you added in the comments representing the programmed values are the 
standard ISA device addresses, so if those are what the firmware is programming then 
there will be no change. You also reported that it works fine with both your MorphOS 
and Linux test images, indicating that neither of these guest OSs require the feature.

If you find a guest OS that needs the functionality then certainly we can talk about 
trying to come up with a solution, but for me the extra complexity of this approach 
and the fact that you're overriding the management of the device by the ISA bus is 
why I haven't given it a R-B tag (I should add that my R-B for v10 using the 
hard-coded ISA addresses still stands).


ATB,

Mark.

On Tue, 23 Mar 2021, Mark Cave-Ayland wrote:
> On 23/03/2021 12:54, BALATON Zoltan wrote:
>> On Wed, 10 Mar 2021, BALATON Zoltan wrote:
>>> In VIA super south bridge the io ranges of superio components
>>> (parallel and serial ports and FDC) can be controlled by superio
>>> config registers to set their base address and enable/disable them.
>>> This is not easy to implement in QEMU because ISA emulation is only
>>> designed to set io base address once on creating the device and io
>>> ranges are registered at creation and cannot easily be disabled or
>>> moved later.
>>> 
>>> In this patch we hack around that but only for serial ports because
>>> those have a single io range at port base that's relatively easy to
>>> handle and it's what guests actually use and set address different
>>> than the default.
>>> 
>>> We do not attempt to handle controlling the parallel and FDC regions
>>> because those have multiple io ranges so handling them would be messy
>>> and guests either don't change their deafult or don't care. We could
>>> even get away with disabling and not emulating them, but since they
>>> are already there, this patch leaves them mapped at their default
>>> address just in case this could be useful for a guest in the future.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> 
>> Could this patch be reviewed now please? I've dropped it from later 
>> versions to avoid this holding back the series but now that it won't be in 
>> 6.0 I'd like to go back to this. This is implementing the behaviour of the 
>> real hardware better than the unsettable default value we have as a 
>> replacement. That approach also works for the guests I've tried (MorphOS 
>> and Linux) but if we can do better than why not do it?
>
> Do bear in mind that several people have already looked at this patch and 
> haven't felt comfortable enough to review it, and I've also said in a

I haven't heard back from many people (mostly only Philippe and you and 
maybe David looked at it but I did not expect him to review it as it's not 
his area) so I had the impression nobody cared or had time to look at it. 
That's why I've raised this again to get a clear answer about it. Now that 
at least you dislike this patch it's good enough reason to drop it for now 
and go with the default value instead until a guest is found to need this 
functionality.

> previous email that this isn't the right approach. Given that the ISA bus 
> partly uses the ioport address to reference the device, manually moving the 
> memory regions around for devices on the bus without the ISA bus seeing those 
> changes is going to cause issues.

The ioport_id you refer to is only used to print device name so the only 
issue is that the device may be currently mapped to a different address 
than its name might suggest when you query it but that should not break 
any functionality so maybe only cosmetic. The current ISA bus emulation in 
QEMU does not care about ISA devices after they are created, they are just 
initialised once and then the bus seems to forget about them. At least 
there are no functions to enable/disable or control them afterwards, only 
to register them at startup. So it's static and does not allow dynamically 
changing devices like we have in these VTxxx chips. (By the way, this is 
the same problem via-ide has that required hacks instead of actually 
emulating what the chip does because we can't have both ISA IDE and PCI 
IDE as the ISA one cannot be moved or disabled once created.) That's what 
this patch tried to circumvent at least for serial ports but without 
completly changing ISA emulation which might be a better approach but well 
beyond the effort I'm willing to put into this.

> IIRC the ability to dynamically change the standard ISA addresses was present 
> in several motherboards from that era, and given that this functionality 
> hasn't been implemented in QEMU this already tells us that no firmware is 
> currently is using it.

QEMU only supports SeaBIOS and on PC the ports are usually at their 
default address at startup so it's probably not firmware but guests that 
could change it. I don't know what other BIOSes might do as those don't 
run on QEMU. But these VTxxx chips are not used on PC machine models. The 
fuloong2e PMON and pegasos2 SmartFirtmware seem to poke the regs enabling 
and setting port address of these at startup (otherwise the chip seems to 
start without these enabled) so I've tried to emulate that. On pegasos2 it 
even uses different than usual standard value (as it only has one serial 
port) and guests boot with that so we can use that default. It's not 
exactly how the real device works but satisfies guests that rely on 
firmware to set up these and don't touch it later.

> I don't understand why using the hard-coded addresses in the v10 is a problem 
> here? The addresses you added in the comments representing the programmed 
> values are the standard ISA device addresses, so if those are what the 
> firmware is programming then there will be no change. You also reported that 
> it works fine with both your MorphOS and Linux test images, indicating that 
> neither of these guest OSs require the feature.

For now the default address will do until a guest is found actually 
needing this. I just wanted to salvage this patch before dismissing it 
without a good reason.

> If you find a guest OS that needs the functionality then certainly we can 
> talk about trying to come up with a solution, but for me the extra complexity 
> of this approach and the fact that you're overriding the management of the 
> device by the ISA bus is why I haven't given it a R-B tag (I should add that 
> my R-B for v10 using the hard-coded ISA addresses still stands).

OK then forget it, at least this is a clear vote against this patch and if 
others don't care that means it should be dropped. Thanks for explaining 
it again.

Regards,
BALATON Zoltan