[v2,10/13] vt82c686: Implement control of serial port io ranges via config regs

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(-)

Cc'ing Paolo (Memory API maintainer).

On 1/9/21 9:16 PM, 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 2921d5c55c..18bd86285b 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;

Shouldn't we use memory_region_find() here?

Also, why not have a proper helper in "hw/isa/isa.h"?

> +            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 Sat, 20 Feb 2021, Philippe Mathieu-Daudé wrote:
> Cc'ing Paolo (Memory API maintainer).
>
> On 1/9/21 9:16 PM, 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 2921d5c55c..18bd86285b 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;
>
> Shouldn't we use memory_region_find() here?

I think I've looked at that but it did not do what I needed (which is 
returning the actual region at the address) but this approach was used 
instead by some s390x code I think where I've got it from.

> Also, why not have a proper helper in "hw/isa/isa.h"?

Maybe because nothing needed it so far? Also I've tried that first but 
some ISA devices have multiple memory regions so it's not easy to return 
all of them so a helper would only work for ISA devices that have a single 
region.

As the patch description says this is basically a hack to be able to 
implement this behaviour of this VIA chip without rewriting ISA emulation 
that I'd rather not do because 1) it could break a lot of other machines 
and 2) takes a lot of time so I went with this approach that's not so bad 
for just this use case and could be cleaned up later if somebody would 
want to clean up ISA emulation to support this use case as well. Currently 
ISA seems to be designed to only handle devices that are created once at 
the start and not changed later but these VIA superio chips have ISA 
devices that can be enabled/disabled and their IO ranges changed by 
writing registers. That's not easily emulated in current QEMU so this is a 
compromise to make it work with the guests but avoid needing extensive 
changes to QEMU ISA parts. (We only really need to change serial address 
because VT8231 has a single serial port and pegasos2 firmware puts it at 
0x2f8 instead of the usual default of 0x3f8 so we can't get away with just 
mapping the serial device at the default address. It could work by putting 
it at 0x2f8 but that may break other guests and this way we can actually 
emulate what the chip does so it works for both 82c686b and 8231 and more 
guests should be happy with it so I think this should be good enough for 
now.)

Regards,
BALATON Zoltan

>> +            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)
>>
>
>