Re: [Qemu-devel] [PATCH v2 1/3] lsi53c895a: hide 53c895a registers in 53c810

[Artyom Tarasenko]

On Mon, May 6, 2019 at 4:27 PM Mark Cave-Ayland
<address@hidden> wrote:
>
> On 06/05/2019 09:42, Artyom Tarasenko wrote:
>
> > On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland
> > <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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?

-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu