[PATCH 0/2] Implement "non 100% native mode" in via-ide

[PATCH 0/2] Implement "non 100% native mode" in via-ide

[BALATON Zoltan]

This small series implements the quirky mode of via-ide found at least
on pegasos2 which is needed for guests that expect this and activate
work arounds on that platform so don't work unless this is emulated.
(Symptom is missing IDE interrupts.) We need a flag to turn this mode
on or off so the first patch repurposes the last remaining CMD646
specific field in PCIIDEState to allow more flags and make room for
the new legacy-irq flag there. (The CMD646 may need similar mode or
something else may need more flags in the future.) Boards using CMD646
and VIA IDE are updated for the above changes.

Tested with Linux and MorphOS on pegasos2 and a Gentoo live CD kernel
for mips_fulong2e that's the only one I could find but being beta not
sure if that fully works on real hardware. (The mips_fulong2e also
seems to have problems with pci devices so to boot Linux you need
-net none -vga none and use serial console otherwise the kernel panics.)

Regards,
BALATON Zoltan

BALATON Zoltan (2):
  ide: Make room for flags in PCIIDEState and add one for legacy IRQ
    routing
  via-ide: Also emulate non 100% native mode

 hw/alpha/dp264.c        |  2 +-
 hw/ide/cmd646.c         | 12 ++++-----
 hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
 hw/mips/mips_fulong2e.c |  2 +-
 hw/sparc64/sun4u.c      |  9 ++-----
 include/hw/ide.h        |  7 ++---
 include/hw/ide/pci.h    |  7 ++++-
 7 files changed, 72 insertions(+), 27 deletions(-)

-- 
2.21.1

[PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[BALATON Zoltan]

We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for furhter flags if needed in
the future. This patch changes the "secondary" field to "flags" and
define the flags for CMD646 and via-ide and change CMD646 and its
users accordingly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/alpha/dp264.c     |  2 +-
 hw/ide/cmd646.c      | 12 ++++++------
 hw/sparc64/sun4u.c   |  9 ++-------
 include/hw/ide.h     |  4 ++--
 include/hw/ide/pci.h |  7 ++++++-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 8d71a30617..e4075feaaf 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
         DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
         ide_drive_get(hd, ARRAY_SIZE(hd));
 
-        pci_cmd646_ide_init(pci_bus, hd, 0);
+        pci_cmd646_ide_init(pci_bus, hd, -1, false);
     }
 
     /* Load PALcode.  Given that this is not "real" cpu palcode,
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 335c060673..0be650791f 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     pci_conf[PCI_CLASS_PROG] = 0x8f;
 
     pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
-    if (d->secondary) {
+    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
         pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
     }
@@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
     }
 }
 
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
-                         int secondary_ide_enabled)
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                         bool secondary_ide_enabled)
 {
     PCIDevice *dev;
 
-    dev = pci_create(bus, -1, "cmd646-ide");
-    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
+    dev = pci_create(bus, devfn, "cmd646-ide");
+    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
     qdev_init_nofail(&dev->qdev);
 
     pci_ide_create_devs(dev, hd_table);
 }
 
 static Property cmd646_ide_properties[] = {
-    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index b7ac42f7a5..b64899300c 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -50,8 +50,7 @@
 #include "hw/sparc/sparc64.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
-#include "hw/ide.h"
-#include "hw/ide/pci.h"
+#include "hw/ide/internal.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
@@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     }
 
     ide_drive_get(hd, ARRAY_SIZE(hd));
-
-    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
-    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
-    qdev_init_nofail(&pci_dev->qdev);
-    pci_ide_create_devs(pci_dev, hd);
+    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
 
     /* Map NVRAM into I/O (ebus) space */
     nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 28d8a06439..d88c5ee695 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
-                         int secondary_ide_enabled);
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                         bool secondary_ide_enabled);
 PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a9f2c33e68..21075edf16 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -40,6 +40,11 @@ typedef struct BMDMAState {
 #define TYPE_PCI_IDE "pci-ide"
 #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
 
+enum {
+    PCI_IDE_SECONDARY, /* used only for cmd646 */
+    PCI_IDE_LEGACY_IRQ
+};
+
 typedef struct PCIIDEState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -47,7 +52,7 @@ typedef struct PCIIDEState {
 
     IDEBus bus[2];
     BMDMAState bmdma[2];
-    uint32_t secondary; /* used only for cmd646 */
+    uint32_t flags;
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
-- 
2.21.1

[PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

Some machines operate in "non 100% native mode" where interrupts are
fixed at legacy IDE interrupts and some guests expect this behaviour
without checking based on knowledge about hardware. Even Linux has
arch specific workarounds for this that are activated on such boards
so this needs to be emulated as well.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
 hw/mips/mips_fulong2e.c |  2 +-
 include/hw/ide.h        |  3 ++-
 3 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 096de8dba0..17418c5822 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -1,9 +1,10 @@
 /*
- * QEMU IDE Emulation: PCI VIA82C686B support.
+ * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
  *
  * Copyright (c) 2003 Fabrice Bellard
  * Copyright (c) 2006 Openedhand Ltd.
  * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com>
+ * Copyright (c) 2019-2020 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -25,6 +26,8 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/range.h"
+#include "hw/qdev-properties.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
@@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level)
     } else {
         d->config[0x70 + n * 8] &= ~0x80;
     }
-
     level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
-    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
-    if (n) {
-        qemu_set_irq(isa_get_irq(NULL, n), level);
+
+    /*
+     * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE
+     * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode.
+     * Some guest drivers expect this, often without checking.
+     */
+    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
+        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
+        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
+    } else {
+        n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
+        if (n) {
+            qemu_set_irq(isa_get_irq(NULL, n), level);
+        }
     }
 }
 
+static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len)
+{
+    /*
+     * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real
+     * hardware it's fixed at 14 and won't change. Some guests also expect
+     * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux
+     * depends on this to read 14. We set it to 14 in the reset method and
+     * also set the wmask to 0 to emulate this but that turns out to be not
+     * enough. QEMU resets the PCI bus after this device and
+     * pci_do_device_reset() called from pci_device_reset() will zero
+     * PCI_INTERRUPT_LINE so this config_read function is to counter that and
+     * restore the correct value, otherwise this should not be needed.
+     */
+    if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) {
+        pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14);
+    }
+    return pci_default_read_config(d, address, len);
+}
+
 static void via_ide_reset(DeviceState *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 
     pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
-    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
+    dev->wmask[PCI_CLASS_PROG] = 5;
+    dev->wmask[PCI_INTERRUPT_LINE] = 0;
 
     memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
                           &d->bus[0], "via-ide0-data", 8);
@@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev)
     }
 }
 
-void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                  bool legacy_irq)
 {
     PCIDevice *dev;
 
-    dev = pci_create_simple(bus, devfn, "via-ide");
+    dev = pci_create(bus, devfn, "via-ide");
+    qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq);
+    qdev_init_nofail(&dev->qdev);
     pci_ide_create_devs(dev, hd_table);
 }
 
+static Property via_ide_properties[] = {
+    DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ,
+                    false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void via_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     dc->reset = via_ide_reset;
     k->realize = via_ide_realize;
     k->exit = via_ide_exitfn;
+    k->config_read = via_ide_config_read;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;
     k->class_id = PCI_CLASS_STORAGE_IDE;
+    device_class_set_props(dc, via_ide_properties);
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 4727b1d3a4..150182c62a 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
     ide_drive_get(hd, ARRAY_SIZE(hd));
-    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
+    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/include/hw/ide.h b/include/hw/ide.h
index d88c5ee695..2a7001ccba 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
-void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+                  bool legacy_irq);
 
 /* ide-mmio.c */
 void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
-- 
2.21.1

Re: [PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[Mark Cave-Ayland]

On 29/02/2020 23:02, BALATON Zoltan wrote:

> We'll need a flag for implementing some device specific behaviour in
> via-ide but we already have a currently CMD646 specific field that can
> be repurposed for this and leave room for furhter flags if needed in
> the future. This patch changes the "secondary" field to "flags" and
> define the flags for CMD646 and via-ide and change CMD646 and its
> users accordingly.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/alpha/dp264.c     |  2 +-
>  hw/ide/cmd646.c      | 12 ++++++------
>  hw/sparc64/sun4u.c   |  9 ++-------
>  include/hw/ide.h     |  4 ++--
>  include/hw/ide/pci.h |  7 ++++++-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 8d71a30617..e4075feaaf 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>          ide_drive_get(hd, ARRAY_SIZE(hd));
>  
> -        pci_cmd646_ide_init(pci_bus, hd, 0);
> +        pci_cmd646_ide_init(pci_bus, hd, -1, false);
>      }
>  
>      /* Load PALcode.  Given that this is not "real" cpu palcode,
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 335c060673..0be650791f 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>      pci_conf[PCI_CLASS_PROG] = 0x8f;
>  
>      pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
> -    if (d->secondary) {
> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>          /* XXX: if not enabled, really disable the seconday IDE controller */
>          pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>      }
> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled)
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled)
>  {
>      PCIDevice *dev;
>  
> -    dev = pci_create(bus, -1, "cmd646-ide");
> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
> +    dev = pci_create(bus, devfn, "cmd646-ide");
> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>      qdev_init_nofail(&dev->qdev);
>  
>      pci_ide_create_devs(dev, hd_table);
>  }

Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
possible, and in fact I posted a patch last week to completely remove it. This is
because using qdev directly allows each board to wire up the device as required,
rather than pushing it down into a set of init functions with different defaults.

Given that you're working in this area, I'd highly recommend doing the same for
via_ide_init() too.

>  static Property cmd646_ide_properties[] = {
> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index b7ac42f7a5..b64899300c 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -50,8 +50,7 @@
>  #include "hw/sparc/sparc64.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> -#include "hw/ide.h"
> -#include "hw/ide/pci.h"
> +#include "hw/ide/internal.h"
>  #include "hw/loader.h"
>  #include "hw/fw-path-provider.h"
>  #include "elf.h"
> @@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      }
>  
>      ide_drive_get(hd, ARRAY_SIZE(hd));
> -
> -    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
> -    qdev_init_nofail(&pci_dev->qdev);
> -    pci_ide_create_devs(pci_dev, hd);
> +    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
>  
>      /* Map NVRAM into I/O (ebus) space */
>      nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 28d8a06439..d88c5ee695 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                          DriveInfo *hd0, DriveInfo *hd1);
>  
>  /* ide-pci.c */
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled);
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                         bool secondary_ide_enabled);
>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index a9f2c33e68..21075edf16 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -40,6 +40,11 @@ typedef struct BMDMAState {
>  #define TYPE_PCI_IDE "pci-ide"
>  #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
>  
> +enum {
> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
> +    PCI_IDE_LEGACY_IRQ
> +};
> +
>  typedef struct PCIIDEState {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -47,7 +52,7 @@ typedef struct PCIIDEState {
>  
>      IDEBus bus[2];
>      BMDMAState bmdma[2];
> -    uint32_t secondary; /* used only for cmd646 */
> +    uint32_t flags;
>      MemoryRegion bmdma_bar;
>      MemoryRegion cmd_bar[2];
>      MemoryRegion data_bar[2];

ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 29/02/2020 23:02, BALATON Zoltan wrote:

> Some machines operate in "non 100% native mode" where interrupts are
> fixed at legacy IDE interrupts and some guests expect this behaviour
> without checking based on knowledge about hardware. Even Linux has
> arch specific workarounds for this that are activated on such boards
> so this needs to be emulated as well.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
>  hw/mips/mips_fulong2e.c |  2 +-
>  include/hw/ide.h        |  3 ++-
>  3 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 096de8dba0..17418c5822 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -1,9 +1,10 @@
>  /*
> - * QEMU IDE Emulation: PCI VIA82C686B support.
> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
>   *
>   * Copyright (c) 2003 Fabrice Bellard
>   * Copyright (c) 2006 Openedhand Ltd.
>   * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com>
> + * Copyright (c) 2019-2020 BALATON Zoltan
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -25,6 +26,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/range.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/pci/pci.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>      } else {
>          d->config[0x70 + n * 8] &= ~0x80;
>      }
> -
>      level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
> -    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
> -    if (n) {
> -        qemu_set_irq(isa_get_irq(NULL, n), level);
> +
> +    /*
> +     * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE
> +     * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode.
> +     * Some guest drivers expect this, often without checking.
> +     */
> +    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
> +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
> +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
> +    } else {
> +        n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
> +        if (n) {
> +            qemu_set_irq(isa_get_irq(NULL, n), level);
> +        }
>      }
>  }
>  
> +static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len)
> +{
> +    /*
> +     * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real
> +     * hardware it's fixed at 14 and won't change. Some guests also expect
> +     * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux
> +     * depends on this to read 14. We set it to 14 in the reset method and
> +     * also set the wmask to 0 to emulate this but that turns out to be not
> +     * enough. QEMU resets the PCI bus after this device and
> +     * pci_do_device_reset() called from pci_device_reset() will zero
> +     * PCI_INTERRUPT_LINE so this config_read function is to counter that and
> +     * restore the correct value, otherwise this should not be needed.
> +     */
> +    if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) {
> +        pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14);
> +    }
> +    return pci_default_read_config(d, address, len);
> +}

This all seems quite strange: so what you're saying is that the VIA is programmed
into PCI native mode, but at least on the Pegasos then it should still be generating
interrupts on both the PCI bus and the compatibility IRQs?

>  static void via_ide_reset(DeviceState *dev)
>  {
>      PCIIDEState *d = PCI_IDE(dev);
> @@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>  
>      pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
>      pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
> -    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
> +    dev->wmask[PCI_CLASS_PROG] = 5;
> +    dev->wmask[PCI_INTERRUPT_LINE] = 0;
>  
>      memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
>                            &d->bus[0], "via-ide0-data", 8);
> @@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                  bool legacy_irq)
>  {
>      PCIDevice *dev;
>  
> -    dev = pci_create_simple(bus, devfn, "via-ide");
> +    dev = pci_create(bus, devfn, "via-ide");
> +    qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq);
> +    qdev_init_nofail(&dev->qdev);
>      pci_ide_create_devs(dev, hd_table);
>  }
>  
> +static Property via_ide_properties[] = {
> +    DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ,
> +                    false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void via_ide_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>      dc->reset = via_ide_reset;
>      k->realize = via_ide_realize;
>      k->exit = via_ide_exitfn;
> +    k->config_read = via_ide_config_read;
>      k->vendor_id = PCI_VENDOR_ID_VIA;
>      k->device_id = PCI_DEVICE_ID_VIA_IDE;
>      k->revision = 0x06;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
> +    device_class_set_props(dc, via_ide_properties);
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
>  
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 4727b1d3a4..150182c62a 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>      isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>  
>      ide_drive_get(hd, ARRAY_SIZE(hd));
> -    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
> +    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false);
>  
>      pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>      pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index d88c5ee695..2a7001ccba 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
> +                  bool legacy_irq);
>  
>  /* ide-mmio.c */
>  void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);

ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 01/03/2020 11:35, Mark Cave-Ayland wrote:
> On 29/02/2020 23:02, BALATON Zoltan wrote:
> 
>> Some machines operate in "non 100% native mode" where interrupts are
>> fixed at legacy IDE interrupts and some guests expect this behaviour
>> without checking based on knowledge about hardware. Even Linux has
>> arch specific workarounds for this that are activated on such boards
>> so this needs to be emulated as well.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
>>  hw/mips/mips_fulong2e.c |  2 +-
>>  include/hw/ide.h        |  3 ++-
>>  3 files changed, 55 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..17418c5822 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -1,9 +1,10 @@
>>  /*
>> - * QEMU IDE Emulation: PCI VIA82C686B support.
>> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
>>   *
>>   * Copyright (c) 2003 Fabrice Bellard
>>   * Copyright (c) 2006 Openedhand Ltd.
>>   * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com>
>> + * Copyright (c) 2019-2020 BALATON Zoltan
>>   *
>>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>>   * of this software and associated documentation files (the "Software"), to deal
>> @@ -25,6 +26,8 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/range.h"
>> +#include "hw/qdev-properties.h"
>>  #include "hw/pci/pci.h"
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>      } else {
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>> -
>>      level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
>> -    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>> -    if (n) {
>> -        qemu_set_irq(isa_get_irq(NULL, n), level);
>> +
>> +    /*
>> +     * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE
>> +     * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode.
>> +     * Some guest drivers expect this, often without checking.
>> +     */
>> +    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
>> +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
>> +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
>> +    } else {
>> +        n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>> +        if (n) {
>> +            qemu_set_irq(isa_get_irq(NULL, n), level);
>> +        }
>>      }
>>  }

The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever
raise a native PCI IRQ - comparing with my experience on cmd646, should there not be
a pci_set_irq(d, level) at the end?


ATB,

Mark.

