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

[PATCH 0/2] 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

Mark Cave-Ayland (2):
  ide/pci.c: introduce pci_ide_update_mode() function
  hw/ide/via: implement legacy/native mode switching

 hw/ide/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/via.c         | 20 +++++++++-
 include/hw/ide/pci.h |  1 +
 3 files changed, 109 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] ide/pci.c: 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ide/pci.h |  1 +
 2 files changed, 91 insertions(+)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a25b352537..9eb30af632 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+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(),
+};
+
+void pci_ide_update_mode(PCIIDEState *s)
+{
+    PCIDevice *d = PCI_DEVICE(s);
+    uint8_t mode = d->config[PCI_CLASS_PROG];
+
+    switch (mode) {
+    case 0x8a:
+        /* Both channels legacy mode */
+
+        /* Zero BARs */
+        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
+        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
+        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
+        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
+
+        /* 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 0x8f:
+        /* 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 2/2] 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 also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
reset occurs then the device configuration is always consistent.

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

diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..e6278dd419 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,6 +129,9 @@ 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);
@@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
     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);
+    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
 
     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
     pci_set_long(pci_conf + 0x40, 0x0a090600);
@@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+                              uint32_t val, int len)
+{
+    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);
@@ -166,7 +182,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;
@@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     /* Reason: only works as function of VIA southbridge */
     dc->user_creatable = false;
 
+    k->config_write = via_ide_cfg_write;
     k->realize = via_ide_realize;
     k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-- 
2.39.2

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

[BALATON Zoltan]

On Thu, 19 Oct 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 also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
> reset occurs then the device configuration is always consistent.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/ide/via.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..e6278dd419 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,6 +129,9 @@ 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);
> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>     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 */

I'd remove these BAR defaults as they are not effective and aren't valid 
BAR values (missing IO bit) so likely would not work even if they weren't 
cleared but if you want to keep them maybe add a comment about that they 
will be zeroed by PCI reset anyway so only here for reference.

> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);

I did not get the commit message why it needs to 0 the intrerrupt pin 
because pci_ide_update_mode will set it above so I think this line could 
just use pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although 
it still contradicts the VT8231 data sheet about the interrupt pin default 
value but I don't care as long as it works.)

Regards,
BALATON Zoltan

>
>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>     pci_set_long(pci_conf + 0x40, 0x0a090600);
> @@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    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);
> @@ -166,7 +182,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;
> @@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>     /* Reason: only works as function of VIA southbridge */
>     dc->user_creatable = false;
>
> +    k->config_write = via_ide_cfg_write;
>     k->realize = via_ide_realize;
>     k->exit = via_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>

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

[BALATON Zoltan]

On Thu, 19 Oct 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>

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Thank you for finishing these. I've verified that AmigaOS and MorphOS 
and Linux still boot on pegasos2 and it also works with AmigaOS on 
amigaone so I'm OK with this. I only had some small comments in patch 2. 
This can replace patch 1 in my series, the other patches should just apply 
on top of this too but I can resubmit based on this when it becomes final.

Regards,
BALATON Zoltan

>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>
> Mark Cave-Ayland (2):
>  ide/pci.c: introduce pci_ide_update_mode() function
>  hw/ide/via: implement legacy/native mode switching
>
> hw/ide/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ide/via.c         | 20 +++++++++-
> include/hw/ide/pci.h |  1 +
> 3 files changed, 109 insertions(+), 2 deletions(-)
>
>

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Bernhard Beschow]

Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ide/pci.h |  1 +
> 2 files changed, 91 insertions(+)
>
>diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>index a25b352537..9eb30af632 100644
>--- a/hw/ide/pci.c
>+++ b/hw/ide/pci.c
>@@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>     .endianness = DEVICE_LITTLE_ENDIAN,
> };
> 
>+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(),
>+};
>+
>+void pci_ide_update_mode(PCIIDEState *s)
>+{
>+    PCIDevice *d = PCI_DEVICE(s);
>+    uint8_t mode = d->config[PCI_CLASS_PROG];
>+
>+    switch (mode) {

Maybe

  switch (mode & 0xf) {

here such that only the bits relevant to the PCI IDE controller specification are analyzed? Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).

>+    case 0x8a:

Perhaps we could add a

  case 0x0:

in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.

>+        /* Both channels legacy mode */
>+
>+        /* Zero BARs */
>+        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>+        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>+        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>+        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>+
>+        /* Clear interrupt pin */
>+        pci_config_set_interrupt_pin(d->config, 0);

Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.

Best regards,
Bernhard

>+
>+        /* 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 0x8f:
>+        /* 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;

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

[Bernhard Beschow]

Am 19. Oktober 2023 13:04:50 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>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>

I've successfully tested this series on top of my pc-via branch, so for the series:
Tested-by: Bernhard Beschow <shentey@gmail.com>

I've added comments to the first patch.

Best regards,
Bernhard

>
>
>[1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>
>Mark Cave-Ayland (2):
>  ide/pci.c: introduce pci_ide_update_mode() function
>  hw/ide/via: implement legacy/native mode switching
>
> hw/ide/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ide/via.c         | 20 +++++++++-
> include/hw/ide/pci.h |  1 +
> 3 files changed, 109 insertions(+), 2 deletions(-)
>

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Bernhard Beschow]

Am 22. Oktober 2023 22:06:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/ide/pci.h |  1 +
>> 2 files changed, 91 insertions(+)
>>
>>diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>index a25b352537..9eb30af632 100644
>>--- a/hw/ide/pci.c
>>+++ b/hw/ide/pci.c
>>@@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>>     .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>> 
>>+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[] = {

Although the naming seems familiar: What about renaming both lists to something like ide_portio_primary_list resp. ide_portio_secondary_list? Having one list carrying a number in its name while omitting it for the other I find rather confusing.

Best regards,
Bernhard

>>+    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>>+    PORTIO_END_OF_LIST(),
>>+};
>>+
>>+void pci_ide_update_mode(PCIIDEState *s)
>>+{
>>+    PCIDevice *d = PCI_DEVICE(s);
>>+    uint8_t mode = d->config[PCI_CLASS_PROG];
>>+
>>+    switch (mode) {
>
>Maybe
>
>  switch (mode & 0xf) {
>
>here such that only the bits relevant to the PCI IDE controller specification are analyzed? Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).
>
>>+    case 0x8a:
>
>Perhaps we could add a
>
>  case 0x0:
>
>in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.
>
>>+        /* Both channels legacy mode */
>>+
>>+        /* Zero BARs */
>>+        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>>+        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>>+        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>>+        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>>+
>>+        /* Clear interrupt pin */
>>+        pci_config_set_interrupt_pin(d->config, 0);
>
>Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.
>
>Best regards,
>Bernhard
>
>>+
>>+        /* 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 0x8f:
>>+        /* 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;

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Mark Cave-Ayland]

On 22/10/2023 23:06, Bernhard Beschow wrote:

> Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/ide/pci.h |  1 +
>> 2 files changed, 91 insertions(+)
>>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index a25b352537..9eb30af632 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> +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(),
>> +};
>> +
>> +void pci_ide_update_mode(PCIIDEState *s)
>> +{
>> +    PCIDevice *d = PCI_DEVICE(s);
>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>> +
>> +    switch (mode) {
> 
> Maybe
> 
>    switch (mode & 0xf) {
> 
> here such that only the bits relevant to the PCI IDE controller specification are analyzed? Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).

I can certainly do that for now, although my latest experiments suggest that the BM 
DMA BAR will need to be disabled in future when using legacy mode.

>> +    case 0x8a:
> 
> Perhaps we could add a
> 
>    case 0x0:
> 
> in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.

Well as mentioned in the cover letter this is an extract from a future series which 
should work for all PCI IDE controllers, but for now I've kept it limited to the 
via-ide device. This allows the basic concept to be tested (particularly for Zoltan's 
PPC machines) without having to worry about introducing regressions that can affect 
other controllers.

In future the aim is to allow it to be used for all PCI IDE controllers, but looking 
at what I have now I don't think I'll be able to get everything ready and tested for 
8.2. So watch this space :)

>> +        /* Both channels legacy mode */
>> +
>> +        /* Zero BARs */
>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>> +
>> +        /* Clear interrupt pin */
>> +        pci_config_set_interrupt_pin(d->config, 0);
> 
> Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.

I'd be fairly confident this is the case, since the PIIX-IDE device is hard-coded to 
legacy mode which means that the PCI interrupt pins aren't used (the values 1-4 
represent INTA-INTD here).

> Best regards,
> Bernhard
> 
>> +
>> +        /* 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 0x8f:
>> +        /* 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;


ATB,

Mark.

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

[Mark Cave-Ayland]

On 22/10/2023 23:10, Bernhard Beschow wrote:

> Am 19. Oktober 2023 13:04:50 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> 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>
> 
> I've successfully tested this series on top of my pc-via branch, so for the series:
> Tested-by: Bernhard Beschow <shentey@gmail.com>
> 
> I've added comments to the first patch.
> 
> Best regards,
> Bernhard

Thanks a lot! I'll make the change to the switch() statement and then post an updated 
v2 later this evening.

>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>>
>> Mark Cave-Ayland (2):
>>   ide/pci.c: introduce pci_ide_update_mode() function
>>   hw/ide/via: implement legacy/native mode switching
>>
>> hw/ide/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>> hw/ide/via.c         | 20 +++++++++-
>> include/hw/ide/pci.h |  1 +
>> 3 files changed, 109 insertions(+), 2 deletions(-)


ATB,

Mark.

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Mark Cave-Ayland]

On 23/10/2023 18:19, Bernhard Beschow wrote:

> Am 22. Oktober 2023 22:06:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>
>>
>> Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>> include/hw/ide/pci.h |  1 +
>>> 2 files changed, 91 insertions(+)
>>>
>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>> index a25b352537..9eb30af632 100644
>>> --- a/hw/ide/pci.c
>>> +++ b/hw/ide/pci.c
>>> @@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>>
>>> +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[] = {
> 
> Although the naming seems familiar: What about renaming both lists to something like ide_portio_primary_list resp. ide_portio_secondary_list? Having one list carrying a number in its name while omitting it for the other I find rather confusing.

The two different portio_lists don't represent the primary and secondary interfaces 
though: they represent the command and data ports for a single interface. I've left 
the naming as-is (at least for now) so that all of the IDEBus fields, ISA IDE ioports 
and PCI IDE ioports all share the same naming convention.

>>> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>>> +    PORTIO_END_OF_LIST(),
>>> +};
>>> +
>>> +void pci_ide_update_mode(PCIIDEState *s)
>>> +{
>>> +    PCIDevice *d = PCI_DEVICE(s);
>>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>>> +
>>> +    switch (mode) {
>>
>> Maybe
>>
>>   switch (mode & 0xf) {
>>
>> here such that only the bits relevant to the PCI IDE controller specification are analyzed? Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).
>>
>>> +    case 0x8a:
>>
>> Perhaps we could add a
>>
>>   case 0x0:
>>
>> in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.
>>
>>> +        /* Both channels legacy mode */
>>> +
>>> +        /* Zero BARs */
>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>>> +
>>> +        /* Clear interrupt pin */
>>> +        pci_config_set_interrupt_pin(d->config, 0);
>>
>> Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.
>>
>> Best regards,
>> Bernhard
>>
>>> +
>>> +        /* 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 0x8f:
>>> +        /* 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;


ATB,

Mark.

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

[Mark Cave-Ayland]

On 20/10/2023 00:09, BALATON Zoltan wrote:

> On Thu, 19 Oct 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 also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
>> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
>> reset occurs then the device configuration is always consistent.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ide/via.c | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..e6278dd419 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,6 +129,9 @@ 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);
>> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>>     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 */
> 
> I'd remove these BAR defaults as they are not effective and aren't valid BAR values 
> (missing IO bit) so likely would not work even if they weren't cleared but if you 
> want to keep them maybe add a comment about that they will be zeroed by PCI reset 
> anyway so only here for reference.

I've had a look again at the other PCI IDE controllers and none of the others attempt 
to set default BAR addresses except for PIIX-IDE, and that is simply to indicate the 
BMDMA BAR is an I/O BAR. So in retrospect, I'll add a commit to remove these BAR 
addresses as you've suggested above.

>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
> 
> I did not get the commit message why it needs to 0 the intrerrupt pin because 
> pci_ide_update_mode will set it above so I think this line could just use 
> pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although it still contradicts 
> the VT8231 data sheet about the interrupt pin default value but I don't care as long 
> as it works.)

