[PATCH v2 0/3] Add emulation of AmigaOne XE board

[PATCH v2 0/3] Add emulation of AmigaOne XE board

[BALATON Zoltan]

Changes in v2:
- Update comment and commit message in patch 1 (Mark)
- Fix irq mapping in patch 2 (Volker)

Regards,
BALATON Zoltan

BALATON Zoltan (3):
  via-ide: Fix legacy mode emulation
  hw/pci-host: Add emulation of Mai Logic Articia S
  hw/ppc: Add emulation of AmigaOne XE board

 MAINTAINERS                             |   8 +
 configs/devices/ppc-softmmu/default.mak |   1 +
 hw/ide/via.c                            |  41 +++-
 hw/pci-host/Kconfig                     |   5 +
 hw/pci-host/articia.c                   | 293 ++++++++++++++++++++++++
 hw/pci-host/meson.build                 |   2 +
 hw/ppc/Kconfig                          |   7 +
 hw/ppc/amigaone.c                       | 164 +++++++++++++
 hw/ppc/meson.build                      |   2 +
 include/hw/pci-host/articia.h           |  17 ++
 10 files changed, 535 insertions(+), 5 deletions(-)
 create mode 100644 hw/pci-host/articia.c
 create mode 100644 hw/ppc/amigaone.c
 create mode 100644 include/hw/pci-host/articia.h

-- 
2.30.9

[PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

The initial value for BARs were set in reset method for emulating
legacy mode at start but this does not work because PCI code resets
BARs after calling device reset method. Remove this ineffective
default to avoid confusion.

Instead move setting the BARs to a callback on writing the PCI config
regsiter that sets legacy mode (which firmwares needing this mode seem
to do) and fix their values to program it to use legacy port numbers
in this case. This does not fully emulate what the data sheet says
(which is not very clear on this) but it implements enogh to allow
both modes as used by firmwares of machines we emulate.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..43e8af8d69 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
 
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
     pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
 
     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
@@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+                              uint32_t val, int len)
+{
+    pci_default_write_config(pd, addr, val, len);
+    /*
+     * Bits 0 and 2 of the PCI programming interface register select between
+     * legacy and native mode for the two IDE channels. We don't emulate this
+     * because we cannot easily switch between ISA and PCI in QEMU so instead
+     * when guest selects legacy mode we set the PCI BARs to legacy ports which
+     * works the same. We also don't care about setting each channel separately
+     * as no guest is known to do or need that. We only do this when BARs are
+     * unset when writing this register as logs from real hardware show that
+     * setting legacy mode after BARs were set it will still use ports set by
+     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
+     * native mode and programmed BARs and calls it non-100% native mode).
+     * But if 0x8a is written righr after reset without setting BARs then we
+     * want legacy ports (this is done by the AmigaOne firmware).
+     */
+    if (addr == PCI_CLASS_PROG && val == 0x8a &&
+        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
+        PCI_BASE_ADDRESS_SPACE_IO) {
+        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
+                     | PCI_BASE_ADDRESS_SPACE_IO);
+        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
+                     | PCI_BASE_ADDRESS_SPACE_IO);
+        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
+                     | PCI_BASE_ADDRESS_SPACE_IO);
+        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
+                     | PCI_BASE_ADDRESS_SPACE_IO);
+        /* BMIBA: 20-23h */
+        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
+                     | PCI_BASE_ADDRESS_SPACE_IO);
+    }
+}
+
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -221,6 +251,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     /* Reason: only works as function of VIA southbridge */
     dc->user_creatable = false;
 
+    k->config_write = via_ide_cfg_write;
     k->realize = via_ide_realize;
     k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-- 
2.30.9

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[Mark Cave-Ayland]

On 09/10/2023 20:54, BALATON Zoltan wrote:

> The initial value for BARs were set in reset method for emulating
> legacy mode at start but this does not work because PCI code resets
> BARs after calling device reset method. Remove this ineffective
> default to avoid confusion.
> 
> Instead move setting the BARs to a callback on writing the PCI config
> regsiter that sets legacy mode (which firmwares needing this mode seem
> to do) and fix their values to program it to use legacy port numbers
> in this case. This does not fully emulate what the data sheet says
> (which is not very clear on this) but it implements enogh to allow
> both modes as used by firmwares of machines we emulate.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..43e8af8d69 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                    PCI_STATUS_DEVSEL_MEDIUM);
>   
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>   
>       /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>   }
>   
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    pci_default_write_config(pd, addr, val, len);
> +    /*
> +     * Bits 0 and 2 of the PCI programming interface register select between
> +     * legacy and native mode for the two IDE channels. We don't emulate this
> +     * because we cannot easily switch between ISA and PCI in QEMU so instead

As per my previous email, this statement is demonstrably false: this is now 
achievable using the portio_list*() APIs.

> +     * when guest selects legacy mode we set the PCI BARs to legacy ports which
> +     * works the same. We also don't care about setting each channel separately
> +     * as no guest is known to do or need that. We only do this when BARs are
> +     * unset when writing this register as logs from real hardware show that
> +     * setting legacy mode after BARs were set it will still use ports set by
> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
> +     * native mode and programmed BARs and calls it non-100% native mode).
> +     * But if 0x8a is written righr after reset without setting BARs then we
> +     * want legacy ports (this is done by the AmigaOne firmware).
> +     */
> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
> +        PCI_BASE_ADDRESS_SPACE_IO) {
> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
> +                     | PCI_BASE_ADDRESS_SPACE_IO);
> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
> +                     | PCI_BASE_ADDRESS_SPACE_IO);
> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
> +                     | PCI_BASE_ADDRESS_SPACE_IO);
> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
> +                     | PCI_BASE_ADDRESS_SPACE_IO);
> +        /* BMIBA: 20-23h */
> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
> +                     | PCI_BASE_ADDRESS_SPACE_IO);
> +    }
> +}

Another hint that this is not the right way to be doing this: the values you are 
placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 and bit 0 set 
to 1 which forces a minimum alignment of 4, so either the addresses 0x3f6/0x376 are 
being rounded internally to 0x3f4/0x374 and/or you're lucky that this just happens to 
work on QEMU.

Using the portio_list*() APIs really is the right way to implement this to avoid 
being affected by such issues.

>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
> @@ -221,6 +251,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>       /* Reason: only works as function of VIA southbridge */
>       dc->user_creatable = false;
>   
> +    k->config_write = via_ide_cfg_write;
>       k->realize = via_ide_realize;
>       k->exit = via_ide_exitfn;
>       k->vendor_id = PCI_VENDOR_ID_VIA;


ATB,

Mark.

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
> On 09/10/2023 20:54, BALATON Zoltan wrote:
>
>> The initial value for BARs were set in reset method for emulating
>> legacy mode at start but this does not work because PCI code resets
>> BARs after calling device reset method. Remove this ineffective
>> default to avoid confusion.
>> 
>> Instead move setting the BARs to a callback on writing the PCI config
>> regsiter that sets legacy mode (which firmwares needing this mode seem
>> to do) and fix their values to program it to use legacy port numbers
>> in this case. This does not fully emulate what the data sheet says
>> (which is not very clear on this) but it implements enogh to allow
>> both modes as used by firmwares of machines we emulate.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 36 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..43e8af8d69 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>> 20-23h */
>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>   }
>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> +                              uint32_t val, int len)
>> +{
>> +    pci_default_write_config(pd, addr, val, len);
>> +    /*
>> +     * Bits 0 and 2 of the PCI programming interface register select 
>> between
>> +     * legacy and native mode for the two IDE channels. We don't emulate 
>> this
>> +     * because we cannot easily switch between ISA and PCI in QEMU so 
>> instead
>
> As per my previous email, this statement is demonstrably false: this is now 
> achievable using the portio_list*() APIs.
>
>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports 
>> which
>> +     * works the same. We also don't care about setting each channel 
>> separately
>> +     * as no guest is known to do or need that. We only do this when BARs 
>> are
>> +     * unset when writing this register as logs from real hardware show 
>> that
>> +     * setting legacy mode after BARs were set it will still use ports set 
>> by
>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware 
>> set
>> +     * native mode and programmed BARs and calls it non-100% native mode).
>> +     * But if 0x8a is written righr after reset without setting BARs then 
>> we
>> +     * want legacy ports (this is done by the AmigaOne firmware).
>> +     */
>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        /* BMIBA: 20-23h */
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +    }
>> +}
>
> Another hint that this is not the right way to be doing this: the values you 
> are placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 
> and bit 0 set to 1 which forces a minimum alignment of 4, so either the 
> addresses 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 and/or 
> you're lucky that this just happens to work on QEMU.

The data sheet lists these values for legacy mode bur it seems that bit 1 
is ignored for BAR here and it ends up set to 0x3x4 with the actual reg 
mapped to 0x3x7 for both values ending in 4 or 6 here and both works the 
same with AmigaOS even if I change the values here to 0x3[7f]4 so I can do 
that and that should then match the default values for these regs but not 
match the values listed for legacy mode so the data sheet is wrong either 
way. It still does not make sense to set these in reset method which will 
be overwritten so only works if I set them here.

> Using the portio_list*() APIs really is the right way to implement this to 
> avoid being affected by such issues.

Can you provide an alternative patch using portio_list? I don't know how 
to do that and have no example to follow either so it would be hard for me 
to figure out. Or give some pointers on how to do this if I missed 
something.

Regards,
BALATON Zoltan

>>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PCIIDEState *d = PCI_IDE(dev);
>> @@ -221,6 +251,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
>> *data)
>>       /* Reason: only works as function of VIA southbridge */
>>       dc->user_creatable = false;
>>   +    k->config_write = via_ide_cfg_write;
>>       k->realize = via_ide_realize;
>>       k->exit = via_ide_exitfn;
>>       k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> ATB,
>
> Mark.
>
>
>

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[Bernhard Beschow]

Am 14. Oktober 2023 16:13:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>> 
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method. Remove this ineffective
>>> default to avoid confusion.
>>> 
>>> Instead move setting the BARs to a callback on writing the PCI config
>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>> to do) and fix their values to program it to use legacy port numbers
>>> in this case. This does not fully emulate what the data sheet says
>>> (which is not very clear on this) but it implements enogh to allow
>>> both modes as used by firmwares of machines we emulate.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..43e8af8d69 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>   }
>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> +                              uint32_t val, int len)
>>> +{
>>> +    pci_default_write_config(pd, addr, val, len);
>>> +    /*
>>> +     * Bits 0 and 2 of the PCI programming interface register select between
>>> +     * legacy and native mode for the two IDE channels. We don't emulate this
>>> +     * because we cannot easily switch between ISA and PCI in QEMU so instead
>> 
>> As per my previous email, this statement is demonstrably false: this is now achievable using the portio_list*() APIs.
>> 
>>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports which
>>> +     * works the same. We also don't care about setting each channel separately
>>> +     * as no guest is known to do or need that. We only do this when BARs are
>>> +     * unset when writing this register as logs from real hardware show that
>>> +     * setting legacy mode after BARs were set it will still use ports set by
>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
>>> +     * native mode and programmed BARs and calls it non-100% native mode).
>>> +     * But if 0x8a is written righr after reset without setting BARs then we
>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>> +     */
>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        /* BMIBA: 20-23h */
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +    }
>>> +}
>> 
>> Another hint that this is not the right way to be doing this: the values you are placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 and bit 0 set to 1 which forces a minimum alignment of 4, so either the addresses 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 and/or you're lucky that this just happens to work on QEMU.
>
>The data sheet lists these values for legacy mode bur it seems that bit 1 is ignored for BAR here and it ends up set to 0x3x4 with the actual reg mapped to 0x3x7 for both values ending in 4 or 6 here and both works the same with AmigaOS even if I change the values here to 0x3[7f]4 so I can do that and that should then match the default values for these regs but not match the values listed for legacy mode so the data sheet is wrong either way.