Re: [PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[BALATON Zoltan]

On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 29/02/2020 23:02, BALATON Zoltan wrote:
>> We'll need a flag for implementing some device specific behaviour in
>> via-ide but we already have a currently CMD646 specific field that can
>> be repurposed for this and leave room for furhter flags if needed in
>> the future. This patch changes the "secondary" field to "flags" and
>> define the flags for CMD646 and via-ide and change CMD646 and its
>> users accordingly.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/alpha/dp264.c     |  2 +-
>>  hw/ide/cmd646.c      | 12 ++++++------
>>  hw/sparc64/sun4u.c   |  9 ++-------
>>  include/hw/ide.h     |  4 ++--
>>  include/hw/ide/pci.h |  7 ++++++-
>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> index 8d71a30617..e4075feaaf 100644
>> --- a/hw/alpha/dp264.c
>> +++ b/hw/alpha/dp264.c
>> @@ -102,7 +102,7 @@ static void clipper_init(MachineState *machine)
>>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>          ide_drive_get(hd, ARRAY_SIZE(hd));
>>
>> -        pci_cmd646_ide_init(pci_bus, hd, 0);
>> +        pci_cmd646_ide_init(pci_bus, hd, -1, false);
>>      }
>>
>>      /* Load PALcode.  Given that this is not "real" cpu palcode,
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 335c060673..0be650791f 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>>      pci_conf[PCI_CLASS_PROG] = 0x8f;
>>
>>      pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
>> -    if (d->secondary) {
>> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>>          /* XXX: if not enabled, really disable the seconday IDE controller */
>>          pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>>      }
>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>      }
>>  }
>>
>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>> -                         int secondary_ide_enabled)
>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                         bool secondary_ide_enabled)
>>  {
>>      PCIDevice *dev;
>>
>> -    dev = pci_create(bus, -1, "cmd646-ide");
>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>      qdev_init_nofail(&dev->qdev);
>>
>>      pci_ide_create_devs(dev, hd_table);
>>  }
>
> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
> possible, and in fact I posted a patch last week to completely remove it. This is
> because using qdev directly allows each board to wire up the device as required,
> rather than pushing it down into a set of init functions with different defaults.
>
> Given that you're working in this area, I'd highly recommend doing the same for
> via_ide_init() too.

I could do that, however these ide init functions seem to exist for piix, 
cmd646 and via-ide so that pci_ide_create_devs function is kept local to 
hw/ide. Nothing else called that func apart from sun4u so I've chosen this 
way to keep consistency (also keeps property type at one place instead of 
needing to change each board that sets property). If the consensus is that 
getting rid of these init funcs even if that means pci_ide_create_devs 
will not be local to ide any more I can go that way but would like to hear 
opinion of ide maintainer as well.

Regards,
BALATON Zoltan

>>  static Property cmd646_ide_properties[] = {
>> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index b7ac42f7a5..b64899300c 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -50,8 +50,7 @@
>>  #include "hw/sparc/sparc64.h"
>>  #include "hw/nvram/fw_cfg.h"
>>  #include "hw/sysbus.h"
>> -#include "hw/ide.h"
>> -#include "hw/ide/pci.h"
>> +#include "hw/ide/internal.h"
>>  #include "hw/loader.h"
>>  #include "hw/fw-path-provider.h"
>>  #include "elf.h"
>> @@ -664,11 +663,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>      }
>>
>>      ide_drive_get(hd, ARRAY_SIZE(hd));
>> -
>> -    pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
>> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
>> -    qdev_init_nofail(&pci_dev->qdev);
>> -    pci_ide_create_devs(pci_dev, hd);
>> +    pci_cmd646_ide_init(pci_busA, hd, PCI_DEVFN(3, 0), true);
>>
>>      /* Map NVRAM into I/O (ebus) space */
>>      nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 28d8a06439..d88c5ee695 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,8 +12,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>>                          DriveInfo *hd0, DriveInfo *hd1);
>>
>>  /* ide-pci.c */
>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>> -                         int secondary_ide_enabled);
>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                         bool secondary_ide_enabled);
>>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index a9f2c33e68..21075edf16 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -40,6 +40,11 @@ typedef struct BMDMAState {
>>  #define TYPE_PCI_IDE "pci-ide"
>>  #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
>>
>> +enum {
>> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
>> +    PCI_IDE_LEGACY_IRQ
>> +};
>> +
>>  typedef struct PCIIDEState {
>>      /*< private >*/
>>      PCIDevice parent_obj;
>> @@ -47,7 +52,7 @@ typedef struct PCIIDEState {
>>
>>      IDEBus bus[2];
>>      BMDMAState bmdma[2];
>> -    uint32_t secondary; /* used only for cmd646 */
>> +    uint32_t flags;
>>      MemoryRegion bmdma_bar;
>>      MemoryRegion cmd_bar[2];
>>      MemoryRegion data_bar[2];
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 29/02/2020 23:02, BALATON Zoltan wrote:
>> Some machines operate in "non 100% native mode" where interrupts are
>> fixed at legacy IDE interrupts and some guests expect this behaviour
>> without checking based on knowledge about hardware. Even Linux has
>> arch specific workarounds for this that are activated on such boards
>> so this needs to be emulated as well.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
>>  hw/mips/mips_fulong2e.c |  2 +-
>>  include/hw/ide.h        |  3 ++-
>>  3 files changed, 55 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..17418c5822 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -1,9 +1,10 @@
>>  /*
>> - * QEMU IDE Emulation: PCI VIA82C686B support.
>> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
>>   *
>>   * Copyright (c) 2003 Fabrice Bellard
>>   * Copyright (c) 2006 Openedhand Ltd.
>>   * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com>
>> + * Copyright (c) 2019-2020 BALATON Zoltan
>>   *
>>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>>   * of this software and associated documentation files (the "Software"), to deal
>> @@ -25,6 +26,8 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/range.h"
>> +#include "hw/qdev-properties.h"
>>  #include "hw/pci/pci.h"
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>      } else {
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>> -
>>      level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
>> -    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>> -    if (n) {
>> -        qemu_set_irq(isa_get_irq(NULL, n), level);
>> +
>> +    /*
>> +     * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE
>> +     * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode.
>> +     * Some guest drivers expect this, often without checking.
>> +     */
>> +    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
>> +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
>> +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
>> +    } else {
>> +        n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>> +        if (n) {
>> +            qemu_set_irq(isa_get_irq(NULL, n), level);
>> +        }
>>      }
>>  }
>>
>> +static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len)
>> +{
>> +    /*
>> +     * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real
>> +     * hardware it's fixed at 14 and won't change. Some guests also expect
>> +     * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux
>> +     * depends on this to read 14. We set it to 14 in the reset method and
>> +     * also set the wmask to 0 to emulate this but that turns out to be not
>> +     * enough. QEMU resets the PCI bus after this device and
>> +     * pci_do_device_reset() called from pci_device_reset() will zero
>> +     * PCI_INTERRUPT_LINE so this config_read function is to counter that and
>> +     * restore the correct value, otherwise this should not be needed.
>> +     */
>> +    if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) {
>> +        pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14);
>> +    }
>> +    return pci_default_read_config(d, address, len);
>> +}
>
> This all seems quite strange: so what you're saying is that the VIA is programmed
> into PCI native mode, but at least on the Pegasos then it should still be generating
> interrupts on both the PCI bus and the compatibility IRQs?

In my understanding in this "non 100% native mode" IDE interrupts are 
using only legacy interrupts 14 and 15 even in native mode and PCI 
interrupts aren't used at all. Linux has this platform specific code:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353

which says forcing legacy mode but that's not what actually happens. 
According to docs in legacy mode (which we don't emulate because we can't 
switch between legacy and native mode) apart from legacy interrupts the io 
adresses should also be fixed to legacy IDE ports but what I've seen from 
firmware and Amiga like OSes on pegasos2 they actually use the controller 
in "half-native" mode where the io addresses are set with the PCI BARs but 
the irq is still using 14 and 15. Even in true, 100% native mode a PCI 
interrupt does not seem to be used but instead the PCI_INTERRUPT_LINE 
config reg selects one of the ISA interrupts which is then used for both 
ide channels. On pegasos2 the firmware writes 9 to PCI_INTERRUPT_LINE and 
without this patch Linux detected that as 100% native mode using IRQ 9 for 
both channels and worked with that but MorphOS and AmigaOS don't check 
interrupt config reg and just expect interrupts on 14 and 15 despite using 
PCI BARs to map io and leaving the mode reg in native mode which the 
firmware also writes there. So this seems to be a kind of half-native mode 
in which PCI BARs map io registers but interrupts are locked to legacy 
interrupts and we need to emulate this for MorphOS and AmigaOS at least. 
Linux could work both ways if regs are set accordingly but when it says 
"forcing legacy mode" in above fixup func that just seems to activate 
other compatibility funcs in

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/ide/setup-pci.c?h=v4.14.172#n484

to know that legacy interrupts should be used instead of the one in config 
reg but PCI BARs are still mapped and io is accessed at those addresses 
which would not make sense in true legacy mode.

I've also got register dumps from a real Pegasos2 which shows 
PCI_INTERRUPT_LINE set to 14 after booting Linux even if firmware tries to 
change that to 9 so I think it's fixed on real hardware and guests work if 
we emulate that in this patch so I think this confirms this matches real 
hardware at least more or less.

This patch fixes interrupt line to 14 but that's not a problem at least 
for Linux or anything that checks the config reg contents, the only 
difference is that without legacy-irq set it will use IRQ 14 for both IDE 
channels that seems to work with the mips_fulong2e which is the only other 
board using via-ide.

All of the above is true for via-ide and the two boards we emulate, I 
don't know if this applies to CMD646 or that conroller and boards using it 
have different quirks. Also via-ide is part of a southbridge chip (or 
several similar southbridges) where connetctions within the chip to IRQs 
may be peculiar while CMD646 may be a more normal PCI device.

Regards,
BALATON Zoltan

>
>>  static void via_ide_reset(DeviceState *dev)
>>  {
>>      PCIIDEState *d = PCI_IDE(dev);
>> @@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>
>>      pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
>>      pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
>> -    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
>> +    dev->wmask[PCI_CLASS_PROG] = 5;
>> +    dev->wmask[PCI_INTERRUPT_LINE] = 0;
>>
>>      memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
>>                            &d->bus[0], "via-ide0-data", 8);
>> @@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev)
>>      }
>>  }
>>
>> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
>> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                  bool legacy_irq)
>>  {
>>      PCIDevice *dev;
>>
>> -    dev = pci_create_simple(bus, devfn, "via-ide");
>> +    dev = pci_create(bus, devfn, "via-ide");
>> +    qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq);
>> +    qdev_init_nofail(&dev->qdev);
>>      pci_ide_create_devs(dev, hd_table);
>>  }
>>
>> +static Property via_ide_properties[] = {
>> +    DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ,
>> +                    false),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void via_ide_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>      dc->reset = via_ide_reset;
>>      k->realize = via_ide_realize;
>>      k->exit = via_ide_exitfn;
>> +    k->config_read = via_ide_config_read;
>>      k->vendor_id = PCI_VENDOR_ID_VIA;
>>      k->device_id = PCI_DEVICE_ID_VIA_IDE;
>>      k->revision = 0x06;
>>      k->class_id = PCI_CLASS_STORAGE_IDE;
>> +    device_class_set_props(dc, via_ide_properties);
>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>  }
>>
>> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
>> index 4727b1d3a4..150182c62a 100644
>> --- a/hw/mips/mips_fulong2e.c
>> +++ b/hw/mips/mips_fulong2e.c
>> @@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>>      isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>>
>>      ide_drive_get(hd, ARRAY_SIZE(hd));
>> -    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
>> +    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false);
>>
>>      pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>>      pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index d88c5ee695..2a7001ccba 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>> +                  bool legacy_irq);
>>
>>  /* ide-mmio.c */
>>  void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 01/03/2020 11:35, Mark Cave-Ayland wrote:
>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>
>>> Some machines operate in "non 100% native mode" where interrupts are
>>> fixed at legacy IDE interrupts and some guests expect this behaviour
>>> without checking based on knowledge about hardware. Even Linux has
>>> arch specific workarounds for this that are activated on such boards
>>> so this needs to be emulated as well.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c            | 60 +++++++++++++++++++++++++++++++++++------
>>>  hw/mips/mips_fulong2e.c |  2 +-
>>>  include/hw/ide.h        |  3 ++-
>>>  3 files changed, 55 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 096de8dba0..17418c5822 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -1,9 +1,10 @@
>>>  /*
>>> - * QEMU IDE Emulation: PCI VIA82C686B support.
>>> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
>>>   *
>>>   * Copyright (c) 2003 Fabrice Bellard
>>>   * Copyright (c) 2006 Openedhand Ltd.
>>>   * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com>
>>> + * Copyright (c) 2019-2020 BALATON Zoltan
>>>   *
>>>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>   * of this software and associated documentation files (the "Software"), to deal
>>> @@ -25,6 +26,8 @@
>>>   */
>>>
>>>  #include "qemu/osdep.h"
>>> +#include "qemu/range.h"
>>> +#include "hw/qdev-properties.h"
>>>  #include "hw/pci/pci.h"
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/module.h"
>>> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>>      } else {
>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>      }
>>> -
>>>      level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
>>> -    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>>> -    if (n) {
>>> -        qemu_set_irq(isa_get_irq(NULL, n), level);
>>> +
>>> +    /*
>>> +     * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE
>>> +     * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode.
>>> +     * Some guest drivers expect this, often without checking.
>>> +     */
>>> +    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
>>> +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
>>> +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
>>> +    } else {
>>> +        n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>>> +        if (n) {
>>> +            qemu_set_irq(isa_get_irq(NULL, n), level);
>>> +        }
>>>      }
>>>  }
>
> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever
> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be
> a pci_set_irq(d, level) at the end?

According to my tests with several guests it seems the via-ide does not 
seem to use PCI interrupts as described in the previous reply, only either 
legacy IRQ14 and 15 or one ISA IRQ line set by a config reg in native mode 
(except on Pegasos2). This may be due to how it's internally connected in 
the southbridge chip it's part of or some other platform specific quirk, 
I'm not sure.

The CMD646 may be a more conformant PCI device and use PCI interrupts 
(unless that also has some kind of legacy mode and similar interconnection 
with ISA interrupts). If it's really using PCI interrupt and Solaris still 
does not get expexted IRQ then maybe the routing is not matching real 
hardware somehow? Maybe it would help finding out what interrupt the 
Solaris driver is checking to find out why it misses the IRQ.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 01/03/2020 16:42, BALATON Zoltan wrote:

>> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever
>> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be
>> a pci_set_irq(d, level) at the end?
> 
> According to my tests with several guests it seems the via-ide does not seem to use
> PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or
> one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be
> due to how it's internally connected in the southbridge chip it's part of or some
> other platform specific quirk, I'm not sure.

I think this is the key part here: how does via-ide switch between legacy and native
mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd
expect to see something similar here.

It might be that the BIOS sets legacy mode on startup, and unless the OS explicitly
switches to native mode then the interrupt routing remains at IRQ 14/15 (or whatever
value is in PCI_INTERRUPT_LINE). Is there a datasheet available for the VIA chip to
check this?


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 01/03/2020 16:42, BALATON Zoltan wrote:
>
>>> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever
>>> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be
>>> a pci_set_irq(d, level) at the end?
>>
>> According to my tests with several guests it seems the via-ide does not seem to use
>> PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or
>> one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be
>> due to how it's internally connected in the southbridge chip it's part of or some
>> other platform specific quirk, I'm not sure.
>
> I think this is the key part here: how does via-ide switch between legacy and native
> mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd
> expect to see something similar here.
>
> It might be that the BIOS sets legacy mode on startup, and unless the OS explicitly
> switches to native mode then the interrupt routing remains at IRQ 14/15 (or whatever
> value is in PCI_INTERRUPT_LINE). Is there a datasheet available for the VIA chip to
> check this?

Yes, searching for datasheets for the southbridge chips containing this 
IDE controller such as VT82C686B and VT8231 might turn up something but 
the docs are not too detailed regarding IRQ mapping. Switching between 
modes is done the same way as on CMD646 via bits of the PROG_IF config reg 
but as it's also noted in Linux fixup func the firmware on pegasos2 does 
set it up as native mode and also writes to the interrupt line reg to set 
it to 9, yet it seems to remain set to 14 (this is confirmed by reg dump 
from Linux on real hardware) and IRQs are expected on 14 and 15 as in 
legacy mode (shown by expectation of Amiga like OSes, regardless of what's 
in config regs). Docs say that in legacy mode io addresses should be 
legacy io ports assigned to IDE, yet firmware and OSes use PCI BARs as in 
native mode but expect interrupts on legacy IRQ as in legacy mode so I 
think it's kind of half-native mode.

This is what the log shows on real PegasosII (using Debian 5 because later 
versions seems to have problems with radeon driver so have no display):

[    0.000000] Linux version 2.6.26-2-powerpc (Debian 2.6.26-29) (dannf@debian.org) (gcc version 4.1.3 20080704 (prerelease) (Debian 4.1.2-25)) #1 Sun Mar 4 06:19:00 UTC 2012

[    0.000000] chrp type = 6 [Genesi Pegasos]

[    1.562960] Fixing VIA IDE, force legacy mode on '0000:00:0c.1'

[ 7.203599] VP_IDE: IDE controller (0x1106:0x0571 rev 0x06) at PCI slot 0000:00:0c.1
[ 7.211068] VP_IDE: not 100% native mode: will probe irqs later
[ 7.218300] VP_IDE: VIA vt8231 (rev 10) IDE UDMA100 controller on pci0000:00:0c.1
[ 7.225534] ide0: BM-DMA at 0x1060-0x1067
[ 7.232794] ide1: BM-DMA at 0x1068-0x106f
[ 7.239927] Probing IDE interface ide0...
[here detects devices attached to ide channels]

