| Message ID | 1557003754-26473-2-git-send-email-atar4qemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve 40p, make AIX 5.1 boot | expand |
On 04/05/2019 22:02, Artyom Tarasenko wrote: > AIX/PReP does access to the aliased IO registers of 53810. > Implement aliasing to make the AIX driver work. > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index da7239d..6b95699 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > LSIState *s = LSI53C895A(dev); > DeviceState *d = DEVICE(dev); > uint8_t *pci_conf; > + uint64_t mmio_size; > + MemoryRegion *mr; > + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; > > pci_conf = dev->config; > > @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > /* Interrupt pin A */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; > > - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, > - "lsi-mmio", 0x400); > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, > "lsi-ram", 0x2000); > memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, > "lsi-io", 256); > - > + if (type == PCI_DEVICE_ID_LSI_53C895A) { > + mmio_size = 0x400; > + } else { > + mr = g_new(MemoryRegion, 1); In general these days it's worth keeping the reference to the MemoryRegion within LSIState since then its lifecycle is more clearly defined. > + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, > + 0, 0x80); > + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); > + mmio_size = 0x80; This feels a little strange - is it possible to see from the datasheets that the 53C895A has 0x400 bytes MMIO whilst the 53C810 has 0x80 bytes MMIO? It's not clear to me where the aliasing is happening. > + } > + memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, > + "lsi-mmio", mmio_size); > address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); > qdev_init_gpio_out(d, &s->ext_irq, 1); > > ATB, Mark.
On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 04/05/2019 22:02, Artyom Tarasenko wrote: > > > AIX/PReP does access to the aliased IO registers of 53810. > > Implement aliasing to make the AIX driver work. > > > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > > --- > > hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > > index da7239d..6b95699 100644 > > --- a/hw/scsi/lsi53c895a.c > > +++ b/hw/scsi/lsi53c895a.c > > @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > > LSIState *s = LSI53C895A(dev); > > DeviceState *d = DEVICE(dev); > > uint8_t *pci_conf; > > + uint64_t mmio_size; > > + MemoryRegion *mr; > > + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; > > > > pci_conf = dev->config; > > > > @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > > /* Interrupt pin A */ > > pci_conf[PCI_INTERRUPT_PIN] = 0x01; > > > > - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, > > - "lsi-mmio", 0x400); > > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, > > "lsi-ram", 0x2000); > > memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, > > "lsi-io", 256); > > - > > + if (type == PCI_DEVICE_ID_LSI_53C895A) { > > + mmio_size = 0x400; > > + } else { > > + mr = g_new(MemoryRegion, 1); > > In general these days it's worth keeping the reference to the MemoryRegion within > LSIState since then its lifecycle is more clearly defined. On the other hand, it's a PCI card, and can not be hot-plugged/removed, so the lifecycle is pretty simple here. Or am I missing something? > > + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, > > + 0, 0x80); > > + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); > > + mmio_size = 0x80; > > This feels a little strange - is it possible to see from the datasheets that the > 53C895A has 0x400 bytes MMIO whilst the 53C810 has 0x80 bytes MMIO? It's not clear to > me where the aliasing is happening. These values are empiric. For 810 it can not be more than 0x80, because the AIX does access the registers with the shift of 0x80. For 895A we did already have 0x400. > > + } > > + memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, > > + "lsi-mmio", mmio_size); > > address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); > > qdev_init_gpio_out(d, &s->ext_irq, 1); > > > > > > > ATB, > > Mark. >
On 06/05/2019 09:42, Artyom Tarasenko wrote: > On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 04/05/2019 22:02, Artyom Tarasenko wrote: >> >>> AIX/PReP does access to the aliased IO registers of 53810. >>> Implement aliasing to make the AIX driver work. >>> >>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>> --- >>> hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- >>> 1 file changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c >>> index da7239d..6b95699 100644 >>> --- a/hw/scsi/lsi53c895a.c >>> +++ b/hw/scsi/lsi53c895a.c >>> @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) >>> LSIState *s = LSI53C895A(dev); >>> DeviceState *d = DEVICE(dev); >>> uint8_t *pci_conf; >>> + uint64_t mmio_size; >>> + MemoryRegion *mr; >>> + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; >>> >>> pci_conf = dev->config; >>> >>> @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) >>> /* Interrupt pin A */ >>> pci_conf[PCI_INTERRUPT_PIN] = 0x01; >>> >>> - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, >>> - "lsi-mmio", 0x400); >>> memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, >>> "lsi-ram", 0x2000); >>> memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, >>> "lsi-io", 256); >>> - >>> + if (type == PCI_DEVICE_ID_LSI_53C895A) { >>> + mmio_size = 0x400; >>> + } else { >>> + mr = g_new(MemoryRegion, 1); >> >> In general these days it's worth keeping the reference to the MemoryRegion within >> LSIState since then its lifecycle is more clearly defined. > > On the other hand, it's a PCI card, and can not be > hot-plugged/removed, so the lifecycle is pretty simple here. > Or am I missing something? Well Thomas has been working on a set of tests that for each machine will plug and unplug each device via the monitor to make sure that init/realize/unrealize work correctly so it would be good to ensure that these tests don't leak. However... >>> + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, >>> + 0, 0x80); >>> + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); >>> + mmio_size = 0x80; >> >> This feels a little strange - is it possible to see from the datasheets that the >> 53C895A has 0x400 bytes MMIO whilst the 53C810 has 0x80 bytes MMIO? It's not clear to >> me where the aliasing is happening. > > These values are empiric. For 810 it can not be more than 0x80, > because the AIX does access the registers with the shift of 0x80. > For 895A we did already have 0x400. After a bit of searching I managed to locate an 810 datasheet and in Chapter 5 it clearly describes the IO space (s->io_io) as being 256 bytes in size which is the same as the 895A, but with 0x80-0xff aliased onto 0x00 - 0x7f. It feels to me that rather than complicate things with an additional alias MemoryRegion, the simplest solution would be to simply change the mask in lsi_io_read() and lsi_io_write() to be 0x7f rather than 0xff if we've instantiated a 810 rather than an 895A. ATB, Mark.
On Mon, May 6, 2019 at 4:27 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 06/05/2019 09:42, Artyom Tarasenko wrote: > > > On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland > > <mark.cave-ayland@ilande.co.uk> wrote: > >> > >> On 04/05/2019 22:02, Artyom Tarasenko wrote: > >> > >>> AIX/PReP does access to the aliased IO registers of 53810. > >>> Implement aliasing to make the AIX driver work. > >>> > >>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > >>> --- > >>> hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- > >>> 1 file changed, 14 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > >>> index da7239d..6b95699 100644 > >>> --- a/hw/scsi/lsi53c895a.c > >>> +++ b/hw/scsi/lsi53c895a.c > >>> @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > >>> LSIState *s = LSI53C895A(dev); > >>> DeviceState *d = DEVICE(dev); > >>> uint8_t *pci_conf; > >>> + uint64_t mmio_size; > >>> + MemoryRegion *mr; > >>> + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; > >>> > >>> pci_conf = dev->config; > >>> > >>> @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) > >>> /* Interrupt pin A */ > >>> pci_conf[PCI_INTERRUPT_PIN] = 0x01; > >>> > >>> - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, > >>> - "lsi-mmio", 0x400); > >>> memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, > >>> "lsi-ram", 0x2000); > >>> memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, > >>> "lsi-io", 256); > >>> - > >>> + if (type == PCI_DEVICE_ID_LSI_53C895A) { > >>> + mmio_size = 0x400; > >>> + } else { > >>> + mr = g_new(MemoryRegion, 1); > >> > >> In general these days it's worth keeping the reference to the MemoryRegion within > >> LSIState since then its lifecycle is more clearly defined. > > > > On the other hand, it's a PCI card, and can not be > > hot-plugged/removed, so the lifecycle is pretty simple here. > > Or am I missing something? > > Well Thomas has been working on a set of tests that for each machine will plug and > unplug each device via the monitor to make sure that init/realize/unrealize work > correctly so it would be good to ensure that these tests don't leak. Makes sense, indeed. > However... > > >>> + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, > >>> + 0, 0x80); > >>> + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); > >>> + mmio_size = 0x80; > >> > >> This feels a little strange - is it possible to see from the datasheets that the > >> 53C895A has 0x400 bytes MMIO whilst the 53C810 has 0x80 bytes MMIO? It's not clear to > >> me where the aliasing is happening. > > > > These values are empiric. For 810 it can not be more than 0x80, > > because the AIX does access the registers with the shift of 0x80. > > For 895A we did already have 0x400. > > After a bit of searching I managed to locate an 810 datasheet and in Chapter 5 it > clearly describes the IO space (s->io_io) as being 256 bytes in size which is the > same as the 895A, but with 0x80-0xff aliased onto 0x00 - 0x7f. > > It feels to me that rather than complicate things with an additional alias > MemoryRegion, the simplest solution would be to simply change the mask in > lsi_io_read() and lsi_io_write() to be 0x7f rather than 0xff if we've instantiated a > 810 rather than an 895A. Initially I implemented it exactly as you suggest, via mask. But then I thought that memory_region_init_alias makes aliasing more obvious. I don't have a strong opinion on this one though. @Paolo, what do you think?
On 07/05/2019 16:03, Artyom Tarasenko wrote: > On Mon, May 6, 2019 at 4:27 PM Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 06/05/2019 09:42, Artyom Tarasenko wrote: >> >>> On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland >>> <mark.cave-ayland@ilande.co.uk> wrote: >>>> >>>> On 04/05/2019 22:02, Artyom Tarasenko wrote: >>>> >>>>> AIX/PReP does access to the aliased IO registers of 53810. >>>>> Implement aliasing to make the AIX driver work. >>>>> >>>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>>>> --- >>>>> hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- >>>>> 1 file changed, 14 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c >>>>> index da7239d..6b95699 100644 >>>>> --- a/hw/scsi/lsi53c895a.c >>>>> +++ b/hw/scsi/lsi53c895a.c >>>>> @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) >>>>> LSIState *s = LSI53C895A(dev); >>>>> DeviceState *d = DEVICE(dev); >>>>> uint8_t *pci_conf; >>>>> + uint64_t mmio_size; >>>>> + MemoryRegion *mr; >>>>> + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; >>>>> >>>>> pci_conf = dev->config; >>>>> >>>>> @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) >>>>> /* Interrupt pin A */ >>>>> pci_conf[PCI_INTERRUPT_PIN] = 0x01; >>>>> >>>>> - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, >>>>> - "lsi-mmio", 0x400); >>>>> memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, >>>>> "lsi-ram", 0x2000); >>>>> memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, >>>>> "lsi-io", 256); >>>>> - >>>>> + if (type == PCI_DEVICE_ID_LSI_53C895A) { >>>>> + mmio_size = 0x400; >>>>> + } else { >>>>> + mr = g_new(MemoryRegion, 1); >>>> >>>> In general these days it's worth keeping the reference to the MemoryRegion within >>>> LSIState since then its lifecycle is more clearly defined. >>> >>> On the other hand, it's a PCI card, and can not be >>> hot-plugged/removed, so the lifecycle is pretty simple here. >>> Or am I missing something? >> >> Well Thomas has been working on a set of tests that for each machine will plug and >> unplug each device via the monitor to make sure that init/realize/unrealize work >> correctly so it would be good to ensure that these tests don't leak. > > Makes sense, indeed. > >> However... >> >>>>> + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, >>>>> + 0, 0x80); >>>>> + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); >>>>> + mmio_size = 0x80; >>>> >>>> This feels a little strange - is it possible to see from the datasheets that the >>>> 53C895A has 0x400 bytes MMIO whilst the 53C810 has 0x80 bytes MMIO? It's not clear to >>>> me where the aliasing is happening. >>> >>> These values are empiric. For 810 it can not be more than 0x80, >>> because the AIX does access the registers with the shift of 0x80. >>> For 895A we did already have 0x400. >> >> After a bit of searching I managed to locate an 810 datasheet and in Chapter 5 it >> clearly describes the IO space (s->io_io) as being 256 bytes in size which is the >> same as the 895A, but with 0x80-0xff aliased onto 0x00 - 0x7f. >> >> It feels to me that rather than complicate things with an additional alias >> MemoryRegion, the simplest solution would be to simply change the mask in >> lsi_io_read() and lsi_io_write() to be 0x7f rather than 0xff if we've instantiated a >> 810 rather than an 895A. > > Initially I implemented it exactly as you suggest, via mask. But then > I thought that memory_region_init_alias makes aliasing more obvious. > I don't have a strong opinion on this one though. > > @Paolo, what do you think? My general feeling is that memory region aliases are more aimed at mapping areas into different address spaces. In this particular case it seems to me that the memory region for both devices is still 256 bytes, but it's just that internally the address decoder ignores bit 7 on the 810. ATB, Mark.
On 17/05/19 16:12, Mark Cave-Ayland wrote: >> Initially I implemented it exactly as you suggest, via mask. But then >> I thought that memory_region_init_alias makes aliasing more obvious. >> I don't have a strong opinion on this one though. >> >> @Paolo, what do you think? > My general feeling is that memory region aliases are more aimed at mapping areas into > different address spaces. In this particular case it seems to me that the memory > region for both devices is still 256 bytes, but it's just that internally the address > decoder ignores bit 7 on the 810. It's the same for me; whatever makes the code simpler is good. RAM regions need to use aliases in order to simulate ignored bits in the decoder, so it's okay to use that for MMIO as well. On the other hand, it's true that a simple "&" does the job just as well. Paolo
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index da7239d..6b95699 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2271,6 +2271,9 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) LSIState *s = LSI53C895A(dev); DeviceState *d = DEVICE(dev); uint8_t *pci_conf; + uint64_t mmio_size; + MemoryRegion *mr; + uint16_t type = PCI_DEVICE_GET_CLASS(dev)->device_id; pci_conf = dev->config; @@ -2279,13 +2282,21 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin A */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; - memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, - "lsi-mmio", 0x400); memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, "lsi-ram", 0x2000); memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); - + if (type == PCI_DEVICE_ID_LSI_53C895A) { + mmio_size = 0x400; + } else { + mr = g_new(MemoryRegion, 1); + memory_region_init_alias(mr, OBJECT(d), "lsi-io-alias", &s->io_io, + 0, 0x80); + memory_region_add_subregion_overlap(&s->io_io, 0x80, mr, -1); + mmio_size = 0x80; + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &lsi_mmio_ops, s, + "lsi-mmio", mmio_size); address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1);
AIX/PReP does access to the aliased IO registers of 53810. Implement aliasing to make the AIX driver work. Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> --- hw/scsi/lsi53c895a.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)