The datasheet lists the command BARs to be mapped to 0x3x4. The command registers (legacy 0x3x6) are mapped at offset 2 in those BARs. So mapping the BARs to 0x3x4 would map the registers to their respective legacy addresses, no?

> It still does not make sense to set these in reset method which will be overwritten so only works if I set them here.
>
>> Using the portio_list*() APIs really is the right way to implement this to avoid being affected by such issues.
>
>Can you provide an alternative patch using portio_list? I don't know how to do that and have no example to follow either so it would be hard for me to figure out. Or give some pointers on how to do this if I missed something.
>
>Regards,
>BALATON Zoltan
>
>>>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>>>   {
>>>       PCIIDEState *d = PCI_IDE(dev);
>>> @@ -221,6 +251,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>>       /* Reason: only works as function of VIA southbridge */
>>>       dc->user_creatable = false;
>>>   +    k->config_write = via_ide_cfg_write;
>>>       k->realize = via_ide_realize;
>>>       k->exit = via_ide_exitfn;
>>>       k->vendor_id = PCI_VENDOR_ID_VIA;
>> 
>> 
>> ATB,
>> 
>> Mark.
>> 
>> 
>> 

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[Mark Cave-Ayland]

On 14/10/2023 17:13, BALATON Zoltan wrote:

> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>>
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method. Remove this ineffective
>>> default to avoid confusion.
>>>
>>> Instead move setting the BARs to a callback on writing the PCI config
>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>> to do) and fix their values to program it to use legacy port numbers
>>> in this case. This does not fully emulate what the data sheet says
>>> (which is not very clear on this) but it implements enogh to allow
>>> both modes as used by firmwares of machines we emulate.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..43e8af8d69 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>   }
>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> +                              uint32_t val, int len)
>>> +{
>>> +    pci_default_write_config(pd, addr, val, len);
>>> +    /*
>>> +     * Bits 0 and 2 of the PCI programming interface register select between
>>> +     * legacy and native mode for the two IDE channels. We don't emulate this
>>> +     * because we cannot easily switch between ISA and PCI in QEMU so instead
>>
>> As per my previous email, this statement is demonstrably false: this is now 
>> achievable using the portio_list*() APIs.
>>
>>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports which
>>> +     * works the same. We also don't care about setting each channel separately
>>> +     * as no guest is known to do or need that. We only do this when BARs are
>>> +     * unset when writing this register as logs from real hardware show that
>>> +     * setting legacy mode after BARs were set it will still use ports set by
>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
>>> +     * native mode and programmed BARs and calls it non-100% native mode).
>>> +     * But if 0x8a is written righr after reset without setting BARs then we
>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>> +     */
>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        /* BMIBA: 20-23h */
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +    }
>>> +}
>>
>> Another hint that this is not the right way to be doing this: the values you are 
>> placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 and bit 0 
>> set to 1 which forces a minimum alignment of 4, so either the addresses 0x3f6/0x376 
>> are being rounded internally to 0x3f4/0x374 and/or you're lucky that this just 
>> happens to work on QEMU.
> 
> The data sheet lists these values for legacy mode bur it seems that bit 1 is ignored 
> for BAR here and it ends up set to 0x3x4 with the actual reg mapped to 0x3x7 for both 
> values ending in 4 or 6 here and both works the same with AmigaOS even if I change 
> the values here to 0x3[7f]4 so I can do that and that should then match the default 
> values for these regs but not match the values listed for legacy mode so the data 
> sheet is wrong either way. It still does not make sense to set these in reset method 
> which will be overwritten so only works if I set them here.
> 
>> Using the portio_list*() APIs really is the right way to implement this to avoid 
>> being affected by such issues.
> 
> Can you provide an alternative patch using portio_list? I don't know how to do that 
> and have no example to follow either so it would be hard for me to figure out. Or 
> give some pointers on how to do this if I missed something.

Here's a hacked up version based upon various bits and pieces from my WIP IDE mode 
switching branch. It's briefly compile tested, and checked with "info mtree" and a 
couple of test boots:


diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..82f2af1c78 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -28,12 +28,27 @@
  #include "hw/pci/pci.h"
  #include "migration/vmstate.h"
  #include "qemu/module.h"
+#include "qemu/range.h"
  #include "sysemu/dma.h"
  #include "hw/isa/vt82c686.h"
  #include "hw/ide/pci.h"
  #include "hw/irq.h"
  #include "trace.h"

+
+/* FIXME: export these from hw/ide/ioport.c */
+static const MemoryRegionPortio ide_portio_list[] = {
+    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
+    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
+    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionPortio ide_portio2_list[] = {
+    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
+    PORTIO_END_OF_LIST(),
+};
+
  static uint64_t bmdma_read(void *opaque, hwaddr addr,
                             unsigned size)
  {
@@ -137,7 +152,10 @@ static void via_ide_reset(DeviceState *dev)
      pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
      pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
      pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
-    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
+    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
+
+    /* Clear subsystem to match real hardware */
+    pci_set_long(pci_conf + 0x2c, 0x0);

      /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
      pci_set_long(pci_conf + 0x40, 0x0a090600);
@@ -159,6 +177,89 @@ static void via_ide_reset(DeviceState *dev)
      pci_set_long(pci_conf + 0xc0, 0x00020001);
  }

+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+                              uint32_t val, int len)
+{
+    uint8_t *pci_conf = pd->config;
+    PCIIDEState *d = PCI_IDE(pd);
+
+    pci_default_write_config(pd, addr, val, len);
+
+    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
+        if (pci_conf[PCI_CLASS_PROG] == 0x8a) {
+            /* FIXME: don't disable BARs
+            pci_default_write_config(pd, PCI_BASE_ADDRESS_0, 0x1, 4);
+            pci_default_write_config(pd, PCI_BASE_ADDRESS_1, 0x1, 4);
+            pci_default_write_config(pd, PCI_BASE_ADDRESS_2, 0x1, 4);
+            pci_default_write_config(pd, PCI_BASE_ADDRESS_3, 0x1, 4);
+            */
+
+            pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x0);
+            pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x0);
+            pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0);
+            pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0);
+
+            /* Clear interrupt pin */
+            pci_config_set_interrupt_pin(pci_conf, 0);
+
+            /* Add legacy IDE ports */
+            if (!d->bus[0].portio_list.owner) {
+                portio_list_init(&d->bus[0].portio_list, OBJECT(pd),
+                                 ide_portio_list, &d->bus[0], "ide");
+                portio_list_add(&d->bus[0].portio_list,
+                                pci_address_space_io(pd), 0x1f0);
+            }
+
+            if (!d->bus[0].portio2_list.owner) {
+                portio_list_init(&d->bus[0].portio2_list, OBJECT(pd),
+                                 ide_portio2_list, &d->bus[0], "ide");
+                portio_list_add(&d->bus[0].portio2_list,
+                                pci_address_space_io(pd), 0x3f6);
+            }
+
+            if (!d->bus[1].portio_list.owner) {
+                portio_list_init(&d->bus[1].portio_list, OBJECT(pd),
+                                 ide_portio_list, &d->bus[1], "ide");
+                portio_list_add(&d->bus[1].portio_list,
+                                pci_address_space_io(pd), 0x170);
+            }
+
+            if (!d->bus[1].portio2_list.owner) {
+                portio_list_init(&d->bus[1].portio2_list, OBJECT(pd),
+                                 ide_portio2_list, &d->bus[1], "ide");
+                portio_list_add(&d->bus[1].portio2_list,
+                                pci_address_space_io(pd), 0x376);
+            }
+        }
+
+        if (pci_conf[PCI_CLASS_PROG] == 0x8f) {
+            /* Set interrupt pin */
+            pci_config_set_interrupt_pin(pci_conf, 1);
+
+            /* Remove legacy IDE ports */
+            if (d->bus[0].portio_list.owner) {
+                portio_list_del(&d->bus[0].portio_list);
+                portio_list_destroy(&d->bus[0].portio_list);
+            }
+
+            if (d->bus[0].portio2_list.owner) {
+                portio_list_del(&d->bus[0].portio2_list);
+                portio_list_destroy(&d->bus[0].portio2_list);
+            }
+
+            if (d->bus[1].portio_list.owner) {
+                portio_list_del(&d->bus[1].portio_list);
+                portio_list_destroy(&d->bus[1].portio_list);
+            }
+
+            if (d->bus[1].portio2_list.owner) {
+                portio_list_del(&d->bus[1].portio2_list);
+                portio_list_destroy(&d->bus[1].portio2_list);
+            }
+        }
+    }
+}
+
  static void via_ide_realize(PCIDevice *dev, Error **errp)
  {
      PCIIDEState *d = PCI_IDE(dev);
@@ -221,6 +322,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
      /* Reason: only works as function of VIA southbridge */
      dc->user_creatable = false;

+    k->config_write = via_ide_cfg_write;
      k->realize = via_ide_realize;
      k->exit = via_ide_exitfn;
      k->vendor_id = PCI_VENDOR_ID_VIA;


Note that this also fixes the output of "lspci -vv" on Linux:

0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master 
SecP PriP])
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping+ SERR- FastB2B- DisINTx-
         Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
         Latency: 0
         Interrupt: pin ? routed to IRQ 14
         Region 0: [virtual] I/O ports at 1000 [size=8]
         Region 1: [virtual] I/O ports at 100c [size=4]
         Region 2: [virtual] I/O ports at 1010 [size=8]
         Region 3: [virtual] I/O ports at 101c [size=4]
         Region 4: I/O ports at 1020 [size=16]
         Kernel driver in use: pata_via


Currently the "[virtual]" prefix is missing in QEMU when compared with your lspci 
output from real hardware: this patch fixes it, because it allows the legacy IDE 
ioports to exist whilst having the BARs set to zero which isn't possible with your 
current patch.


ATB,

Mark.

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

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

