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

[Mark Cave-Ayland]

On 05/10/2023 23:13, 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.

This is certainly something I've noticed when testing previous versions of the VIA 
patches. Perhaps it's worth a separate thread to the PCI devs?

> Additionally the values
> written to BARs were also wrong.

I don't believe this is correct: according to the datasheet the values on reset are 
the ones given in the current reset code, so even if the reset function is overridden 
at a later data during PCI bus reset, I would leave these for now since it is a 
different issue.

> Move setting the BARs to a callback on writing the PCI config regsiter
> that sets the compatibility mode (which firmwares needing this mode
> seem to do) and fix their values to program it to use legacy port
> numbers. As noted in a comment, we only do this when the BARs were
> unset before, because logs from real machine show this is how real
> chip works, even if it contradicts the data sheet which is not very
> clear about this.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ide/via.c | 35 ++++++++++++++++++++++++++++++-----
>   1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..8186190207 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,35 @@ 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);
> +    /*
> +     * Only set BARs if they are unset. Logs from real hardware show that
> +     * writing class_prog to enable compatibility mode after BARs were set
> +     * (possibly by firmware) it will use ports set by BARs not ISA ports
> +     * (e.g. pegasos2 Linux does this and calls it non-100% native mode).

Can you remind me again where the references are to non-100% native mode? The only 
thing I can find in Linux is 
https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360 
but that simply forces a switch to legacy mode, with no mention of "non-100% native 
mode".

> +     * But if 0x8a is written after reset without setting BARs then we want
> +     * legacy ports (this is done by AmigaOne firmware for example).
> +     */

What should happen here is that writing 0x8a should *always* switch to legacy mode, 
so the BARs are unused...

> +    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);
> +    }
> +}

... but what you're doing here is effectively forcing the PCI BARs to the legacy 
addresses. The reason we know this is because that is why you have the off-by-2 error 
in BARs 1 and 3.

I could live with this approach for now if you're willing to adjust the comments 
accordingly clarifying that forcing the PCI BARs to the legacy addresses is a hack to 
be replaced in future with proper legacy ioports. At some point I will revive my PoC 
series on top of Bernhard's last series that implements this properly.

>   static void via_ide_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
> @@ -221,6 +245,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.