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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 18/10/2023 21:42, 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). This does not fully emulate what the data sheet says (which is
> not very clear on this) but it implements enough to allow both modes
> as used by firmwares and guests on machines we emulate.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ide/via.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..5a1fe38fb8 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,6 +28,7 @@
>   #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"
> @@ -132,11 +133,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 +155,42 @@ 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 are documented to
> +     * select between legacy and native mode for the two IDE channels. when the
> +     * guest selects legacy mode we reset the PCI BARs to legacy ports which is
> +     * their default value. We don't care about setting each channel separately
> +     * as no guest is known to do or need that. But 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 will still use ports set by BARs
> +     * not ISA ports (Linux on pegasos2 does this after firmware set native
> +     * mode and programmed BARs). If 0x8a is written after reset without
> +     * setting BARs then we want legacy ports (this is done by the AmigaOne
> +     * firmware). We can't set these in via_ide_reset() because PCI code clears
> +     * BARs after calling device reset method.
> +     */
> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG) &&
> +        pd->config[PCI_CLASS_PROG] == 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, 0x3f4
> +                     | 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, 0x374
> +                     | 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 +253,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;

As promised I've posted an updated version of the proposed patch which better matches 
the hardware (and is also less invasive to hw/ide/via.c) at 
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg06416.html.


ATB,

Mark.