On Sun, 15 Oct 2023, Mark Cave-Ayland wrote:
> On 14/10/2023 17:13, BALATON Zoltan wrote:
>> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>>> 
>>>> The initial value for BARs were set in reset method for emulating
>>>> legacy mode at start but this does not work because PCI code resets
>>>> BARs after calling device reset method. Remove this ineffective
>>>> default to avoid confusion.
>>>> 
>>>> Instead move setting the BARs to a callback on writing the PCI config
>>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>>> to do) and fix their values to program it to use legacy port numbers
>>>> in this case. This does not fully emulate what the data sheet says
>>>> (which is not very clear on this) but it implements enogh to allow
>>>> both modes as used by firmwares of machines we emulate.
>>>> 
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index fff23803a6..43e8af8d69 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>>>> 20-23h */
>>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO 
>>>> Configuration*/
>>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>>   }
>>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>> +                              uint32_t val, int len)
>>>> +{
>>>> +    pci_default_write_config(pd, addr, val, len);
>>>> +    /*
>>>> +     * Bits 0 and 2 of the PCI programming interface register select 
>>>> between
>>>> +     * legacy and native mode for the two IDE channels. We don't emulate 
>>>> this
>>>> +     * because we cannot easily switch between ISA and PCI in QEMU so 
>>>> instead
>>> 
>>> As per my previous email, this statement is demonstrably false: this is 
>>> now achievable using the portio_list*() APIs.
>>> 
>>>> +     * when guest selects legacy mode we set the PCI BARs to legacy 
>>>> ports which
>>>> +     * works the same. We also don't care about setting each channel 
>>>> separately
>>>> +     * as no guest is known to do or need that. We only do this when 
>>>> BARs are
>>>> +     * unset when writing this register as logs from real hardware show 
>>>> that
>>>> +     * setting legacy mode after BARs were set it will still use ports 
>>>> set by
>>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware 
>>>> set
>>>> +     * native mode and programmed BARs and calls it non-100% native 
>>>> mode).
>>>> +     * But if 0x8a is written righr after reset without setting BARs 
>>>> then we
>>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>>> +     */
>>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        /* BMIBA: 20-23h */
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +    }
>>>> +}
>>> 
>>> Another hint that this is not the right way to be doing this: the values 
>>> you are placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced 
>>> to 0 and bit 0 set to 1 which forces a minimum alignment of 4, so either 
>>> the addresses 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 
>>> and/or you're lucky that this just happens to work on QEMU.
>> 
>> The data sheet lists these values for legacy mode bur it seems that bit 1 
>> is ignored for BAR here and it ends up set to 0x3x4 with the actual reg 
>> mapped to 0x3x7 for both values ending in 4 or 6 here and both works the 
>> same with AmigaOS even if I change the values here to 0x3[7f]4 so I can do 
>> that and that should then match the default values for these regs but not 
>> match the values listed for legacy mode so the data sheet is wrong either 
>> way. It still does not make sense to set these in reset method which will 
>> be overwritten so only works if I set them here.
>> 
>>> Using the portio_list*() APIs really is the right way to implement this to 
>>> avoid being affected by such issues.
>> 
>> Can you provide an alternative patch using portio_list? I don't know how to 
>> do that and have no example to follow either so it would be hard for me to 
>> figure out. Or give some pointers on how to do this if I missed something.
>
> Here's a hacked up version based upon various bits and pieces from my WIP IDE 
> mode switching branch. It's briefly compile tested, and checked with "info 
> mtree" and a couple of test boots:

Thanks. Could you please make it a proper patch so we can test this and if 
works with all guests could use as a replacement for patch 1 in the 
series? It probably just needs a commit message and your S-o-b without 
which I can't incorporate it in the series.

Regards,
BALATON Zoltan

> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..82f2af1c78 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,12 +28,27 @@
> #include "hw/pci/pci.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> +#include "qemu/range.h"
> #include "sysemu/dma.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/ide/pci.h"
> #include "hw/irq.h"
> #include "trace.h"
>
> +
> +/* FIXME: export these from hw/ide/ioport.c */
> +static const MemoryRegionPortio ide_portio_list[] = {
> +    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
> +    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
> +    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> +static const MemoryRegionPortio ide_portio2_list[] = {
> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
> @@ -137,7 +152,10 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
> 20-23h */
> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
> +
> +    /* Clear subsystem to match real hardware */
> +    pci_set_long(pci_conf + 0x2c, 0x0);
>
>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>     pci_set_long(pci_conf + 0x40, 0x0a090600);
> @@ -159,6 +177,89 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    uint8_t *pci_conf = pd->config;
> +    PCIIDEState *d = PCI_IDE(pd);
> +
> +    pci_default_write_config(pd, addr, val, len);
> +
> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
> +        if (pci_conf[PCI_CLASS_PROG] == 0x8a) {
> +            /* FIXME: don't disable BARs
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_0, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_1, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_2, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_3, 0x1, 4);
> +            */
> +
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0);
> +
> +            /* Clear interrupt pin */
> +            pci_config_set_interrupt_pin(pci_conf, 0);
> +
> +            /* Add legacy IDE ports */
> +            if (!d->bus[0].portio_list.owner) {
> +                portio_list_init(&d->bus[0].portio_list, OBJECT(pd),
> +                                 ide_portio_list, &d->bus[0], "ide");
> +                portio_list_add(&d->bus[0].portio_list,
> +                                pci_address_space_io(pd), 0x1f0);
> +            }
> +
> +            if (!d->bus[0].portio2_list.owner) {
> +                portio_list_init(&d->bus[0].portio2_list, OBJECT(pd),
> +                                 ide_portio2_list, &d->bus[0], "ide");
> +                portio_list_add(&d->bus[0].portio2_list,
> +                                pci_address_space_io(pd), 0x3f6);
> +            }
> +
> +            if (!d->bus[1].portio_list.owner) {
> +                portio_list_init(&d->bus[1].portio_list, OBJECT(pd),
> +                                 ide_portio_list, &d->bus[1], "ide");
> +                portio_list_add(&d->bus[1].portio_list,
> +                                pci_address_space_io(pd), 0x170);
> +            }
> +
> +            if (!d->bus[1].portio2_list.owner) {
> +                portio_list_init(&d->bus[1].portio2_list, OBJECT(pd),
> +                                 ide_portio2_list, &d->bus[1], "ide");
> +                portio_list_add(&d->bus[1].portio2_list,
> +                                pci_address_space_io(pd), 0x376);
> +            }
> +        }
> +
> +        if (pci_conf[PCI_CLASS_PROG] == 0x8f) {
> +            /* Set interrupt pin */
> +            pci_config_set_interrupt_pin(pci_conf, 1);
> +
> +            /* Remove legacy IDE ports */
> +            if (d->bus[0].portio_list.owner) {
> +                portio_list_del(&d->bus[0].portio_list);
> +                portio_list_destroy(&d->bus[0].portio_list);
> +            }
> +
> +            if (d->bus[0].portio2_list.owner) {
> +                portio_list_del(&d->bus[0].portio2_list);
> +                portio_list_destroy(&d->bus[0].portio2_list);
> +            }
> +
> +            if (d->bus[1].portio_list.owner) {
> +                portio_list_del(&d->bus[1].portio_list);
> +                portio_list_destroy(&d->bus[1].portio_list);
> +            }
> +
> +            if (d->bus[1].portio2_list.owner) {
> +                portio_list_del(&d->bus[1].portio2_list);
> +                portio_list_destroy(&d->bus[1].portio2_list);
> +            }
> +        }
> +    }
> +}
> +
> static void via_ide_realize(PCIDevice *dev, Error **errp)
> {
>     PCIIDEState *d = PCI_IDE(dev);
> @@ -221,6 +322,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
> *data)
>     /* Reason: only works as function of VIA southbridge */
>     dc->user_creatable = false;
>
> +    k->config_write = via_ide_cfg_write;
>     k->realize = via_ide_realize;
>     k->exit = via_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> Note that this also fixes the output of "lspci -vv" on Linux:
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a 
> [Master SecP PriP])
>        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping+ SERR- FastB2B- DisINTx-
>        Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>        Latency: 0
>        Interrupt: pin ? routed to IRQ 14
>        Region 0: [virtual] I/O ports at 1000 [size=8]
>        Region 1: [virtual] I/O ports at 100c [size=4]
>        Region 2: [virtual] I/O ports at 1010 [size=8]
>        Region 3: [virtual] I/O ports at 101c [size=4]
>        Region 4: I/O ports at 1020 [size=16]
>        Kernel driver in use: pata_via
>
>
> Currently the "[virtual]" prefix is missing in QEMU when compared with your 
> lspci output from real hardware: this patch fixes it, because it allows the 
> legacy IDE ioports to exist whilst having the BARs set to zero which isn't 
> possible with your current patch.
>
>
> ATB,
>
> Mark.
>
>
>

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

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

On Sun, 15 Oct 2023, Mark Cave-Ayland wrote:
> On 14/10/2023 17:13, BALATON Zoltan wrote:
>> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>>>> The initial value for BARs were set in reset method for emulating
>>>> legacy mode at start but this does not work because PCI code resets
>>>> BARs after calling device reset method. Remove this ineffective
>>>> default to avoid confusion.
>>>> 
>>>> Instead move setting the BARs to a callback on writing the PCI config
>>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>>> to do) and fix their values to program it to use legacy port numbers
>>>> in this case. This does not fully emulate what the data sheet says
>>>> (which is not very clear on this) but it implements enogh to allow
>>>> both modes as used by firmwares of machines we emulate.
>>>> 
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index fff23803a6..43e8af8d69 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>>>> 20-23h */
>>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO 
>>>> Configuration*/
>>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>>   }
>>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>> +                              uint32_t val, int len)
>>>> +{
>>>> +    pci_default_write_config(pd, addr, val, len);
>>>> +    /*
>>>> +     * Bits 0 and 2 of the PCI programming interface register select 
>>>> between
>>>> +     * legacy and native mode for the two IDE channels. We don't emulate 
>>>> this
>>>> +     * because we cannot easily switch between ISA and PCI in QEMU so 
>>>> instead
>>> 
>>> As per my previous email, this statement is demonstrably false: this is 
>>> now achievable using the portio_list*() APIs.
>>> 
>>>> +     * when guest selects legacy mode we set the PCI BARs to legacy 
>>>> ports which
>>>> +     * works the same. We also don't care about setting each channel 
>>>> separately
>>>> +     * as no guest is known to do or need that. We only do this when 
>>>> BARs are
>>>> +     * unset when writing this register as logs from real hardware show 
>>>> that
>>>> +     * setting legacy mode after BARs were set it will still use ports 
>>>> set by
>>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware 
>>>> set
>>>> +     * native mode and programmed BARs and calls it non-100% native 
>>>> mode).
>>>> +     * But if 0x8a is written righr after reset without setting BARs 
>>>> then we
>>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>>> +     */
>>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +        /* BMIBA: 20-23h */
>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>> +    }
>>>> +}
>>> 
>>> Another hint that this is not the right way to be doing this: the values 
>>> you are placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced 
>>> to 0 and bit 0 set to 1 which forces a minimum alignment of 4, so either 
>>> the addresses 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 
>>> and/or you're lucky that this just happens to work on QEMU.
>> 
>> The data sheet lists these values for legacy mode bur it seems that bit 1 
>> is ignored for BAR here and it ends up set to 0x3x4 with the actual reg 
>> mapped to 0x3x7 for both values ending in 4 or 6 here and both works the 
>> same with AmigaOS even if I change the values here to 0x3[7f]4 so I can do 
>> that and that should then match the default values for these regs but not 
>> match the values listed for legacy mode so the data sheet is wrong either 
>> way. It still does not make sense to set these in reset method which will 
>> be overwritten so only works if I set them here.
>> 
>>> Using the portio_list*() APIs really is the right way to implement this to 
>>> avoid being affected by such issues.
>> 
>> Can you provide an alternative patch using portio_list? I don't know how to 
>> do that and have no example to follow either so it would be hard for me to 
>> figure out. Or give some pointers on how to do this if I missed something.
>
> Here's a hacked up version based upon various bits and pieces from my WIP IDE 
> mode switching branch. It's briefly compile tested, and checked with "info 
> mtree" and a couple of test boots:


I gave this a try and while it works with the guests I've tried I'm still 
not convinced. See comments below.


> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..82f2af1c78 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,12 +28,27 @@
> #include "hw/pci/pci.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> +#include "qemu/range.h"
> #include "sysemu/dma.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/ide/pci.h"
> #include "hw/irq.h"
> #include "trace.h"
>
> +
> +/* FIXME: export these from hw/ide/ioport.c */
> +static const MemoryRegionPortio ide_portio_list[] = {
> +    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
> +    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
> +    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> +static const MemoryRegionPortio ide_portio2_list[] = {
> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
> @@ -137,7 +152,10 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
> 20-23h */
> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);

The vt8231 data sheet says the interrupt pin should always be 1 while 
according to the vt82c686b data sheet this means legacy or native 
interrupt routing and defaults to 0. We probably don't do anything with it 
so no matter what we have here and we can change it to 0 but maybe there's 
someting off here in any case.

> +
> +    /* Clear subsystem to match real hardware */
> +    pci_set_long(pci_conf + 0x2c, 0x0);
>
>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>     pci_set_long(pci_conf + 0x40, 0x0a090600);
> @@ -159,6 +177,89 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    uint8_t *pci_conf = pd->config;
> +    PCIIDEState *d = PCI_IDE(pd);
> +
> +    pci_default_write_config(pd, addr, val, len);
> +
> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
> +        if (pci_conf[PCI_CLASS_PROG] == 0x8a) {
> +            /* FIXME: don't disable BARs
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_0, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_1, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_2, 0x1, 4);
> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_3, 0x1, 4);
> +            */
> +
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0);
> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0);

This is the part why I think this is not ready for merge yet. It seems to 
leave BARs enabled but 0 their regs so now we have ide ports *both* at the 
previous BAR values and at legacy ports while BAR values are 0. This does 
not look right and seems much more hacky than my patch that changes BARs 
to legacy ports to emulate legacy mode. You probably need this becuase of 
what Linux does on pegasos2 which I think is proving real chip is also 
using BARs as my patch.

Therefore I think we should go with my patch for now to not hold up adding 
amigaone machine and allow progress with it and then when you have more 
time to elaborate this idea of using portio_list to remove the FIXME 
comments from this PoC patch we can revisit this any time later. There's 
no reason to hold other's work back to get this sorted out now and solving 
a problem with my patch now does not mean we cannot improve this device 
model further in the future. So if you can't make this a clean patch now 
that can replace my patch please let my patch accepted until you can make 
this a viable alternative. For now this just seems like an idea that needs 
more work but not ready yet.

Regards,
BALATON Zoltan

> +
> +            /* Clear interrupt pin */
> +            pci_config_set_interrupt_pin(pci_conf, 0);
> +
> +            /* Add legacy IDE ports */
> +            if (!d->bus[0].portio_list.owner) {
> +                portio_list_init(&d->bus[0].portio_list, OBJECT(pd),
> +                                 ide_portio_list, &d->bus[0], "ide");
> +                portio_list_add(&d->bus[0].portio_list,
> +                                pci_address_space_io(pd), 0x1f0);
> +            }
> +
> +            if (!d->bus[0].portio2_list.owner) {
> +                portio_list_init(&d->bus[0].portio2_list, OBJECT(pd),
> +                                 ide_portio2_list, &d->bus[0], "ide");
> +                portio_list_add(&d->bus[0].portio2_list,
> +                                pci_address_space_io(pd), 0x3f6);
> +            }
> +
> +            if (!d->bus[1].portio_list.owner) {
> +                portio_list_init(&d->bus[1].portio_list, OBJECT(pd),
> +                                 ide_portio_list, &d->bus[1], "ide");
> +                portio_list_add(&d->bus[1].portio_list,
> +                                pci_address_space_io(pd), 0x170);
> +            }
> +
> +            if (!d->bus[1].portio2_list.owner) {
> +                portio_list_init(&d->bus[1].portio2_list, OBJECT(pd),
> +                                 ide_portio2_list, &d->bus[1], "ide");
> +                portio_list_add(&d->bus[1].portio2_list,
> +                                pci_address_space_io(pd), 0x376);
> +            }
> +        }
> +
> +        if (pci_conf[PCI_CLASS_PROG] == 0x8f) {
> +            /* Set interrupt pin */
> +            pci_config_set_interrupt_pin(pci_conf, 1);
> +
> +            /* Remove legacy IDE ports */
> +            if (d->bus[0].portio_list.owner) {
> +                portio_list_del(&d->bus[0].portio_list);
> +                portio_list_destroy(&d->bus[0].portio_list);
> +            }
> +
> +            if (d->bus[0].portio2_list.owner) {
> +                portio_list_del(&d->bus[0].portio2_list);
> +                portio_list_destroy(&d->bus[0].portio2_list);
> +            }
> +
> +            if (d->bus[1].portio_list.owner) {
> +                portio_list_del(&d->bus[1].portio_list);
> +                portio_list_destroy(&d->bus[1].portio_list);
> +            }
> +
> +            if (d->bus[1].portio2_list.owner) {
> +                portio_list_del(&d->bus[1].portio2_list);
> +                portio_list_destroy(&d->bus[1].portio2_list);
> +            }
> +        }
> +    }
> +}
> +
> static void via_ide_realize(PCIDevice *dev, Error **errp)
> {
>     PCIIDEState *d = PCI_IDE(dev);
> @@ -221,6 +322,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
> *data)
>     /* Reason: only works as function of VIA southbridge */
>     dc->user_creatable = false;
>
> +    k->config_write = via_ide_cfg_write;
>     k->realize = via_ide_realize;
>     k->exit = via_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> Note that this also fixes the output of "lspci -vv" on Linux:
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a 
> [Master SecP PriP])
>        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping+ SERR- FastB2B- DisINTx-
>        Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>        Latency: 0
>        Interrupt: pin ? routed to IRQ 14
>        Region 0: [virtual] I/O ports at 1000 [size=8]
>        Region 1: [virtual] I/O ports at 100c [size=4]
>        Region 2: [virtual] I/O ports at 1010 [size=8]
>        Region 3: [virtual] I/O ports at 101c [size=4]
>        Region 4: I/O ports at 1020 [size=16]
>        Kernel driver in use: pata_via
>
>
> Currently the "[virtual]" prefix is missing in QEMU when compared with your 
> lspci output from real hardware: this patch fixes it, because it allows the 
> legacy IDE ioports to exist whilst having the BARs set to zero which isn't 
> possible with your current patch.
>
>
> ATB,
>
> Mark.
>
>
>

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[Mark Cave-Ayland]

On 16/10/2023 23:16, BALATON Zoltan wrote:

> On Sun, 15 Oct 2023, Mark Cave-Ayland wrote:
>> On 14/10/2023 17:13, BALATON Zoltan wrote:
>>> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>>>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>>>>> The initial value for BARs were set in reset method for emulating
>>>>> legacy mode at start but this does not work because PCI code resets
>>>>> BARs after calling device reset method. Remove this ineffective
>>>>> default to avoid confusion.
>>>>>
>>>>> Instead move setting the BARs to a callback on writing the PCI config
>>>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>>>> to do) and fix their values to program it to use legacy port numbers
>>>>> in this case. This does not fully emulate what the data sheet says
>>>>> (which is not very clear on this) but it implements enogh to allow
>>>>> both modes as used by firmwares of machines we emulate.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>> index fff23803a6..43e8af8d69 100644
>>>>> --- a/hw/ide/via.c
>>>>> +++ b/hw/ide/via.c
>>>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>>>   }
>>>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>>>> +                              uint32_t val, int len)
>>>>> +{
>>>>> +    pci_default_write_config(pd, addr, val, len);
>>>>> +    /*
>>>>> +     * Bits 0 and 2 of the PCI programming interface register select between
>>>>> +     * legacy and native mode for the two IDE channels. We don't emulate this
>>>>> +     * because we cannot easily switch between ISA and PCI in QEMU so instead
>>>>
>>>> As per my previous email, this statement is demonstrably false: this is now 
>>>> achievable using the portio_list*() APIs.
>>>>
>>>>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports which
>>>>> +     * works the same. We also don't care about setting each channel separately
>>>>> +     * as no guest is known to do or need that. We only do this when BARs are
>>>>> +     * unset when writing this register as logs from real hardware show that
>>>>> +     * setting legacy mode after BARs were set it will still use ports set by
>>>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
>>>>> +     * native mode and programmed BARs and calls it non-100% native mode).
>>>>> +     * But if 0x8a is written righr after reset without setting BARs then we
>>>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>>>> +     */
>>>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>>> +        /* BMIBA: 20-23h */
>>>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>>>> +    }
>>>>> +}
>>>>
>>>> Another hint that this is not the right way to be doing this: the values you are 
>>>> placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 and bit 0 
>>>> set to 1 which forces a minimum alignment of 4, so either the addresses 
>>>> 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 and/or you're lucky that 
>>>> this just happens to work on QEMU.
>>>
>>> The data sheet lists these values for legacy mode bur it seems that bit 1 is 
>>> ignored for BAR here and it ends up set to 0x3x4 with the actual reg mapped to 
>>> 0x3x7 for both values ending in 4 or 6 here and both works the same with AmigaOS 
>>> even if I change the values here to 0x3[7f]4 so I can do that and that should then 
>>> match the default values for these regs but not match the values listed for legacy 
>>> mode so the data sheet is wrong either way. It still does not make sense to set 
>>> these in reset method which will be overwritten so only works if I set them here.
>>>
>>>> Using the portio_list*() APIs really is the right way to implement this to avoid 
>>>> being affected by such issues.
>>>
>>> Can you provide an alternative patch using portio_list? I don't know how to do 
>>> that and have no example to follow either so it would be hard for me to figure 
>>> out. Or give some pointers on how to do this if I missed something.
>>
>> Here's a hacked up version based upon various bits and pieces from my WIP IDE mode 
>> switching branch. It's briefly compile tested, and checked with "info mtree" and a 
>> couple of test boots:
> 
> 
> I gave this a try and while it works with the guests I've tried I'm still not 
> convinced. See comments below.

Okay, that's good news and proves the basic concept works as expected.

>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..82f2af1c78 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -28,12 +28,27 @@
>> #include "hw/pci/pci.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> +#include "qemu/range.h"
>> #include "sysemu/dma.h"
>> #include "hw/isa/vt82c686.h"
>> #include "hw/ide/pci.h"
>> #include "hw/irq.h"
>> #include "trace.h"
>>
>> +
>> +/* FIXME: export these from hw/ide/ioport.c */
>> +static const MemoryRegionPortio ide_portio_list[] = {
>> +    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
>> +    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
>> +    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
>> +    PORTIO_END_OF_LIST(),
>> +};
>> +
>> +static const MemoryRegionPortio ide_portio2_list[] = {
>> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>> +    PORTIO_END_OF_LIST(),
>> +};
>> +
>> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>                            unsigned size)
>> {
>> @@ -137,7 +152,10 @@ static void via_ide_reset(DeviceState *dev)
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
> 
> The vt8231 data sheet says the interrupt pin should always be 1 while according to 
> the vt82c686b data sheet this means legacy or native interrupt routing and defaults 
> to 0. We probably don't do anything with it so no matter what we have here and we can 
> change it to 0 but maybe there's someting off here in any case.

According to the VT8231 datasheet I have here, the interrupt pin register is 
read-only but defaults to zero. But then that's fine because that is the expected 
value in legacy mode which is what you would expect with a default PCI_CLASS_PROG set 
to 0x8a.

>> +
>> +    /* Clear subsystem to match real hardware */
>> +    pci_set_long(pci_conf + 0x2c, 0x0);
>>
>>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>     pci_set_long(pci_conf + 0x40, 0x0a090600);
>> @@ -159,6 +177,89 @@ static void via_ide_reset(DeviceState *dev)
>>     pci_set_long(pci_conf + 0xc0, 0x00020001);
>> }
>>
>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> +                              uint32_t val, int len)
>> +{
>> +    uint8_t *pci_conf = pd->config;
>> +    PCIIDEState *d = PCI_IDE(pd);
>> +
>> +    pci_default_write_config(pd, addr, val, len);
>> +
>> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
>> +        if (pci_conf[PCI_CLASS_PROG] == 0x8a) {
>> +            /* FIXME: don't disable BARs
>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_0, 0x1, 4);
>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_1, 0x1, 4);
>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_2, 0x1, 4);
>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_3, 0x1, 4);
>> +            */
>> +
>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x0);
>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x0);
>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0);
>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0);
> 
> This is the part why I think this is not ready for merge yet. It seems to leave BARs 
> enabled but 0 their regs so now we have ide ports *both* at the previous BAR values 
> and at legacy ports while BAR values are 0. This does not look right and seems much 
> more hacky than my patch that changes BARs to legacy ports to emulate legacy mode. 
> You probably need this becuase of what Linux does on pegasos2 which I think is 
> proving real chip is also using BARs as my patch.

