[PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

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)
- Add comments in pci_ide_update_mode() suggested by Kevin
- Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
  around zero BARs feels different enough here

v2:
- Rebase onto master
- Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1
- Add patch 2 to remove the default BAR addresses to avoid confusion
- Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set
  by pci_ide_update_mode() in patch 3, and reword the commit message accordingly
- Add Tested-By tags from Zoltan and Bernhard


Mark Cave-Ayland (4):
  ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
    to IDE core
  ide/pci: introduce pci_ide_update_mode() function
  ide/via: don't attempt to set default BAR addresses
  hw/ide/via: implement legacy/native mode switching

 hw/ide/core.c             | 12 ++++++
 hw/ide/ioport.c           | 12 ------
 hw/ide/pci.c              | 84 +++++++++++++++++++++++++++++++++++++++
 hw/ide/via.c              | 44 ++++++++++++++++----
 include/hw/ide/internal.h |  3 ++
 include/hw/ide/pci.h      |  1 +
 6 files changed, 137 insertions(+), 19 deletions(-)

-- 
2.39.2

[PATCH v3 1/4] ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions to IDE core

[Mark Cave-Ayland]

These definitions are present in ioport.c which is currently only available when
CONFIG_IDE_ISA is enabled. Move them to the IDE core so that they can be made
available to PCI IDE controllers that support switching to legacy mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/core.c             | 12 ++++++++++++
 hw/ide/ioport.c           | 12 ------------
 include/hw/ide/internal.h |  3 +++
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 63ba665f3d..8a0579bff4 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -81,6 +81,18 @@ static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
 
 static void ide_dummy_transfer_stop(IDEState *s);
 
+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(),
+};
+
+const MemoryRegionPortio ide_portio2_list[] = {
+    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
+    PORTIO_END_OF_LIST(),
+};
+
 static void padstr(char *str, const char *src, int len)
 {
     int i, v;
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e2ecc6230c..0b283ac783 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -28,18 +28,6 @@
 #include "hw/ide/internal.h"
 #include "trace.h"
 
-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(),
-};
-
 int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
     int ret;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 2bfa7533d6..3bdcc75597 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -354,6 +354,9 @@ enum ide_dma_cmd {
 
 extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
 
+extern const MemoryRegionPortio ide_portio_list[];
+extern const MemoryRegionPortio ide_portio2_list[];
+
 #define ide_cmd_is_read(s) \
         ((s)->dma_cmd == IDE_DMA_READ)
 
-- 
2.39.2

Re: [PATCH v3 1/4] ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions to IDE core

[Philippe Mathieu-Daudé]

On 16/11/23 11:33, Mark Cave-Ayland wrote:
> These definitions are present in ioport.c which is currently only available when
> CONFIG_IDE_ISA is enabled. Move them to the IDE core so that they can be made
> available to PCI IDE controllers that support switching to legacy mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ide/core.c             | 12 ++++++++++++
>   hw/ide/ioport.c           | 12 ------------
>   include/hw/ide/internal.h |  3 +++
>   3 files changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 2/4] ide/pci: introduce pci_ide_update_mode() function

[Mark Cave-Ayland]

This function reads the value of the PCI_CLASS_PROG register for PCI IDE
controllers and configures the PCI BARs and/or IDE ioports accordingly.

In the case where we switch to legacy mode, the PCI BARs are set to return zero
(as suggested in the "PCI IDE Controller" specification), the legacy IDE ioports
are enabled, and the PCI interrupt pin cleared to indicate legacy IRQ routing.

Conversely when we switch to native mode, the legacy IDE ioports are disabled
and the PCI interrupt pin set to indicate native IRQ routing. The contents of
the PCI BARs are unspecified, but this is not an issue since if a PCI IDE
controller has been switched to native mode then its BARs will need to be
programmed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/pci.c         | 84 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ide/pci.h |  1 +
 2 files changed, 85 insertions(+)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a25b352537..810c6b6d98 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -104,6 +104,90 @@ const MemoryRegionOps pci_ide_data_le_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+void pci_ide_update_mode(PCIIDEState *s)
+{
+    PCIDevice *d = PCI_DEVICE(s);
+    uint8_t mode = d->config[PCI_CLASS_PROG];
+
+    /*
+     * This function only configures the BARs/ioports for now: PCI IDE
+     * controllers must manage their own IRQ routing
+     */
+
+    switch (mode & 0xf) {
+    case 0xa:
+        /* Both channels legacy mode */
+
+        /*
+         * TODO: according to the PCI IDE specification the BARs should
+         * be completely disabled, however Linux for the pegasos2
+         * machine stil accesses the BAR addresses after switching to legacy
+         * mode. Hence we leave them active for now.
+         */
+
+        /* Clear interrupt pin */
+        pci_config_set_interrupt_pin(d->config, 0);
+
+        /* Add legacy IDE ports */
+        if (!s->bus[0].portio_list.owner) {
+            portio_list_init(&s->bus[0].portio_list, OBJECT(d),
+                             ide_portio_list, &s->bus[0], "ide");
+            portio_list_add(&s->bus[0].portio_list,
+                            pci_address_space_io(d), 0x1f0);
+        }
+
+        if (!s->bus[0].portio2_list.owner) {
+            portio_list_init(&s->bus[0].portio2_list, OBJECT(d),
+                             ide_portio2_list, &s->bus[0], "ide");
+            portio_list_add(&s->bus[0].portio2_list,
+                            pci_address_space_io(d), 0x3f6);
+        }
+
+        if (!s->bus[1].portio_list.owner) {
+            portio_list_init(&s->bus[1].portio_list, OBJECT(d),
+                                ide_portio_list, &s->bus[1], "ide");
+            portio_list_add(&s->bus[1].portio_list,
+                            pci_address_space_io(d), 0x170);
+        }
+
+        if (!s->bus[1].portio2_list.owner) {
+            portio_list_init(&s->bus[1].portio2_list, OBJECT(d),
+                             ide_portio2_list, &s->bus[1], "ide");
+            portio_list_add(&s->bus[1].portio2_list,
+                            pci_address_space_io(d), 0x376);
+        }
+        break;
+
+    case 0xf:
+        /* Both channels native mode */
+
+        /* Set interrupt pin */
+        pci_config_set_interrupt_pin(d->config, 1);
+
+        /* Remove legacy IDE ports */
+        if (s->bus[0].portio_list.owner) {
+            portio_list_del(&s->bus[0].portio_list);
+            portio_list_destroy(&s->bus[0].portio_list);
+        }
+
+        if (s->bus[0].portio2_list.owner) {
+            portio_list_del(&s->bus[0].portio2_list);
+            portio_list_destroy(&s->bus[0].portio2_list);
+        }
+
+        if (s->bus[1].portio_list.owner) {
+            portio_list_del(&s->bus[1].portio_list);
+            portio_list_destroy(&s->bus[1].portio_list);
+        }
+
+        if (s->bus[1].portio2_list.owner) {
+            portio_list_del(&s->bus[1].portio2_list);
+            portio_list_destroy(&s->bus[1].portio2_list);
+        }
+        break;
+    }
+}
+
 static IDEState *bmdma_active_if(BMDMAState *bmdma)
 {
     assert(bmdma->bus->retry_unit != (uint8_t)-1);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 1ff469de87..a814a0a7c3 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -61,6 +61,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 void bmdma_status_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
 void pci_ide_create_devs(PCIDevice *dev);
+void pci_ide_update_mode(PCIIDEState *s);
 
 extern const VMStateDescription vmstate_ide_pci;
 extern const MemoryRegionOps pci_ide_cmd_le_ops;
-- 
2.39.2

[PATCH v3 3/4] ide/via: don't attempt to set default BAR addresses

[Mark Cave-Ayland]

The via-ide device currently attempts to set the default BAR addresses to the
values shown in the datasheet, but this doesn't work for 2 reasons: firstly
BARS 1-4 do not set the bottom 2 bits to PCI_BASE_ADDRESS_SPACE_IO, and
secondly the initial PCI bus reset clears the values of all PCI device BARs
after the device itself has been reset.

Remove the setting of the default BAR addresses from via_ide_reset() to ensure
there is no doubt that these values are never exposed to the guest.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/via.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..87b134083a 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*/
-- 
2.39.2

Re: [PATCH v3 3/4] ide/via: don't attempt to set default BAR addresses

[Philippe Mathieu-Daudé]

On 16/11/23 11:33, Mark Cave-Ayland wrote:
> The via-ide device currently attempts to set the default BAR addresses to the
> values shown in the datasheet, but this doesn't work for 2 reasons: firstly
> BARS 1-4 do not set the bottom 2 bits to PCI_BASE_ADDRESS_SPACE_IO, and
> secondly the initial PCI bus reset clears the values of all PCI device BARs
> after the device itself has been reset.
> 
> Remove the setting of the default BAR addresses from via_ide_reset() to ensure
> there is no doubt that these values are never exposed to the guest.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ide/via.c | 5 -----
>   1 file changed, 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 4/4] hw/ide/via: implement legacy/native mode switching

[Mark Cave-Ayland]

Allow the VIA IDE controller to switch between both legacy and native modes by
calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG
is updated.

This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to
via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI
bus reset since this is now managed by pci_ide_update_mode(). This ensures that
the device configuration is always consistent with respect to the currently
selected mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/via.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 87b134083a..47223b1268 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"
@@ -128,11 +129,14 @@ static void via_ide_reset(DeviceState *dev)
         ide_bus_reset(&d->bus[i]);
     }
 
+    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
+    pci_ide_update_mode(d);
+
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
 
-    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
+    pci_set_byte(pci_conf + PCI_INTERRUPT_LINE, 0xe);
 
     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
     pci_set_long(pci_conf + 0x40, 0x0a090600);
@@ -154,6 +158,36 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
+static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
+{
+    uint32_t val = pci_default_read_config(pd, addr, len);
+    uint8_t mode = pd->config[PCI_CLASS_PROG];
+
+    if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
+                                              PCI_BASE_ADDRESS_0, 24)) {
+        /* BARs always read back zero in legacy mode */
+        for (int i = addr; i < addr + len; i++) {
+            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+                val &= ~(0xffULL << ((i - addr) << 3));
+            }
+        }
+    }
+
+    return val;
+}
+
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+                              uint32_t val, int len)
+{
+    PCIIDEState *d = PCI_IDE(pd);
+
+    pci_default_write_config(pd, addr, val, len);
+
+    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
+        pci_ide_update_mode(d);
+    }
+}
+
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -161,7 +195,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     uint8_t *pci_conf = dev->config;
     int i;
 
-    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
     dev->wmask[PCI_INTERRUPT_LINE] = 0;
     dev->wmask[PCI_CLASS_PROG] = 5;
@@ -216,6 +249,8 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     /* Reason: only works as function of VIA southbridge */
     dc->user_creatable = false;
 
+    k->config_read = via_ide_cfg_read;
+    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.39.2

Re: [PATCH v3 4/4] hw/ide/via: implement legacy/native mode switching

[BALATON Zoltan]

On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
> Allow the VIA IDE controller to switch between both legacy and native modes by
> calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG
> is updated.
>
> This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to
> via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI
> bus reset since this is now managed by pci_ide_update_mode(). This ensures that
> the device configuration is always consistent with respect to the currently
> selected mode.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/ide/via.c | 39 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 87b134083a..47223b1268 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"
> @@ -128,11 +129,14 @@ static void via_ide_reset(DeviceState *dev)
>         ide_bus_reset(&d->bus[i]);
>     }
>
> +    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
> +    pci_ide_update_mode(d);
> +
>     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                  PCI_STATUS_DEVSEL_MEDIUM);
>
> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
> +    pci_set_byte(pci_conf + PCI_INTERRUPT_LINE, 0xe);
>
>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>     pci_set_long(pci_conf + 0x40, 0x0a090600);
> @@ -154,6 +158,36 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
> +{
> +    uint32_t val = pci_default_read_config(pd, addr, len);
> +    uint8_t mode = pd->config[PCI_CLASS_PROG];
> +
> +    if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
> +                                              PCI_BASE_ADDRESS_0, 24)) {

You could break the line after && which is more readable. I don't get what 
the below calculation does. Does it really have to be so complicated? It's 
not likely BARs will be accesssed unaligned or did you find something that 
does that? I think it would be insane for a guest to do a 4 bytes read at 
PCI_BASE_ADDRESS_0 - 2 and expect to get something useful so don't think 
we need to handle that case. So maybe just care about
addr >= PCI_BASE_ADDRESS_0 %% addr + len < PCI_BASE_ADDRESS_0 + 24 and 
return 0 without fancy calculation that's likely never needed.

> +        /* BARs always read back zero in legacy mode */
> +        for (int i = addr; i < addr + len; i++) {
> +            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
> +                val &= ~(0xffULL << ((i - addr) << 3));
> +            }
> +        }
> +    }
> +
> +    return val;
> +}
> +
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    PCIIDEState *d = PCI_IDE(pd);
> +
> +    pci_default_write_config(pd, addr, val, len);
> +
> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
> +        pci_ide_update_mode(d);
> +    }

Have you missed this reply to your previous version or just ignored it?

https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html

You'd need to set BAR4 to the default value when switching to legacy mode 
(you could check if it's already set and only reset when unset if you're 
concerned with that but I think it would not matter in practice) otherwise 
UDMA does not work in AmigaOS.

Other than that I'd still like to keep potio_list arrays static somehow 
but that may need changes to isa_register_portio_list() so this could be 
addressed later but you could still consider moving the portio stuff from 
patch 2 into a function operating on IDEBus which can be in core.c next to 
the arrays as this could be reused by ide-isa later. Or is that just the 
same as ide_init_ioport without the ISADevice *dev argument and 
isa_register_portio_list inlined. The only thing isa_register_portio_list 
has is an additional call to isa_init_ioport but maybe that could be done 
separately then ide_init_ioport is not dependent on ISA any more so can be 
moved back to core.c?

Regards,
BALATON Zoltan

> +}
> +
> static void via_ide_realize(PCIDevice *dev, Error **errp)
> {
>     PCIIDEState *d = PCI_IDE(dev);
> @@ -161,7 +195,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>     uint8_t *pci_conf = dev->config;
>     int i;
>
> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
>     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
>     dev->wmask[PCI_INTERRUPT_LINE] = 0;
>     dev->wmask[PCI_CLASS_PROG] = 5;
> @@ -216,6 +249,8 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>     /* Reason: only works as function of VIA southbridge */
>     dc->user_creatable = false;
>
> +    k->config_read = via_ide_cfg_read;
> +    k->config_write = via_ide_cfg_write;
>     k->realize = via_ide_realize;
>     k->exit = via_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben:
> 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)
> - Add comments in pci_ide_update_mode() suggested by Kevin
> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
>   around zero BARs feels different enough here

Thanks, applied to the block branch.

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

On Thu, 16 Nov 2023, Kevin Wolf wrote:
> Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben:
>> 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)
>> - Add comments in pci_ide_update_mode() suggested by Kevin
>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
>>   around zero BARs feels different enough here
>
> Thanks, applied to the block branch.

I feel a bit left out of this conversation... Did Google or some other 
spam filter decide again to filter my messages so you did not see them at 
all? Could you confitm that you've got my previous two replies in this 
thread so I know I'm not sending comments to /dev/null please?

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

On Thu, 16 Nov 2023, BALATON Zoltan wrote:
> On Thu, 16 Nov 2023, Kevin Wolf wrote:
>> Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben:
>>> 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)
>>> - Add comments in pci_ide_update_mode() suggested by Kevin
>>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, 
>>> the behaviour
>>>   around zero BARs feels different enough here
>> 
>> Thanks, applied to the block branch.
>
> I feel a bit left out of this conversation... Did Google or some other spam 
> filter decide again to filter my messages so you did not see them at all? 
> Could you confitm that you've got my previous two replies in this thread so I 
> know I'm not sending comments to /dev/null please?

Looks like there's some issue with these mails. They appear in the list 
archive but maybe not in people's mailboxes? Did any of you got this 
message and previous ones I've sent?
https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html 
https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

Am 17.11.2023 um 15:23 hat BALATON Zoltan geschrieben:
> On Thu, 16 Nov 2023, BALATON Zoltan wrote:
> > On Thu, 16 Nov 2023, Kevin Wolf wrote:
> > > Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben:
> > > > 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)
> > > > - Add comments in pci_ide_update_mode() suggested by Kevin
> > > > - Drop the existing R-B and T-B tags: whilst this passes my
> > > > local tests, the behaviour
> > > >   around zero BARs feels different enough here
> > > 
> > > Thanks, applied to the block branch.
> > 
> > I feel a bit left out of this conversation... Did Google or some other
> > spam filter decide again to filter my messages so you did not see them
> > at all? Could you confitm that you've got my previous two replies in
> > this thread so I know I'm not sending comments to /dev/null please?
> 
> Looks like there's some issue with these mails. They appear in the list
> archive but maybe not in people's mailboxes? Did any of you got this message
> and previous ones I've sent?
> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html
> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html