[    9.013151] ide0 at 0x1040-0x1047,0x104e on irq 14
[    9.020384] ide1 at 0x1050-0x1057,0x105e on irq 15

lspci says:

0000:00:0c.1 IDE interface: VIA Technologies, Inc. VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master SecP PriP])
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
         Latency: 0
         Interrupt: pin ? routed to IRQ 14
         Region 0: [virtual] I/O ports at 1040 [size=8]
         Region 1: [virtual] I/O ports at 104c [size=4]
         Region 2: [virtual] I/O ports at 1050 [size=8]
         Region 3: [virtual] I/O ports at 105c [size=4]
         Region 4: I/O ports at 1060 [size=16]
         Capabilities: [c0] Power Management version 2
                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
         Kernel driver in use: VIA_IDE
         Kernel modules: via82cxxx

If this was legacy mode io ports should not be what seem to be coming from 
PCI BARs so either docs are not correct about how legacy mode works or 
this is not legacy mode but "not 100% native mode". The prog-if is set to 
0x8a which corresponds to native mode but this is what the Linux fixup 
function does, firmware sets it to 0x8f which means native mode.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sun, 1 Mar 2020, BALATON Zoltan wrote:
> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a 
> which corresponds to native mode but this is what the Linux fixup function 
> does, firmware sets it to 0x8f which means native mode.

I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f 
and Amiga like OSes reading that yet expecting legacy interrupts. Linux 
fixes up prog-if so its driver detects legacy interrupts but still uses 
ioports from PCI BARs.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 01/03/2020 18:53, BALATON Zoltan wrote:

> On Sun, 1 Mar 2020, BALATON Zoltan wrote:
>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which
>> corresponds to native mode but this is what the Linux fixup function does, firmware
>> sets it to 0x8f which means native mode.
> 
> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga
> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its
> driver detects legacy interrupts but still uses ioports from PCI BARs.

I see. Note that it is also possible to have a prog-if value of 0x80 which is where
the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the
case for Pegasos, since it would explain why attempts to switch the mode via prog-if
are ignored?

I don't see the PCI BARs being a problem since native drivers wouldn't touch the
memory/IO space enable bits, and the BARs are disabled by default. It could just be
that the VIA chipset simply doesn't lock the PCI memory/IO space bits in
compatibility mode if an OS does decide to use them and program the BARs as it would
in native mode.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
> On 01/03/2020 18:53, BALATON Zoltan wrote:
>> On Sun, 1 Mar 2020, BALATON Zoltan wrote:
>>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which
>>> corresponds to native mode but this is what the Linux fixup function does, firmware
>>> sets it to 0x8f which means native mode.
>>
>> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga
>> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its
>> driver detects legacy interrupts but still uses ioports from PCI BARs.
>
> I see. Note that it is also possible to have a prog-if value of 0x80 which is where
> the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the
> case for Pegasos, since it would explain why attempts to switch the mode via prog-if
> are ignored?

I've seen such option in CMD646 docs but couldn't find similar in VT8231. 
Genesi has published the schematics of Pegasos II (linked from my 
https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could 
check if you can tell which pin is that. But we get 0x8a in Linux lspci 
output on real hardware for prog-if which is explained by firmare setting 
it to 0x8f then Linux fixup function clearing bits 0 and 2 so does not 
seem it started as 0x80 because then firmware should not be able to set it 
to 0x8f either.

> I don't see the PCI BARs being a problem since native drivers wouldn't touch the
> memory/IO space enable bits, and the BARs are disabled by default. It could just be
> that the VIA chipset simply doesn't lock the PCI memory/IO space bits in
> compatibility mode if an OS does decide to use them and program the BARs as it would
> in native mode.

I think you mean legacy drivers should not touch BARs. That could also be 
a way (the docs do say that default values for BARs match legacy ports so 
it's possible that setting legacy mode resets these and uses them as they 
were enabled but does not prevent changes) but I don't see how could we 
implement that in QEMU because we either have legacy ports or PCI IDE in 
QEMU and BARs are handled by PCI code so to keep those enabled for legacy 
emulation even if they would be disabled for PCI would need some change in 
PCI code that's probably not a good idea to touch as a lot of things 
depend on that. This patch I've come up with is confined to PCI IDE code 
and the end result is the same for at least the boards and clients we care 
about so I'd go with this for now.

Regards,
BALATON Zoltan

Re: [PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>> We'll need a flag for implementing some device specific behaviour in
>>> via-ide but we already have a currently CMD646 specific field that can
>>> be repurposed for this and leave room for furhter flags if needed in
>>> the future. This patch changes the "secondary" field to "flags" and
>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>> users accordingly.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/alpha/dp264.c     |  2 +-
>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>  include/hw/ide.h     |  4 ++--
>>>  include/hw/ide/pci.h |  7 ++++++-
>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
[...]
>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>      }
>>>  }
>>>
>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>> -                         int secondary_ide_enabled)
>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>> +                         bool secondary_ide_enabled)
>>>  {
>>>      PCIDevice *dev;
>>>
>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>      qdev_init_nofail(&dev->qdev);
>>>
>>>      pci_ide_create_devs(dev, hd_table);
>>>  }
>>
>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>> possible, and in fact I posted a patch last week to completely remove it. This is
>> because using qdev directly allows each board to wire up the device as required,
>> rather than pushing it down into a set of init functions with different defaults.
>>
>> Given that you're working in this area, I'd highly recommend doing the same for
>> via_ide_init() too.
>
> I could do that, however these ide init functions seem to exist for
> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
> local to hw/ide. Nothing else called that func apart from sun4u so
> I've chosen this way to keep consistency (also keeps property type at
> one place instead of needing to change each board that sets
> property). If the consensus is that getting rid of these init funcs
> even if that means pci_ide_create_devs will not be local to ide any
> more I can go that way but would like to hear opinion of ide
> maintainer as well.

I think Mark's point is that modelling a device and wiring up a device
model are separate things, and the latter belongs to the board.

pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
elsewhere.

In the oldest stratum of qdev code, such helpers were static inline
functions in the device model's .h.  That way, they're kind of separate
from the device model proper, in the .c, and kind of in the board code
where they belong, via inlining.  I've always considered that a terrible
idea; it's "kind of" as in "not really".  Over time, practice moved
first to putting the helpers into .c, then to open-coding the wiring
where it belongs: in the boards.

A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
and the IDE helpers we're discussing here.

Of course, when the code to wire up certain devices gets overly
repetitive, factoring out common code into helpers can make sense.  But
where to put them?  I can't see an obvious home for common board
helpers.  We tend to put these wiring helpers into a device model's .c
code for want of a better place.  Tolerable, I think.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 01/03/2020 21:30, BALATON Zoltan wrote:

> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>> On 01/03/2020 18:53, BALATON Zoltan wrote:
>>> On Sun, 1 Mar 2020, BALATON Zoltan wrote:
>>>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which
>>>> corresponds to native mode but this is what the Linux fixup function does, firmware
>>>> sets it to 0x8f which means native mode.
>>>
>>> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga
>>> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its
>>> driver detects legacy interrupts but still uses ioports from PCI BARs.
>>
>> I see. Note that it is also possible to have a prog-if value of 0x80 which is where
>> the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the
>> case for Pegasos, since it would explain why attempts to switch the mode via prog-if
>> are ignored?
> 
> I've seen such option in CMD646 docs but couldn't find similar in VT8231. Genesi has
> published the schematics of Pegasos II (linked from my
> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could check if
> you can tell which pin is that. But we get 0x8a in Linux lspci output on real
> hardware for prog-if which is explained by firmare setting it to 0x8f then Linux
> fixup function clearing bits 0 and 2 so does not seem it started as 0x80 because then
> firmware should not be able to set it to 0x8f either.

I had a quick look at the schematics linked from the page above, and they confirm
that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI
interrupt pins. So on that basis the best explanation as to what is happening is the
comment in the link you provided here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353

/* Pegasos2 firmware version 20040810 configures the built-in IDE controller
 * in legacy mode, but sets the PCI registers to PCI native mode.
 * The chip can only operate in legacy mode, so force the PCI class into legacy
 * mode as well. The same fixup must be done to the class-code property in
 * the IDE node /pci@80000000/ide@C,1
 */

Given that the DT is wrong, then we should assume that all OSs would have to
compensate for this in the same way as Linux, and therefore this should be handled
automatically.

AFAICT this then only leaves the question: why does the firmware set
PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
MorphOS under QEMU.

Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE
to 14? You should be able to confirm this easily on real hardware using the Forth
config-* words on the IDE node and reading the prog-if byte before and after.


ATB,

Mark.

Re: [PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[Mark Cave-Ayland]

On 02/03/2020 08:10, Markus Armbruster wrote:

> BALATON Zoltan <balaton@eik.bme.hu> writes:
> 
>> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>>> We'll need a flag for implementing some device specific behaviour in
>>>> via-ide but we already have a currently CMD646 specific field that can
>>>> be repurposed for this and leave room for furhter flags if needed in
>>>> the future. This patch changes the "secondary" field to "flags" and
>>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>>> users accordingly.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/alpha/dp264.c     |  2 +-
>>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>>  include/hw/ide.h     |  4 ++--
>>>>  include/hw/ide/pci.h |  7 ++++++-
>>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> [...]
>>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>>      }
>>>>  }
>>>>
>>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>>> -                         int secondary_ide_enabled)
>>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>>> +                         bool secondary_ide_enabled)
>>>>  {
>>>>      PCIDevice *dev;
>>>>
>>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>      qdev_init_nofail(&dev->qdev);
>>>>
>>>>      pci_ide_create_devs(dev, hd_table);
>>>>  }
>>>
>>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>>> possible, and in fact I posted a patch last week to completely remove it. This is
>>> because using qdev directly allows each board to wire up the device as required,
>>> rather than pushing it down into a set of init functions with different defaults.
>>>
>>> Given that you're working in this area, I'd highly recommend doing the same for
>>> via_ide_init() too.
>>
>> I could do that, however these ide init functions seem to exist for
>> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
>> local to hw/ide. Nothing else called that func apart from sun4u so
>> I've chosen this way to keep consistency (also keeps property type at
>> one place instead of needing to change each board that sets
>> property). If the consensus is that getting rid of these init funcs
>> even if that means pci_ide_create_devs will not be local to ide any
>> more I can go that way but would like to hear opinion of ide
>> maintainer as well.
> 
> I think Mark's point is that modelling a device and wiring up a device
> model are separate things, and the latter belongs to the board.
> 
> pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
> elsewhere.
> 
> In the oldest stratum of qdev code, such helpers were static inline
> functions in the device model's .h.  That way, they're kind of separate
> from the device model proper, in the .c, and kind of in the board code
> where they belong, via inlining.  I've always considered that a terrible
> idea; it's "kind of" as in "not really".  Over time, practice moved
> first to putting the helpers into .c, then to open-coding the wiring
> where it belongs: in the boards.
> 
> A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
> and the IDE helpers we're discussing here.
> 
> Of course, when the code to wire up certain devices gets overly
> repetitive, factoring out common code into helpers can make sense.  But
> where to put them?  I can't see an obvious home for common board
> helpers.  We tend to put these wiring helpers into a device model's .c
> code for want of a better place.  Tolerable, I think.

Right, thanks for the more detailed explanation of what I was trying to say above.

As you say having helpers can definitely help avoid repetitive code, however there
was a case a few releases back when someone flipped a qdev property in a device
_init() helper function used to initialise one of the more common devices and it
broke several of the older machines.

So now I'm mainly of the opinion that if the helper is just instantiating a device,
setting qdev properties and then returning the device then you're better off moving
the initialisation into the board code to prevent problems like this occurring again
(and certainly this nudges us towards building machines from config files since all
the configuration/wiring is now done at board level).


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Mon, 2 Mar 2020, Mark Cave-Ayland wrote:
> On 01/03/2020 21:30, BALATON Zoltan wrote:
>> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>>> On 01/03/2020 18:53, BALATON Zoltan wrote:
>>>> On Sun, 1 Mar 2020, BALATON Zoltan wrote:
>>>>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which
>>>>> corresponds to native mode but this is what the Linux fixup function does, firmware
>>>>> sets it to 0x8f which means native mode.
>>>>
>>>> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga
>>>> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its
>>>> driver detects legacy interrupts but still uses ioports from PCI BARs.
>>>
>>> I see. Note that it is also possible to have a prog-if value of 0x80 which is where
>>> the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the
>>> case for Pegasos, since it would explain why attempts to switch the mode via prog-if
>>> are ignored?
>>
>> I've seen such option in CMD646 docs but couldn't find similar in VT8231. Genesi has
>> published the schematics of Pegasos II (linked from my
>> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could check if
>> you can tell which pin is that. But we get 0x8a in Linux lspci output on real
>> hardware for prog-if which is explained by firmare setting it to 0x8f then Linux
>> fixup function clearing bits 0 and 2 so does not seem it started as 0x80 because then
>> firmware should not be able to set it to 0x8f either.
>
> I had a quick look at the schematics linked from the page above, and they confirm
> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI
> interrupt pins.

Where did you see that? What I got from trying to look this up in the 
schematics was that VT8231 has two pins named IRQ14 and IRQ15 (but 
described as Primary and Secondary Channel Interrupt Request in doc) where 
the interrupt lines of the two IDE ports/channels are connected but how 
they are routed within the chip after that was not clear. The chip doc 
says that in native mode the interrupt should be configurable and use a 
single interrupt for both channels and in legacy mode they use the usual 
14 and 15 but this is not what guest drivers expect so I think not how 
really works on PegasosII. It is true however that connection to PCI 
interrupts aren't mentioned so it always uses ISA IRQ numbers, it just 
depends on legacy vs. native mode which line is raised. But that was never 
really a question for VT8231 and maybe only CMD646 could have such 
interconnection with PCI interrupts. (Proabable reason is that via-ide is 
part of a southbridge chip where it has connections to ISA bus while 
CMD646 is a PCI IDE controller but I could be wrong as my knowledge is 
limited about these.)

> So on that basis the best explanation as to what is happening is the
> comment in the link you provided here:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>
> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
> * in legacy mode, but sets the PCI registers to PCI native mode.
> * The chip can only operate in legacy mode, so force the PCI class into legacy
> * mode as well. The same fixup must be done to the class-code property in
> * the IDE node /pci@80000000/ide@C,1
> */

I'm not sure that it makes much sense that the firmware configures the 
chip one way then puts info about a different way in the device tree. 
There could be bugs but this is not likely. Especially because I see in 
traces that the firmware does try to configure the device in native mode. 
These are the first few accesses of the firmware to via-ide:

pci_cfg_write via-ide 12:1 @0x9 <- 0xf
pci_cfg_write via-ide 12:1 @0x40 <- 0xb
pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
pci_cfg_write via-ide 12:1 @0x43 <- 0x35
pci_cfg_write via-ide 12:1 @0x44 <- 0x18
pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
pci_cfg_write via-ide 12:1 @0x54 <- 0x14
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
pci_cfg_read via-ide 12:1 @0xc -> 0x0
pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
pci_cfg_write via-ide 12:1 @0x3c <- 0x109

The very first write is to turn on native mode, so I think it's not about 
what the firmware does but something about how hardware is wired on 
Pegasos II or the VT8231 chip itself that only allows legacy interrupts 
instead of 100% native mode for IDE.

> Given that the DT is wrong, then we should assume that all OSs would have to
> compensate for this in the same way as Linux, and therefore this should be handled
> automatically.
>
> AFAICT this then only leaves the question: why does the firmware set
> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
> MorphOS under QEMU.

Linux does try to handle both true native mode and half-native mode. It 
only uses half-native mode if finds IRQ14 on Pegasos, otherwise skips 
Pegasos specific fixup and uses true native mode setup. I don't know what 
MorphOS does but I think it justs knows that Pegasos2 has this quirk and 
does not look at the device tree at all.

> Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE
> to 14? You should be able to confirm this easily on real hardware using the Forth
> config-* words on the IDE node and reading the prog-if byte before and after.

I don't have direct access to real hardware and would also need to come up 
with some Forth to verify that but given the above trace that the firmware 
does before we can enter any Forth we would likely find @0x9 = 0x8f and 
@0x3c = 0x0e because after booting Linux we see 0x8a and 0x0e and Linux 
only touches the two mode bits.

So I don't know the ultimate reason why it works that way but I'm quite 
sure that this emulation is close enough and guests are happy with it too.

Regards,
BALATON Zoltan

Re: [PATCH 1/2] ide: Make room for flags in PCIIDEState and add one for legacy IRQ routing

[BALATON Zoltan]

On Mon, 2 Mar 2020, Mark Cave-Ayland wrote:
> On 02/03/2020 08:10, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 29/02/2020 23:02, BALATON Zoltan wrote:
>>>>> We'll need a flag for implementing some device specific behaviour in
>>>>> via-ide but we already have a currently CMD646 specific field that can
>>>>> be repurposed for this and leave room for furhter flags if needed in
>>>>> the future. This patch changes the "secondary" field to "flags" and
>>>>> define the flags for CMD646 and via-ide and change CMD646 and its
>>>>> users accordingly.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/alpha/dp264.c     |  2 +-
>>>>>  hw/ide/cmd646.c      | 12 ++++++------
>>>>>  hw/sparc64/sun4u.c   |  9 ++-------
>>>>>  include/hw/ide.h     |  4 ++--
>>>>>  include/hw/ide/pci.h |  7 ++++++-
>>>>>  5 files changed, 17 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> [...]
>>>>> @@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>>>>      }
>>>>>  }
>>>>>
>>>>> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>>>>> -                         int secondary_ide_enabled)
>>>>> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
>>>>> +                         bool secondary_ide_enabled)
>>>>>  {
>>>>>      PCIDevice *dev;
>>>>>
>>>>> -    dev = pci_create(bus, -1, "cmd646-ide");
>>>>> -    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>> +    dev = pci_create(bus, devfn, "cmd646-ide");
>>>>> +    qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
>>>>>      qdev_init_nofail(&dev->qdev);
>>>>>
>>>>>      pci_ide_create_devs(dev, hd_table);
>>>>>  }
>>>>
>>>> Note that legacy init functions such as pci_cmd646_ide_init() should be removed where
>>>> possible, and in fact I posted a patch last week to completely remove it. This is
>>>> because using qdev directly allows each board to wire up the device as required,
>>>> rather than pushing it down into a set of init functions with different defaults.
>>>>
>>>> Given that you're working in this area, I'd highly recommend doing the same for
>>>> via_ide_init() too.
>>>
>>> I could do that, however these ide init functions seem to exist for
>>> piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
>>> local to hw/ide. Nothing else called that func apart from sun4u so
>>> I've chosen this way to keep consistency (also keeps property type at
>>> one place instead of needing to change each board that sets
>>> property). If the consensus is that getting rid of these init funcs
>>> even if that means pci_ide_create_devs will not be local to ide any
>>> more I can go that way but would like to hear opinion of ide
>>> maintainer as well.
>>
>> I think Mark's point is that modelling a device and wiring up a device
>> model are separate things, and the latter belongs to the board.
>>
>> pci_cmd646_ide_init() is a helper for boards.  Similar helpers exist
>> elsewhere.
>>
>> In the oldest stratum of qdev code, such helpers were static inline
>> functions in the device model's .h.  That way, they're kind of separate
>> from the device model proper, in the .c, and kind of in the board code
>> where they belong, via inlining.  I've always considered that a terrible
>> idea; it's "kind of" as in "not really".  Over time, practice moved
>> first to putting the helpers into .c, then to open-coding the wiring
>> where it belongs: in the boards.
>>
>> A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
>> and the IDE helpers we're discussing here.
>>
>> Of course, when the code to wire up certain devices gets overly
>> repetitive, factoring out common code into helpers can make sense.  But
>> where to put them?  I can't see an obvious home for common board
>> helpers.  We tend to put these wiring helpers into a device model's .c
>> code for want of a better place.  Tolerable, I think.
>
> Right, thanks for the more detailed explanation of what I was trying to say above.
>
> As you say having helpers can definitely help avoid repetitive code, however there
> was a case a few releases back when someone flipped a qdev property in a device
> _init() helper function used to initialise one of the more common devices and it
> broke several of the older machines.
>
> So now I'm mainly of the opinion that if the helper is just instantiating a device,
> setting qdev properties and then returning the device then you're better off moving
> the initialisation into the board code to prevent problems like this occurring again
> (and certainly this nudges us towards building machines from config files since all
> the configuration/wiring is now done at board level).

The conflicting interests here seem to be

1. Keeping pci_ide_create_devs() local to hw/ide
2. keeping things related to the board in board code and getting rid of 
init functions

I can't decide which of the above is more important or nicer but the patch 
I've proposed at least kept consistency with existing practice. If it's 
decided that board code should use pci_ide_create_devs directly instead we 
could either address that in a separate clean up series also getting rid 
of the piix init functions at the same time later or if Mark's series gets 
merged first I can rebase on that.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 02/03/2020 21:40, BALATON Zoltan wrote:

>> I had a quick look at the schematics linked from the page above, and they confirm
>> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI
>> interrupt pins.
> 
> Where did you see that? What I got from trying to look this up in the schematics was
> that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and
> Secondary Channel Interrupt Request in doc) where the interrupt lines of the two IDE
> ports/channels are connected but how they are routed within the chip after that was
> not clear. The chip doc says that in native mode the interrupt should be configurable
> and use a single interrupt for both channels and in legacy mode they use the usual 14
> and 15 but this is not what guest drivers expect so I think not how really works on
> PegasosII. It is true however that connection to PCI interrupts aren't mentioned so
> it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which line
> is raised. But that was never really a question for VT8231 and maybe only CMD646
> could have such interconnection with PCI interrupts. (Proabable reason is that
> via-ide is part of a southbridge chip where it has connections to ISA bus while
> CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited about
> these.)

Presumably the VIA southbridge has its own internal pair of cascaded 8259s so the IRQ
line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can see
this in the "Common Hardware Reference Platform: I/O Device Reference" PDF section 4.1.

>> So on that basis the best explanation as to what is happening is the
>> comment in the link you provided here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>>
>>
>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
>> * in legacy mode, but sets the PCI registers to PCI native mode.
>> * The chip can only operate in legacy mode, so force the PCI class into legacy
>> * mode as well. The same fixup must be done to the class-code property in
>> * the IDE node /pci@80000000/ide@C,1
>> */
> 
> I'm not sure that it makes much sense that the firmware configures the chip one way
> then puts info about a different way in the device tree. There could be bugs but this
> is not likely. Especially because I see in traces that the firmware does try to
> configure the device in native mode. These are the first few accesses of the firmware
> to via-ide:

But that is exactly what is happening! The comment above clearly indicates the
firmware incorrectly sets the IDE controller in native mode which is in exact
agreement with the trace you provide below. And in fact if you look at
https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc
hack that was released in order to fix the device tree to boot Linux which alters the
class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good
enough here).

> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
> pci_cfg_read via-ide 12:1 @0xc -> 0x0
> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
> 
> The very first write is to turn on native mode, so I think it's not about what the
> firmware does but something about how hardware is wired on Pegasos II or the VT8231
> chip itself that only allows legacy interrupts instead of 100% native mode for IDE.
> 
>> Given that the DT is wrong, then we should assume that all OSs would have to
>> compensate for this in the same way as Linux, and therefore this should be handled
>> automatically.
>>
>> AFAICT this then only leaves the question: why does the firmware set
>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>> MorphOS under QEMU.
> 
> Linux does try to handle both true native mode and half-native mode. It only uses
> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
> and uses true native mode setup. I don't know what MorphOS does but I think it justs
> knows that Pegasos2 has this quirk and does not look at the device tree at all.
> 
>> Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE
>> to 14? You should be able to confirm this easily on real hardware using the Forth
>> config-* words on the IDE node and reading the prog-if byte before and after.
> 
> I don't have direct access to real hardware and would also need to come up with some
> Forth to verify that but given the above trace that the firmware does before we can
> enter any Forth we would likely find @0x9 = 0x8f and @0x3c = 0x0e because after
> booting Linux we see 0x8a and 0x0e and Linux only touches the two mode bits.

Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a
standard chip, so let's try and figure out exactly what is happening using a real
firmware and emulate that behaviour in QEMU. This should then make all guests happy,
regardless of architecture, without requiring the introduction of feature bits or
risk of introducing other incompatibilities.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Tue, 3 Mar 2020, Mark Cave-Ayland wrote:
> On 02/03/2020 21:40, BALATON Zoltan wrote:
>>> I had a quick look at the schematics linked from the page above, and they confirm
>>> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI
>>> interrupt pins.
>>
>> Where did you see that? What I got from trying to look this up in the schematics was
>> that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and
>> Secondary Channel Interrupt Request in doc) where the interrupt lines of the two IDE
>> ports/channels are connected but how they are routed within the chip after that was
>> not clear. The chip doc says that in native mode the interrupt should be configurable
>> and use a single interrupt for both channels and in legacy mode they use the usual 14
>> and 15 but this is not what guest drivers expect so I think not how really works on
>> PegasosII. It is true however that connection to PCI interrupts aren't mentioned so
>> it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which line
>> is raised. But that was never really a question for VT8231 and maybe only CMD646
>> could have such interconnection with PCI interrupts. (Proabable reason is that
>> via-ide is part of a southbridge chip where it has connections to ISA bus while
>> CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited about
>> these.)
>
> Presumably the VIA southbridge has its own internal pair of cascaded 8259s so the IRQ
> line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can see
> this in the "Common Hardware Reference Platform: I/O Device Reference" PDF section 4.1.

Yes the VIA southbridge chip integrates all usual PC devices including 
PICs so these may have connection internally. I haven't checked these docs 
before but now I think another chapter in the doc you referenced and its 
companion "Common Hardware Reference Platform: A System Architecture" may 
explain the unusal combination of PCI like io regs with legacy interrupts 
the Pegasos2 seems to have. In the I/O Device Reference, chapter 11 about 
IDE says that the device must be addressed only with PCI I/O addresses 
where io addresses are completely relocatable and that when OpenFirmware 
passes control to the OS IDE must be configured as a PCI device. So this 
probably means io addresses coming from PCI BARs. But the CHRP System 
Architecture, chapter 9.2 about ISA devices says that "ISA devices 
included in a PCI part must route their interrupt signals to the legacy 
interrupt controller" and this is what the Pegasos2 seems to do. This may 
be a misinterpretation of the spec because elsewhere (I've lost the exact 
reference and have no time to find it again) it also says something about 
native devices must be using OpenPIC but Pegasos2 does not have OpenPIC so 
that part surely cannot apply anyway.

>>> So on that basis the best explanation as to what is happening is the
>>> comment in the link you provided here:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>>>
>>>
>>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
>>> * in legacy mode, but sets the PCI registers to PCI native mode.
>>> * The chip can only operate in legacy mode, so force the PCI class into legacy
>>> * mode as well. The same fixup must be done to the class-code property in
>>> * the IDE node /pci@80000000/ide@C,1
>>> */
>>
>> I'm not sure that it makes much sense that the firmware configures the chip one way
>> then puts info about a different way in the device tree. There could be bugs but this
>> is not likely. Especially because I see in traces that the firmware does try to
>> configure the device in native mode. These are the first few accesses of the firmware
>> to via-ide:
>
> But that is exactly what is happening! The comment above clearly indicates the
> firmware incorrectly sets the IDE controller in native mode which is in exact
> agreement with the trace you provide below. And in fact if you look at

I may be reading the comment wrong but to me that says that "firmware 
configures IDE in _legacy_ mode" whereas the trace shows it actually 
configures it in _native_ mode which is complying to the CHRP doc above. 
But since it cannot comply to the "native devices using OpenPIC" part it 
probably tries to apply the "ISA devices embedded in PCI" part and locks 
IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much as 
possible and the designers decided they will use IRQ14 and 15 for IDE.

> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc
> hack that was released in order to fix the device tree to boot Linux which alters the
> class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good
> enough here).

Isn't this the same fixup that newer Linux kernels already include? Maybe 
this was needed before Linux properly supported Pegasos2 but later kernels 
will do this anyway. This does not give us any new info we did not have 
before I think maybe just easier to see all fixups in one place.

>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
>>
>> The very first write is to turn on native mode, so I think it's not about what the
>> firmware does but something about how hardware is wired on Pegasos II or the VT8231
>> chip itself that only allows legacy interrupts instead of 100% native mode for IDE.
>>
>>> Given that the DT is wrong, then we should assume that all OSs would have to
>>> compensate for this in the same way as Linux, and therefore this should be handled
>>> automatically.
>>>
>>> AFAICT this then only leaves the question: why does the firmware set
>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>>> MorphOS under QEMU.
>>
>> Linux does try to handle both true native mode and half-native mode. It only uses
>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
>> and uses true native mode setup. I don't know what MorphOS does but I think it justs
>> knows that Pegasos2 has this quirk and does not look at the device tree at all.
>>
>>> Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE
>>> to 14? You should be able to confirm this easily on real hardware using the Forth
>>> config-* words on the IDE node and reading the prog-if byte before and after.
>>
>> I don't have direct access to real hardware and would also need to come up with some
>> Forth to verify that but given the above trace that the firmware does before we can
>> enter any Forth we would likely find @0x9 = 0x8f and @0x3c = 0x0e because after
>> booting Linux we see 0x8a and 0x0e and Linux only touches the two mode bits.
>
> Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a
> standard chip, so let's try and figure out exactly what is happening using a real
> firmware and emulate that behaviour in QEMU. This should then make all guests happy,
> regardless of architecture, without requiring the introduction of feature bits or
> risk of introducing other incompatibilities.

I think I've already done that with this patch (within the limits possible 
in QEMU). The Pegasos2 seems to always use IRQ14 and 15 even when IDE is 
set to native mode (which the firmware does immediately, I'm using a real 
firmware to test) and all guests are happy with this. This behaviour is 
confirmed by excpectations of AmigaOS and MorphOS drivers and also Linux 
comments (although those comments may get the reason wrong, they confirm 
the behaviour). I'm not sure how real hardware implements this behaviour 
and also not sure if it's a bug in the firmware or rather a peculiar 
design choice for Pegasos2. But that probably does not matter for the fact 
that it's how it works which is all we need to emulate it.

Also consider that QEMU via-ide is only emulating native mode of the chip 
because we can't switch between the two modes so it's either legacy only 
or native only because all other implemented controllers are either ISA or 
native PCI so QEMU does not have way to deregister ISA IDE and PCI code 
does not have way to use io BARs despite not being enabled via PCI config 
which could be needed to use them when device is in legacy mode. 
Previously via-ide was emulating legacy only and that worked with Linux 
but not with anything else. I'm not planning to rewrite large parts of the 
IDE and PCI code to allow switching back and forth which is not even 
needed. Unless you know a better way to implement this I think the 
proposed patch is achieving this with minimal changes. I don't see a need 
to more exactly emulate some kind of hardware bug or peculiar design 
choices of a board than what's needed to make clients happy and boot.

Is this series good enough now to be merged or are any changes needed? I'd 
like to not miss the deadline for the freeze and get this delayed for 
months for not good reason because I'm not sure when will I have time to 
work on it again.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 04/03/2020 00:22, BALATON Zoltan wrote:

>>>> So on that basis the best explanation as to what is happening is the
>>>> comment in the link you provided here:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>>>>
>>>>
>>>>
>>>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
>>>> * in legacy mode, but sets the PCI registers to PCI native mode.
>>>> * The chip can only operate in legacy mode, so force the PCI class into legacy
>>>> * mode as well. The same fixup must be done to the class-code property in
>>>> * the IDE node /pci@80000000/ide@C,1
>>>> */
>>>
>>> I'm not sure that it makes much sense that the firmware configures the chip one way
>>> then puts info about a different way in the device tree. There could be bugs but this
>>> is not likely. Especially because I see in traces that the firmware does try to
>>> configure the device in native mode. These are the first few accesses of the firmware
>>> to via-ide:
>>
>> But that is exactly what is happening! The comment above clearly indicates the
>> firmware incorrectly sets the IDE controller in native mode which is in exact
>> agreement with the trace you provide below. And in fact if you look at
> 
> I may be reading the comment wrong but to me that says that "firmware configures IDE
> in _legacy_ mode" whereas the trace shows it actually configures it in _native_ mode
> which is complying to the CHRP doc above. But since it cannot comply to the "native
> devices using OpenPIC" part it probably tries to apply the "ISA devices embedded in
> PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much
> as possible and the designers decided they will use IRQ14 and 15 for IDE.

Interesting. My interpretation of the comment was that the hardware can only operate
in legacy mode, even though the firmware configures the PCI registers to enable
native mode (which is why the class-code and IRQ routing are wrong).

>> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc
>> hack that was released in order to fix the device tree to boot Linux which alters the
>> class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good
>> enough here).
> 
> Isn't this the same fixup that newer Linux kernels already include? Maybe this was
> needed before Linux properly supported Pegasos2 but later kernels will do this
> anyway. This does not give us any new info we did not have before I think maybe just
> easier to see all fixups in one place.
> 
>>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
>>>
>>> The very first write is to turn on native mode, so I think it's not about what the
>>> firmware does but something about how hardware is wired on Pegasos II or the VT8231
>>> chip itself that only allows legacy interrupts instead of 100% native mode for IDE.
>>>
>>>> Given that the DT is wrong, then we should assume that all OSs would have to
>>>> compensate for this in the same way as Linux, and therefore this should be handled
>>>> automatically.
>>>>
>>>> AFAICT this then only leaves the question: why does the firmware set
>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>>>> MorphOS under QEMU.
>>>
>>> Linux does try to handle both true native mode and half-native mode. It only uses
>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
>>> and uses true native mode setup. I don't know what MorphOS does but I think it justs
>>> knows that Pegasos2 has this quirk and does not look at the device tree at all.

I think this is the other way around? From the code above:

	if (viaide->irq != 14)
		return;

Doesn't this suggest that chrp_pci_fixup_vt8231_ata() exits without applying the fix
if it finds PCI_INTERRUPT_LINE set to 9 from the firmware above?

Do you have a copy of the full DT and the firmware revision number that was used to
generate your Linux boot output on real hardware?

>> Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a
>> standard chip, so let's try and figure out exactly what is happening using a real
>> firmware and emulate that behaviour in QEMU. This should then make all guests happy,
>> regardless of architecture, without requiring the introduction of feature bits or
>> risk of introducing other incompatibilities.
> 
> I think I've already done that with this patch (within the limits possible in QEMU).
> The Pegasos2 seems to always use IRQ14 and 15 even when IDE is set to native mode
> (which the firmware does immediately, I'm using a real firmware to test) and all
> guests are happy with this. This behaviour is confirmed by excpectations of AmigaOS
> and MorphOS drivers and also Linux comments (although those comments may get the
> reason wrong, they confirm the behaviour). I'm not sure how real hardware implements
> this behaviour and also not sure if it's a bug in the firmware or rather a peculiar
> design choice for Pegasos2. But that probably does not matter for the fact that it's
> how it works which is all we need to emulate it.
> 
> Also consider that QEMU via-ide is only emulating native mode of the chip because we
> can't switch between the two modes so it's either legacy only or native only because
> all other implemented controllers are either ISA or native PCI so QEMU does not have
> way to deregister ISA IDE and PCI code does not have way to use io BARs despite not
> being enabled via PCI config which could be needed to use them when device is in
> legacy mode. Previously via-ide was emulating legacy only and that worked with Linux
> but not with anything else. I'm not planning to rewrite large parts of the IDE and
> PCI code to allow switching back and forth which is not even needed. Unless you know
> a better way to implement this I think the proposed patch is achieving this with
> minimal changes. I don't see a need to more exactly emulate some kind of hardware bug
> or peculiar design choices of a board than what's needed to make clients happy and boot.
> 
> Is this series good enough now to be merged or are any changes needed? I'd like to
> not miss the deadline for the freeze and get this delayed for months for not good
> reason because I'm not sure when will I have time to work on it again.

I think there's still time to get something done before freeze, however I'm not
convinced that we understand the actual problem correctly (and also the use of
feature bits feels somewhat odd to me).

One more thing I don't understand: I had a glance over the logs you posted over at
https://osdn.net/projects/qmiga/ticket/38949 and you mention that everything works up
to the point where BMDMA is enabled.

From memory working with cmd646 both the BMDMA and non-BMDMA interrupts end up
calling into the same *_set_irq() function in the emulated controller. So what is the
difference between the initial state where IRQs function enough to start to load an
OS, and at the point where BMDMA is enabled by the OS driver and things stop working?
How does the IRQ routing compare?


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Wed, 4 Mar 2020, Mark Cave-Ayland wrote:
> On 04/03/2020 00:22, BALATON Zoltan wrote:
>>>>> So on that basis the best explanation as to what is happening is the
>>>>> comment in the link you provided here:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353
>>>>>
>>>>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller
>>>>> * in legacy mode, but sets the PCI registers to PCI native mode.
>>>>> * The chip can only operate in legacy mode, so force the PCI class into legacy
>>>>> * mode as well. The same fixup must be done to the class-code property in
>>>>> * the IDE node /pci@80000000/ide@C,1
>>>>> */
>>>>
>>>> I'm not sure that it makes much sense that the firmware configures the chip one way
>>>> then puts info about a different way in the device tree. There could be bugs but this
>>>> is not likely. Especially because I see in traces that the firmware does try to
>>>> configure the device in native mode. These are the first few accesses of the firmware
>>>> to via-ide:
>>>
>>> But that is exactly what is happening! The comment above clearly indicates the
>>> firmware incorrectly sets the IDE controller in native mode which is in exact
>>> agreement with the trace you provide below. And in fact if you look at
>>
>> I may be reading the comment wrong but to me that says that "firmware configures IDE
>> in _legacy_ mode" whereas the trace shows it actually configures it in _native_ mode
>> which is complying to the CHRP doc above. But since it cannot comply to the "native
>> devices using OpenPIC" part it probably tries to apply the "ISA devices embedded in
>> PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much
>> as possible and the designers decided they will use IRQ14 and 15 for IDE.
>
> Interesting. My interpretation of the comment was that the hardware can only operate
> in legacy mode, even though the firmware configures the PCI registers to enable
> native mode (which is why the class-code and IRQ routing are wrong).

I think you may give more significance to this comment than it really has. 
I don't know who put it there but maybe was also guessing what really 
happens and it's not really an authorative answer why it behaves like 
that. It seems to come from this commit and not from Genesi/bPlan but 
sounds more like a bugfix from some user:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/powerpc/platforms/chrp/pci.c?h=v3.16.82&id=556ecf9be66f4d493e19bc71a7ce84366d512b71

I give more credibility to what MorphOS expects because the developers of 
that were closely cooperating with the board designers:

https://en.wikipedia.org/wiki/Bplan#Community_support
https://en.wikipedia.org/wiki/Pegasos#Operating_system_support

>>> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc
>>> hack that was released in order to fix the device tree to boot Linux which alters the
>>> class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good
>>> enough here).
>>
>> Isn't this the same fixup that newer Linux kernels already include? Maybe this was
>> needed before Linux properly supported Pegasos2 but later kernels will do this
>> anyway. This does not give us any new info we did not have before I think maybe just
>> easier to see all fixups in one place.
>>
>>>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>>>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>>>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>>>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>>>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>>>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>>>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>>>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>>>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>>>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>>>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
>>>>
>>>> The very first write is to turn on native mode, so I think it's not about what the
>>>> firmware does but something about how hardware is wired on Pegasos II or the VT8231
>>>> chip itself that only allows legacy interrupts instead of 100% native mode for IDE.
>>>>
>>>>> Given that the DT is wrong, then we should assume that all OSs would have to
>>>>> compensate for this in the same way as Linux, and therefore this should be handled
>>>>> automatically.
>>>>>
>>>>> AFAICT this then only leaves the question: why does the firmware set
>>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>>>>> MorphOS under QEMU.
>>>>
>>>> Linux does try to handle both true native mode and half-native mode. It only uses
>>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
>>>> and uses true native mode setup. I don't know what MorphOS does but I think it justs
>>>> knows that Pegasos2 has this quirk and does not look at the device tree at all.
>
> I think this is the other way around? From the code above:
>
> 	if (viaide->irq != 14)
> 		return;
>
> Doesn't this suggest that chrp_pci_fixup_vt8231_ata() exits without applying the fix
> if it finds PCI_INTERRUPT_LINE set to 9 from the firmware above?

Yes but the fixup is clearing the bits saying the controller is in native 
mode so code detecting IRQ lines later will use the legacy 14 and 15 
because that decides based on this config value. This corresponds to 
half-native mode, as io addresses are still from BARs not the legacy IDE 
ports as can be seen from the dmesg output about ide addresses. If irq is 
not set to 14, fixup function leaves config in native mode and Linux will 
try to use the single IRQ line set by PCI_INTERRUPT_LINE (which the 
firmware sets to 9) for both channels. This works with Linux both ways as 
long as we emulate the same but doesn't with other OSes which always use 
PCI BARs to access io regs but expect interrupts on 14 and 15 regardless 
of the mode (and they leave it in native mode don't try setting to legacy) 
so I think they just know Pegasos2 has this half-native mode and use that 
without trying to fix up anything. From this I think we know how it works 
on real hardware and this patch tries to emulate that which also seems to 
work with all OSes. What else is needed?

> Do you have a copy of the full DT and the firmware revision number that was used to
> generate your Linux boot output on real hardware?

Firmware dumps are also linked from

https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2

in the section about firmware and also show mode set to 0x8f by firmware 
20040810.

>>> Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a
>>> standard chip, so let's try and figure out exactly what is happening using a real
>>> firmware and emulate that behaviour in QEMU. This should then make all guests happy,
>>> regardless of architecture, without requiring the introduction of feature bits or
>>> risk of introducing other incompatibilities.
>>
>> I think I've already done that with this patch (within the limits possible in QEMU).
>> The Pegasos2 seems to always use IRQ14 and 15 even when IDE is set to native mode
>> (which the firmware does immediately, I'm using a real firmware to test) and all
>> guests are happy with this. This behaviour is confirmed by excpectations of AmigaOS
>> and MorphOS drivers and also Linux comments (although those comments may get the
>> reason wrong, they confirm the behaviour). I'm not sure how real hardware implements
>> this behaviour and also not sure if it's a bug in the firmware or rather a peculiar
>> design choice for Pegasos2. But that probably does not matter for the fact that it's
>> how it works which is all we need to emulate it.
>>
>> Also consider that QEMU via-ide is only emulating native mode of the chip because we
>> can't switch between the two modes so it's either legacy only or native only because
>> all other implemented controllers are either ISA or native PCI so QEMU does not have
>> way to deregister ISA IDE and PCI code does not have way to use io BARs despite not
>> being enabled via PCI config which could be needed to use them when device is in
>> legacy mode. Previously via-ide was emulating legacy only and that worked with Linux
>> but not with anything else. I'm not planning to rewrite large parts of the IDE and
>> PCI code to allow switching back and forth which is not even needed. Unless you know
>> a better way to implement this I think the proposed patch is achieving this with
>> minimal changes. I don't see a need to more exactly emulate some kind of hardware bug
>> or peculiar design choices of a board than what's needed to make clients happy and boot.
>>
>> Is this series good enough now to be merged or are any changes needed? I'd like to
>> not miss the deadline for the freeze and get this delayed for months for not good
>> reason because I'm not sure when will I have time to work on it again.
>
> I think there's still time to get something done before freeze, however I'm not
> convinced that we understand the actual problem correctly (and also the use of
> feature bits feels somewhat odd to me).

There may be some time until the freeze but not sure I'll have much time 
until maybe Easter or later so I'd rather get over this now if possible 
than do it in the last minute. The feature bit was needed because the 
fulong2e does not seem to have this behaviour (although Linux has a 
different set of fixups for that board but not about IDE IRQs) so we need 
to emulate two different behaviours: half-native mode for pegasos2 (which 
I think is confirmed by evidence in multiple OSes even if we don't know 
the exact reason behind it) and 100% native mode for fulong2e hence we 
need a way to tell the device emulation which board's expectations it 
should fulfil for which introducing a flag seemed to be the simplest 
solution.

> One more thing I don't understand: I had a glance over the logs you posted over at
> https://osdn.net/projects/qmiga/ticket/38949 and you mention that everything works up
> to the point where BMDMA is enabled.
>
> From memory working with cmd646 both the BMDMA and non-BMDMA interrupts end up
> calling into the same *_set_irq() function in the emulated controller. So what is the
> difference between the initial state where IRQs function enough to start to load an
> OS, and at the point where BMDMA is enabled by the OS driver and things stop working?
> How does the IRQ routing compare?

I'm not sure either why that's the case but maybe the firmware does not 
use interrupts and MorphOS also only enables it when enabling BMDMA. 
Before that they could just poll the PIO regs?

What other OSes are we interested in running on pegasos2 than Linux, 
MorphOS and AmigaOS? Once these are happy do we need to spend more time on 
this until an actual problem comes up? I think there's no need to further 
refine this relatively rarely used device model now. Unless you can 
propose a better solution that works with these OSes, otherwise that could 
be addressed in a later patch and the current one is good enough to enable 
these guests for now.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 04/03/2020 22:33, BALATON Zoltan wrote:

>>>>>> AFAICT this then only leaves the question: why does the firmware set
>>>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>>>>>> MorphOS under QEMU.
>>>>>
>>>>> Linux does try to handle both true native mode and half-native mode. It only uses
>>>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
>>>>> and uses true native mode setup. I don't know what MorphOS does but I think it
>>>>> justs
>>>>> knows that Pegasos2 has this quirk and does not look at the device tree at all.

I just a quick look at the PCI specification and found this interesting paragraph in
the section about "Interrupt Line":


"The Interrupt Line register is an eight-bit register used to communicate interrupt
line routing information. The register is read/write and must be implemented by any
device (or device function) that uses an interrupt pin. POST software will write the
routing information into this register as it initializes and configures the system."

"The value in this register tells which input of the system interrupt controller(s)
the device's interrupt pin is connected to. The device itself does not use this
value, rather it is used by device drivers and operating systems. Device drivers and
operating systems can use this information to determine priority and vector
information. Values in this register are architecture-specific [43]."

[43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15)
of the standard dual 8259 configuration. The value 255 is defined as meaning
"unknown" or "no connection" to the interrupt controller. Values between 15 and 254
are reserved.


The key part here is "The device itself does not use this value, rather it is used by
device drivers and operating systems" since this immediately tells us that the
existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is
incorrect and should be removed.

If we do that the next question is how does the VIA know whether the use the PCI
interrupt or the legacy interrupt? Another look at the datasheet showed that there is
another possibility: PCI configuration space register 0x3d (Interrupt pin) is
documented as having value 0 == Legacy IRQ routing which should be the initial value
on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.

In your previous email you included a trace of the PCI configuration accesses to the
via-ide device. Can you try this again with the following diff and post the same
output once again?

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 096de8dba0..db9f4af861 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -139,7 +139,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);


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:
> On 04/03/2020 22:33, BALATON Zoltan wrote:
>>>>>>> AFAICT this then only leaves the question: why does the firmware set
>>>>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
>>>>>>> MorphOS under QEMU.
>>>>>>
>>>>>> Linux does try to handle both true native mode and half-native mode. It only uses
>>>>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup
>>>>>> and uses true native mode setup. I don't know what MorphOS does but I think it
>>>>>> justs
>>>>>> knows that Pegasos2 has this quirk and does not look at the device tree at all.
>
> I just a quick look at the PCI specification and found this interesting paragraph in
> the section about "Interrupt Line":
>
>
> "The Interrupt Line register is an eight-bit register used to communicate interrupt
> line routing information. The register is read/write and must be implemented by any
> device (or device function) that uses an interrupt pin. POST software will write the
> routing information into this register as it initializes and configures the system."
>
> "The value in this register tells which input of the system interrupt controller(s)
> the device's interrupt pin is connected to. The device itself does not use this
> value, rather it is used by device drivers and operating systems. Device drivers and
> operating systems can use this information to determine priority and vector
> information. Values in this register are architecture-specific [43]."
>
> [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15)
> of the standard dual 8259 configuration. The value 255 is defined as meaning
> "unknown" or "no connection" to the interrupt controller. Values between 15 and 254
> are reserved.
>
>
> The key part here is "The device itself does not use this value, rather it is used by
> device drivers and operating systems" since this immediately tells us that the
> existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is
> incorrect and should be removed.

On real hardware this may be true but in QEMU how would it otherwise raise 
the correct interrupt line the guest expects? This probably does not 
matter for pegasos2 but I think is needed for 100% native mode used with 
the fulong2e so it gets the IRQ it expects.

> If we do that the next question is how does the VIA know whether the use the PCI
> interrupt or the legacy interrupt? Another look at the datasheet showed that there is

I don't think via-ide ever uses a PCI interrupt, if you look at its 
datasheet the description of the prog-if reg (0x9) says in native mode irq 
is programmable via config reg 0x3c which then lists all the ISA IRQs as 
possible values, default 14 and 0 meaning disable.

> another possibility: PCI configuration space register 0x3d (Interrupt pin) is
> documented as having value 0 == Legacy IRQ routing which should be the initial value
> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.

The VT8231 docs say this should always read 1 but may be this is somehow 
set to 0 on the Pegasos2. What does that mean? Should we use this value 
instead of the feature bit to force using legacy interrupts? We'd still 
need a property in via-ide to set this reg or is it possible to set it 
from board code overriding the default after device is created? That would 
allow to drop patch 1. I can try this.

> In your previous email you included a trace of the PCI configuration accesses to the
> via-ide device. Can you try this again with the following diff and post the same
> output once again?
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 096de8dba0..db9f4af861 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -139,7 +139,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);

This does not change much:

pci_cfg_write via-ide 12:1 @0x9 <- 0xf
pci_cfg_write via-ide 12:1 @0x40 <- 0xb
pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
pci_cfg_write via-ide 12:1 @0x43 <- 0x35
pci_cfg_write via-ide 12:1 @0x44 <- 0x18
pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
pci_cfg_write via-ide 12:1 @0x54 <- 0x14
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
pci_cfg_read via-ide 12:1 @0xc -> 0x0
pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
pci_cfg_read via-ide 12:1 @0x3c -> 0xe
pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
pci_cfg_read via-ide 12:1 @0x3c -> 0xe
pci_cfg_write via-ide 12:1 @0x3c <- 0x9

compared to

> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
> pci_cfg_read via-ide 12:1 @0xc -> 0x0
> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
> pci_cfg_write via-ide 12:1 @0x3c <- 0x109

firmware does not seem to care about this value.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Fri, 6 Mar 2020, BALATON Zoltan wrote:
> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:
>> On 04/03/2020 22:33, BALATON Zoltan wrote:
>> another possibility: PCI configuration space register 0x3d (Interrupt pin) 
>> is
>> documented as having value 0 == Legacy IRQ routing which should be the 
>> initial value
>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ 
>> routing.
>
> The VT8231 docs say this should always read 1 but may be this is somehow set 
> to 0 on the Pegasos2. What does that mean? Should we use this value instead 
> of the feature bit to force using legacy interrupts? We'd still need a 
> property in via-ide to set this reg or is it possible to set it from board 
> code overriding the default after device is created? That would allow to drop 
> patch 1. I can try this.

This seemed like it could simplify patches a bit but it does not work. 
Setting this reg to 0 breaks Linux and MorphOS which then think the device 
does not have an interrupt at all and fail as before waiting for the irq. 
So we still need the feature bit, cant use this reg to force legacy 
interrupts. I've spent considerable time testing different OSes before 
I've ended up with this patch series I've submitted and I could not find a 
simpler way that works with everything.

Regards,
BALATON Zoltan

>> In your previous email you included a trace of the PCI configuration 
>> accesses to the
>> via-ide device. Can you try this again with the following diff and post the 
>> same
>> output once again?
>> 
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..db9f4af861 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -139,7 +139,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);
>
> This does not change much:
>
> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
> pci_cfg_read via-ide 12:1 @0xc -> 0x0
> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
> pci_cfg_read via-ide 12:1 @0x3c -> 0xe
> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
> pci_cfg_read via-ide 12:1 @0x3c -> 0xe
> pci_cfg_write via-ide 12:1 @0x3c <- 0x9
>
> compared to
>
>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
>
> firmware does not seem to care about this value.
>
> Regards,
> BALATON Zoltan
>

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 05/03/2020 23:35, BALATON Zoltan wrote:

>> I just a quick look at the PCI specification and found this interesting paragraph in
>> the section about "Interrupt Line":
>>
>>
>> "The Interrupt Line register is an eight-bit register used to communicate interrupt
>> line routing information. The register is read/write and must be implemented by any
>> device (or device function) that uses an interrupt pin. POST software will write the
>> routing information into this register as it initializes and configures the system."
>>
>> "The value in this register tells which input of the system interrupt controller(s)
>> the device's interrupt pin is connected to. The device itself does not use this
>> value, rather it is used by device drivers and operating systems. Device drivers and
>> operating systems can use this information to determine priority and vector
>> information. Values in this register are architecture-specific [43]."
>>
>> [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15)
>> of the standard dual 8259 configuration. The value 255 is defined as meaning
>> "unknown" or "no connection" to the interrupt controller. Values between 15 and 254
>> are reserved.
>>
>>
>> The key part here is "The device itself does not use this value, rather it is used by
>> device drivers and operating systems" since this immediately tells us that the
>> existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is
>> incorrect and should be removed.
> 
> On real hardware this may be true but in QEMU how would it otherwise raise the
> correct interrupt line the guest expects? This probably does not matter for pegasos2
> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ
> it expects.

That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
wire it up to your interrupt controller.

>> If we do that the next question is how does the VIA know whether the use the PCI
>> interrupt or the legacy interrupt? Another look at the datasheet showed that there is
> 
> I don't think via-ide ever uses a PCI interrupt, if you look at its datasheet the
> description of the prog-if reg (0x9) says in native mode irq is programmable via
> config reg 0x3c which then lists all the ISA IRQs as possible values, default 14 and
> 0 meaning disable.
> 
>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is
>> documented as having value 0 == Legacy IRQ routing which should be the initial value
>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.
> 
> The VT8231 docs say this should always read 1 but may be this is somehow set to 0 on
> the Pegasos2. What does that mean? Should we use this value instead of the feature
> bit to force using legacy interrupts? We'd still need a property in via-ide to set
> this reg or is it possible to set it from board code overriding the default after
> device is created? That would allow to drop patch 1. I can try this.

Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B
datasheets, and there is a difference here. You are correct in what you say above in
that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not
the case.

What is rather unusual here is that both the 8231 and 686B have exactly the same
device and vendor ids, so I'm not sure how you'd distinguish between them?

>> In your previous email you included a trace of the PCI configuration accesses to the
>> via-ide device. Can you try this again with the following diff and post the same
>> output once again?
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..db9f4af861 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -139,7 +139,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);
> 
> This does not change much:
> 
> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
> pci_cfg_read via-ide 12:1 @0xc -> 0x0
> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
> pci_cfg_read via-ide 12:1 @0x3c -> 0xe
> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
> pci_cfg_read via-ide 12:1 @0x3c -> 0xe
> pci_cfg_write via-ide 12:1 @0x3c <- 0x9
> 
> compared to
> 
>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
> 
> firmware does not seem to care about this value.

I think this proves what I was saying about the legacy interrupt routing, but of
course this is based upon the 686B rather than the 8231.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 06/03/2020 00:21, BALATON Zoltan wrote:

> On Fri, 6 Mar 2020, BALATON Zoltan wrote:
>> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:
>>> On 04/03/2020 22:33, BALATON Zoltan wrote:
>>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is
>>> documented as having value 0 == Legacy IRQ routing which should be the initial value
>>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.
>>
>> The VT8231 docs say this should always read 1 but may be this is somehow set to 0
>> on the Pegasos2. What does that mean? Should we use this value instead of the
>> feature bit to force using legacy interrupts? We'd still need a property in via-ide
>> to set this reg or is it possible to set it from board code overriding the default
>> after device is created? That would allow to drop patch 1. I can try this.
> 
> This seemed like it could simplify patches a bit but it does not work. Setting this
> reg to 0 breaks Linux and MorphOS which then think the device does not have an
> interrupt at all and fail as before waiting for the irq. So we still need the feature
> bit, cant use this reg to force legacy interrupts. I've spent considerable time
> testing different OSes before I've ended up with this patch series I've submitted and
> I could not find a simpler way that works with everything.

I appreciate that testing these things can take a lot of time, but what is important
thing to ask here is whether these hacks are attempting to work around something in
QEMU that doesn't match the hardware specification, and to me it feels that this is
what is happening here.

Obviously this thread has become quite long (and even I'm struggling to find previous
discussions) but here is my summary below:

- I don't think the patch in its current form is the right way to do this. Instead of
adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing
is wrong, let's fix the existing IRQ routing instead.

- There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this
is simply a term used within the Linux patches. The controller is either in native
mode, or legacy mode. It may be that guests are making use of some undefined
behaviour here.

- The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your
patch comment points out, some guests ignore it anyway).

- There is different behaviour here between the 8231 and 686B in this area, despite
having the same vendor/device id.


The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by
removing the existing code and instead expose a qdev named gpio "legacy-irq" and
wiring that up to your interrupt controller. Note you'd have to do the same for
fulong2e, but that is reasonably trivial.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
> On 05/03/2020 23:35, BALATON Zoltan wrote:
>> On real hardware this may be true but in QEMU how would it otherwise raise the
>> correct interrupt line the guest expects? This probably does not matter for pegasos2
>> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ
>> it expects.
>
> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
> wire it up to your interrupt controller.

This "chip" is part of an integrated southbridge/superio/everything chip 
the also includes the two PICs and how they are internally connected is 
not known so we would be guessing here anyway. I don't see a need to make 
it more complicated than it is now by modeling internal pins but how would 
I wire up gpio to the i8259 model and where should I connect the PCI irq?

> Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B
> datasheets, and there is a difference here. You are correct in what you say above in
> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not
> the case.

The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt 
routing and 1 is native mode. Given that we only model native mode of the 
chip it does not make sense to set it to anything else than 1 and setting 
it to 0 confuses MorphOS and Linux on pegasos2 while setting it to 1 works 
with everything I've tried both on pegasos2 and fulong2e even if that may 
not completely match how it's implemented in hardware.

> What is rather unusual here is that both the 8231 and 686B have exactly the same
> device and vendor ids, so I'm not sure how you'd distinguish between them?

Guests distinguish by looking at the parent device (function 0) which is 
the chip this IDE device is part of (on function 1).

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Artyom Tarasenko]

Hi guys,

On Sun, Mar 1, 2020 at 6:54 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 01/03/2020 16:42, BALATON Zoltan wrote:
>
> >> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever
> >> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be
> >> a pci_set_irq(d, level) at the end?
> >
> > According to my tests with several guests it seems the via-ide does not seem to use
> > PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or
> > one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be
> > due to how it's internally connected in the southbridge chip it's part of or some
> > other platform specific quirk, I'm not sure.
>
> I think this is the key part here: how does via-ide switch between legacy and native
> mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd
> expect to see something similar here.

I haven't read the complete discussion yet, but  checked how it's done
in OFW. OFW did definitely work on via boards. Surprisingly OFW
Switches all IDE boards into native mode the same way:

my-space 9 + dup " config-b@"  $call-parent
    05 or  swap  " config-b!"  $call-parent

HTH

-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
> On 06/03/2020 00:21, BALATON Zoltan wrote:
>> On Fri, 6 Mar 2020, BALATON Zoltan wrote:
>>> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 04/03/2020 22:33, BALATON Zoltan wrote:
>>>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is
>>>> documented as having value 0 == Legacy IRQ routing which should be the initial value
>>>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.
>>>
>>> The VT8231 docs say this should always read 1 but may be this is somehow set to 0
>>> on the Pegasos2. What does that mean? Should we use this value instead of the
>>> feature bit to force using legacy interrupts? We'd still need a property in via-ide
>>> to set this reg or is it possible to set it from board code overriding the default
>>> after device is created? That would allow to drop patch 1. I can try this.
>>
>> This seemed like it could simplify patches a bit but it does not work. Setting this
>> reg to 0 breaks Linux and MorphOS which then think the device does not have an
>> interrupt at all and fail as before waiting for the irq. So we still need the feature
>> bit, cant use this reg to force legacy interrupts. I've spent considerable time
>> testing different OSes before I've ended up with this patch series I've submitted and
>> I could not find a simpler way that works with everything.
>
> I appreciate that testing these things can take a lot of time, but what is important
> thing to ask here is whether these hacks are attempting to work around something in
> QEMU that doesn't match the hardware specification, and to me it feels that this is
> what is happening here.

It may be we need to work around some incomplete modelling of devices in 
QEMU, e.g. we only model the native mode of these IDE interfaces so 
anything involving legacy mode is out of scope. To also emulate legacy 
mode we'd need changing common ISA code and maybe PIC code as well. As 
those parts are also used by other more commonly used machine models I'd 
avoid breaking those and rather implement it confined to these machines 
that are not yet finished or complete anyway than try to change all 
dependent devices that would need even more testing. These "hacks" could 
be cleaned up later and this would not be the only hack in QEMU, I don't 
have time to fix everything and it's unreasonable to demand it I think. 
I'd suggest to take this patch as it is now and if you don't like it you 
can submit patches that clean it up the way you think is correct or submit 
an alternative patch now that shows how do you think it can be done in a 
cleaner way because I don't see it and don't have more time for it now.

> Obviously this thread has become quite long (and even I'm struggling to find previous
> discussions) but here is my summary below:
>
> - I don't think the patch in its current form is the right way to do this. Instead of
> adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing
> is wrong, let's fix the existing IRQ routing instead.

I think that would involve changing parts which could break other machines 
so I'd rather go with a featute bit only affecting pegasos2 and fulonge2 
than touch i8259 or ISA emulation basing that on some guess how the real 
chip may be implemented. Is it possible to implement what you propose 
without changing common IDE, ISA and PIC emulation only in via-ide and 
fulong2e code?

> - There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this
> is simply a term used within the Linux patches. The controller is either in native
> mode, or legacy mode. It may be that guests are making use of some undefined
> behaviour here.

Yes, this is a Linux term and Linux also uses a feature bit to enable this 
workaround. If that's good enough for Linux why isn't it good enough for 
you?

> - The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your
> patch comment points out, some guests ignore it anyway).

You're misunderstanding the comment. The via_ide_config_read function is 
needed to restore value in interrupt line that common PCI reset code 
deletes. Linux depends on this value to be the same as on real hardware so 
this is needed to work around QEMU and Linux pecularities.

I've tried using PCI_INTERRUPT_PIN in place of the feature bit but setting 
that to 0 breaks Linux and MorphOS on pegasos2 because these apparently 
expect this to be set to 1 corresponding to native mode. (Firmware only 
sets native mode enable bits in prog-if but datasheet says this reg should 
be 1 by default and other PCI docs say 0 here means no interrupt used so 
maybe that's why Linux and MorphOS don't like it.)

> - There is different behaviour here between the 8231 and 686B in this area, despite
> having the same vendor/device id.
>
>
> The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by
> removing the existing code and instead expose a qdev named gpio "legacy-irq" and
> wiring that up to your interrupt controller. Note you'd have to do the same for
> fulong2e, but that is reasonably trivial.

Please go ahead and do it but if you don't submit an alternative patch 
before the freeze I'd ask John and Peter to make a judgement here and tell 
if my series is acceptable or not in its current form and if it is then 
please merge it and leave clean ups for subsequent patches. This is 
blocking my further patches to implement pegasos2 emulation.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 06/03/2020 12:06, BALATON Zoltan wrote:

> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>> On 05/03/2020 23:35, BALATON Zoltan wrote:
>>> On real hardware this may be true but in QEMU how would it otherwise raise the
>>> correct interrupt line the guest expects? This probably does not matter for pegasos2
>>> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ
>>> it expects.
>>
>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
>> wire it up to your interrupt controller.
> 
> This "chip" is part of an integrated southbridge/superio/everything chip the also
> includes the two PICs and how they are internally connected is not known so we would
> be guessing here anyway. I don't see a need to make it more complicated than it is
> now by modeling internal pins but how would I wire up gpio to the i8259 model and
> where should I connect the PCI irq?

For now I would say not to worry about the PCI IRQ: the reason for discussing this
before was because we believed that if the controller was in native mode it must be
using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification
we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that
the existing via-ide device doesn't currently attempt to use the PCI IRQ in
via_ide_set_irq() then we should be good.

If someone had a machine somewhere that did use the PCI IRQ then it would need
investigation, but since there isn't then I don't see any need to do this now.

>> Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B
>> datasheets, and there is a difference here. You are correct in what you say above in
>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not
>> the case.
> 
> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is
> native mode. Given that we only model native mode of the chip it does not make sense
> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on
> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and
> fulong2e even if that may not completely match how it's implemented in hardware.
> 
>> What is rather unusual here is that both the 8231 and 686B have exactly the same
>> device and vendor ids, so I'm not sure how you'd distinguish between them?
> 
> Guests distinguish by looking at the parent device (function 0) which is the chip
> this IDE device is part of (on function 1).

Okay thanks, that's useful to know.

I've done a quick grep of the source tree and AFAICT the only IDE controller that
tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
fairly easy. In short:

1) Add qemu_irq legacy_irqs[2] into PCIIDEState

(You could argue that it should belong in a separate VIAIDEState, however quite a few
of the BMDMA controllers in QEMU don't have their own device state and just use
PCIIDEState. And whilst via-ide is the only one that currently needs support for
legacy IRQs, I think it's good to put it there in case other controllers need it in
future)

2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
name of "legacy-irq" to it

3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
to pci_create() because the device shouldn't be realised immediately

4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
device

5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
seems that both drives on MIPS and Pegasos both use IRQ 14).


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 06/03/2020 12:40, BALATON Zoltan wrote:

> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>> On 06/03/2020 00:21, BALATON Zoltan wrote:
>>> On Fri, 6 Mar 2020, BALATON Zoltan wrote:
>>>> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:
>>>>> On 04/03/2020 22:33, BALATON Zoltan wrote:
>>>>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is
>>>>> documented as having value 0 == Legacy IRQ routing which should be the initial
>>>>> value
>>>>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.
>>>>
>>>> The VT8231 docs say this should always read 1 but may be this is somehow set to 0
>>>> on the Pegasos2. What does that mean? Should we use this value instead of the
>>>> feature bit to force using legacy interrupts? We'd still need a property in via-ide
>>>> to set this reg or is it possible to set it from board code overriding the default
>>>> after device is created? That would allow to drop patch 1. I can try this.
>>>
>>> This seemed like it could simplify patches a bit but it does not work. Setting this
>>> reg to 0 breaks Linux and MorphOS which then think the device does not have an
>>> interrupt at all and fail as before waiting for the irq. So we still need the feature
>>> bit, cant use this reg to force legacy interrupts. I've spent considerable time
>>> testing different OSes before I've ended up with this patch series I've submitted and
>>> I could not find a simpler way that works with everything.
>>
>> I appreciate that testing these things can take a lot of time, but what is important
>> thing to ask here is whether these hacks are attempting to work around something in
>> QEMU that doesn't match the hardware specification, and to me it feels that this is
>> what is happening here.
> 
> It may be we need to work around some incomplete modelling of devices in QEMU, e.g.
> we only model the native mode of these IDE interfaces so anything involving legacy
> mode is out of scope. To also emulate legacy mode we'd need changing common ISA code
> and maybe PIC code as well. As those parts are also used by other more commonly used
> machine models I'd avoid breaking those and rather implement it confined to these
> machines that are not yet finished or complete anyway than try to change all
> dependent devices that would need even more testing. These "hacks" could be cleaned
> up later and this would not be the only hack in QEMU, I don't have time to fix
> everything and it's unreasonable to demand it I think. I'd suggest to take this patch
> as it is now and if you don't like it you can submit patches that clean it up the way
> you think is correct or submit an alternative patch now that shows how do you think
> it can be done in a cleaner way because I don't see it and don't have more time for
> it now.
> 
>> Obviously this thread has become quite long (and even I'm struggling to find previous
>> discussions) but here is my summary below:
>>
>> - I don't think the patch in its current form is the right way to do this. Instead of
>> adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing
>> is wrong, let's fix the existing IRQ routing instead.
> 
> I think that would involve changing parts which could break other machines so I'd
> rather go with a featute bit only affecting pegasos2 and fulonge2 than touch i8259 or
> ISA emulation basing that on some guess how the real chip may be implemented. Is it
> possible to implement what you propose without changing common IDE, ISA and PIC
> emulation only in via-ide and fulong2e code?
> 
>> - There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this
>> is simply a term used within the Linux patches. The controller is either in native
>> mode, or legacy mode. It may be that guests are making use of some undefined
>> behaviour here.
> 
> Yes, this is a Linux term and Linux also uses a feature bit to enable this
> workaround. If that's good enough for Linux why isn't it good enough for you?
> 
>> - The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your
>> patch comment points out, some guests ignore it anyway).
> 
> You're misunderstanding the comment. The via_ide_config_read function is needed to
> restore value in interrupt line that common PCI reset code deletes. Linux depends on
> this value to be the same as on real hardware so this is needed to work around QEMU
> and Linux pecularities.
> 
> I've tried using PCI_INTERRUPT_PIN in place of the feature bit but setting that to 0
> breaks Linux and MorphOS on pegasos2 because these apparently expect this to be set
> to 1 corresponding to native mode. (Firmware only sets native mode enable bits in
> prog-if but datasheet says this reg should be 1 by default and other PCI docs say 0
> here means no interrupt used so maybe that's why Linux and MorphOS don't like it.)
> 
>> - There is different behaviour here between the 8231 and 686B in this area, despite
>> having the same vendor/device id.
>>
>>
>> The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by
>> removing the existing code and instead expose a qdev named gpio "legacy-irq" and
>> wiring that up to your interrupt controller. Note you'd have to do the same for
>> fulong2e, but that is reasonably trivial.
> 
> Please go ahead and do it but if you don't submit an alternative patch before the
> freeze I'd ask John and Peter to make a judgement here and tell if my series is
> acceptable or not in its current form and if it is then please merge it and leave
> clean ups for subsequent patches. This is blocking my further patches to implement
> pegasos2 emulation.

I believe I've answered this in detail in my previous email, so I suggest we keep the
discussion there so it's all in one place.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
> On 06/03/2020 12:06, BALATON Zoltan wrote:
>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>>> On 05/03/2020 23:35, BALATON Zoltan wrote:
>>>> On real hardware this may be true but in QEMU how would it otherwise raise the
>>>> correct interrupt line the guest expects? This probably does not matter for pegasos2
>>>> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ
>>>> it expects.
>>>
>>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
>>> wire it up to your interrupt controller.
>>
>> This "chip" is part of an integrated southbridge/superio/everything chip the also
>> includes the two PICs and how they are internally connected is not known so we would
>> be guessing here anyway. I don't see a need to make it more complicated than it is
>> now by modeling internal pins but how would I wire up gpio to the i8259 model and
>> where should I connect the PCI irq?
>
> For now I would say not to worry about the PCI IRQ: the reason for discussing this
> before was because we believed that if the controller was in native mode it must be
> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification
> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that
> the existing via-ide device doesn't currently attempt to use the PCI IRQ in
> via_ide_set_irq() then we should be good.
>
> If someone had a machine somewhere that did use the PCI IRQ then it would need
> investigation, but since there isn't then I don't see any need to do this now.
>
>>> Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B
>>> datasheets, and there is a difference here. You are correct in what you say above in
>>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not
>>> the case.
>>
>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is
>> native mode. Given that we only model native mode of the chip it does not make sense
>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on
>> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and
>> fulong2e even if that may not completely match how it's implemented in hardware.
>>
>>> What is rather unusual here is that both the 8231 and 686B have exactly the same
>>> device and vendor ids, so I'm not sure how you'd distinguish between them?
>>
>> Guests distinguish by looking at the parent device (function 0) which is the chip
>> this IDE device is part of (on function 1).
>
> Okay thanks, that's useful to know.
>
> I've done a quick grep of the source tree and AFAICT the only IDE controller that
> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
> fairly easy. In short:
>
> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>
> (You could argue that it should belong in a separate VIAIDEState, however quite a few
> of the BMDMA controllers in QEMU don't have their own device state and just use
> PCIIDEState. And whilst via-ide is the only one that currently needs support for
> legacy IRQs, I think it's good to put it there in case other controllers need it in
> future)
>
> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
> name of "legacy-irq" to it

I don't like this. This adds two via-ide specific data to to common PCI 
IDE code where it does not belong and subclassing it just for this also 
seems to be more changes than really needed. Reusing the existing CMD646 
field and generalising it to allow implementation specific feature flags 
seems much less intrusive and not less clean than your proposal.

> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
> to pci_create() because the device shouldn't be realised immediately
>
> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
> device

How do I connect gpios to 8259 interrupts? That seems to be internal 
state of 8259 that I'm not sure how to access cleanly from code 
instantiating it. Is this better than my patch? It seems it achieves the 
same via-ide specific behaviour just in a more complicated way and would 
still need the feature bit to know when to use legacy_irq[1].

> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
> seems that both drives on MIPS and Pegasos both use IRQ 14).

According to the 8231 datasheet in legacy mode (and on pegasos2's 
half-native mode) the interrupts should be 14 and 15 so legacy_irq[n] with 
your way but in 100% native mode (used on the fulong2e) it should be the 
one set in PCI_INTERRUPT_LINE. The 686B datasheet does not detail this but 
I believe it works the same. Since we currently fixed the native mode 
interrupt to 14 to work around pegasos2 firmware and QEMU PCI reset 
interactions using legacy_irq[0] might work but is not correct, the 
current way using PCI_INTERRUPT_LINE is better modelling what hardware 
does in my opinion.

Before I spend time modifying this patch I'd really like to hear what John 
as the IDE maintainer prefers for this. John could you chime in and share 
your views please?

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 06/03/2020 19:38, BALATON Zoltan wrote:

> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>> On 06/03/2020 12:06, BALATON Zoltan wrote:
>>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 05/03/2020 23:35, BALATON Zoltan wrote:
>>>>> On real hardware this may be true but in QEMU how would it otherwise raise the
>>>>> correct interrupt line the guest expects? This probably does not matter for
>>>>> pegasos2
>>>>> but I think is needed for 100% native mode used with the fulong2e so it gets the
>>>>> IRQ
>>>>> it expects.
>>>>
>>>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
>>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
>>>> wire it up to your interrupt controller.
>>>
>>> This "chip" is part of an integrated southbridge/superio/everything chip the also
>>> includes the two PICs and how they are internally connected is not known so we would
>>> be guessing here anyway. I don't see a need to make it more complicated than it is
>>> now by modeling internal pins but how would I wire up gpio to the i8259 model and
>>> where should I connect the PCI irq?
>>
>> For now I would say not to worry about the PCI IRQ: the reason for discussing this
>> before was because we believed that if the controller was in native mode it must be
>> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification
>> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that
>> the existing via-ide device doesn't currently attempt to use the PCI IRQ in
>> via_ide_set_irq() then we should be good.
>>
>> If someone had a machine somewhere that did use the PCI IRQ then it would need
>> investigation, but since there isn't then I don't see any need to do this now.
>>
>>>> Okay so this is interesting: I've been switching between the VT8231 and the
>>>> VT82C686B
>>>> datasheets, and there is a difference here. You are correct in what you say above in
>>>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly
>>>> not
>>>> the case.
>>>
>>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is
>>> native mode. Given that we only model native mode of the chip it does not make sense
>>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on
>>> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and
>>> fulong2e even if that may not completely match how it's implemented in hardware.
>>>
>>>> What is rather unusual here is that both the 8231 and 686B have exactly the same
>>>> device and vendor ids, so I'm not sure how you'd distinguish between them?
>>>
>>> Guests distinguish by looking at the parent device (function 0) which is the chip
>>> this IDE device is part of (on function 1).
>>
>> Okay thanks, that's useful to know.
>>
>> I've done a quick grep of the source tree and AFAICT the only IDE controller that
>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
>> fairly easy. In short:
>>
>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>>
>> (You could argue that it should belong in a separate VIAIDEState, however quite a few
>> of the BMDMA controllers in QEMU don't have their own device state and just use
>> PCIIDEState. And whilst via-ide is the only one that currently needs support for
>> legacy IRQs, I think it's good to put it there in case other controllers need it in
>> future)
>>
>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
>> name of "legacy-irq" to it
> 
> I don't like this. This adds two via-ide specific data to to common PCI IDE code
> where it does not belong and subclassing it just for this also seems to be more
> changes than really needed. Reusing the existing CMD646 field and generalising it to
> allow implementation specific feature flags seems much less intrusive and not less
> clean than your proposal.

It's not VIA-specific though: the ISA legacy and PCI buses have different electrical
characteristics and so by definition their signals must be driven by separate
physical pins. Have a look at the CMD646 datasheet for example, and you will see that
separate pins exist for legacy and PCI native IRQs.

>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
>> to pci_create() because the device shouldn't be realised immediately
>>
>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
>> device
> 
> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259
> that I'm not sure how to access cleanly from code instantiating it. Is this better
> than my patch? It seems it achieves the same via-ide specific behaviour just in a
> more complicated way and would still need the feature bit to know when to use
> legacy_irq[1].

We know from the PCI specification that the existing code for via-ide is incorrect,
and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in
this way then both of these strongly suggest that current via-ide implementation is
wrong. Rather than add a hack on top of a hack then the simplest solution is to
physically wire the IRQ pin using qdev at the board level, as is done on real hardware.

Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should
just be a case of returning the nth entry and using that with qdev_init_gpio_out_named().

>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
>> seems that both drives on MIPS and Pegasos both use IRQ 14).
> 
> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode)
> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native
> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B
> datasheet does not detail this but I believe it works the same. Since we currently
> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI
> reset interactions using legacy_irq[0] might work but is not correct, the current way
> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.

This is not correct though: please re-read my previous email which quotes from the
PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the
device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
placing a value other than 14 there, but if it does then that suggests the board IRQs
are physically wired differently which again should be handled at board level by qdev.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
> On 06/03/2020 19:38, BALATON Zoltan wrote:
>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>>> On 06/03/2020 12:06, BALATON Zoltan wrote:
>>>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>>>>> On 05/03/2020 23:35, BALATON Zoltan wrote:
>>>>>> On real hardware this may be true but in QEMU how would it otherwise raise the
>>>>>> correct interrupt line the guest expects? This probably does not matter for
>>>>>> pegasos2
>>>>>> but I think is needed for 100% native mode used with the fulong2e so it gets the
>>>>>> IRQ
>>>>>> it expects.
>>>>>
>>>>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the
>>>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to
>>>>> wire it up to your interrupt controller.
>>>>
>>>> This "chip" is part of an integrated southbridge/superio/everything chip the also
>>>> includes the two PICs and how they are internally connected is not known so we would
>>>> be guessing here anyway. I don't see a need to make it more complicated than it is
>>>> now by modeling internal pins but how would I wire up gpio to the i8259 model and
>>>> where should I connect the PCI irq?
>>>
>>> For now I would say not to worry about the PCI IRQ: the reason for discussing this
>>> before was because we believed that if the controller was in native mode it must be
>>> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification
>>> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that
>>> the existing via-ide device doesn't currently attempt to use the PCI IRQ in
>>> via_ide_set_irq() then we should be good.
>>>
>>> If someone had a machine somewhere that did use the PCI IRQ then it would need
>>> investigation, but since there isn't then I don't see any need to do this now.
>>>
>>>>> Okay so this is interesting: I've been switching between the VT8231 and the
>>>>> VT82C686B
>>>>> datasheets, and there is a difference here. You are correct in what you say above in
>>>>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly
>>>>> not
>>>>> the case.
>>>>
>>>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is
>>>> native mode. Given that we only model native mode of the chip it does not make sense
>>>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on
>>>> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and
>>>> fulong2e even if that may not completely match how it's implemented in hardware.
>>>>
>>>>> What is rather unusual here is that both the 8231 and 686B have exactly the same
>>>>> device and vendor ids, so I'm not sure how you'd distinguish between them?
>>>>
>>>> Guests distinguish by looking at the parent device (function 0) which is the chip
>>>> this IDE device is part of (on function 1).
>>>
>>> Okay thanks, that's useful to know.
>>>
>>> I've done a quick grep of the source tree and AFAICT the only IDE controller that
>>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
>>> fairly easy. In short:
>>>
>>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>>>
>>> (You could argue that it should belong in a separate VIAIDEState, however quite a few
>>> of the BMDMA controllers in QEMU don't have their own device state and just use
>>> PCIIDEState. And whilst via-ide is the only one that currently needs support for
>>> legacy IRQs, I think it's good to put it there in case other controllers need it in
>>> future)
>>>
>>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
>>> name of "legacy-irq" to it
>>
>> I don't like this. This adds two via-ide specific data to to common PCI IDE code
>> where it does not belong and subclassing it just for this also seems to be more
>> changes than really needed. Reusing the existing CMD646 field and generalising it to
>> allow implementation specific feature flags seems much less intrusive and not less
>> clean than your proposal.
>
> It's not VIA-specific though: the ISA legacy and PCI buses have different electrical
> characteristics and so by definition their signals must be driven by separate
> physical pins. Have a look at the CMD646 datasheet for example, and you will see that
> separate pins exist for legacy and PCI native IRQs.

For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy 
mode and thus those pins are not modelled so not needed now. For via-ide 
we only use ISA interrupts because even if we don't model legacy mode, 
boards expect ISA interrupts also in native mode maybe because this 
controller is not a separate PCI device only found embedded in 
southbridge/superio chips where they connect to the also embedded ISA PICs 
so even in native mode it should raise one of the ISA IRQs. My patch 
accesses ISA irqs with isa_get_irq() so no gpios and legacy irqs in 
PCIIDEState is neeeded and I don't see the need to introduce this 
complexity here. Also newer PCI ATA and SATA controllers such as sii3112 
do not have a legacy mode so I'd keep things related to that out of common 
PCI IDE code and model it instead in the controllers that have this as 
this does not seem to belong to PCI IDE.

>>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
>>> to pci_create() because the device shouldn't be realised immediately
>>>
>>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
>>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
>>> device
>>
>> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259
>> that I'm not sure how to access cleanly from code instantiating it. Is this better
>> than my patch? It seems it achieves the same via-ide specific behaviour just in a
>> more complicated way and would still need the feature bit to know when to use
>> legacy_irq[1].
>
> We know from the PCI specification that the existing code for via-ide is incorrect,
> and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in
> this way then both of these strongly suggest that current via-ide implementation is
> wrong. Rather than add a hack on top of a hack then the simplest solution is to
> physically wire the IRQ pin using qdev at the board level, as is done on real hardware.

But it's not done that way on real hardware and via-ide is not a PCI 
device but all this is internal to the VT8231 chip, the PICs, via-ide and 
a lot of other things which are modelled in QEMU by reusing existing 
components but I think we don't want to model the internal of the chip 
down to the smallest detail. In via-ide's case the PCI_INTERRUPT_LINE is 
probably not used by the IDE controller function but is used by some other 
function of the southbridge chip that implements interrupt controller but 
we don't have a separate model of that in QEMU so we can just emulate this 
function within via-ide which I think is OK as this IDE controller is part 
of these southbridges and not used anywhere else. This partly mixes IDE 
controller function and interrupt controller function but probably OK 
until we want to model this southbridge in more detail which could be done 
in later clean up patches. The piix model seems to do the same embedding a 
8259 which even less belongs to an IDE controller and acceses irqs array 
directly.

> Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should
> just be a case of returning the nth entry and using that with qdev_init_gpio_out_named().
>
>>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
>>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
>>> seems that both drives on MIPS and Pegasos both use IRQ 14).
>>
>> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode)
>> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native
>> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B
>> datasheet does not detail this but I believe it works the same. Since we currently
>> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI
>> reset interactions using legacy_irq[0] might work but is not correct, the current way
>> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.
>
> This is not correct though: please re-read my previous email which quotes from the
> PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the
> device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
> placing a value other than 14 there, but if it does then that suggests the board IRQs
> are physically wired differently which again should be handled at board level by qdev.