Well we both agree there are some quirks with this chip, but the reason for pursuing 
this approach is for 2 reasons: i) it matches the dumps you provided from real 
hardware (unlike your existing patch) and ii) it proves the basic concept and allows 
us to move forward with the consolidation work done by myself, Phil and Bernhard. 
Here's what I have in my tests here:


lspci output from real hardware (provided by you)
=================================================

0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master 
SecP PriP])
          Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
          Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
          Latency: 0
          Interrupt: pin ? routed to IRQ 14
          Region 0: [virtual] I/O ports at 1000 [size=8]
          Region 1: [virtual] I/O ports at 100c [size=4]
          Region 2: [virtual] I/O ports at 1010 [size=8]
          Region 3: [virtual] I/O ports at 101c [size=4]
          Region 4: I/O ports at 1020 [size=16]
          Capabilities: [c0] Power Management version 2
                  Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
PME(D0-,D1-,D2-,D3hot-,D3cold-)
                  Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
          Kernel driver in use: pata_via

0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
00: 06 11 71 05 07 00 90 02 06 8a 01 01 00 00 00 00
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 61 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 00 00 00
40: 0b f2 09 35 18 1c c0 00 99 20 20 20 02 00 20 20
50: 17 f4 f0 f0 14 00 00 00 a8 a8 a8 a8 00 00 00 00
60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
70: 02 01 00 00 00 00 00 00 02 01 00 00 00 00 00 00
80: 00 e0 b0 2f 00 00 00 00 00 f0 b0 2f 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 06 00 71 05 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Note that BARS 0-3 are all zeroed, the IDE controller reports legacy mode (0x8a) and 
the interrupt pin is set to 0x0.


lsipci output from your proposed patch
======================================

0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master 
SecP PriP])
         Subsystem: Red Hat, Inc Device 1100
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping+ SERR- FastB2B- DisINTx-
         Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
         Latency: 0
         Interrupt: pin A routed to IRQ 14
         Region 0: I/O ports at 1000 [size=8]
         Region 1: I/O ports at 100c [size=4]
         Region 2: I/O ports at 1010 [size=8]
         Region 3: I/O ports at 101c [size=4]
         Region 4: I/O ports at 1020 [size=16]
         Kernel driver in use: pata_via
00: 06 11 71 05 87 00 80 02 06 8a 01 01 00 00 00 00
10: 01 10 00 00 0d 10 00 00 11 10 00 00 1d 10 00 00
20: 21 10 00 00 00 00 00 00 00 00 00 00 f4 1a 00 11
30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 01 00 00
40: 0b f2 09 35 18 1c c0 00 99 31 99 99 a2 00 31 a8
50: 17 f0 17 17 14 00 00 00 00 00 00 00 00 00 00 00
60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Note that BARS 0-3 have values (incorrect), the IDE controller reports legacy mode 
(0x8a) and the interrupt pin is set to 0x1 (incorrect).


lsipci output from my proposed patch
====================================

0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master 
SecP PriP])
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping+ SERR- FastB2B- DisINTx-
         Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- 
<MAbort- >SERR- <PERR- INTx-
         Latency: 0
         Interrupt: pin ? routed to IRQ 14
         Region 0: [virtual] I/O ports at 1000 [size=8]
         Region 1: [virtual] I/O ports at 100c [size=4]
         Region 2: [virtual] I/O ports at 1010 [size=8]
         Region 3: [virtual] I/O ports at 101c [size=4]
         Region 4: I/O ports at 1020 [size=16]
         Kernel driver in use: pata_via
00: 06 11 71 05 87 00 80 02 06 8a 01 01 00 00 00 00
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 21 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 00 00 00
40: 0b f2 09 35 18 1c c0 00 99 31 99 99 a2 00 31 a8
50: 17 f0 17 17 14 00 00 00 00 00 00 00 00 00 00 00
60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Note that BARS 0-3 are all zeroed (correct), the IDE controller reports legacy mode 
(0x8a) and the interrupt pin is set to 0x0 (correct).


On the legacy ports being present on Linux, that is to be expected when setting the 
hardware to legacy mode which Linux does by writing 0x8a to PCI_CLASS_PROG. Why the 
BARs are still active in legacy mode is the better question to ask, but we won't know 
for sure unless someone can give me access to real hardware. The fact the legacy 
ports are there isn't an issue even if BARs are active as firmware avoids assigning 
PCI IO memory below 0x400 to avoid clashes with legacy and/or buggy hardware.

> Therefore I think we should go with my patch for now to not hold up adding amigaone 
> machine and allow progress with it and then when you have more time to elaborate this 
> idea of using portio_list to remove the FIXME comments from this PoC patch we can 
> revisit this any time later. There's no reason to hold other's work back to get this 
> sorted out now and solving a problem with my patch now does not mean we cannot 
> improve this device model further in the future. So if you can't make this a clean 
> patch now that can replace my patch please let my patch accepted until you can make 
> this a viable alternative. For now this just seems like an idea that needs more work 
> but not ready yet.

How is this holding your work back? I've pointed out the issues with your patch and 
provided you a near complete solution that i) you have confirmed working with your 
guests, ii) allows further work from myself and others in this area and iii) matches 
the information in the hardware dumps you provided from real hardware. I think it's 
fair to say that both ii) and iii) are notable improvements on your existing patch.

The FIXMEs are there because this patch comes from a branch with further work in this 
area: the only thing that remains are to split out the ide_portio_list[] and 
ide_portio2_list[] arrays so they can be reused from hw/ide/ioport.c and delete the 
second FIXME comment.

I'm sure you're more than capable of making these changes, and I'd appreciate that 
given my current time constraints. If not, then I can do this but it won't be for a 
few days - fortunately freeze is still a few weeks away, so there is no need for 
immediate urgency.


ATB,

Mark.

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

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

On Tue, 17 Oct 2023, Mark Cave-Ayland wrote:
> On 16/10/2023 23:16, BALATON Zoltan wrote:
>> On Sun, 15 Oct 2023, Mark Cave-Ayland wrote:
>>> On 14/10/2023 17:13, BALATON Zoltan wrote:
>>>> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>>>>> Using the portio_list*() APIs really is the right way to implement this 
>>>>> to avoid being affected by such issues.
>>>> 
>>>> Can you provide an alternative patch using portio_list? I don't know how 
>>>> to do that and have no example to follow either so it would be hard for 
>>>> me to figure out. Or give some pointers on how to do this if I missed 
>>>> something.
>>> 
>>> Here's a hacked up version based upon various bits and pieces from my WIP 
>>> IDE mode switching branch. It's briefly compile tested, and checked with 
>>> "info mtree" and a couple of test boots:
>> 
>> 
>> I gave this a try and while it works with the guests I've tried I'm still 
>> not convinced. See comments below.
>
> Okay, that's good news and proves the basic concept works as expected.

Well it works but seems much more hacky and complex at the moment than my 
patch which also not perfect but works as well and solves the problem with 
much less change so I still don't like this patch of yours too much. But 
if you can clean it in time before the freeze so this won't cause my other 
patches depending on this missing the release I'm OK with this alternative 
approach even if I think it's not necessary and could be done any time 
later. My patch does not prevent you to take this futther and do this 
later, but you not allowing my patch and then not providing alternative in 
time for the series to be merged would make me not able to progress with 
this machine model and prevent users start using it.

>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..82f2af1c78 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -28,12 +28,27 @@
>>> #include "hw/pci/pci.h"
>>> #include "migration/vmstate.h"
>>> #include "qemu/module.h"
>>> +#include "qemu/range.h"
>>> #include "sysemu/dma.h"
>>> #include "hw/isa/vt82c686.h"
>>> #include "hw/ide/pci.h"
>>> #include "hw/irq.h"
>>> #include "trace.h"
>>> 
>>> +
>>> +/* FIXME: export these from hw/ide/ioport.c */
>>> +static const MemoryRegionPortio ide_portio_list[] = {
>>> +    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
>>> +    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
>>> +    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
>>> +    PORTIO_END_OF_LIST(),
>>> +};
>>> +
>>> +static const MemoryRegionPortio ide_portio2_list[] = {
>>> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>>> +    PORTIO_END_OF_LIST(),
>>> +};
>>> +
>>> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>>                            unsigned size)
>>> {
>>> @@ -137,7 +152,10 @@ static void via_ide_reset(DeviceState *dev)
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>>> 20-23h */
>>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
>> 
>> The vt8231 data sheet says the interrupt pin should always be 1 while 
>> according to the vt82c686b data sheet this means legacy or native interrupt 
>> routing and defaults to 0. We probably don't do anything with it so no 
>> matter what we have here and we can change it to 0 but maybe there's 
>> someting off here in any case.
>
> According to the VT8231 datasheet I have here, the interrupt pin register is 
> read-only but defaults to zero. But then that's fine because that is the 
> expected value in legacy mode which is what you would expect with a default 
> PCI_CLASS_PROG set to 0x8a.

According to the data sheet I have (VT8231 South Bridge Revision 2.32, 
September 1, 2004) IDE func config reg 0x3d Interrupt Pin defaults to 01. 
Maybe you're looking at an even more buggy preliminary data sheet that I 
think Bernhard had before too. But it probably does not matter what's in 
this reg anyway so this is a small detail.

>>> +
>>> +    /* Clear subsystem to match real hardware */
>>> +    pci_set_long(pci_conf + 0x2c, 0x0);
>>> 
>>>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>>     pci_set_long(pci_conf + 0x40, 0x0a090600);
>>> @@ -159,6 +177,89 @@ static void via_ide_reset(DeviceState *dev)
>>>     pci_set_long(pci_conf + 0xc0, 0x00020001);
>>> }
>>> 
>>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> +                              uint32_t val, int len)
>>> +{
>>> +    uint8_t *pci_conf = pd->config;
>>> +    PCIIDEState *d = PCI_IDE(pd);
>>> +
>>> +    pci_default_write_config(pd, addr, val, len);
>>> +
>>> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
>>> +        if (pci_conf[PCI_CLASS_PROG] == 0x8a) {
>>> +            /* FIXME: don't disable BARs
>>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_0, 0x1, 4);
>>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_1, 0x1, 4);
>>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_2, 0x1, 4);
>>> +            pci_default_write_config(pd, PCI_BASE_ADDRESS_3, 0x1, 4);
>>> +            */
>>> +
>>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x0);
>>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x0);
>>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0);
>>> +            pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0);
>> 
>> This is the part why I think this is not ready for merge yet. It seems to 
>> leave BARs enabled but 0 their regs so now we have ide ports *both* at the 
>> previous BAR values and at legacy ports while BAR values are 0. This does 
>> not look right and seems much more hacky than my patch that changes BARs to 
>> legacy ports to emulate legacy mode. You probably need this becuase of what 
>> Linux does on pegasos2 which I think is proving real chip is also using 
>> BARs as my patch.
>
> Well we both agree there are some quirks with this chip, but the reason for 
> pursuing this approach is for 2 reasons: i) it matches the dumps you provided 
> from real hardware (unlike your existing patch) and ii) it proves the basic