I received it, but at least the second one only after sending the mail
you replied to here.

For your specific concern, it seems to me that resetting BAR 4 on a
switch between compatibility mode and native mode is another via quirk
and should therefore be implemented in via.c rather than pci.c?

If I get a new version of the patches in the next few hours, I can
update the queued patches before sending a pull request, but otherwise
this can still be done in a separate patch.

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

On Mon, 20 Nov 2023, Kevin Wolf wrote:
> Am 17.11.2023 um 15:23 hat BALATON Zoltan geschrieben:
>> On Thu, 16 Nov 2023, BALATON Zoltan wrote:
>>> On Thu, 16 Nov 2023, Kevin Wolf wrote:
>>>> Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben:
>>>>> 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)
>>>>> - Add comments in pci_ide_update_mode() suggested by Kevin
>>>>> - Drop the existing R-B and T-B tags: whilst this passes my
>>>>> local tests, the behaviour
>>>>>   around zero BARs feels different enough here
>>>>
>>>> Thanks, applied to the block branch.
>>>
>>> I feel a bit left out of this conversation... Did Google or some other
>>> spam filter decide again to filter my messages so you did not see them
>>> at all? Could you confitm that you've got my previous two replies in
>>> this thread so I know I'm not sending comments to /dev/null please?
>>
>> Looks like there's some issue with these mails. They appear in the list
>> archive but maybe not in people's mailboxes? Did any of you got this message
>> and previous ones I've sent?
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html
>
> I received it, but at least the second one only after sending the mail
> you replied to here.
>
> For your specific concern, it seems to me that resetting BAR 4 on a
> switch between compatibility mode and native mode is another via quirk
> and should therefore be implemented in via.c rather than pci.c?

I think in real chip the default value is there so no need to reset it 
when switching mode. Also the defeult mode is legacy so writing 0x8a first 
does nothing on real chip but we can't do that in QEMU becaise PCI code 
resets BARs after calling device reset method so we can't set reset 
degaults there but have to establish them on mode switch instead when the 
BARs are yet unset. Setting the BMDMA BAR4 in via.c is fine as well and 
could be done as a follow up so that's OK.

> If I get a new version of the patches in the next few hours, I can
> update the queued patches before sending a pull request, but otherwise
> this can still be done in a separate patch.

This v3 series does not work with latest AmigaOS driver as I wrote in my 
last reply. So maybe wait for a v4 or merge either v2 from Mark or my 
single patch fix for now. Those are better than v3 that's queued now.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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.

Regards,
BALATON Zoltan

> - Add comments in pci_ide_update_mode() suggested by Kevin
> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
>  around zero BARs feels different enough here
>
> v2:
> - Rebase onto master
> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1
> - Add patch 2 to remove the default BAR addresses to avoid confusion
> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set
>  by pci_ide_update_mode() in patch 3, and reword the commit message accordingly
> - Add Tested-By tags from Zoltan and Bernhard
>
>
> Mark Cave-Ayland (4):
>  ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>    to IDE core
>  ide/pci: introduce pci_ide_update_mode() function
>  ide/via: don't attempt to set default BAR addresses
>  hw/ide/via: implement legacy/native mode switching
>
> hw/ide/core.c             | 12 ++++++
> hw/ide/ioport.c           | 12 ------
> hw/ide/pci.c              | 84 +++++++++++++++++++++++++++++++++++++++
> hw/ide/via.c              | 44 ++++++++++++++++----
> include/hw/ide/internal.h |  3 ++
> include/hw/ide/pci.h      |  1 +
> 6 files changed, 137 insertions(+), 19 deletions(-)
>
>

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

On Sun, 19 Nov 2023, 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.

The change log of the AmigaOS driver version which only works with v2 of 
this series says:

"Replaced ReadConfigLong(PCI_BASE_ADDRESS_x) calls with their
PCI interface variant GetResourceRange(x)->BaseAddress to get
the correct base address instead of the *bus* base address."

but not sure what that means. Apparently it reads BAR values differently 
and crashes before finding the IDE ports with v3 but not with v2. As it 
crashes it does not even print debug logs to check what it read.

(Additionally would also need the default value for BMDMA BAR4 as I wrote 
before but that's disabled by default so only an issue if one tries to 
enable it. Would be nics to have though as UDMA is faster.)

Regards,
BALATON Zoltan
>
>> - Add comments in pci_ide_update_mode() suggested by Kevin
>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, 
>> the behaviour
>>  around zero BARs feels different enough here
>> 
>> v2:
>> - Rebase onto master
>> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in 
>> patch 1
>> - Add patch 2 to remove the default BAR addresses to avoid confusion
>> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already 
>> set
>>  by pci_ide_update_mode() in patch 3, and reword the commit message 
>> accordingly
>> - Add Tested-By tags from Zoltan and Bernhard
>> 
>> 
>> Mark Cave-Ayland (4):
>>  ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>>    to IDE core
>>  ide/pci: introduce pci_ide_update_mode() function
>>  ide/via: don't attempt to set default BAR addresses
>>  hw/ide/via: implement legacy/native mode switching
>> 
>> hw/ide/core.c             | 12 ++++++
>> hw/ide/ioport.c           | 12 ------
>> hw/ide/pci.c              | 84 +++++++++++++++++++++++++++++++++++++++
>> hw/ide/via.c              | 44 ++++++++++++++++----
>> include/hw/ide/internal.h |  3 ++
>> include/hw/ide/pci.h      |  1 +
>> 6 files changed, 137 insertions(+), 19 deletions(-)
>> 
>> 
>
>

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

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.

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?

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..2d3124ebd7 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -164,10 +164,10 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, 
int len)
      uint8_t mode = pd->config[PCI_CLASS_PROG];

      if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
