Re: [PATCH v7 1/8] vt82c686: Implement control of serial port io ranges via config regs

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7167 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]