Matching the dumps does not matter as long as guests work. All of QEMU is 
aiming to model devices enough to allow guests to run but does not care to 
faithfully emulate every detail unless that's needed for guest. It seems 
guests don't care about this so not absolutely needed to be modeled. It 
may be nice to do but should not be a requirement.

> concept and allows us to move forward with the consolidation work done by 
> myself, Phil and Bernhard. Here's what I have in my tests here:

As I said above taking my patch now does not prevent that and then you're 
free to submit these consolidation patches any time later when they are 
ready. There's not urgent need to do them now and hold back this very 
simple series by that and threaten with missing the release because of 
this.

> lspci output from real hardware (provided by you)
> =================================================
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a 
> [Master SecP PriP])
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 0
>         Interrupt: pin ? routed to IRQ 14
>         Region 0: [virtual] I/O ports at 1000 [size=8]
>         Region 1: [virtual] I/O ports at 100c [size=4]
>         Region 2: [virtual] I/O ports at 1010 [size=8]
>         Region 3: [virtual] I/O ports at 101c [size=4]
>         Region 4: I/O ports at 1020 [size=16]
>         Capabilities: [c0] Power Management version 2
>                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
> PME(D0-,D1-,D2-,D3hot-,D3cold-)
>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>         Kernel driver in use: pata_via
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
> 00: 06 11 71 05 07 00 90 02 06 8a 01 01 00 00 00 00
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 20: 61 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 00 00 00
> 40: 0b f2 09 35 18 1c c0 00 99 20 20 20 02 00 20 20
> 50: 17 f4 f0 f0 14 00 00 00 a8 a8 a8 a8 00 00 00 00
> 60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
> 70: 02 01 00 00 00 00 00 00 02 01 00 00 00 00 00 00
> 80: 00 e0 b0 2f 00 00 00 00 00 f0 b0 2f 00 00 00 00
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
> d0: 06 00 71 05 00 00 00 00 00 00 00 00 00 00 00 00
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Note that BARS 0-3 are all zeroed, the IDE controller reports legacy mode 
> (0x8a) and the interrupt pin is set to 0x0.
>
>
> lsipci output from your proposed patch
> ======================================
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a 
> [Master SecP PriP])
>        Subsystem: Red Hat, Inc Device 1100
>        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping+ SERR- FastB2B- DisINTx-
>        Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>        Latency: 0
>        Interrupt: pin A routed to IRQ 14
>        Region 0: I/O ports at 1000 [size=8]
>        Region 1: I/O ports at 100c [size=4]
>        Region 2: I/O ports at 1010 [size=8]
>        Region 3: I/O ports at 101c [size=4]
>        Region 4: I/O ports at 1020 [size=16]
>        Kernel driver in use: pata_via
> 00: 06 11 71 05 87 00 80 02 06 8a 01 01 00 00 00 00
> 10: 01 10 00 00 0d 10 00 00 11 10 00 00 1d 10 00 00
> 20: 21 10 00 00 00 00 00 00 00 00 00 00 f4 1a 00 11
> 30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 01 00 00
> 40: 0b f2 09 35 18 1c c0 00 99 31 99 99 a2 00 31 a8
> 50: 17 f0 17 17 14 00 00 00 00 00 00 00 00 00 00 00
> 60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Note that BARS 0-3 have values (incorrect), the IDE controller reports legacy 
> mode (0x8a) and the interrupt pin is set to 0x1 (incorrect).
>
>
> lsipci output from my proposed patch
> ====================================
>
> 0000:00:0c.1 IDE interface: VIA Technologies, Inc. 
> VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a 
> [Master SecP PriP])
>        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping+ SERR- FastB2B- DisINTx-
>        Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>        Latency: 0
>        Interrupt: pin ? routed to IRQ 14
>        Region 0: [virtual] I/O ports at 1000 [size=8]
>        Region 1: [virtual] I/O ports at 100c [size=4]
>        Region 2: [virtual] I/O ports at 1010 [size=8]
>        Region 3: [virtual] I/O ports at 101c [size=4]
>        Region 4: I/O ports at 1020 [size=16]
>        Kernel driver in use: pata_via
> 00: 06 11 71 05 87 00 80 02 06 8a 01 01 00 00 00 00
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 20: 21 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 30: 00 00 00 00 c0 00 00 00 00 00 00 00 0e 00 00 00
> 40: 0b f2 09 35 18 1c c0 00 99 31 99 99 a2 00 31 a8
> 50: 17 f0 17 17 14 00 00 00 00 00 00 00 00 00 00 00
> 60: 00 02 00 00 00 00 00 00 00 02 00 00 00 00 00 00
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> c0: 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Note that BARS 0-3 are all zeroed (correct), the IDE controller reports 
> legacy mode (0x8a) and the interrupt pin is set to 0x0 (correct).

But these differences aren't solving any issues with guests that I know of 
just makes the patch more complex for no good reason. So I don't see why 
these should matter now or be modelled unless we know this make a 
difference. We have other differences in other regs too that we don't care 
about. Why these should be important to match?

> On the legacy ports being present on Linux, that is to be expected when 
> setting the hardware to legacy mode which Linux does by writing 0x8a to 
> PCI_CLASS_PROG. Why the BARs are still active in legacy mode is the better 
> question to ask, but we won't know for sure unless someone can give me access 
> to real hardware. The fact the legacy ports are there isn't an issue even if 
> BARs are active as firmware avoids assigning PCI IO memory below 0x400 to 
> avoid clashes with legacy and/or buggy hardware.

Exactly because we can't answer these questions is why I think we should 
postpone this debate to when we have more information and answers as the 
change I proposed does not prevent this to be done later. Unless we find 
issue with my patch that cause a problem in a guest I don't see a need to 
make it more complex now. Your patch adds dependeny on portio that's not 
yet cleanly separated from ISA, dependency on ISA_IDE not marked in 
Kconfig (although likely satisfied by VT82C686) and causes two mappings of 
IDE ports in legacy mode which we are not sure matches real hardware so 
I'd argue my patch is a smaller change that's good enough to make some 
progress now and all these can be sorted out any time later without doing 
it all now.

>> Therefore I think we should go with my patch for now to not hold up adding 
>> amigaone machine and allow progress with it and then when you have more 
>> time to elaborate this idea of using portio_list to remove the FIXME 
>> comments from this PoC patch we can revisit this any time later. There's no 
>> reason to hold other's work back to get this sorted out now and solving a 
>> problem with my patch now does not mean we cannot improve this device model 
>> further in the future. So if you can't make this a clean patch now that can 
>> replace my patch please let my patch accepted until you can make this a 
>> viable alternative. For now this just seems like an idea that needs more 
>> work but not ready yet.
>
> How is this holding your work back? I've pointed out the issues with your

I think I answered that above but the point is that my goal is to add 
amigaone machine and this is a prerequisite for that. The patch I proposed 
is a simple fix allowing that while yours is more complex and not yet 
completely elaborated so I don't want this to hold back progress with 
amigaone, especially because my patch would not hold back your 
consolidation of this any time later.

> patch and provided you a near complete solution that i) you have confirmed 
> working with your guests, ii) allows further work from myself and others in 
> this area and iii) matches the information in the hardware dumps you provided 
> from real hardware. I think it's fair to say that both ii) and iii) are 
> notable improvements on your existing patch.

I don't say my patch is perfect and cannot be improved. What I say is that 
my patch solves the problem for now, does not make the model worse than it 
is and does not prevent your work in this area as it's easy to later 
replace that with your way when the still open questions about that are 
resolved. You blocking my patch to do all that now howaver would block my 
work on amigaone machine therefore I'd like to get a solution to this 
that's acceptable as soon as possible.

> The FIXMEs are there because this patch comes from a branch with further work 
> in this area: the only thing that remains are to split out the 
> ide_portio_list[] and ide_portio2_list[] arrays so they can be reused from 
> hw/ide/ioport.c and delete the second FIXME comment.
>
> I'm sure you're more than capable of making these changes, and I'd appreciate 
> that given my current time constraints. If not, then I can do this but it 
> won't be for a few days - fortunately freeze is still a few weeks away, so 
> there is no need for immediate urgency.

I also have time constraints and plan to furhtet improve amigaone machine 
rather than spending more time with this vie-ide model now that my simple 
patch already makes it suitable for amigaone without breaking what already 
works. Also I don't see your future plans with these and don't see where 
these are going so to me this looks just unnecessary compication that I'm 
not keen on working when I don't see why this would be necessary. So I 
think it would be best if we could agree on some solution that's 
acceptable for now and I don't mind getting back to it later and further 
improve it when it's found necessary or you finish these patches but I 
don't see this as something that needs to be done now or blocking the 
series. You were just reminded about it by this change and want to solve 
it now in a much more elaborate way than probably necessary.

Because of that if you can finish it at least 2 weeks before the freeze 
(that means practically this week or by next Monday latest) to make sure 
they don't miss the next release I'm OK with your proposed patch too but I 
don't have time and interest to realise your plans that you don't have 
time now (but that's also not needed as if you can't do it now I already 
have a working patch and that does not mean it can't be done later). I 
hope we can agree on that and you understand my point of view as well.

Regards,
BALATON Zoltan

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[BALATON Zoltan]

On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
> On 09/10/2023 20:54, BALATON Zoltan wrote:
>> The initial value for BARs were set in reset method for emulating
>> legacy mode at start but this does not work because PCI code resets
>> BARs after calling device reset method. Remove this ineffective
>> default to avoid confusion.
>> 
>> Instead move setting the BARs to a callback on writing the PCI config
>> regsiter that sets legacy mode (which firmwares needing this mode seem
>> to do) and fix their values to program it to use legacy port numbers
>> in this case. This does not fully emulate what the data sheet says
>> (which is not very clear on this) but it implements enogh to allow
>> both modes as used by firmwares of machines we emulate.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 36 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..43e8af8d69 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>> 20-23h */
>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>   }
>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> +                              uint32_t val, int len)
>> +{
>> +    pci_default_write_config(pd, addr, val, len);
>> +    /*
>> +     * Bits 0 and 2 of the PCI programming interface register select 
>> between
>> +     * legacy and native mode for the two IDE channels. We don't emulate 
>> this
>> +     * because we cannot easily switch between ISA and PCI in QEMU so 
>> instead
>
> As per my previous email, this statement is demonstrably false: this is now 
> achievable using the portio_list*() APIs.
>
>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports 
>> which
>> +     * works the same. We also don't care about setting each channel 
>> separately
>> +     * as no guest is known to do or need that. We only do this when BARs 
>> are
>> +     * unset when writing this register as logs from real hardware show 
>> that
>> +     * setting legacy mode after BARs were set it will still use ports set 
>> by
>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware 
>> set
>> +     * native mode and programmed BARs and calls it non-100% native mode).
>> +     * But if 0x8a is written righr after reset without setting BARs then 
>> we
>> +     * want legacy ports (this is done by the AmigaOne firmware).
>> +     */
>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +        /* BMIBA: 20-23h */
>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>> +    }
>> +}
>
> Another hint that this is not the right way to be doing this: the values you 
> are placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 
> and bit 0 set to 1 which forces a minimum alignment of 4, so either the 
> addresses 0x3f6/0x376 are being rounded internally to 0x3f4/0x374 and/or 
> you're lucky that this just happens to work on QEMU.
>
> Using the portio_list*() APIs really is the right way to implement this to 
> avoid being affected by such issues.