-                                              PCI_BASE_ADDRESS_0, 24)) {
+                                              PCI_BASE_ADDRESS_0, 16)) {
          /* BARs always read back zero in legacy mode */
          for (int i = addr; i < addr + len; i++) {
-            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
                  val &= ~(0xffULL << ((i - addr) << 3));
              }
          }

>> - Add comments in pci_ide_update_mode() suggested by Kevin
>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
>>  around zero BARs feels different enough here
>>
>> v2:
>> - Rebase onto master
>> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1
>> - Add patch 2 to remove the default BAR addresses to avoid confusion
>> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set
>>  by pci_ide_update_mode() in patch 3, and reword the commit message accordingly
>> - Add Tested-By tags from Zoltan and Bernhard
>>
>>
>> Mark Cave-Ayland (4):
>>  ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>>    to IDE core
>>  ide/pci: introduce pci_ide_update_mode() function
>>  ide/via: don't attempt to set default BAR addresses
>>  hw/ide/via: implement legacy/native mode switching
>>
>> hw/ide/core.c             | 12 ++++++
>> hw/ide/ioport.c           | 12 ------
>> hw/ide/pci.c              | 84 +++++++++++++++++++++++++++++++++++++++
>> hw/ide/via.c              | 44 ++++++++++++++++----
>> include/hw/ide/internal.h |  3 ++
>> include/hw/ide/pci.h      |  1 +
>> 6 files changed, 137 insertions(+), 19 deletions(-)


ATB,

Mark.

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

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.

Regards,
BALATON Zoltan

> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 47223b1268..2d3124ebd7 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -164,10 +164,10 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, 
> uint32_t addr, int len)
>     uint8_t mode = pd->config[PCI_CLASS_PROG];
>
>     if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
> -                                              PCI_BASE_ADDRESS_0, 24)) {
> +                                              PCI_BASE_ADDRESS_0, 16)) {
>         /* BARs always read back zero in legacy mode */
>         for (int i = addr; i < addr + len; i++) {
> -            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
> +            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
>                 val &= ~(0xffULL << ((i - addr) << 3));
>             }
>         }
>
>>> - Add comments in pci_ide_update_mode() suggested by Kevin
>>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, 
>>> the behaviour
>>>  around zero BARs feels different enough here
>>> 
>>> v2:
>>> - Rebase onto master
>>> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in 
>>> patch 1
>>> - Add patch 2 to remove the default BAR addresses to avoid confusion
>>> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already 
>>> set
>>>  by pci_ide_update_mode() in patch 3, and reword the commit message 
>>> accordingly
>>> - Add Tested-By tags from Zoltan and Bernhard
>>> 
>>> 
>>> Mark Cave-Ayland (4):
>>>  ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>>>    to IDE core
>>>  ide/pci: introduce pci_ide_update_mode() function
>>>  ide/via: don't attempt to set default BAR addresses
>>>  hw/ide/via: implement legacy/native mode switching
>>> 
>>> hw/ide/core.c             | 12 ++++++
>>> hw/ide/ioport.c           | 12 ------
>>> hw/ide/pci.c              | 84 +++++++++++++++++++++++++++++++++++++++
>>> hw/ide/via.c              | 44 ++++++++++++++++----
>>> include/hw/ide/internal.h |  3 ++
>>> include/hw/ide/pci.h      |  1 +
>>> 6 files changed, 137 insertions(+), 19 deletions(-)
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

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.

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?

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

On Mon, 20 Nov 2023, 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.
>
> 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.

UDMA works with my original series, did not work with earlier versions of 
this alternative from Mark but could be fixed up on top unless Mark can 
send a v4 now.

> 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?

I could give that a try and see if that helps but all this 
via_ide_cfg_read() seems like an unnecessary complication to me. Why can't 
we just set the BARs (o for BAR1-3 and default for BAR4) then we don't 
need to override config read?

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

On Mon, 20 Nov 2023, BALATON Zoltan wrote:
> On Mon, 20 Nov 2023, 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.
>> 
>> 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.
>
> UDMA works with my original series, did not work with earlier versions of 
> this alternative from Mark but could be fixed up on top unless Mark can send 
> a v4 now.
>
>> 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?
>
> I could give that a try and see if that helps but all this via_ide_cfg_read() 
> seems like an unnecessary complication to me. Why can't we just set the BARs 
> (o for BAR1-3 and default for BAR4) then we don't need to override config 
> read?

Seems to work with this:

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..214ebe48a1 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -164,14 +164,17 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
      uint8_t mode = pd->config[PCI_CLASS_PROG];

      if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
-                                              PCI_BASE_ADDRESS_0, 24)) {
+                                              PCI_BASE_ADDRESS_0, 16)) {
          /* BARs always read back zero in legacy mode */
          for (int i = addr; i < addr + len; i++) {
-            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+            if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
                  val &= ~(0xffULL << ((i - addr) << 3));
              }
          }
      }
+    if (addr == PCI_BASE_ADDRESS_4) {
+        val = 0xcc01;
+    }

      return val;
  }