If you're happy then I'll make the change to use pci_set_byte().

> Regards,
> BALATON Zoltan
> 
>>
>>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>     pci_set_long(pci_conf + 0x40, 0x0a090600);
>> @@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
>>     pci_set_long(pci_conf + 0xc0, 0x00020001);
>> }
>>
>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> +                              uint32_t val, int len)
>> +{
>> +    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);
>> @@ -166,7 +182,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;
>> @@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>     /* Reason: only works as function of VIA southbridge */
>>     dc->user_creatable = false;
>>
>> +    k->config_write = via_ide_cfg_write;
>>     k->realize = via_ide_realize;
>>     k->exit = via_ide_exitfn;
>>     k->vendor_id = PCI_VENDOR_ID_VIA;


ATB,

Mark.

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

[BALATON Zoltan]

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

On Mon, 23 Oct 2023, Mark Cave-Ayland wrote:
> On 20/10/2023 00:09, BALATON Zoltan wrote:
>> On Thu, 19 Oct 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 also moves the setting of PCI_CLASS_PROG from via_ide_realize() 
>>> to
>>> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI 
>>> device
>>> reset occurs then the device configuration is always consistent.
>>> 
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/ide/via.c | 20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..e6278dd419 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,6 +129,9 @@ 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);
>>> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>>>     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 */
>> 
>> I'd remove these BAR defaults as they are not effective and aren't valid 
>> BAR values (missing IO bit) so likely would not work even if they weren't 
>> cleared but if you want to keep them maybe add a comment about that they 
>> will be zeroed by PCI reset anyway so only here for reference.
>
> I've had a look again at the other PCI IDE controllers and none of the others 
> attempt to set default BAR addresses except for PIIX-IDE, and that is simply 
> to indicate the BMDMA BAR is an I/O BAR. So in retrospect, I'll add a commit 
> to remove these BAR addresses as you've suggested above.
>
>>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
>> 
>> I did not get the commit message why it needs to 0 the intrerrupt pin 
>> because pci_ide_update_mode will set it above so I think this line could 
>> just use pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although 
>> it still contradicts the VT8231 data sheet about the interrupt pin default 
>> value but I don't care as long as it works.)
>
> If you're happy then I'll make the change to use pci_set_byte().