On second thought I don't think that would work as pegaos2 Linux writes 
0x8a to prog_if but then keeps using the ports as set by BARs so if you 
use ISA ports in this case this will break. I think that proves that real 
chip also uses BARs to emulate legacy mode similar to this approach so 
what we have in this patch is close enough and works well. Your proposed 
alternative is more complex, would not work any better and probably even 
does not work for all cases this works for so I think this is the better 
way now. I've sent a v3 with values changed to match BAR default values 
with 0x3x4 and updating comment and commit message.

Regards,
BALATON Zoltan

>>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PCIIDEState *d = PCI_IDE(dev);
>> @@ -221,6 +251,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
>> *data)
>>       /* Reason: only works as function of VIA southbridge */
>>       dc->user_creatable = false;
>>   +    k->config_write = via_ide_cfg_write;
>>       k->realize = via_ide_realize;
>>       k->exit = via_ide_exitfn;
>>       k->vendor_id = PCI_VENDOR_ID_VIA;
>
>
> ATB,
>
> Mark.
>
>
>

Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation

[Mark Cave-Ayland]

On 14/10/2023 20:43, BALATON Zoltan wrote:

> On Sat, 14 Oct 2023, Mark Cave-Ayland wrote:
>> On 09/10/2023 20:54, BALATON Zoltan wrote:
>>> The initial value for BARs were set in reset method for emulating
>>> legacy mode at start but this does not work because PCI code resets
>>> BARs after calling device reset method. Remove this ineffective
>>> default to avoid confusion.
>>>
>>> Instead move setting the BARs to a callback on writing the PCI config
>>> regsiter that sets legacy mode (which firmwares needing this mode seem
>>> to do) and fix their values to program it to use legacy port numbers
>>> in this case. This does not fully emulate what the data sheet says
>>> (which is not very clear on this) but it implements enogh to allow
>>> both modes as used by firmwares of machines we emulate.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ide/via.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..43e8af8d69 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>                    PCI_STATUS_DEVSEL_MEDIUM);
>>>   -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x000001f0);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x000003f4);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
>>>       pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>>         /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>> @@ -159,6 +154,41 @@ static void via_ide_reset(DeviceState *dev)
>>>       pci_set_long(pci_conf + 0xc0, 0x00020001);
>>>   }
>>>   +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>>> +                              uint32_t val, int len)
>>> +{
>>> +    pci_default_write_config(pd, addr, val, len);
>>> +    /*
>>> +     * Bits 0 and 2 of the PCI programming interface register select between
>>> +     * legacy and native mode for the two IDE channels. We don't emulate this
>>> +     * because we cannot easily switch between ISA and PCI in QEMU so instead
>>
>> As per my previous email, this statement is demonstrably false: this is now 
>> achievable using the portio_list*() APIs.
>>
>>> +     * when guest selects legacy mode we set the PCI BARs to legacy ports which
>>> +     * works the same. We also don't care about setting each channel separately
>>> +     * as no guest is known to do or need that. We only do this when BARs are
>>> +     * unset when writing this register as logs from real hardware show that
>>> +     * setting legacy mode after BARs were set it will still use ports set by
>>> +     * BARs not ISA ports (e.g. pegasos2 Linux does this after firmware set
>>> +     * native mode and programmed BARs and calls it non-100% native mode).
>>> +     * But if 0x8a is written righr after reset without setting BARs then we
>>> +     * want legacy ports (this is done by the AmigaOne firmware).
>>> +     */
>>> +    if (addr == PCI_CLASS_PROG && val == 0x8a &&
>>> +        pci_get_long(pd->config + PCI_BASE_ADDRESS_0) ==
>>> +        PCI_BASE_ADDRESS_SPACE_IO) {
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_0, 0x1f0
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_1, 0x3f6
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_2, 0x170
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_3, 0x376
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +        /* BMIBA: 20-23h */
>>> +        pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00
>>> +                     | PCI_BASE_ADDRESS_SPACE_IO);
>>> +    }
>>> +}
>>
>> Another hint that this is not the right way to be doing this: the values you are 
>> placing in BARS 1 and 3 are illegal. PCI IO BARs have bit 1 forced to 0 and bit 0 
>> set to 1 which forces a minimum alignment of 4, so either the addresses 0x3f6/0x376 
>> are being rounded internally to 0x3f4/0x374 and/or you're lucky that this just 
>> happens to work on QEMU.
>>
>> Using the portio_list*() APIs really is the right way to implement this to avoid 
>> being affected by such issues.
> 
> On second thought I don't think that would work as pegaos2 Linux writes 0x8a to 
> prog_if but then keeps using the ports as set by BARs so if you use ISA ports in this 
> case this will break. I think that proves that real chip also uses BARs to emulate 
> legacy mode similar to this approach so what we have in this patch is close enough 
> and works well. Your proposed alternative is more complex, would not work any better 
> and probably even does not work for all cases this works for so I think this is the 
> better way now. I've sent a v3 with values changed to match BAR default values with 
> 0x3x4 and updating comment and commit message.

I've posted a hacked up (hopefully working) example from my IDE switching branch to 
demonstrate the concept, plus also confirms that BARs aren't being used on real 
hardware because they cannot be set to zero whilst legacy mode is enabled.

Another point to bear in mind is that with Bernhard's series and few extra patches 
from my series, using this approach it becomes possible to move all of this logic 
from the VIA device (and indeed all PCI IDE controllers, including the common BMDMA 
code) into PCIIDEState so the final result will end up much simpler. Obviously there 
is a need to allow behaviourable quirks for things such as VIA, but that can be 
solved easily enough with properties.


ATB,

Mark.

[PATCH v2 2/3] hw/pci-host: Add emulation of Mai Logic Articia S

[BALATON Zoltan]

The Articia S is a generic chipset supporting several different CPUs
that were among others used on some PPC boards. This is a minimal
emulation of the parts needed for emulating the AmigaOne board.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/Kconfig           |   5 +
 hw/pci-host/articia.c         | 293 ++++++++++++++++++++++++++++++++++
 hw/pci-host/meson.build       |   2 +
 include/hw/pci-host/articia.h |  17 ++
 4 files changed, 317 insertions(+)
 create mode 100644 hw/pci-host/articia.c
 create mode 100644 include/hw/pci-host/articia.h

diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index a07070eddf..33014c80a4 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -73,6 +73,11 @@ config SH_PCI
     bool
     select PCI
 
+config ARTICIA
+    bool
+    select PCI
+    select I8259
+
 config MV64361
     bool
     select PCI
diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
new file mode 100644
index 0000000000..f3fcc49f81
--- /dev/null
+++ b/hw/pci-host/articia.c
@@ -0,0 +1,293 @@
+/*
+ * Mai Logic Articia S emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/pci_host.h"
+#include "hw/irq.h"
+#include "hw/i2c/bitbang_i2c.h"
+#include "hw/intc/i8259.h"
+#include "hw/pci-host/articia.h"
+
+/*
+ * This is a minimal emulation of this chip as used in AmigaOne board.
+ * Most features are missing but those are not needed by firmware and guests.
+ */
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
+struct ArticiaHostState {
+    PCIDevice parent_obj;
+
+    ArticiaState *as;
+};
+
+/* TYPE_ARTICIA */
+
+struct ArticiaState {
+    PCIHostState parent_obj;
+
+    qemu_irq irq[PCI_NUM_PINS];
+    MemoryRegion io;
+    MemoryRegion mem;
+    MemoryRegion reg;
+
+    bitbang_i2c_interface smbus;
+    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
+    hwaddr gpio_base;
+    MemoryRegion gpio_reg;
+};
+
+static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    ArticiaState *s = opaque;
+
+    return (s->gpio >> (addr * 8)) & 0xff;
+}
+
+static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
+                               unsigned int size)
+{
+    ArticiaState *s = opaque;
+    uint32_t sh = addr * 8;
+
+    if (addr == 0) {
+        /* in bits read only? */
+        return;
+    }
+
+    if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
+        s->gpio &= ~(0xff << sh | 0xff);
+        s->gpio |= (val & 0xff) << sh;
+        s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
+                                   s->gpio & BIT(16) ?
+                                   !!(s->gpio & BIT(8)) : 1);
+        if ((s->gpio & BIT(17))) {
+            s->gpio &= ~BIT(0);
+            s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
+                                       !!(s->gpio & BIT(9)));
+        }
+    }
+}
+
+static const MemoryRegionOps articia_gpio_ops = {
+    .read = articia_gpio_read,
+    .write = articia_gpio_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 1,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    ArticiaState *s = opaque;
+    uint64_t ret = UINT_MAX;
+
+    switch (addr) {
+    case 0xc00cf8:
+        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
+        break;
+    case 0xe00cfc ... 0xe00cff:
+        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
+        break;
+    case 0xf00000:
+        ret = pic_read_irq(isa_pic);
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
+                      HWADDR_PRIx " %d\n", __func__, addr, size);
+        break;
+    }
+    return ret;
+}
+
+static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned int size)
+{
+    ArticiaState *s = opaque;
+
+    switch (addr) {
+    case 0xc00cf8:
+        pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
+        break;
+    case 0xe00cfc ... 0xe00cff:
+        pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
+                      HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
+        break;
+    }
+}
+
+static const MemoryRegionOps articia_reg_ops = {
+    .read = articia_reg_read,
+    .write = articia_reg_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void articia_pcihost_set_irq(void *opaque, int n, int level)
+{
+    ArticiaState *s = opaque;
+    qemu_set_irq(s->irq[n], level);
+}
+
+/*
+ * AmigaOne SE PCI slot to IRQ routing
+ *
+ * repository: https://source.denx.de/u-boot/custodians/u-boot-avr32.git
+ * refspec: v2010.06
+ * file: board/MAI/AmigaOneG3SE/articiaS_pci.c
+ */
+static int amigaone_pcihost_bus0_map_irq(PCIDevice *pdev, int pin)
+{
+    int devfn_slot = PCI_SLOT(pdev->devfn);
+
+    switch (devfn_slot) {
+    case 6:  /* On board ethernet */
+        return 3;
+    case 7:  /* South bridge */
+        return pin;
+    default: /* PCI Slot 1 Devfn slot 8, Slot 2 Devfn 9, Slot 3 Devfn 10 */
+        return pci_swizzle(devfn_slot, pin);
+    }
+
+}
+
+static void articia_realize(DeviceState *dev, Error **errp)
+{
+    ArticiaState *s = ARTICIA(dev);
+    PCIHostState *h = PCI_HOST_BRIDGE(dev);
+    PCIDevice *pdev;
+
+    bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
+    memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
+                          TYPE_ARTICIA, 4);
+
+    memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
+    memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
+    memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
+                          TYPE_ARTICIA, 0x1000000);
+    memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
+
+    /* devfn_min is 8 that matches first PCI slot in AmigaOne */
+    h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
+                                   amigaone_pcihost_bus0_map_irq, dev, &s->mem,
+                                   &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
+    pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
+                                           TYPE_ARTICIA_PCI_HOST);
+    ARTICIA_PCI_HOST(pdev)->as = s;
+    pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
+
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
+    qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
+}
+
+static void articia_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = articia_realize;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+/* TYPE_ARTICIA_PCI_HOST */
+
+static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
+                                       uint32_t val, int len)
+{
+    ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
+
+    pci_default_write_config(d, addr, val, len);
+    switch (addr) {
+    case 0x40:
+        s->gpio_base = val;
+        break;
+    case 0x44:
+        if (val != 0x11) {
+            /* FIXME what do the bits actually mean? */
+            break;
+        }
+        if (memory_region_is_mapped(&s->gpio_reg)) {
+            memory_region_del_subregion(&s->io, &s->gpio_reg);
+        }
+        memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
+        break;
+    }
+}
+
+static void articia_pci_host_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->config_write = articia_pci_host_cfg_write;
+    k->vendor_id = 0x10cc;
+    k->device_id = 0x0660;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
+    /*
+     * PCI-facing part of the host bridge,
+     * not usable without the host-facing part
+     */
+    dc->user_creatable = false;
+}
+
+/* TYPE_ARTICIA_PCI_BRIDGE */
+
+static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->vendor_id = 0x10cc;
+    k->device_id = 0x0661;
+    k->class_id = PCI_CLASS_BRIDGE_HOST;
+    /*
+     * PCI-facing part of the host bridge,
+     * not usable without the host-facing part
+     */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo articia_types[] = {
+    {
+        .name          = TYPE_ARTICIA,
+        .parent        = TYPE_PCI_HOST_BRIDGE,
+        .instance_size = sizeof(ArticiaState),
+        .class_init    = articia_class_init,
+    },
+    {
+        .name          = TYPE_ARTICIA_PCI_HOST,
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(ArticiaHostState),
+        .class_init    = articia_pci_host_class_init,
+        .interfaces = (InterfaceInfo[]) {
+              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+              { },
+        },
+    },
+    {
+        .name          = TYPE_ARTICIA_PCI_BRIDGE,
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(PCIDevice),
+        .class_init    = articia_pci_bridge_class_init,
+        .interfaces = (InterfaceInfo[]) {
+              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+              { },
+        },
+    },
+};
+
+DEFINE_TYPES(articia_types)
diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index 64eada76fe..40de48eb7f 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -20,6 +20,8 @@ pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
 pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
 # PowerPC E500 boards
 pci_ss.add(when: 'CONFIG_PPCE500_PCI', if_true: files('ppce500.c'))