But I still think all the above mess could be avoided and just set the BAR 
value to 0 for BAR0-3 and to 0xcc01 for BAR4 instead should be enough and 
would not mess up the device model unnecessarily.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben:
> On Mon, 20 Nov 2023, Kevin Wolf wrote:
> > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
> > > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
> > > > 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.
> > 
> > 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.
> 
> UDMA works with my original series, did not work with earlier versions of
> this alternative from Mark but could be fixed up on top unless Mark can send
> a v4 now.
> 
> > 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?
> 
> I could give that a try and see if that helps but all this
> via_ide_cfg_read() seems like an unnecessary complication to me. Why can't
> we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need
> to override config read?

I would be fine with setting 0xcc00 as the default value for BAR 4, but
as you said yourself, we can't do that in reset because it will be
overwritten by the PCI core code. Where else could we meaningfully do
that? As far as I understand, we don't have any hint that the
native/compatibility mode switch resets it on real hardware, so I'm
hesitant to do it there (and if the guest OS doesn't even switch, it
would never get set).

As for BAR 0-3, didn't we conclude that the via device still accepts I/O
to the configured addresses even though they read as zeros? Having
inconsistent config space and PCIIORegion seems like a bad idea, the
next call to pci_update_mappings() would break it.

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

On Mon, 20 Nov 2023, Kevin Wolf wrote:
> Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben:
>> On Mon, 20 Nov 2023, Kevin Wolf wrote:
>>> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
>>>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
>>>>> 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.
>>>
>>> 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.
>>
>> UDMA works with my original series, did not work with earlier versions of
>> this alternative from Mark but could be fixed up on top unless Mark can send
>> a v4 now.
>>
>>> 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?
>>
>> I could give that a try and see if that helps but all this
>> via_ide_cfg_read() seems like an unnecessary complication to me. Why can't
>> we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need
>> to override config read?
>
> I would be fine with setting 0xcc00 as the default value for BAR 4, but
> as you said yourself, we can't do that in reset because it will be
> overwritten by the PCI core code. Where else could we meaningfully do
> that? As far as I understand, we don't have any hint that the
> native/compatibility mode switch resets it on real hardware, so I'm
> hesitant to do it there (and if the guest OS doesn't even switch, it
> would never get set).

Luckily machines which need legacy mode also seem to set it explicitly on 
startup so we can set the defaults there. The check to see if something 
changed the BARs before is enough to avoid breaking it when legacy mode is 
set after native mode which does not seem to reset BARs according to how 
pegasos2 Linux behaves that sets legacy mode after firmware set native and 
proframmed BARs but the keep using BAR addresses. The AmigaOne I think 
just uses the default values with setting legacy mode doing nothing as 
that's the default but we can detect this as setting legacy mode with BARs 
unset so that's a good place to set default values which is what my patch 
did and I added a lot of comments trying to explain this.

> As for BAR 0-3, didn't we conclude that the via device still accepts I/O
> to the configured addresses even though they read as zeros? Having
> inconsistent config space and PCIIORegion seems like a bad idea, the
> next call to pci_update_mappings() would break it.

I don't quite get this but then we could also just leave BARs alone and it 
would still work. It probably does not matter what it reads back when the 
device is in legacy mode. What would call pci_update_mappings() if device 
is in legacy and if something switches it to native it will very likely 
also program BARs. I can't imagine what would want to turn on native mode 
without trying to use a PCI driver and program BARs.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

Am 20.11.2023 um 16:11 hat BALATON Zoltan geschrieben:
> On Mon, 20 Nov 2023, Kevin Wolf wrote:
> > Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben:
> > > On Mon, 20 Nov 2023, Kevin Wolf wrote:
> > > > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
> > > > > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
> > > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > UDMA works with my original series, did not work with earlier versions of
> > > this alternative from Mark but could be fixed up on top unless Mark can send
> > > a v4 now.
> > > 
> > > > 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?
> > > 
> > > I could give that a try and see if that helps but all this
> > > via_ide_cfg_read() seems like an unnecessary complication to me. Why can't
> > > we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need
> > > to override config read?
> > 
> > I would be fine with setting 0xcc00 as the default value for BAR 4, but
> > as you said yourself, we can't do that in reset because it will be
> > overwritten by the PCI core code. Where else could we meaningfully do
> > that? As far as I understand, we don't have any hint that the
> > native/compatibility mode switch resets it on real hardware, so I'm
> > hesitant to do it there (and if the guest OS doesn't even switch, it
> > would never get set).
> 
> Luckily machines which need legacy mode also seem to set it explicitly
> on startup so we can set the defaults there.

With machines, you mean the QEMU board code? Where does this happen? Or
just what guests usually do?

> The check to see if something changed the BARs before is enough to
> avoid breaking it when legacy mode is set after native mode which does
> not seem to reset BARs according to how pegasos2 Linux behaves that
> sets legacy mode after firmware set native and proframmed BARs but the
> keep using BAR addresses. The AmigaOne I think just uses the default
> values with setting legacy mode doing nothing as that's the default
> but we can detect this as setting legacy mode with BARs unset so
> that's a good place to set default values which is what my patch did
> and I added a lot of comments trying to explain this.

I guess it's a question of your philosophy if you focus on just making a
specific set of OSes work, or if you focus on trying to do what hardware
actually does. Of course, figuring out what hardware actually does
proved a bit harder than I would have hoped in this case.

> > As for BAR 0-3, didn't we conclude that the via device still accepts I/O
> > to the configured addresses even though they read as zeros? Having
> > inconsistent config space and PCIIORegion seems like a bad idea, the
> > next call to pci_update_mappings() would break it.
> 
> I don't quite get this but then we could also just leave BARs alone
> and it would still work.

Didn't we have config space dumps from real hardware that showed that
this isn't what it does? Otherwise, this would be simpler indeed.

