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

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

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 (3):
  ide/pci.c: 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/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/via.c         | 25 ++++++++----
 include/hw/ide/pci.h |  1 +
 3 files changed, 109 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] 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>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Bernhard Beschow <shentey@gmail.com>
---
 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..5be643b460 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 & 0xf) {
+    case 0xa:
+        /* 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 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 v2 2/3] 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>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Bernhard Beschow <shentey@gmail.com>
---
 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

[PATCH v2 3/3] 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>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Bernhard Beschow <shentey@gmail.com>
---
 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 87b134083a..0cb65c8a3d 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,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);
@@ -161,7 +177,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 +231,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 v2 2/3] ide/via: don't attempt to set default BAR addresses

[BALATON Zoltan]

On Tue, 24 Oct 2023, 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.

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

this was taken from my original patch so I could also add R-b but being in 
two tags already is enough so let others listed if they want :-)

Regards,
BALATON Zoltan

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Tested-by: Bernhard Beschow <shentey@gmail.com>
> ---
> 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*/
>

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

[BALATON Zoltan]

On Tue, 24 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.

This is needed for my amigaone machine to boot (as that uses the legacy 
mode of this controller) so is somebody looking at merging this series in 
time for next release? Only one week left until the freeze.

Regards,
BALATON Zoltan

> 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
>
> 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 (3):
>  ide/pci.c: 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/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ide/via.c         | 25 ++++++++----
> include/hw/ide/pci.h |  1 +
> 3 files changed, 109 insertions(+), 7 deletions(-)
>
>

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

[Bernhard Beschow]

Am 24. Oktober 2023 22:40:53 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>

FWIW:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>
>[1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>
>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 (3):
>  ide/pci.c: 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/pci.c         | 90 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ide/via.c         | 25 ++++++++----
> include/hw/ide/pci.h |  1 +
> 3 files changed, 109 insertions(+), 7 deletions(-)
>

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

[Kevin Wolf]

Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
> 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>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Tested-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  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..5be643b460 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(),
> +};

This is duplicated from hw/ide/ioport.c. I think it would be better to
use the arrays already defined there, ideally by calling ioport.c
functions to setup and release the I/O ports.

> +void pci_ide_update_mode(PCIIDEState *s)
> +{
> +    PCIDevice *d = PCI_DEVICE(s);
> +    uint8_t mode = d->config[PCI_CLASS_PROG];
> +
> +    switch (mode & 0xf) {
> +    case 0xa:
> +        /* Both channels legacy mode */

Why is it ok to handle only the case where both channels are set to the
same mode? The spec describes mixed-mode setups, too, and doesn't seem
to allow ignoring a mode change if it's only for one of the channels.

> +
> +        /* 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);

Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
making it read-only and returning 0) while in compatibility mode doesn't
necessarily mean that the value of the register is lost. In other words,
are we sure that a driver can't expect that the old value is still there
when it re-enables native mode?

> +        /* Clear interrupt pin */
> +        pci_config_set_interrupt_pin(d->config, 0);

Unlike for the BARs, I don't see anything in the spec that allows
disabling this byte. I doubt it hurts in practice, but did you see any
drivers requiring this? According to the spec, we just must not use the
PCI interrupt in compatbility mode, but the registers stay accessible.

As far as I can see, the whole PCI interrupt configuration is currently
unused anyway, and nothing in this series seems to change it. So won't
we incorrectly continue to use the legacy interrupt even in native mode?
(Actually, cmd646 seems to get it wrong the other way around and uses
the PCI interrupt even in compatibility mode.)

I think this means that BMDMAState needs to have two irq lines (a legacy
and a PCI one) and select the right one in bmdma_irq() depending on
which mode we're in currently.

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

This is essentially ide_init_ioport(), except that it handles the case
where it is already initialised. Let's reuse it.

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

And this part could be an ioport.c function as well.

> +    }
> +}

Kevin

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

[Kevin Wolf]

Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
> 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>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Tested-by: Bernhard Beschow <shentey@gmail.com>

As I already noted in patch 1, the interrupt handling seems to be wrong
here, it continues to use the ISA IRQ in via_ide_set_irq() even after
switching to native mode.

Other than this, the patch looks good to me.

Kevin

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

[BALATON Zoltan]

On Mon, 6 Nov 2023, Kevin Wolf wrote:
> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>> 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>
>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>
> As I already noted in patch 1, the interrupt handling seems to be wrong
> here, it continues to use the ISA IRQ in via_ide_set_irq() even after
> switching to native mode.

That's a peculiarity of this via-ide device. It always uses 14/15 legacy 
interrupts even in native mode and guests expect that so using native 
interrupts would break pegasos2 guests. This was discussed and tested 
extensively before.

Regards,
BALATON Zoltan

> Other than this, the patch looks good to me.
>
> Kevin
>
>

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

[Mark Cave-Ayland]

On 06/11/2023 14:12, Kevin Wolf wrote:

Hi Kevin,

Thanks for taking the time to review this. I'll reply inline below.

> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>> 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>
>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   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..5be643b460 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(),
>> +};
> 
> This is duplicated from hw/ide/ioport.c. I think it would be better to
> use the arrays already defined there, ideally by calling ioport.c
> functions to setup and release the I/O ports.

The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, and so if we 
did that then all PCI IDE controllers would become dependent upon ISA too, regardless 
of whether they implement compatibility mode or not. What do you think is the best 
solution here? Perhaps moving ide_init_ioport() to a more ISA-specific place? I know 
that both myself and Phil have considered whether ide_init_ioport() should be 
replaced by something else further down the line.