+# AmigaOne
+pci_ss.add(when: 'CONFIG_ARTICIA', if_true: files('articia.c'))
 # Pegasos2
 pci_ss.add(when: 'CONFIG_MV64361', if_true: files('mv64361.c'))
 
diff --git a/include/hw/pci-host/articia.h b/include/hw/pci-host/articia.h
new file mode 100644
index 0000000000..529c240274
--- /dev/null
+++ b/include/hw/pci-host/articia.h
@@ -0,0 +1,17 @@
+/*
+ * Mai Logic Articia S emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#ifndef ARTICIA_H
+#define ARTICIA_H
+
+#define TYPE_ARTICIA "articia"
+#define TYPE_ARTICIA_PCI_HOST "articia-pci-host"
+#define TYPE_ARTICIA_PCI_BRIDGE "articia-pci-bridge"
+
+#endif
-- 
2.30.9

[PATCH v2 3/3] hw/ppc: Add emulation of AmigaOne XE board

[BALATON Zoltan]

The AmigaOne is a rebranded MAI Teron board that uses U-Boot firmware
with patches to support AmigaOS and is very similar to pegasos2 so can
be easily emulated sharing most code with pegasos2. The reason to
emulate it is that AmigaOS comes in different versions for AmigaOne
and PegasosII which only have drivers for one machine and firmware so
these only run on the specific machine. Adding this board allows
another AmigaOS version to be used reusing already existing peagasos2
emulation. (The AmigaOne was the first of these boards so likely most
widespread which then inspired Pegasos that was later replaced with
PegasosII due to problems with Articia S, so these have a lot of
similarity. Pegasos mainly ran MorphOS while the PegasosII version of
AmigaOS was added later and therefore less common than the AmigaOne
version.)

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 MAINTAINERS                             |   8 ++
 configs/devices/ppc-softmmu/default.mak |   1 +
 hw/ppc/Kconfig                          |   7 +
 hw/ppc/amigaone.c                       | 164 ++++++++++++++++++++++++
 hw/ppc/meson.build                      |   2 +
 5 files changed, 182 insertions(+)
 create mode 100644 hw/ppc/amigaone.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f0e20fde6..03f908c153 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1490,6 +1490,14 @@ F: hw/pci-host/mv64361.c
 F: hw/pci-host/mv643xx.h
 F: include/hw/pci-host/mv64361.h
 
+amigaone
+M: BALATON Zoltan <balaton@eik.bme.hu>
+L: qemu-ppc@nongnu.org
+S: Maintained
+F: hw/ppc/amigaone.c
+F: hw/pci-host/articia.c
+F: include/hw/pci-host/articia.h
+
 Virtual Open Firmware (VOF)
 M: Alexey Kardashevskiy <aik@ozlabs.ru>
 R: David Gibson <david@gibson.dropbear.id.au>
diff --git a/configs/devices/ppc-softmmu/default.mak b/configs/devices/ppc-softmmu/default.mak
index a887f5438b..b85fd2bcd7 100644
--- a/configs/devices/ppc-softmmu/default.mak
+++ b/configs/devices/ppc-softmmu/default.mak
@@ -14,6 +14,7 @@ CONFIG_SAM460EX=y
 CONFIG_MAC_OLDWORLD=y
 CONFIG_MAC_NEWWORLD=y
 
+CONFIG_AMIGAONE=y
 CONFIG_PEGASOS2=y
 
 # For PReP
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 5dfbf47ef5..56f0475a8e 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -69,6 +69,13 @@ config SAM460EX
     select USB_OHCI
     select FDT_PPC
 
+config AMIGAONE
+    bool
+    imply ATI_VGA
+    select ARTICIA
+    select VT82C686
+    select SMBUS_EEPROM
+
 config PEGASOS2
     bool
     imply ATI_VGA
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
new file mode 100644
index 0000000000..3589924c8a
--- /dev/null
+++ b/hw/ppc/amigaone.c
@@ -0,0 +1,164 @@
+/*
+ * QEMU Eyetech AmigaOne/Mai Logic Teron emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu/datadir.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/ppc/ppc.h"
+#include "hw/boards.h"
+#include "hw/loader.h"
+#include "hw/pci-host/articia.h"
+#include "hw/isa/vt82c686.h"
+#include "hw/ide/pci.h"
+#include "hw/i2c/smbus_eeprom.h"
+#include "hw/ppc/ppc.h"
+#include "sysemu/reset.h"
+#include "kvm_ppc.h"
+
+#define BUS_FREQ_HZ 100000000
+
+/*
+ * Firmware binary available at
+ * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
+ * then "tail -c 524288 updater.image >u-boot-amigaone.bin"
+ *
+ * BIOS emulator in firmware cannot run QEMU vgabios and hangs on it, use
+ * -device VGA,romfile=VGABIOS-lgpl-latest.bin
+ * from http://www.nongnu.org/vgabios/ instead.
+ */
+#define PROM_FILENAME "u-boot-amigaone.bin"
+#define PROM_ADDR 0xfff00000
+#define PROM_SIZE (512 * KiB)
+
+static void amigaone_cpu_reset(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+
+    cpu_reset(CPU(cpu));
+    cpu_ppc_tb_reset(&cpu->env);
+}
+
+static void fix_spd_data(uint8_t *spd)
+{
+    uint32_t bank_size = 4 * MiB * spd[31];
+    uint32_t rows = bank_size / spd[13] / spd[17];
+    spd[3] = ctz32(rows) - spd[4];
+}
+
+static void amigaone_init(MachineState *machine)
+{
+    PowerPCCPU *cpu;
+    CPUPPCState *env;
+    MemoryRegion *rom, *pci_mem, *mr;
+    const char *fwname = machine->firmware ?: PROM_FILENAME;
+    char *filename;
+    ssize_t sz;
+    PCIBus *pci_bus;
+    Object *via;
+    DeviceState *dev;
+    I2CBus *i2c_bus;
+    uint8_t *spd_data;
+    int i;
+
+    /* init CPU */
+    cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
+    env = &cpu->env;
+    if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
+        error_report("Incompatible CPU, only 6xx bus supported");
+        exit(1);
+    }
+    cpu_ppc_tb_init(env, BUS_FREQ_HZ / 4);
+    qemu_register_reset(amigaone_cpu_reset, cpu);
+
+    /* RAM */
+    if (machine->ram_size > 2 * GiB) {
+        error_report("RAM size more than 2 GiB is not supported");
+        exit(1);
+    }
+    memory_region_add_subregion(get_system_memory(), 0, machine->ram);
+    if (machine->ram_size < 1 * GiB + 32 * KiB) {
+        /* Firmware uses this area for startup */
+        mr = g_new(MemoryRegion, 1);
+        memory_region_init_ram(mr, NULL, "init-cache", 32 * KiB, &error_fatal);
+        memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
+    }
+
+    /* allocate and load firmware */
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
+    if (!filename) {
+        error_report("Could not find firmware '%s'", fwname);
+        exit(1);
+    }
+    rom = g_new(MemoryRegion, 1);
+    memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
+    memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
+    sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
+    if (sz <= 0 || sz > PROM_SIZE) {
+        error_report("Could not load firmware '%s'", filename);
+        exit(1);
+    }
+    g_free(filename);
+
+    /* Articia S */
+    dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
+
+    i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
+    if (machine->ram_size > 512 * MiB) {
+        spd_data = spd_data_generate(SDR, machine->ram_size / 2);
+    } else {
+        spd_data = spd_data_generate(SDR, machine->ram_size);
+    }
+    fix_spd_data(spd_data);
+    smbus_eeprom_init_one(i2c_bus, 0x51, spd_data);
+    if (machine->ram_size > 512 * MiB) {
+        smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
+    }
+
+    pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
+    mr = g_new(MemoryRegion, 1);
+    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
+                             0, 0x1000000);
+    memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
+    mr = g_new(MemoryRegion, 1);
+    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
+                             0x80000000, 0x7d000000);
+    memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
+    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
+
+    /* VIA VT82c686B South Bridge (multifunction PCI device) */
+    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
+                                                 TYPE_VT82C686B_ISA));
+    object_property_add_alias(OBJECT(machine), "rtc-time",
+                              object_resolve_path_component(via, "rtc"),
+                              "date");
+    qdev_connect_gpio_out(DEVICE(via), 0,
+                          qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
+                                                             "pirq", i));
+    }
+    pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via, "ide")));
+    pci_vga_init(pci_bus);
+}
+
+static void amigaone_machine_init(MachineClass *mc)
+{
+    mc->desc = "Eyetech AmigaOne/Mai Logic Teron";
+    mc->init = amigaone_init;
+    mc->block_default_type = IF_IDE;
+    mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("7457_v1.2");
+    mc->default_display = "std";
+    mc->default_ram_id = "ram";
+    mc->default_ram_size = 128 * MiB;
+}
+
+DEFINE_MACHINE("amigaone", amigaone_machine_init)
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 7c2c52434a..0f76f4cce4 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -81,6 +81,8 @@ ppc_ss.add(when: 'CONFIG_E500', if_true: files(
 ))
 # PowerPC 440 Xilinx ML507 reference board.
 ppc_ss.add(when: 'CONFIG_VIRTEX', if_true: files('virtex_ml507.c'))
+# a1xe
+ppc_ss.add(when: 'CONFIG_AMIGAONE', if_true: files('amigaone.c'))
 # Pegasos2
 ppc_ss.add(when: 'CONFIG_PEGASOS2', if_true: files('pegasos2.c'))
 
-- 
2.30.9