Correct or not or follows the spec or not this is how it works on real 
hardware and to get guests running we need to emulate this. Again, this 
controller is embedded in the 868B and 8231 southbridge chips so they may 
not completely confirm to PCI spec but their own datasheets. We can argue 
in how much detail do we want to model the internals of these chips (which 
we don't know for sure) but I think this patch is good enough for now and 
could be refined later or we likely won't be able to finish this before 
the freeze.

Another way to look at it is that this patch gets some guests running and 
does not break anything as far as I know so is there anything in it that's 
unacceptable now and needs to be changed for it to be merged? Unless you 
can propose a way to achieve the same in a simpler way now I think we 
could go with this and you can submit follow up patches to clean it up as 
you like later.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 06/03/2020 22:59, BALATON Zoltan wrote:

>>>> I've done a quick grep of the source tree and AFAICT the only IDE controller that
>>>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
>>>> fairly easy. In short:
>>>>
>>>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>>>>
>>>> (You could argue that it should belong in a separate VIAIDEState, however quite a
>>>> few
>>>> of the BMDMA controllers in QEMU don't have their own device state and just use
>>>> PCIIDEState. And whilst via-ide is the only one that currently needs support for
>>>> legacy IRQs, I think it's good to put it there in case other controllers need it in
>>>> future)
>>>>
>>>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
>>>> name of "legacy-irq" to it
>>>
>>> I don't like this. This adds two via-ide specific data to to common PCI IDE code
>>> where it does not belong and subclassing it just for this also seems to be more
>>> changes than really needed. Reusing the existing CMD646 field and generalising it to
>>> allow implementation specific feature flags seems much less intrusive and not less
>>> clean than your proposal.
>> It's not VIA-specific though: the ISA legacy and PCI buses have different electrical
>> characteristics and so by definition their signals must be driven by separate
>> physical pins. Have a look at the CMD646 datasheet for example, and you will see that
>> separate pins exist for legacy and PCI native IRQs.
> 
> For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and thus
> those pins are not modelled so not needed now.

Correct. It seems to me that having it there in PCIIDEState provides a convenient
place for all controllers that support legacy mode to store the IRQ in case someone
would like to add legacy support to the device at a later date.

If you really object to this then as I mentioned above, you'll need to add a
VIAIDEState or similar and have the legacy IRQs there for the qdev gpio connector.

> For via-ide we only use ISA interrupts
> because even if we don't model legacy mode, boards expect ISA interrupts also in
> native mode maybe because this controller is not a separate PCI device only found
> embedded in southbridge/superio chips where they connect to the also embedded ISA
> PICs so even in native mode it should raise one of the ISA IRQs.

I certainly agree with your analysis here, and that is borne out by the fact that you
are able to boot your OSs using your current hack with no PCI interrupts.

> My patch accesses
> ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is neeeded and
> I don't see the need to introduce this complexity here.

This isn't complexity though, it is just normal qdev usage. In fact if you look at
isa_get_irq() it contains this large comment above the function itself:

/*
 * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
 *
 * This function is only for special cases such as the 'ferr', and
 * temporary use for normal devices until they are converted to qdev.
 */

It was a temporary hack to allow old devices to access the 8259 IRQs before devices
were converted to qdev. And via-ide is a qdev device, so that's how you need to wire
it up.

> Also newer PCI ATA and SATA
> controllers such as sii3112 do not have a legacy mode so I'd keep things related to
> that out of common PCI IDE code and model it instead in the controllers that have
> this as this does not seem to belong to PCI IDE.

Like I said above: if you really don't want to put the legacy IRQs there then you
need to create a VIAIDEState for the qdev gpio connector.

>>>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
>>>> to pci_create() because the device shouldn't be realised immediately
>>>>
>>>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
>>>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
>>>> device
>>>
>>> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259
>>> that I'm not sure how to access cleanly from code instantiating it. Is this better
>>> than my patch? It seems it achieves the same via-ide specific behaviour just in a
>>> more complicated way and would still need the feature bit to know when to use
>>> legacy_irq[1].
>>
>> We know from the PCI specification that the existing code for via-ide is incorrect,
>> and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in
>> this way then both of these strongly suggest that current via-ide implementation is
>> wrong. Rather than add a hack on top of a hack then the simplest solution is to
>> physically wire the IRQ pin using qdev at the board level, as is done on real
>> hardware.
> 
> But it's not done that way on real hardware and via-ide is not a PCI device but all
> this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things
> which are modelled in QEMU by reusing existing components but I think we don't want
> to model the internal of the chip down to the smallest detail.

That's not what I'm suggesting though.

> In via-ide's case the
> PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is used by
> some other function of the southbridge chip that implements interrupt controller

What evidence do you have for this?

> but
> we don't have a separate model of that in QEMU so we can just emulate this function
> within via-ide which I think is OK as this IDE controller is part of these
> southbridges and not used anywhere else. This partly mixes IDE controller function
> and interrupt controller function but probably OK until we want to model this
> southbridge in more detail which could be done in later clean up patches. The piix
> model seems to do the same embedding a 8259 which even less belongs to an IDE
> controller and acceses irqs array directly.
> 
>> Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should
>> just be a case of returning the nth entry and using that with
>> qdev_init_gpio_out_named().
>>
>>>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
>>>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
>>>> seems that both drives on MIPS and Pegasos both use IRQ 14).
>>>
>>> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode)
>>> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native
>>> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B
>>> datasheet does not detail this but I believe it works the same. Since we currently
>>> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI
>>> reset interactions using legacy_irq[0] might work but is not correct, the current way
>>> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.
>>
>> This is not correct though: please re-read my previous email which quotes from the
>> PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the
>> device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
>> placing a value other than 14 there, but if it does then that suggests the board IRQs
>> are physically wired differently which again should be handled at board level by qdev.
> 
> Correct or not or follows the spec or not this is how it works on real hardware and
> to get guests running we need to emulate this. 

This may be a separate issue. As you mentioned in your email most guest OSs on
Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the IRQs
are hardwired. However you may also be getting caught out by the bug that the current
implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is
incorrect.

Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a default
value of 0xe, so I believe that once you've done the qdev gpio part, keeping this
part of your patchset as a separate patch should fix this.

> Again, this controller is embedded in
> the 868B and 8231 southbridge chips so they may not completely confirm to PCI spec
> but their own datasheets. We can argue in how much detail do we want to model the
> internals of these chips (which we don't know for sure) but I think this patch is
> good enough for now and could be refined later or we likely won't be able to finish
> this before the freeze.
> 
> Another way to look at it is that this patch gets some guests running and does not
> break anything as far as I know so is there anything in it that's unacceptable now
> and needs to be changed for it to be merged? Unless you can propose a way to achieve
> the same in a simpler way now I think we could go with this and you can submit follow
> up patches to clean it up as you like later.

I don't know what else to say except other than what I have already said: both the
current via-ide IRQ implementation and the changes in your patchset are incorrect.

I've given you the outline of what you need to do to switch the device over to qdev
which agrees with the datasheet, the PCI specification, comments within the QEMU code
itself and the results of your experiments. The proposed change is also simple to
implement (less than an hour's work) using standard APIs.

At this time I don't see any point in continuing to repeat myself over and over
again: if you can make the qdev change and post an updated patchset then I'm happy to
review and continue this discussion, but otherwise I don't see any purpose in
continuing this thread.


ATB,

Mark.

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Sat, 7 Mar 2020, Mark Cave-Ayland wrote:
> On 06/03/2020 22:59, BALATON Zoltan wrote:
>>>>> I've done a quick grep of the source tree and AFAICT the only IDE controller that
>>>>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be
>>>>> fairly easy. In short:
>>>>>
>>>>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>>>>>
>>>>> (You could argue that it should belong in a separate VIAIDEState, however quite a
>>>>> few
>>>>> of the BMDMA controllers in QEMU don't have their own device state and just use
>>>>> PCIIDEState. And whilst via-ide is the only one that currently needs support for
>>>>> legacy IRQs, I think it's good to put it there in case other controllers need it in
>>>>> future)
>>>>>
>>>>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a
>>>>> name of "legacy-irq" to it
>>>>
>>>> I don't like this. This adds two via-ide specific data to to common PCI IDE code
>>>> where it does not belong and subclassing it just for this also seems to be more
>>>> changes than really needed. Reusing the existing CMD646 field and generalising it to
>>>> allow implementation specific feature flags seems much less intrusive and not less
>>>> clean than your proposal.
>>> It's not VIA-specific though: the ISA legacy and PCI buses have different electrical
>>> characteristics and so by definition their signals must be driven by separate
>>> physical pins. Have a look at the CMD646 datasheet for example, and you will see that
>>> separate pins exist for legacy and PCI native IRQs.
>>
>> For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and thus
>> those pins are not modelled so not needed now.
>
> Correct. It seems to me that having it there in PCIIDEState provides a convenient
> place for all controllers that support legacy mode to store the IRQ in case someone
> would like to add legacy support to the device at a later date.

Why introduce it now and not only when someone would like to add legacy 
mode? I guess nobody needs legacy mode so we can avoid complexity 
modelling that unused detail now and only add it when needed.

> If you really object to this then as I mentioned above, you'll need to add a
> VIAIDEState or similar and have the legacy IRQs there for the qdev gpio connector.
>
>> For via-ide we only use ISA interrupts
>> because even if we don't model legacy mode, boards expect ISA interrupts also in
>> native mode maybe because this controller is not a separate PCI device only found
>> embedded in southbridge/superio chips where they connect to the also embedded ISA
>> PICs so even in native mode it should raise one of the ISA IRQs.
>
> I certainly agree with your analysis here, and that is borne out by the fact that you
> are able to boot your OSs using your current hack with no PCI interrupts.
>
>> My patch accesses
>> ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is neeeded and
>> I don't see the need to introduce this complexity here.
>
> This isn't complexity though, it is just normal qdev usage. In fact if you look at

Then qdev is probably overly complex for simple things.

> isa_get_irq() it contains this large comment above the function itself:
>
> /*
> * isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
> *
> * This function is only for special cases such as the 'ferr', and
> * temporary use for normal devices until they are converted to qdev.
> */
>
> It was a temporary hack to allow old devices to access the 8259 IRQs before devices
> were converted to qdev. And via-ide is a qdev device, so that's how you need to wire
> it up.
>
>> Also newer PCI ATA and SATA
>> controllers such as sii3112 do not have a legacy mode so I'd keep things related to
>> that out of common PCI IDE code and model it instead in the controllers that have
>> this as this does not seem to belong to PCI IDE.
>
> Like I said above: if you really don't want to put the legacy IRQs there then you
> need to create a VIAIDEState for the qdev gpio connector.

I still think what you propose is more complex than my patch and does not 
achieve any cleaner model. If you object to using isa_get_irq here I can 
go the piix way and embed the PIC array as well (or set a pointer to that 
with a property somehow) so I can use isa_irq[n] rather than adding 
non-existing pins for this. I won't do anything though until IDE and QEMU 
maintainers indicate what their preference is.

>>>>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple()
>>>>> to pci_create() because the device shouldn't be realised immediately
>>>>>
>>>>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
>>>>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the
>>>>> device
>>>>
>>>> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259
>>>> that I'm not sure how to access cleanly from code instantiating it. Is this better
>>>> than my patch? It seems it achieves the same via-ide specific behaviour just in a
>>>> more complicated way and would still need the feature bit to know when to use
>>>> legacy_irq[1].
>>>
>>> We know from the PCI specification that the existing code for via-ide is incorrect,
>>> and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in
>>> this way then both of these strongly suggest that current via-ide implementation is
>>> wrong. Rather than add a hack on top of a hack then the simplest solution is to
>>> physically wire the IRQ pin using qdev at the board level, as is done on real
>>> hardware.
>>
>> But it's not done that way on real hardware and via-ide is not a PCI device but all
>> this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things
>> which are modelled in QEMU by reusing existing components but I think we don't want
>> to model the internal of the chip down to the smallest detail.
>
> That's not what I'm suggesting though.

I think you did not like emulating the interrupt selection in via-ide that 
decides which irq to raise (which is really just one if() ) and would like 
to push it out to board code instead connecting it via gpios. But that's 
also not quite correct and would not even get rid of the feature bit you 
also disliked. In fact we don't actually have a qdev model for the 
VT82C686B where all this probably belongs but I don't plan to make a 
detailed qdevified model for that now (could be done later in a clean up 
patch maybe when I'll clean up pegasos2 patches for inclusion) and since 
via-ide is considered part of that southbridge (and reused in similar 
VT8231 model) we can include the interrupt controller emulation (this one 
if() ) as well for now and not worry about PCI specs that don't really 
apply to a "chip" that's not a standalone PCI IDE controller but part of a 
bigger southbridge chip. It's just reuses PCI IDE emulation from QEMU 
where appropriate.

>> In via-ide's case the
>> PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is used by
>> some other function of the southbridge chip that implements interrupt controller
>
> What evidence do you have for this?

The datasheet (of the IDE function) says that PCI_INTERRUPT_LINE selects 
the ISA interrupt that's raised in native mode for both channels and 
guests work if we emulate that. Since via-ide is part of the southbridge 
that also includes the PICs they are somehow connected inside and 
something selects the interrupt line within the chip but I don't know how 
exactly that's implemented in hardware. If not the ide part directly as 
you propose then some other interrupt controller part has to do this and 
we need to do the same somewhere in our model. Since we don't emulate the 
southbridge in more detail I've just put it in via-ide which seems to be a 
good place as it's only used in these southbridges and this could be 
changed later if via-ide were used elsewhere. No need to be more complex 
now than that I think.

>> but
>> we don't have a separate model of that in QEMU so we can just emulate this function
>> within via-ide which I think is OK as this IDE controller is part of these
>> southbridges and not used anywhere else. This partly mixes IDE controller function
>> and interrupt controller function but probably OK until we want to model this
>> southbridge in more detail which could be done in later clean up patches. The piix
>> model seems to do the same embedding a 8259 which even less belongs to an IDE
>> controller and acceses irqs array directly.
>>
>>> Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should
>>> just be a case of returning the nth entry and using that with
>>> qdev_init_gpio_out_named().
>>>
>>>>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do
>>>>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it
>>>>> seems that both drives on MIPS and Pegasos both use IRQ 14).
>>>>
>>>> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode)
>>>> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native
>>>> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B
>>>> datasheet does not detail this but I believe it works the same. Since we currently
>>>> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI
>>>> reset interactions using legacy_irq[0] might work but is not correct, the current way
>>>> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.
>>>
>>> This is not correct though: please re-read my previous email which quotes from the
>>> PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the
>>> device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
>>> placing a value other than 14 there, but if it does then that suggests the board IRQs
>>> are physically wired differently which again should be handled at board level by qdev.
>>
>> Correct or not or follows the spec or not this is how it works on real hardware and
>> to get guests running we need to emulate this.
>
> This may be a separate issue. As you mentioned in your email most guest OSs on
> Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the IRQs
> are hardwired. However you may also be getting caught out by the bug that the current
> implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is
> incorrect.

It's not incorrect. It follows what the datasheet says at least for 
VT8231, the 686B does not detail this but the Linux driver also works on 
fulong2e with this so I think it's correct for that as well.

> Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a default
> value of 0xe, so I believe that once you've done the qdev gpio part, keeping this
> part of your patchset as a separate patch should fix this.

I don't get this. My patch emulates that already, what do you want to be 
changed here?

>> Again, this controller is embedded in
>> the 868B and 8231 southbridge chips so they may not completely confirm to PCI spec
>> but their own datasheets. We can argue in how much detail do we want to model the
>> internals of these chips (which we don't know for sure) but I think this patch is
>> good enough for now and could be refined later or we likely won't be able to finish
>> this before the freeze.
>>
>> Another way to look at it is that this patch gets some guests running and does not
>> break anything as far as I know so is there anything in it that's unacceptable now
>> and needs to be changed for it to be merged? Unless you can propose a way to achieve
>> the same in a simpler way now I think we could go with this and you can submit follow
>> up patches to clean it up as you like later.
>
> I don't know what else to say except other than what I have already said: both the
> current via-ide IRQ implementation and the changes in your patchset are incorrect.

Why do you think so? You base this on PCI docs but those don't necessarily 
apply to via-ide which is not a standalone PCI device but embedded in 
integrated southbridges whose datasheets say what I've implemented and 
guests are happy with that. So I don't think it's incorrect maybe just not 
to your taste.

> I've given you the outline of what you need to do to switch the device over to qdev
> which agrees with the datasheet, the PCI specification, comments within the QEMU code
> itself and the results of your experiments. The proposed change is also simple to
> implement (less than an hour's work) using standard APIs.

Then please do that and submit alternative patches. I don't understand all 
of your proposals so it's easier if you can do it in less than an hour. It 
would probably take me more time. I'll test them with the clients if you 
provide patches or I've pushed my working version now to

https://osdn.net/projects/qmiga/scm/git/qemu/tree/pegasos2/

so you can try as well.

> At this time I don't see any point in continuing to repeat myself over and over
> again: if you can make the qdev change and post an updated patchset then I'm happy to
> review and continue this discussion, but otherwise I don't see any purpose in
> continuing this thread.

I also think we're not getting anywhere so I hope John and Peter can give 
some advice here what is needed to get these patches merged. I've run out 
of time for now so I won't have time to make extensive changes to these 
patches and can only make reasonable changes or test what you provide.

You seem to want to force me again to qdevify everything now which I don't 
have time for. You did the same to my OpenBIOS patches too where you 
wanted me to clean up PCI code and implement handling of multiple PCI 
buses when I just wanted to include a simple workaround to get MorphOS 
running until the bigger clean up is eventually done. Even though some 
years have passed you did still not come around to do the OpenBIOS PCI 
clean up but also not accepted my workaround in the meantime so I need to 
use patched OpenBIOS for MorphOS ever since. I think we'd end up in the 
same situation with these patches. (It would be OK if OpenBIOS were 
otherwise very clean but it's full of other workarounds like patching 
device tree and installing QEMU VGA driver to any VGA card it sees and 
others that are much worse than what I've proposed and you don't seem to 
mind those which come from you just those that someone else propose.) 
Don't get this wrong, we don't have any personal problem with each other, 
at least I don't have anything against you but your habit of forcing 
people to do much more additional work just to satisfy your pedantry is 
not something I have time to comply with. If you propose changes that make 
the patches simpler or in some way better I'd do that but otherwise I'll 
only do what QEMU maintainers also think is necessary.

Regards,
BALATON Zoltan