On 06/05/2019 09:42, Artyom Tarasenko wrote:
> On Sun, May 5, 2019 at 12:43 PM Mark Cave-Ayland
> <mark.cave-ayl...@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 <atar4q...@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.