Of course, the spec also mandates that the values in the first four BARs
are ignored in compatibility mode (i.e. we would have to unregister the
memory regions), but we already figured that the via controller doesn't
do this. So that's something we would only have to solve if we want pci.c
code to be actually generic, but not for via.

> It probably does not matter what it reads back when the device is in
> legacy mode. What would call pci_update_mappings() if device is in
> legacy and if something switches it to native it will very likely also
> program BARs. I can't imagine what would want to turn on native mode
> without trying to use a PCI driver and program BARs.

For example, I see vmstate_info_pci_config -> get_pci_config_device ->
pci_update_mappings. Would live migration be enough to trigger this?

Maybe the relevant machines don't even support live migration (I haven't
checked), but it just doesn't feel robust to have inconsistent values in
data structures that are supposed to be in sync.

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

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).


ATB,

Mark.

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

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.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Kevin Wolf]

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.

Kevin

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

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

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Peter Maydell]

On Tue, 21 Nov 2023 at 10:18, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> 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:

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

I'm definitely not very enthusiastic about hacks which
increase the usage of qemu_register_reset() and rely
on reset-hook call order. We really need to try to have
another go at cleaning up the reset mess and this would be
yet another thing somebody's going to have to undo some day.
Unregistering your reset function in the reset function is
also rather curious...

thanks
-- PMM

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

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.

Is this document here accurate as to how it works on real hardware? 
https://intuitionbase.com/hints.php.

I can't understand why the base OS is attempting any access to BAR 4 if BMDMA isn't 
enabled by default on real hardware due to hardware bugs. Are we sure that the 
idetool hacks given in the link above to enable BMDMA haven't already been run on the 
AmigaOS install when testing an earlier version of the patches?

Finally is there a bootable AmigaOS demo ISO somewhere that can be used for testing?


ATB,

Mark.

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

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.
>
> Is this document here accurate as to how it works on real hardware? 
> https://intuitionbase.com/hints.php.

