qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mark Cave-Ayland
Subject: Re: [PATCH v2 1/3] via-ide: Fix legacy mode emulation
Date: Sun, 15 Oct 2023 14:43:52 +0100
User-agent: Mozilla Thunderbird

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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]