I think only setting the interrupt line here is enough but as I said I 
don't care that much as long as it works. Although the vt8231 data sheet 
says interrupt pin should be 1 the vt82c686b data sheet is not clear about 
that and your new mode setting func will override it anyway so maybe it's 
not needed to touch other values than the interuupt line here.

Regards,
BALATON Zoltan

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Bernhard Beschow]

Am 23. Oktober 2023 21:06:11 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 23/10/2023 18:19, Bernhard Beschow wrote:
>
>> Am 22. Oktober 2023 22:06:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> 
>>> 
>>> Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>>> include/hw/ide/pci.h |  1 +
>>>> 2 files changed, 91 insertions(+)
>>>> 
>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>> index a25b352537..9eb30af632 100644
>>>> --- a/hw/ide/pci.c
>>>> +++ b/hw/ide/pci.c
>>>> @@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>>>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>>> };
>>>> 
>>>> +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[] = {
>> 
>> Although the naming seems familiar: What about renaming both lists to something like ide_portio_primary_list resp. ide_portio_secondary_list? Having one list carrying a number in its name while omitting it for the other I find rather confusing.
>
>The two different portio_lists don't represent the primary and secondary interfaces though: they represent the command and data ports for a single interface.

Ah, right.

> I've left the naming as-is (at least for now) so that all of the IDEBus fields, ISA IDE ioports and PCI IDE ioports all share the same naming convention.

Okay. At some point we should really harmonize the names to avoid above confusion. The PCI IDE BAR code does a much better job at naming and could serve as a template. Then all IDE code would clearly communicate that these are all the same concepts. I could send a patch for it once this series is in.

>
>>>> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>>>> +    PORTIO_END_OF_LIST(),
>>>> +};
>>>> +
>>>> +void pci_ide_update_mode(PCIIDEState *s)
>>>> +{
>>>> +    PCIDevice *d = PCI_DEVICE(s);
>>>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>>>> +
>>>> +    switch (mode) {
>>> 
>>> Maybe
>>> 
>>>   switch (mode & 0xf) {
>>> 
>>> here such that only the bits relevant to the PCI IDE controller specification are analyzed?

Due to the above conversation I realize that s->bus[] could be iterated over such that only the two bits of each bus could be switch()ed over. This would avoid some duplicate code, model the specification closer and allow for catching illegal states. Illegal states could be logged as guest errors. But it would also complicate dealing with the interrupt pin. So this might be a future extension.

Best regards,
Bernhard

>>> Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).
>>> 
>>>> +    case 0x8a:
>>> 
>>> Perhaps we could add a
>>> 
>>>   case 0x0:
>>> 
>>> in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.
>>> 
>>>> +        /* Both channels legacy mode */
>>>> +
>>>> +        /* Zero BARs */
>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>>>> +
>>>> +        /* Clear interrupt pin */
>>>> +        pci_config_set_interrupt_pin(d->config, 0);
>>> 
>>> Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.
>>> 
>>> Best regards,
>>> Bernhard
>>> 
>>>> +
>>>> +        /* 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 0x8f:
>>>> +        /* 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;
>
>
>ATB,
>
>Mark.
>

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

[Mark Cave-Ayland]

On 24/10/2023 08:08, Bernhard Beschow wrote:

> Am 23. Oktober 2023 21:06:11 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 23/10/2023 18:19, Bernhard Beschow wrote:
>>
>>> Am 22. Oktober 2023 22:06:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>>
>>>>
>>>> Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>>> 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         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> include/hw/ide/pci.h |  1 +
>>>>> 2 files changed, 91 insertions(+)
>>>>>
>>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>>> index a25b352537..9eb30af632 100644
>>>>> --- a/hw/ide/pci.c
>>>>> +++ b/hw/ide/pci.c
>>>>> @@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
>>>>>       .endianness = DEVICE_LITTLE_ENDIAN,
>>>>> };
>>>>>
>>>>> +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[] = {
>>>
>>> Although the naming seems familiar: What about renaming both lists to something like ide_portio_primary_list resp. ide_portio_secondary_list? Having one list carrying a number in its name while omitting it for the other I find rather confusing.
>>
>> The two different portio_lists don't represent the primary and secondary interfaces though: they represent the command and data ports for a single interface.
> 
> Ah, right.
> 
>> I've left the naming as-is (at least for now) so that all of the IDEBus fields, ISA IDE ioports and PCI IDE ioports all share the same naming convention.
> 
> Okay. At some point we should really harmonize the names to avoid above confusion. The PCI IDE BAR code does a much better job at naming and could serve as a template. Then all IDE code would clearly communicate that these are all the same concepts. I could send a patch for it once this series is in.

I agree that would be useful: and also same for the memory region names which could 
be updated so it's possible to tell the difference between the command and data ports 
(or indeed base/control as indicated in other online references).

>>
>>>>> +    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
>>>>> +    PORTIO_END_OF_LIST(),
>>>>> +};
>>>>> +
>>>>> +void pci_ide_update_mode(PCIIDEState *s)
>>>>> +{
>>>>> +    PCIDevice *d = PCI_DEVICE(s);
>>>>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>>>>> +
>>>>> +    switch (mode) {
>>>>
>>>> Maybe
>>>>
>>>>    switch (mode & 0xf) {
>>>>
>>>> here such that only the bits relevant to the PCI IDE controller specification are analyzed?
> 
> Due to the above conversation I realize that s->bus[] could be iterated over such that only the two bits of each bus could be switch()ed over. This would avoid some duplicate code, model the specification closer and allow for catching illegal states. Illegal states could be logged as guest errors. But it would also complicate dealing with the interrupt pin. So this might be a future extension.

Agreed. That particular logic will also change as a result of adding support for 
switching individual buses on the PCI IDE controller: not that I've seen anything do 
this to date, but it makes sense to implement what the documentation says where possible.

I didn't manage to send out the v2 yesterday evening, but I'll do so now.

> Best regards,
> Bernhard
> 
>>>> Then we can omit the high '8' nibble in the case labels which indicate bus master capability which is obviously out of scope of the switch statement (since you're not touching the BM DMA BAR).
>>>>
>>>>> +    case 0x8a:
>>>>
>>>> Perhaps we could add a
>>>>
>>>>    case 0x0:
>>>>
>>>> in front of the above label for compatibility with PIIX-IDE? That way, this function could be reused in the future for resetting PIIX-IDE.
>>>>
>>>>> +        /* Both channels legacy mode */
>>>>> +
>>>>> +        /* Zero BARs */
>>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
>>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
>>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
>>>>> +        pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
>>>>> +
>>>>> +        /* Clear interrupt pin */
>>>>> +        pci_config_set_interrupt_pin(d->config, 0);
>>>>
>>>> Do we really need to toggle the interrupt pin in this function? Or is this VIA-specific? This byte isn't even defined for PIIX-IDE.
>>>>
>>>> Best regards,
>>>> Bernhard
>>>>
>>>>> +
>>>>> +        /* 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 0x8f:
>>>>> +        /* 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;


ATB,

Mark.