That should be about right. On QEMU the U-boot env vars won't work because 
NVRAM is not emulated yet so they can't be saved but you can call idetool 
from startup-sequence boot script instead for same effect (UDMA is enabled 
a bit later with that but after that it's the same).

> I can't understand why the base OS is attempting any access to BAR 4 if BMDMA 
> isn't enabled by default on real hardware due to hardware bugs.

Real hardware had problems with DMA (the VIA chip was also infamous for it 
on PC hardware and later also the ArticiaS was found to have its own 
problems) so the default is to use IDE in PIO mode and UDMA has to be 
enabled manually. But if it works (and it should on QEMU) it's much 
faster so we want to enable it.

> Are we sure 
> that the idetool hacks given in the link above to enable BMDMA haven't 
> already been run on the AmigaOS install when testing an earlier version of 
> the patches?

It was tested with my original series and works with that as my patch sets 
the default vaules for BARs and the driver reads it correctly. Then we 
tested your series too and I've noted for v2 already that it misses the 
degault value for BAR4. Other BARs don't matter as it will apparently use 
ISA IDE ports when it gets 0 or it knows that in legacy mode these shoud 
be the port values but seems to read BAR4 for UDMA and only works if the 
right default value is there, otherwise it lists DMA BAR 0 on start.

> Finally is there a bootable AmigaOS demo ISO somewhere that can be used for 
> testing?

Unfortunately AmigaOS4 is only available commercially, no free demo is 
avaialable.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

On Mon, 20 Nov 2023, BALATON Zoltan wrote:
> 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.
>> 
>> Is this document here accurate as to how it works on real hardware? 
>> https://intuitionbase.com/hints.php.
>
> That should be about right. On QEMU the U-boot env vars won't work because 
> NVRAM is not emulated yet so they can't be saved but you can call idetool 
> from startup-sequence boot script instead for same effect (UDMA is enabled a 
> bit later with that but after that it's the same).
>
>> I can't understand why the base OS is attempting any access to BAR 4 if 
>> BMDMA isn't enabled by default on real hardware due to hardware bugs.
>
> Real hardware had problems with DMA (the VIA chip was also infamous for it on 
> PC hardware and later also the ArticiaS was found to have its own problems) 
> so the default is to use IDE in PIO mode and UDMA has to be enabled manually. 
> But if it works (and it should on QEMU) it's much faster so we want to enable 
> it.
>
>> Are we sure that the idetool hacks given in the link above to enable BMDMA 
>> haven't already been run on the AmigaOS install when testing an earlier 
>> version of the patches?
>
> It was tested with my original series and works with that as my patch sets 
> the default vaules for BARs and the driver reads it correctly. Then we tested 
> your series too and I've noted for v2 already that it misses the degault 
> value for BAR4. Other BARs don't matter as it will apparently use ISA IDE 
> ports when it gets 0 or it knows that in legacy mode these shoud be the port 
> values but seems to read BAR4 for UDMA and only works if the right default 
> value is there, otherwise it lists DMA BAR 0 on start.

This probably wasn't clear so what I mean is:

a1ide.device 53.22 (28.6.2017)
[a1ide/dev_init] Found chip #0
[a1ide/init_port] ---> Port 0
[a1ide/init_port] IOBase 000001F0, AltBase 000003F6
[a1ide/init_port] bmcr_base 0000CC00
[a1ide/init_port] MMIOBase 00000000

This is with my patch: 
https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ 
With yours bmcr_base is 0 and then when enabling UDMA with idetool it 
hangs as it needs this value from BAR4.

Regards,
BALATON Zoltan

>> Finally is there a bootable AmigaOS demo ISO somewhere that can be used for 
>> testing?
>
> Unfortunately AmigaOS4 is only available commercially, no free demo is 
> avaialable.
>
> Regards,
> BALATON Zoltan

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[Mark Cave-Ayland]

On 20/11/2023 13:30, BALATON Zoltan wrote:

> On Mon, 20 Nov 2023, BALATON Zoltan wrote:
>> 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.
>>>
>>> Is this document here accurate as to how it works on real hardware? 
>>> https://intuitionbase.com/hints.php.
>>
>> That should be about right. On QEMU the U-boot env vars won't work because NVRAM is 
>> not emulated yet so they can't be saved but you can call idetool from 
>> startup-sequence boot script instead for same effect (UDMA is enabled a bit later 
>> with that but after that it's the same).
>>
>>> I can't understand why the base OS is attempting any access to BAR 4 if BMDMA 
>>> isn't enabled by default on real hardware due to hardware bugs.
>>
>> Real hardware had problems with DMA (the VIA chip was also infamous for it on PC 
>> hardware and later also the ArticiaS was found to have its own problems) so the 
>> default is to use IDE in PIO mode and UDMA has to be enabled manually. But if it 
>> works (and it should on QEMU) it's much faster so we want to enable it.
>>
>>> Are we sure that the idetool hacks given in the link above to enable BMDMA haven't 
>>> already been run on the AmigaOS install when testing an earlier version of the 
>>> patches?
>>
>> It was tested with my original series and works with that as my patch sets the 
>> default vaules for BARs and the driver reads it correctly. Then we tested your 
>> series too and I've noted for v2 already that it misses the degault value for BAR4. 
>> Other BARs don't matter as it will apparently use ISA IDE ports when it gets 0 or 
>> it knows that in legacy mode these shoud be the port values but seems to read BAR4 
>> for UDMA and only works if the right default value is there, otherwise it lists DMA 
>> BAR 0 on start.
> 
> This probably wasn't clear so what I mean is:
> 
> a1ide.device 53.22 (28.6.2017)
> [a1ide/dev_init] Found chip #0
> [a1ide/init_port] ---> Port 0
> [a1ide/init_port] IOBase 000001F0, AltBase 000003F6
> [a1ide/init_port] bmcr_base 0000CC00
> [a1ide/init_port] MMIOBase 00000000
> 
> This is with my patch: 
> https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ With yours bmcr_base is 0 and then when enabling UDMA with idetool it hangs as it needs this value from BAR4.

Right, so what you're saying is that AmigaOS will run fine with the v3 series 
out-of-the-box, but only fails when you try and enable UDMA with idetool (which is 
explicitly changing the configuration from the AmigaOS default)?


ATB,

Mark.

Re: [PATCH v3 0/4] ide: implement simple legacy/native mode switching for PCI IDE controllers

[BALATON Zoltan]

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

On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
> On 20/11/2023 13:30, BALATON Zoltan wrote:
>> On Mon, 20 Nov 2023, BALATON Zoltan wrote:
>>> 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.
>>>> 
>>>> Is this document here accurate as to how it works on real hardware? 
>>>> https://intuitionbase.com/hints.php.
>>> 
>>> That should be about right. On QEMU the U-boot env vars won't work because 
>>> NVRAM is not emulated yet so they can't be saved but you can call idetool 
>>> from startup-sequence boot script instead for same effect (UDMA is enabled 
>>> a bit later with that but after that it's the same).
>>> 
>>>> I can't understand why the base OS is attempting any access to BAR 4 if 
>>>> BMDMA isn't enabled by default on real hardware due to hardware bugs.
>>> 
>>> Real hardware had problems with DMA (the VIA chip was also infamous for it 
>>> on PC hardware and later also the ArticiaS was found to have its own 
>>> problems) so the default is to use IDE in PIO mode and UDMA has to be 
>>> enabled manually. But if it works (and it should on QEMU) it's much faster 
>>> so we want to enable it.
>>> 
>>>> Are we sure that the idetool hacks given in the link above to enable 
>>>> BMDMA haven't already been run on the AmigaOS install when testing an 
>>>> earlier version of the patches?
>>> 
>>> It was tested with my original series and works with that as my patch sets 
>>> the default vaules for BARs and the driver reads it correctly. Then we 
>>> tested your series too and I've noted for v2 already that it misses the 
>>> degault value for BAR4. Other BARs don't matter as it will apparently use 
>>> ISA IDE ports when it gets 0 or it knows that in legacy mode these shoud 
>>> be the port values but seems to read BAR4 for UDMA and only works if the 
>>> right default value is there, otherwise it lists DMA BAR 0 on start.
>> 
>> This probably wasn't clear so what I mean is:
>> 
>> a1ide.device 53.22 (28.6.2017)
>> [a1ide/dev_init] Found chip #0
>> [a1ide/init_port] ---> Port 0
>> [a1ide/init_port] IOBase 000001F0, AltBase 000003F6
>> [a1ide/init_port] bmcr_base 0000CC00
>> [a1ide/init_port] MMIOBase 00000000
>> 
>> This is with my patch: 
>> https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ 
>> With yours bmcr_base is 0 and then when enabling UDMA with idetool it hangs 
>> as it needs this value from BAR4.
>
> Right, so what you're saying is that AmigaOS will run fine with the v3 series 
> out-of-the-box, but only fails when you try and enable UDMA with idetool 
> (which is explicitly changing the configuration from the AmigaOS default)?

No, what I said is it boots with v2 but fails when enabling UDMA but does 
not even boot with v3 as it crashes even before printing the above debug 
logs with v3. The changes you've and Kevin proposed fix v3 to boot and 
allow UDMA too which I've tested and sent another reply about.

Regards,
BALATON Zoltan