qemu-block
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching fo

From: Mark Cave-Ayland
Subject: Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers
Date: Tue, 21 Nov 2023 10:16:50 +0000
User-agent: Mozilla Thunderbird 
On 21/11/2023 09:12, Kevin Wolf wrote:


Am 20.11.2023 um 16:02 hat BALATON Zoltan geschrieben:

On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:

On 20/11/2023 13:42, Kevin Wolf wrote:

Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:

On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:

On 19/11/2023 21:43, BALATON Zoltan wrote:

On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:

This series adds a simple implementation of legacy/native mode
switching for PCI
IDE controllers and updates the via-ide device to use it.

The approach I take here is to add a new pci_ide_update_mode()
function which handles
management of the PCI BARs and legacy IDE ioports for each mode
to avoid exposing
details of the internal logic to individual PCI IDE controllers.

As noted in [1] this is extracted from a local WIP branch I have
which contains
further work in this area. However for the moment
I've kept it simple (and
restricted it to the via-ide device) which is good
enough for Zoltan's PPC
images whilst paving the way for future improvements after 8.2.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html

v3:
- Rebase onto master
- Move ide_portio_list[] and ide_portio_list2[] to IDE core to
prevent duplication in
   hw/ide/pci.c
- Don't zero BARs when switching from native mode to legacy
mode, instead always force
   them to read zero as suggested in the PCI IDE specification
(note: this also appears
   to fix the fuloong2e machine booting from IDE)


Not sure you're getting this, see also:
https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
but this seems to break latest version of the AmigaOS driver for
some reason. I assume this is the BAR zeroing that causes this as it
works with v2 series and nothing else changed in v3 that could cause
this. Testing was done by Rene Engel, cc'd so maybe he can add more
info. It seems to work with my patch that sets BARs to legacy values
and with v2 that sets them to 0 but not with v3 which should also
read 0 but maybe something is off here.


I've been AFK for a few days, so just starting to catch up on various
bits and pieces.


OK just wasn't sure if you saw my emails at all as it happened before that
some spam filters disliked my mail server and put messages in the spam
folder.


The only difference I can think of regarding the BAR zeroing is that the
BMDMA BAR is zeroed here. Does the following diff fix things?


This helps, with this the latest driver does not crash but still
reads BAR4
as 0 instead of 0xcc00 so UDMA won't work but at least it boots.


And disabling only the first four BARs is actually what the spec says,
too. So I'll make this change to the queued patches.


That was definitely something that I thought about: what should happen
to BARs outside of the ones mentioned in the PCI IDE controller
specification? It seems reasonable to me just to consider BARS 0-3 for
zeroing here.


If I understand correctly, UDMA didn't work before this series either,
so it's a separate goal and doing it in its own patch is best anyway.

As we don't seem to have a good place to set a default, maybe just
overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in
compatibility mode is enough?


It's difficult to know whether switching to legacy mode on the via-ide
device resets BAR4 to its default value, or whether it is simply left
unaltered. For 8.2 I don't mind too much as long as the logic is
separate from the BAR zeroing logic (which will eventually be lifted up
into hw/ide/pci.c).


My original patch checked for BAR being unset and only reset to defailt in
that case so it won't clobber a value set by something (like pegasos2
firmware) but will set default for amigaone which does not program the BAR
just uses the default legacy mode (which is the default on real chip but we
have to make that happen on QEMU after reset). So setting it to default if
it's unset when switching to legacy seems like a safe bet and testing with
my patch did not find problem with that.


How about setting the default if it's unset on the first read after
reset instead of only on switching modes? I'd like to avoid that the
guest could observe surprising state changes, even if the OSes we're
currently looking at don't do that. It just seems more robust than
introducing random magic at arbitrary points.
Note that Zoltan's image will work with just the change suggested in https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg04331.html, but without BMDMA so any discussion re: default BAR addresses can be handled separately from the currently queued patches.
On real AmigaOne hardware BMDMA isn't well tested because it needs a hardware modification and configuration changes for U-Boot [1], and given that BAR4 isn't programmed by either the OS or the firmware, I'd be inclined to say that the fact it even works at all is because of a happy coincidence of bugs.
In the meantime the department of hacks has been looking at ways of trying to set BAR addresses during reset, and humbly submits the following for consideration: 


diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..bec4a3d09b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -30,6 +30,7 @@
 #include "qemu/module.h"
 #include "qemu/range.h"
 #include "sysemu/dma.h"
+#include "sysemu/reset.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/ide/pci.h"
 #include "hw/irq.h"
@@ -118,6 +119,23 @@ static void via_ide_set_irq(void *opaque, int n, int level)
     qemu_set_irq(s->isa_irq[n], level);
 }

 #include "hw/irq.h"
@@ -118,6 +119,23 @@ static void via_ide_set_irq(void *opaque, int n, int level)
     qemu_set_irq(s->isa_irq[n], level);
 }

+static void via_ide_bar_reset(void *opaque)
+{
+    PCIIDEState *d = PCI_IDE(opaque);
+    PCIDevice *pd = PCI_DEVICE(d);
+    uint8_t *pci_conf = pd->config;
+
+    /*
+     * Some OSs e.g. AmigaOS rely on the default BMDMA BAR value being present
+     * to initialise correctly, even in legacy mode(!)
+     */
+    pci_set_long(pci_conf + PCI_BASE_ADDRESS_4,
+                 0xcc00 | PCI_BASE_ADDRESS_SPACE_IO);
+
+    /* Unregister reset function */
+    qemu_unregister_reset(via_ide_bar_reset, opaque);
+}
+
 static void via_ide_reset(DeviceState *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -156,6 +174,9 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + 0x68, 0x00000200);
     /* PCI PM Block */
     pci_set_long(pci_conf + 0xc0, 0x00020001);
+
+    /* Register separate function to set BAR values after PCI bus reset */
+    qemu_register_reset(via_ide_bar_reset, d);
 }

 static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
This works because the reset handlers are stored in an ordered list, so via_ide_bar_reset() is guaranteed to be called after the PCI bus reset occurs. 


ATB,

Mark.

[1] https://intuitionbase.com/hints.php
reply via email to
[Prev in Thread] Current Thread [Next in Thread]