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

[Mark Cave-Ayland]

On 07/05/2019 16:03, Artyom Tarasenko wrote:

> 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?

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.