>> +void pci_ide_update_mode(PCIIDEState *s)
>> +{
>> +    PCIDevice *d = PCI_DEVICE(s);
>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>> +
>> +    switch (mode & 0xf) {
>> +    case 0xa:
>> +        /* Both channels legacy mode */
> 
> Why is it ok to handle only the case where both channels are set to the
> same mode? The spec describes mixed-mode setups, too, and doesn't seem
> to allow ignoring a mode change if it's only for one of the channels.

Certainly that can be done: only both channels were implemented initially because 
that was the test case immediately available using the VIA. I can have a look at 
implementing both channels separately in v2.

>> +
>> +        /* 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);
> 
> Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
> making it read-only and returning 0) while in compatibility mode doesn't
> necessarily mean that the value of the register is lost. In other words,
> are we sure that a driver can't expect that the old value is still there
> when it re-enables native mode?

The specification is definitely a bit vague on the details here. In the testing I've 
done with the VIA, there is only ever a switch from native to legacy mode, and not 
the other way around. I can see the logic that once you've gone from the native to 
legacy mode, the memory allocated for the BARs is already reserved by the OS so in 
theory it could be possible to switch back to native mode again... and that would 
work if the BARs are preserved.

Would it happen in practice? I'm not really sure, but I can try to implement this if 
you think it makes sense to take a safer approach.

>> +        /* Clear interrupt pin */
>> +        pci_config_set_interrupt_pin(d->config, 0);
> 
> Unlike for the BARs, I don't see anything in the spec that allows
> disabling this byte. I doubt it hurts in practice, but did you see any
> drivers requiring this? According to the spec, we just must not use the
> PCI interrupt in compatbility mode, but the registers stay accessible.

The PCI config dumps taken from a real VIA indicate that this byte is cleared in 
legacy mode, and that appears to make sense here. If you imagine an early PCI IDE 
controller, it will always start up in legacy mode and so you don't want to indicate 
to the guest OS that PCI IRQ routing is required unless it has been switched to 
native mode first.

> As far as I can see, the whole PCI interrupt configuration is currently
> unused anyway, and nothing in this series seems to change it. So won't
> we incorrectly continue to use the legacy interrupt even in native mode?
> (Actually, cmd646 seems to get it wrong the other way around and uses
> the PCI interrupt even in compatibility mode.)
> 
> I think this means that BMDMAState needs to have two irq lines (a legacy
> and a PCI one) and select the right one in bmdma_irq() depending on
> which mode we're in currently.

I need to flesh out the details a bit more (in particular testing with more than just 
the VIA PCI IDE controller), but yes the eventual aim is to consolidate the majority 
of the BMDMA and mode switching code into hw/ide/pci.c so the individual controllers 
don't need to worry about this, and everything "just works".

>> +        /* 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;
> 
> This is essentially ide_init_ioport(), except that it handles the case
> where it is already initialised. Let's reuse it.

This still has the dependency issue with CONFIG_ISA, but I'm happy to implement what 
you think is the best solution (as discussed above).

>> +    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;
> 
> And this part could be an ioport.c function as well.
> 
>> +    }
>> +}
> 
> Kevin


ATB,

Mark.

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

[Kevin Wolf]

Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben:
> On Mon, 6 Nov 2023, Kevin Wolf wrote:
> > Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
> > > 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>
> > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > Tested-by: Bernhard Beschow <shentey@gmail.com>
> > 
> > As I already noted in patch 1, the interrupt handling seems to be wrong
> > here, it continues to use the ISA IRQ in via_ide_set_irq() even after
> > switching to native mode.
> 
> That's a peculiarity of this via-ide device. It always uses 14/15 legacy
> interrupts even in native mode and guests expect that so using native
> interrupts would break pegasos2 guests. This was discussed and tested
> extensively before.

This definitely needs a comment to explain the situation then because
this is in violation of the spec. If real hardware behaves like this,
it's what we should do, of course, but it's certainly unexpected and we
should explicitly document it to avoid breaking it later when someone
touches the code who doesn't know about this peculiarity.

Kevin

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

[Kevin Wolf]

Am 06.11.2023 um 23:41 hat Mark Cave-Ayland geschrieben:
> On 06/11/2023 14:12, Kevin Wolf wrote:
> 
> Hi Kevin,
> 
> Thanks for taking the time to review this. I'll reply inline below.
> 
> > Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
> > > 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>
> > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > Tested-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > >   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..5be643b460 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(),
> > > +};
> > 
> > This is duplicated from hw/ide/ioport.c. I think it would be better to
> > use the arrays already defined there, ideally by calling ioport.c
> > functions to setup and release the I/O ports.
> 
> The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, and
> so if we did that then all PCI IDE controllers would become dependent upon
> ISA too, regardless of whether they implement compatibility mode or not.
> What do you think is the best solution here? Perhaps moving
> ide_init_ioport() to a more ISA-specific place? I know that both myself and
> Phil have considered whether ide_init_ioport() should be replaced by
> something else further down the line.

Hm, yes, I didn't think about this.

Splitting ioport.c is one option, but even the port lists are really
made for ISA, so the whole file is really ISA related.

On the other hand, pci_ide_update_mode() isn't really a pure PCI
function, it's at the intersection of PCI and ISA. Can we just #ifdef it
out if ISA isn't built? Devices that don't support compatibility mode
should never try to call pci_ide_update_mode().

> > > +void pci_ide_update_mode(PCIIDEState *s)
> > > +{
> > > +    PCIDevice *d = PCI_DEVICE(s);
> > > +    uint8_t mode = d->config[PCI_CLASS_PROG];
> > > +
> > > +    switch (mode & 0xf) {
> > > +    case 0xa:
> > > +        /* Both channels legacy mode */
> > 
> > Why is it ok to handle only the case where both channels are set to the
> > same mode? The spec describes mixed-mode setups, too, and doesn't seem
> > to allow ignoring a mode change if it's only for one of the channels.
> 
> Certainly that can be done: only both channels were implemented initially
> because that was the test case immediately available using the VIA. I can
> have a look at implementing both channels separately in v2.

I don't think it would make the code more complicated, so it feels like
implementing it right away would be nice.

On the other hand, if you want to see this in 8.2, I'm happy to merge
this part as it is and then we can improve it on top.

> > > +
> > > +        /* 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);
> > 
> > Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
> > making it read-only and returning 0) while in compatibility mode doesn't
> > necessarily mean that the value of the register is lost. In other words,
> > are we sure that a driver can't expect that the old value is still there
> > when it re-enables native mode?
> 
> The specification is definitely a bit vague on the details here. In the
> testing I've done with the VIA, there is only ever a switch from native to
> legacy mode, and not the other way around. I can see the logic that once
> you've gone from the native to legacy mode, the memory allocated for the
> BARs is already reserved by the OS so in theory it could be possible to
> switch back to native mode again... and that would work if the BARs are
> preserved.
> 
> Would it happen in practice? I'm not really sure, but I can try to implement
> this if you think it makes sense to take a safer approach.

I'm not sure if any driver tries to do something like this. Maybe a
situation where BIOS switches to compatibility mode and then the OS to
native again? But it does feel safer.

The other option would be to not zero out the BAR at all. Doing that is
optional according to the spec. But then we need to make sure that we
ignore any access to the memory region behind the BAR even though its
address is still there.

Come to think of it, don't we need to somehow disable the memory regions
described by the BARs even when we zero them out? I think updating the
config space without calling pci_update_mappings() doesn't actually stop
QEMU from reacting to reads and writes there, does it?

Otherwise, the guest doesn't see the memory region in the BAR any more,
but it would still be active (which is almost the opposite of what we're
supposed to do).

> > > +        /* Clear interrupt pin */
> > > +        pci_config_set_interrupt_pin(d->config, 0);
> > 
> > Unlike for the BARs, I don't see anything in the spec that allows
> > disabling this byte. I doubt it hurts in practice, but did you see any
> > drivers requiring this? According to the spec, we just must not use the
> > PCI interrupt in compatbility mode, but the registers stay accessible.
> 
> The PCI config dumps taken from a real VIA indicate that this byte is
> cleared in legacy mode, and that appears to make sense here. If you imagine
> an early PCI IDE controller, it will always start up in legacy mode and so
> you don't want to indicate to the guest OS that PCI IRQ routing is required
> unless it has been switched to native mode first.

Ok. I assume that with a per-channel control, you would clear it only if
both channels are in compatibility mode, and set it as soon as one
channel is in native mode?

> > As far as I can see, the whole PCI interrupt configuration is currently
> > unused anyway, and nothing in this series seems to change it. So won't
> > we incorrectly continue to use the legacy interrupt even in native mode?
> > (Actually, cmd646 seems to get it wrong the other way around and uses
> > the PCI interrupt even in compatibility mode.)
> > 
> > I think this means that BMDMAState needs to have two irq lines (a legacy
> > and a PCI one) and select the right one in bmdma_irq() depending on
> > which mode we're in currently.
> 
> I need to flesh out the details a bit more (in particular testing with more
> than just the VIA PCI IDE controller), but yes the eventual aim is to
> consolidate the majority of the BMDMA and mode switching code into
> hw/ide/pci.c so the individual controllers don't need to worry about this,
> and everything "just works".

Zoltan's reply for patch 3 actuallys say that it's correct like this for
the VIA controller, in conflict with what the spec says. So it seems
that we can't make it "just work" for everyone, but we still need to
allow devices to intercept it at least.

As I said there, adding a comment in the via emulation should be enough
for now, and we can leave proper generic interrupt handling for another
day (when we want to add switching to a model that's actually consistent
with the spec).

Maybe also leave a TODO comment at the top of this function to remind
other users that interrupt handling needs to be covered by individual
devices for now.

Kevin

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

[Mark Cave-Ayland]

On 07/11/2023 10:43, Kevin Wolf wrote:

> Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben:
>> On Mon, 6 Nov 2023, Kevin Wolf wrote:
>>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>>>> 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>
>>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>>>
>>> As I already noted in patch 1, the interrupt handling seems to be wrong
>>> here, it continues to use the ISA IRQ in via_ide_set_irq() even after
>>> switching to native mode.
>>
>> That's a peculiarity of this via-ide device. It always uses 14/15 legacy
>> interrupts even in native mode and guests expect that so using native
>> interrupts would break pegasos2 guests. This was discussed and tested
>> extensively before.
> 
> This definitely needs a comment to explain the situation then because
> this is in violation of the spec. If real hardware behaves like this,
> it's what we should do, of course, but it's certainly unexpected and we
> should explicitly document it to avoid breaking it later when someone
> touches the code who doesn't know about this peculiarity.

It's a little bit more complicated than this: in native mode it is possible to route 
the IRQs for each individual channel to a small select number of IRQs by configuring 
special registers on the VIA.

The complication here is that it isn't immediately obvious how the QEMU PCI routing 
code can do this - I did post about this at 
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg10552.html asking the best 
way to resolve this, but haven't had any replies yet.

Fortunately it seems that all the guests tested so far stick with the IRQ 14/15 
defaults which is why this happens to work, so short-term this is a lower priority 
when looking at consolidating the switching logic.


ATB,

Mark.

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

[Mark Cave-Ayland]

On 07/11/2023 11:11, Kevin Wolf wrote:

> Am 06.11.2023 um 23:41 hat Mark Cave-Ayland geschrieben:
>> On 06/11/2023 14:12, Kevin Wolf wrote:
>>
>> Hi Kevin,
>>
>> Thanks for taking the time to review this. I'll reply inline below.
>>
>>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>>>> 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>
>>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>    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..5be643b460 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(),
>>>> +};
>>>
>>> This is duplicated from hw/ide/ioport.c. I think it would be better to
>>> use the arrays already defined there, ideally by calling ioport.c
>>> functions to setup and release the I/O ports.
>>
>> The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, and
>> so if we did that then all PCI IDE controllers would become dependent upon
>> ISA too, regardless of whether they implement compatibility mode or not.
>> What do you think is the best solution here? Perhaps moving
>> ide_init_ioport() to a more ISA-specific place? I know that both myself and
>> Phil have considered whether ide_init_ioport() should be replaced by
>> something else further down the line.
> 
> Hm, yes, I didn't think about this.
> 
> Splitting ioport.c is one option, but even the port lists are really
> made for ISA, so the whole file is really ISA related.
> 
> On the other hand, pci_ide_update_mode() isn't really a pure PCI
> function, it's at the intersection of PCI and ISA. Can we just #ifdef it
> out if ISA isn't built? Devices that don't support compatibility mode
> should never try to call pci_ide_update_mode().

In terms of the QEMU modelling, the PCI IDE controllers are modelled as a PCIDevice 
rather than an ISADevice and that's why ide_init_ioport() doesn't really make sense 
in PCI IDE controllers. Currently its only PCIDevice user is hw/ide/piix.c and that 
passes ISADevice as NULL, because there is no underlying ISADevice.

The only ISADevice user is in hw/ide/isa.c so I think a better solution here would be 
to inline ide_init_ioport() into isa_ide_realizefn() and then add a separate function 
for PCI IDE controllers which is what I've attempted to do here.

How about moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c instead? 
The definitions in include/hw/ide/internal.h already have a dependency on PortioList 
so there should be no issue, and it allows them to be shared between both PCI and ISA 
devices.

>>>> +void pci_ide_update_mode(PCIIDEState *s)
>>>> +{
>>>> +    PCIDevice *d = PCI_DEVICE(s);
>>>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>>>> +
>>>> +    switch (mode & 0xf) {
>>>> +    case 0xa:
>>>> +        /* Both channels legacy mode */
>>>
>>> Why is it ok to handle only the case where both channels are set to the
>>> same mode? The spec describes mixed-mode setups, too, and doesn't seem
>>> to allow ignoring a mode change if it's only for one of the channels.
>>
>> Certainly that can be done: only both channels were implemented initially
>> because that was the test case immediately available using the VIA. I can
>> have a look at implementing both channels separately in v2.
> 
> I don't think it would make the code more complicated, so it feels like
> implementing it right away would be nice.
> 
> On the other hand, if you want to see this in 8.2, I'm happy to merge
> this part as it is and then we can improve it on top.

I think this helps Zoltan boot AmigaOS on the new AmigaOne machine, and I am 
certainly planning more work in this area during the 9.0 cycle.

>>>> +
>>>> +        /* 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);
>>>
>>> Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
>>> making it read-only and returning 0) while in compatibility mode doesn't
>>> necessarily mean that the value of the register is lost. In other words,
>>> are we sure that a driver can't expect that the old value is still there
>>> when it re-enables native mode?
>>
>> The specification is definitely a bit vague on the details here. In the
>> testing I've done with the VIA, there is only ever a switch from native to
>> legacy mode, and not the other way around. I can see the logic that once
>> you've gone from the native to legacy mode, the memory allocated for the
>> BARs is already reserved by the OS so in theory it could be possible to
>> switch back to native mode again... and that would work if the BARs are
>> preserved.
>>
>> Would it happen in practice? I'm not really sure, but I can try to implement
>> this if you think it makes sense to take a safer approach.
> 
> I'm not sure if any driver tries to do something like this. Maybe a
> situation where BIOS switches to compatibility mode and then the OS to
> native again? But it does feel safer.
> 
> The other option would be to not zero out the BAR at all. Doing that is
> optional according to the spec. But then we need to make sure that we
> ignore any access to the memory region behind the BAR even though its
> address is still there.
> 
> Come to think of it, don't we need to somehow disable the memory regions
> described by the BARs even when we zero them out? I think updating the
> config space without calling pci_update_mappings() doesn't actually stop
> QEMU from reacting to reads and writes there, does it?
> 
> Otherwise, the guest doesn't see the memory region in the BAR any more,
> but it would still be active (which is almost the opposite of what we're
> supposed to do).

At least the VIA appears to still respond to the BAR addresses even when switched 
back to compatibility mode (bug?), but without more testing after making the 
functionality more generic it's difficult to say. Should the BARs be disabled when 
switched to legacy mode? According to the spec that should be the case, but then a 
switch from native -> legacy mode doesn't matter because the BAR addresses are 
already reserved, and a switch from legacy -> native mode would require the BARs to 
be (re)programmed anyway. I will definitely do some testing with the functionality 
enabled for all PCI IDE controllers in future, however I'm minded to keep it as 
simple as possible.

>>>> +        /* Clear interrupt pin */
>>>> +        pci_config_set_interrupt_pin(d->config, 0);
>>>
>>> Unlike for the BARs, I don't see anything in the spec that allows
>>> disabling this byte. I doubt it hurts in practice, but did you see any
>>> drivers requiring this? According to the spec, we just must not use the
>>> PCI interrupt in compatbility mode, but the registers stay accessible.
>>
>> The PCI config dumps taken from a real VIA indicate that this byte is
>> cleared in legacy mode, and that appears to make sense here. If you imagine
>> an early PCI IDE controller, it will always start up in legacy mode and so
>> you don't want to indicate to the guest OS that PCI IRQ routing is required
>> unless it has been switched to native mode first.
> 
> Ok. I assume that with a per-channel control, you would clear it only if
> both channels are in compatibility mode, and set it as soon as one
> channel is in native mode?

Yes, I believe that's correct.

>>> As far as I can see, the whole PCI interrupt configuration is currently
>>> unused anyway, and nothing in this series seems to change it. So won't
>>> we incorrectly continue to use the legacy interrupt even in native mode?
>>> (Actually, cmd646 seems to get it wrong the other way around and uses
>>> the PCI interrupt even in compatibility mode.)
>>>
>>> I think this means that BMDMAState needs to have two irq lines (a legacy
>>> and a PCI one) and select the right one in bmdma_irq() depending on
>>> which mode we're in currently.
>>
>> I need to flesh out the details a bit more (in particular testing with more
>> than just the VIA PCI IDE controller), but yes the eventual aim is to
>> consolidate the majority of the BMDMA and mode switching code into
>> hw/ide/pci.c so the individual controllers don't need to worry about this,
>> and everything "just works".
> 
> Zoltan's reply for patch 3 actuallys say that it's correct like this for
> the VIA controller, in conflict with what the spec says. So it seems
> that we can't make it "just work" for everyone, but we still need to
> allow devices to intercept it at least.

I've replied separately to this, and as mentioned that's something that will likely 
require some future updates to the PCI IRQ routing code.

> As I said there, adding a comment in the via emulation should be enough
> for now, and we can leave proper generic interrupt handling for another
> day (when we want to add switching to a model that's actually consistent
> with the spec).

Agreed.

> Maybe also leave a TODO comment at the top of this function to remind
> other users that interrupt handling needs to be covered by individual
> devices for now.

Yes I can definitely add a comment with a TODO. For now in terms of 8.2 would you be 
happy with:

- Moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c

- Change the configuration space access to read all zeros for BARs in compatibility
   mode instead of zeroing the BARs

- Leave it so that both channels are switched at the same time

- Add a TODO comment to pci_ide_update_mode() indicating that individual PCI IDE
   controllers are responsible for their own IRQ routing


ATB,

Mark.

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

[BALATON Zoltan]

On Mon, 13 Nov 2023, Mark Cave-Ayland wrote:
> On 07/11/2023 11:11, Kevin Wolf wrote:
>> Am 06.11.2023 um 23:41 hat Mark Cave-Ayland geschrieben:
>>> On 06/11/2023 14:12, Kevin Wolf wrote:
>>> 
>>> Hi Kevin,
>>> 
>>> Thanks for taking the time to review this. I'll reply inline below.
>>> 
>>>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>>>>> 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>
>>>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>    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..5be643b460 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(),
>>>>> +};
>>>> 
>>>> This is duplicated from hw/ide/ioport.c. I think it would be better to
>>>> use the arrays already defined there, ideally by calling ioport.c
>>>> functions to setup and release the I/O ports.
>>> 
>>> The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, 
>>> and
>>> so if we did that then all PCI IDE controllers would become dependent upon
>>> ISA too, regardless of whether they implement compatibility mode or not.
>>> What do you think is the best solution here? Perhaps moving
>>> ide_init_ioport() to a more ISA-specific place? I know that both myself 
>>> and
>>> Phil have considered whether ide_init_ioport() should be replaced by
>>> something else further down the line.
>> 
>> Hm, yes, I didn't think about this.
>> 
>> Splitting ioport.c is one option, but even the port lists are really
>> made for ISA, so the whole file is really ISA related.
>> 
>> On the other hand, pci_ide_update_mode() isn't really a pure PCI
>> function, it's at the intersection of PCI and ISA. Can we just #ifdef it
>> out if ISA isn't built? Devices that don't support compatibility mode
>> should never try to call pci_ide_update_mode().
>
> In terms of the QEMU modelling, the PCI IDE controllers are modelled as a 
> PCIDevice rather than an ISADevice and that's why ide_init_ioport() doesn't 
> really make sense in PCI IDE controllers. Currently its only PCIDevice user 
> is hw/ide/piix.c and that passes ISADevice as NULL, because there is no 
> underlying ISADevice.
>
> The only ISADevice user is in hw/ide/isa.c so I think a better solution here 
> would be to inline ide_init_ioport() into isa_ide_realizefn() and then add a 
> separate function for PCI IDE controllers which is what I've attempted to do 
> here.
>
> How about moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c 
> instead? The definitions in include/hw/ide/internal.h already have a 
> dependency on PortioList so there should be no issue, and it allows them to 
> be shared between both PCI and ISA devices.

That's where these came from in commit 83d14054f9555 and the reason was to 
get rid of the ISA dependency for machines that don't need it. Would it be 
possible to make a function that only registers the portio stuff (e.g. 
ide_register_ports) that's not depenent on either ISADevice nor 
PCIIDEState but takes an IDEBus? That could be used by both ide-isa and 
PCI devices. This would just do the portio stuff for a single bus and you 
could call it twice from your pci_ide_update_mode function then rhe 
portio_list arrays can remain static to core.c. That seems better than 
duplicating code and exporting these arrays.

>>>>> +void pci_ide_update_mode(PCIIDEState *s)
>>>>> +{
>>>>> +    PCIDevice *d = PCI_DEVICE(s);
>>>>> +    uint8_t mode = d->config[PCI_CLASS_PROG];
>>>>> +
>>>>> +    switch (mode & 0xf) {
>>>>> +    case 0xa:
>>>>> +        /* Both channels legacy mode */
>>>> 
>>>> Why is it ok to handle only the case where both channels are set to the
>>>> same mode? The spec describes mixed-mode setups, too, and doesn't seem
>>>> to allow ignoring a mode change if it's only for one of the channels.
>>> 
>>> Certainly that can be done: only both channels were implemented initially
>>> because that was the test case immediately available using the VIA. I can
>>> have a look at implementing both channels separately in v2.
>> 
>> I don't think it would make the code more complicated, so it feels like
>> implementing it right away would be nice.
>> 
>> On the other hand, if you want to see this in 8.2, I'm happy to merge
>> this part as it is and then we can improve it on top.
>
> I think this helps Zoltan boot AmigaOS on the new AmigaOne machine, and I am 
> certainly planning more work in this area during the 9.0 cycle.

As said before for booting AmigaOS we need either this series or my 
original fix: 
https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ 
for which you posted this as a replacement. If it's deemed too much change 
for the freeze or you need more time ro finish this series we can take my 
patch for now and come back to this later. (Recently I've found that my 
patch using PCI BARs set to legacy address has a side effect that BAR4 
shadows some ports of the FDC because BARs are 4 bytes which your series 
avoids as it can register port 0x376 as single byte but it's easy to 
disable floppy driver in AmigaOS so this is not a problem and can be fixed 
later. Nobody really needs a floppy as it boots from HDD or CD and uses 
network or other means to transfer files rather than floppies so it's not 
needed and the additional step to disable driver is not a problem because 
adding a graphics driver in the same place is also needed so users will 
need to edit that config anyway. Other than this my patch solves all 
problems for now in a simpler way and then can be replaced with your 
series for 9.0.)

>>>>> +
>>>>> +        /* 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);
>>>> 
>>>> Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
>>>> making it read-only and returning 0) while in compatibility mode doesn't
>>>> necessarily mean that the value of the register is lost. In other words,
>>>> are we sure that a driver can't expect that the old value is still there
>>>> when it re-enables native mode?
>>> 
>>> The specification is definitely a bit vague on the details here. In the
>>> testing I've done with the VIA, there is only ever a switch from native to
>>> legacy mode, and not the other way around. I can see the logic that once
>>> you've gone from the native to legacy mode, the memory allocated for the
>>> BARs is already reserved by the OS so in theory it could be possible to
>>> switch back to native mode again... and that would work if the BARs are
>>> preserved.
>>> 
>>> Would it happen in practice? I'm not really sure, but I can try to 
>>> implement
>>> this if you think it makes sense to take a safer approach.
>> 
>> I'm not sure if any driver tries to do something like this. Maybe a
>> situation where BIOS switches to compatibility mode and then the OS to
>> native again? But it does feel safer.
>> 
>> The other option would be to not zero out the BAR at all. Doing that is
>> optional according to the spec. But then we need to make sure that we
>> ignore any access to the memory region behind the BAR even though its
>> address is still there.
>> 
>> Come to think of it, don't we need to somehow disable the memory regions
>> described by the BARs even when we zero them out? I think updating the
>> config space without calling pci_update_mappings() doesn't actually stop
>> QEMU from reacting to reads and writes there, does it?
>> 
>> Otherwise, the guest doesn't see the memory region in the BAR any more,
>> but it would still be active (which is almost the opposite of what we're
>> supposed to do).
>
> At least the VIA appears to still respond to the BAR addresses even when 
> switched back to compatibility mode (bug?), but without more testing after 
> making the functionality more generic it's difficult to say. Should the BARs 
> be disabled when switched to legacy mode? According to the spec that should

I think the problem case is pegasos2 where firmware enables native mode 
(with IRQ 14/15 but programming BARs) but Linux expects native mode to use 
a PCI interrupt which does not work so it sets prog-if back to legacy mode 
to make its driver use legacy interrupts but then keeps using BARs which 
works on real hardware. Other guests don't set prog-if just use BARs and 
IRQ 14/15. So it looks like real chip either still uses BARs after they 
are programmed or you can't really swirch from native mode back to legacy 
mode and native mode still uses legacy interrupts. I think this chip does 
not really follow PCI specs (it's an early part from when PCI was new and 
likely developed from an ISA superIO chip with some PCI support added so 
maybe it only partially implemented the spec or had some excensions not 
compying with it.)

> be the case, but then a switch from native -> legacy mode doesn't matter 
> because the BAR addresses are already reserved, and a switch from legacy -> 
> native mode would require the BARs to be (re)programmed anyway. I will 
> definitely do some testing with the functionality enabled for all PCI IDE 
> controllers in future, however I'm minded to keep it as simple as possible.
>
>>>>> +        /* Clear interrupt pin */
>>>>> +        pci_config_set_interrupt_pin(d->config, 0);
>>>> 
>>>> Unlike for the BARs, I don't see anything in the spec that allows
>>>> disabling this byte. I doubt it hurts in practice, but did you see any
>>>> drivers requiring this? According to the spec, we just must not use the
>>>> PCI interrupt in compatbility mode, but the registers stay accessible.
>>> 
>>> The PCI config dumps taken from a real VIA indicate that this byte is
>>> cleared in legacy mode, and that appears to make sense here. If you 
>>> imagine
>>> an early PCI IDE controller, it will always start up in legacy mode and so
>>> you don't want to indicate to the guest OS that PCI IRQ routing is 
>>> required
>>> unless it has been switched to native mode first.
>> 
>> Ok. I assume that with a per-channel control, you would clear it only if
>> both channels are in compatibility mode, and set it as soon as one
>> channel is in native mode?
>
> Yes, I believe that's correct.
>
>>>> As far as I can see, the whole PCI interrupt configuration is currently
>>>> unused anyway, and nothing in this series seems to change it. So won't
>>>> we incorrectly continue to use the legacy interrupt even in native mode?
>>>> (Actually, cmd646 seems to get it wrong the other way around and uses
>>>> the PCI interrupt even in compatibility mode.)
>>>> 
>>>> I think this means that BMDMAState needs to have two irq lines (a legacy
>>>> and a PCI one) and select the right one in bmdma_irq() depending on
>>>> which mode we're in currently.
>>> 
>>> I need to flesh out the details a bit more (in particular testing with 
>>> more
>>> than just the VIA PCI IDE controller), but yes the eventual aim is to
>>> consolidate the majority of the BMDMA and mode switching code into
>>> hw/ide/pci.c so the individual controllers don't need to worry about this,
>>> and everything "just works".
>> 
>> Zoltan's reply for patch 3 actuallys say that it's correct like this for
>> the VIA controller, in conflict with what the spec says. So it seems
>> that we can't make it "just work" for everyone, but we still need to
>> allow devices to intercept it at least.
>
> I've replied separately to this, and as mentioned that's something that will 
> likely require some future updates to the PCI IRQ routing code.
>
>> As I said there, adding a comment in the via emulation should be enough
>> for now, and we can leave proper generic interrupt handling for another
>> day (when we want to add switching to a model that's actually consistent
>> with the spec).
>
> Agreed.
>
>> Maybe also leave a TODO comment at the top of this function to remind
>> other users that interrupt handling needs to be covered by individual
>> devices for now.
>
> Yes I can definitely add a comment with a TODO. For now in terms of 8.2 would 
> you be happy with:
>
> - Moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c
>
> - Change the configuration space access to read all zeros for BARs in 
> compatibility mode instead of zeroing the BARs

What would this achieve other than having more complex code for nothing? 
If anything enables native mode it will also program BARs, it's very 
unlikely any guest would switch to legacy mode than back to native and 
expect some values programmed before be still in BARs so you could either 
zero the values or just leave them as nothing cares anyway. Adding a 
separare read function for this seems like unnecessary complication to me. 
So I'd leave this point out, the rest seems OK.

> - Leave it so that both channels are switched at the same time
>
> - Add a TODO comment to pci_ide_update_mode() indicating that individual PCI 
> IDE
>  controllers are responsible for their own IRQ routing

One more thing is that you should set the deafult value of the BMDMA BAR 
somewhere, this part in my patch

pci_set_long(pd->config + PCI_BASE_ADDRESS_4, 0xcc00 | PCI_BASE_ADDRESS_SPACE_IO);

Otherwise enabling UDMA mode does not work. It's disabled by default on 
amigaone because real machine had problems with DMA, both the VIA chip and 
ArticiaS were notorious for DMA errors but it can be enabled which works 
with my patch and is much faster but hangs with your patch because AmigaOS 
reads the BMDMA BAR value but does not set it so there should be the 
default value there.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Mon, 13 Nov 2023, Mark Cave-Ayland wrote:
> On 07/11/2023 10:43, Kevin Wolf wrote:
>> Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben:
>>> On Mon, 6 Nov 2023, Kevin Wolf wrote:
>>>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
>>>>> 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>
>>>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> Tested-by: Bernhard Beschow <shentey@gmail.com>
>>>> 
>>>> As I already noted in patch 1, the interrupt handling seems to be wrong
>>>> here, it continues to use the ISA IRQ in via_ide_set_irq() even after
>>>> switching to native mode.
>>> 
>>> That's a peculiarity of this via-ide device. It always uses 14/15 legacy
>>> interrupts even in native mode and guests expect that so using native
>>> interrupts would break pegasos2 guests. This was discussed and tested
>>> extensively before.
>> 
>> This definitely needs a comment to explain the situation then because
>> this is in violation of the spec. If real hardware behaves like this,
>> it's what we should do, of course, but it's certainly unexpected and we
>> should explicitly document it to avoid breaking it later when someone
>> touches the code who doesn't know about this peculiarity.
>
> It's a little bit more complicated than this: in native mode it is possible 
> to route the IRQs for each individual channel to a small select number of 
> IRQs by configuring special registers on the VIA.

That's documented for VT82c686B but VT8231 doc says other values are 
reserved and only IRQ 14/15 is valid. So even if it worked nothing uses it 
and we don't have to be concerned about it and just using these hard coded 
14/15 is enough. It probably does not worth trying to emulate chip 
functions that no guest uses, especially when we're not sure how the real 
chip works as we can't test on real machine.

Regards,
BALATON Zoltan

> The complication here is that it isn't immediately obvious how the QEMU PCI 
> routing code can do this - I did post about this at 
> https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg10552.html asking 
> the best way to resolve this, but haven't had any replies yet.
>
> Fortunately it seems that all the guests tested so far stick with the IRQ 
> 14/15 defaults which is why this happens to work, so short-term this is a 
> lower priority when looking at consolidating the switching logic.
>
>
> ATB,
>
> Mark.
>
>

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

[Kevin Wolf]

Am 13.11.2023 um 22:24 hat Mark Cave-Ayland geschrieben:
> On 07/11/2023 11:11, Kevin Wolf wrote:
> 
> > Am 06.11.2023 um 23:41 hat Mark Cave-Ayland geschrieben:
> > > On 06/11/2023 14:12, Kevin Wolf wrote:
> > > 
> > > Hi Kevin,
> > > 
> > > Thanks for taking the time to review this. I'll reply inline below.
> > > 
> > > > Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
> > > > > 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>
> > > > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > Tested-by: Bernhard Beschow <shentey@gmail.com>
> > > > > ---
> > > > >    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..5be643b460 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(),
> > > > > +};
> > > > 
> > > > This is duplicated from hw/ide/ioport.c. I think it would be better to
> > > > use the arrays already defined there, ideally by calling ioport.c
> > > > functions to setup and release the I/O ports.
> > > 
> > > The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, and
> > > so if we did that then all PCI IDE controllers would become dependent upon
> > > ISA too, regardless of whether they implement compatibility mode or not.
> > > What do you think is the best solution here? Perhaps moving
> > > ide_init_ioport() to a more ISA-specific place? I know that both myself and
> > > Phil have considered whether ide_init_ioport() should be replaced by
> > > something else further down the line.
> > 
> > Hm, yes, I didn't think about this.
> > 
> > Splitting ioport.c is one option, but even the port lists are really
> > made for ISA, so the whole file is really ISA related.
> > 
> > On the other hand, pci_ide_update_mode() isn't really a pure PCI
> > function, it's at the intersection of PCI and ISA. Can we just #ifdef it
> > out if ISA isn't built? Devices that don't support compatibility mode
> > should never try to call pci_ide_update_mode().
> 
> In terms of the QEMU modelling, the PCI IDE controllers are modelled as a
> PCIDevice rather than an ISADevice and that's why ide_init_ioport() doesn't
> really make sense in PCI IDE controllers. Currently its only PCIDevice user
> is hw/ide/piix.c and that passes ISADevice as NULL, because there is no
> underlying ISADevice.
> 
> The only ISADevice user is in hw/ide/isa.c so I think a better solution here
> would be to inline ide_init_ioport() into isa_ide_realizefn() and then add a
> separate function for PCI IDE controllers which is what I've attempted to do
> here.
> 
> How about moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c
> instead? The definitions in include/hw/ide/internal.h already have a
> dependency on PortioList so there should be no issue, and it allows them to
> be shared between both PCI and ISA devices.

If you move ide_init_ioport() to isa.c, then nothing ISA specific is
left in ioport.c and you can keep the rest there. Probably with
functions that work directly on portio lists without being tied to ISA.

But I'm also not opposed to moving the port lists to core.c if that
works better in practice.

> > > > > +void pci_ide_update_mode(PCIIDEState *s)
> > > > > +{
> > > > > +    PCIDevice *d = PCI_DEVICE(s);
> > > > > +    uint8_t mode = d->config[PCI_CLASS_PROG];
> > > > > +
> > > > > +    switch (mode & 0xf) {
> > > > > +    case 0xa:
> > > > > +        /* Both channels legacy mode */
> > > > 
> > > > Why is it ok to handle only the case where both channels are set to the
> > > > same mode? The spec describes mixed-mode setups, too, and doesn't seem
> > > > to allow ignoring a mode change if it's only for one of the channels.
> > > 
> > > Certainly that can be done: only both channels were implemented initially
> > > because that was the test case immediately available using the VIA. I can
> > > have a look at implementing both channels separately in v2.
> > 
> > I don't think it would make the code more complicated, so it feels like
> > implementing it right away would be nice.
> > 
> > On the other hand, if you want to see this in 8.2, I'm happy to merge
> > this part as it is and then we can improve it on top.
> 
> I think this helps Zoltan boot AmigaOS on the new AmigaOne machine, and I am
> certainly planning more work in this area during the 9.0 cycle.
> 
> > > > > +
> > > > > +        /* 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);
> > > > 
> > > > Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
> > > > making it read-only and returning 0) while in compatibility mode doesn't
> > > > necessarily mean that the value of the register is lost. In other words,
> > > > are we sure that a driver can't expect that the old value is still there
> > > > when it re-enables native mode?
> > > 
> > > The specification is definitely a bit vague on the details here. In the
> > > testing I've done with the VIA, there is only ever a switch from native to
> > > legacy mode, and not the other way around. I can see the logic that once
> > > you've gone from the native to legacy mode, the memory allocated for the
> > > BARs is already reserved by the OS so in theory it could be possible to
> > > switch back to native mode again... and that would work if the BARs are
> > > preserved.
> > > 
> > > Would it happen in practice? I'm not really sure, but I can try to implement
> > > this if you think it makes sense to take a safer approach.
> > 
> > I'm not sure if any driver tries to do something like this. Maybe a
> > situation where BIOS switches to compatibility mode and then the OS to
> > native again? But it does feel safer.
> > 
> > The other option would be to not zero out the BAR at all. Doing that is
> > optional according to the spec. But then we need to make sure that we
> > ignore any access to the memory region behind the BAR even though its
> > address is still there.
> > 
> > Come to think of it, don't we need to somehow disable the memory regions
> > described by the BARs even when we zero them out? I think updating the
> > config space without calling pci_update_mappings() doesn't actually stop
> > QEMU from reacting to reads and writes there, does it?
> > 
> > Otherwise, the guest doesn't see the memory region in the BAR any more,
> > but it would still be active (which is almost the opposite of what we're
> > supposed to do).
> 
> At least the VIA appears to still respond to the BAR addresses even when
> switched back to compatibility mode (bug?), but without more testing after
> making the functionality more generic it's difficult to say.

Oh. So this really seems to be a device that interprets specs only as
loose guidelines... Isn't real hardware fun?

Then your implementation would do the right thing for this device, but
of course not in the general case. As the code is only used by the VIA
controller after this patch series, that's okay for me as long as it's
documented that we know that it's against the spec, but real VIA
hardware works like this - same thing as with the IRQs really.

In the long run, we need to find a way to do the proper thing in pci.c
and have the special case in via.c.

> Should the BARs
> be disabled when switched to legacy mode? According to the spec that should
> be the case, but then a switch from native -> legacy mode doesn't matter
> because the BAR addresses are already reserved, and a switch from legacy ->
> native mode would require the BARs to be (re)programmed anyway. I will
> definitely do some testing with the functionality enabled for all PCI IDE
> controllers in future, however I'm minded to keep it as simple as possible.
> 
> > > > > +        /* Clear interrupt pin */
> > > > > +        pci_config_set_interrupt_pin(d->config, 0);
> > > > 
> > > > Unlike for the BARs, I don't see anything in the spec that allows
> > > > disabling this byte. I doubt it hurts in practice, but did you see any
> > > > drivers requiring this? According to the spec, we just must not use the
> > > > PCI interrupt in compatbility mode, but the registers stay accessible.
> > > 
> > > The PCI config dumps taken from a real VIA indicate that this byte is
> > > cleared in legacy mode, and that appears to make sense here. If you imagine
> > > an early PCI IDE controller, it will always start up in legacy mode and so
> > > you don't want to indicate to the guest OS that PCI IRQ routing is required
> > > unless it has been switched to native mode first.
> > 
> > Ok. I assume that with a per-channel control, you would clear it only if
> > both channels are in compatibility mode, and set it as soon as one
> > channel is in native mode?
> 
> Yes, I believe that's correct.
> 
> > > > As far as I can see, the whole PCI interrupt configuration is currently
> > > > unused anyway, and nothing in this series seems to change it. So won't
> > > > we incorrectly continue to use the legacy interrupt even in native mode?
> > > > (Actually, cmd646 seems to get it wrong the other way around and uses
> > > > the PCI interrupt even in compatibility mode.)
> > > > 
> > > > I think this means that BMDMAState needs to have two irq lines (a legacy
> > > > and a PCI one) and select the right one in bmdma_irq() depending on
> > > > which mode we're in currently.
> > > 
> > > I need to flesh out the details a bit more (in particular testing with more
> > > than just the VIA PCI IDE controller), but yes the eventual aim is to
> > > consolidate the majority of the BMDMA and mode switching code into
> > > hw/ide/pci.c so the individual controllers don't need to worry about this,
> > > and everything "just works".
> > 
> > Zoltan's reply for patch 3 actuallys say that it's correct like this for
> > the VIA controller, in conflict with what the spec says. So it seems
> > that we can't make it "just work" for everyone, but we still need to
> > allow devices to intercept it at least.
> 
> I've replied separately to this, and as mentioned that's something that will
> likely require some future updates to the PCI IRQ routing code.
> 
> > As I said there, adding a comment in the via emulation should be enough
> > for now, and we can leave proper generic interrupt handling for another
> > day (when we want to add switching to a model that's actually consistent
> > with the spec).
> 
> Agreed.
> 
> > Maybe also leave a TODO comment at the top of this function to remind
> > other users that interrupt handling needs to be covered by individual
> > devices for now.
> 
> Yes I can definitely add a comment with a TODO. For now in terms of 8.2
> would you be happy with:
> 
> - Moving ide_portio_list[] and ide_portio_list2[] to hw/ide/core.c
> 
> - Change the configuration space access to read all zeros for BARs in compatibility
>   mode instead of zeroing the BARs
> 
> - Leave it so that both channels are switched at the same time
> 
> - Add a TODO comment to pci_ide_update_mode() indicating that individual PCI IDE
>   controllers are responsible for their own IRQ routing

Works for me, with the addition of a comment about the non-standard BAR
behaviour like I suggested above.

Kevin