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

[PATCH v2 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 and 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            | 57 +++++++++++++++++++++++++++++++++++------
 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, 69 insertions(+), 27 deletions(-)

-- 
2.21.1

[PATCH v2 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 d28f57199f..9c90b9355b 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 d33e84f831..fbe6790847 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 v2 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>
---
v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()

 hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
 hw/mips/mips_fulong2e.c |  2 +-
 include/hw/ide.h        |  3 ++-
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 096de8dba0..44ecc2af29 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,40 @@ 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 {
+        qemu_set_irq(isa_get_irq(NULL, 14), 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 +198,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 +243,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 +268,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 v2 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 09/03/2020 00:42, 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>
> ---
> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
> 
>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>  hw/mips/mips_fulong2e.c |  2 +-
>  include/hw/ide.h        |  3 ++-
>  3 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 096de8dba0..44ecc2af29 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,40 @@ 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 {
> +        qemu_set_irq(isa_get_irq(NULL, 14), level);
>      }
>  }

There's still the need to convert this to qdev gpio, but for now I'll review the
updated version of this patch (and provide an example once the rest of the patchset
is okay).

This looks much closer to what I was expecting with the fixed IRQs, and in fact
doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG bit is now
the single source of truth as to whether native or legacy IRQs are used, and the code
routes the IRQ accordingly.

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

The comment here is interesting so I had a quick look at pci_do_device_reset() to see
what is happening there, and to me it seems that the real bug is
pci_do_device_reset() doesn't honour wmask for PCI_INTERRUPT_LINE. This is because
normally the BIOS would write this value long after the PCI bus has been physically
reset and since via-ide is the first device to hardwire a value here, this wouldn't
have been needed up until now.

Fortunately it seems that there is already precedent for this so does the following
diff work?

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e1ed6677e1..4ae0e0e90f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -302,8 +302,10 @@ static void pci_do_device_reset(PCIDevice *dev)
     pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
                                  pci_get_word(dev->wmask + PCI_STATUS) |
                                  pci_get_word(dev->w1cmask + PCI_STATUS));
+    pci_word_test_and_clear_mask(dev->config + PCI_INTERRUPT_LINE,
+                                 pci_get_word(dev->wmask + PCI_INTERRUPT_LINE) |
+                                 pci_get_word(dev->w1cmask + PCI_INTERRUPT_LINE));
     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
-    dev->config[PCI_INTERRUPT_LINE] = 0x0;
     for (r = 0; r < PCI_NUM_REGIONS; ++r) {
         PCIIORegion *region = &dev->io_regions[r];
         if (!region->size) {

If this works, it will help simplify your patch even further.

>  static void via_ide_reset(DeviceState *dev)
>  {
>      PCIIDEState *d = PCI_IDE(dev);
> @@ -169,7 +198,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;

Possibly a separate patch for PCI_CLASS_PROG change? Although with the latest version
of the patchset is it even still needed?

> +    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 +243,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 +268,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 v2 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
> On 09/03/2020 00:42, 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>
>> ---
>> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
>>
>>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>>  hw/mips/mips_fulong2e.c |  2 +-
>>  include/hw/ide.h        |  3 ++-
>>  3 files changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..44ecc2af29 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,40 @@ 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 {
>> +        qemu_set_irq(isa_get_irq(NULL, 14), level);
>>      }
>>  }
>
> There's still the need to convert this to qdev gpio, but for now I'll review the
> updated version of this patch (and provide an example once the rest of the patchset
> is okay).

There's no need to do that now. I think it only makes sense to do that 
when the 686B and VT8231 models are also qdevified (which I'll plan to do 
when cleaning up my pegasos2 patches later) since that may change how this 
should be qdevified. But I don't have time to fully do it now so don't 
even ask. This will have to do for now as it's not worse than it is 
already and does fix clients so I see no immediate need to force more 
clean ups upon me.

> This looks much closer to what I was expecting with the fixed IRQs, and in fact
> doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG bit is now
> the single source of truth as to whether native or legacy IRQs are used, and the code
> routes the IRQ accordingly.

No, the feature bit is still needed to flag if this device should work 
with 100% native mode as on fulong2e or with forced legacy IRQ non-100% 
native mode as on pegasos2. In both cases PCI_CLASS_PROG is actually set 
to native mode (most of the time, Linux fixes this up on pegasos2 for it's 
own convenience but other OSes don't care about it but we still need to 
know to use legacy interrupts) so the feature bit is needed to know when 
to use legacy and when native interrupts.

The hardcoded IRQ14 in native mode is also wrong, If you check VT8231 
datasheet it clearly says that the IRQ raised is selected by 
PCI_INTERRUPT_LINE so I think my previous version was correct but this 
still works because we fixed PCI_INTERRUPT_LINE to 14 so we know here it's 
14 without looking at the config reg that you forbid me to do. But due to 
coincidence it still works and matches your ideals about PCI specs that I 
don't think apply for this device but I could not convince you about that 
so if this is what it takes then so be it. As long as it works with 
clients I don't care, we can always clean this up later.

>> +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);
>> +}
>
> The comment here is interesting so I had a quick look at pci_do_device_reset() to see
> what is happening there, and to me it seems that the real bug is
> pci_do_device_reset() doesn't honour wmask for PCI_INTERRUPT_LINE. This is because
> normally the BIOS would write this value long after the PCI bus has been physically
> reset and since via-ide is the first device to hardwire a value here, this wouldn't
> have been needed up until now.
>
> Fortunately it seems that there is already precedent for this so does the following
> diff work?
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e1ed6677e1..4ae0e0e90f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -302,8 +302,10 @@ static void pci_do_device_reset(PCIDevice *dev)
>     pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
>                                  pci_get_word(dev->wmask + PCI_STATUS) |
>                                  pci_get_word(dev->w1cmask + PCI_STATUS));
> +    pci_word_test_and_clear_mask(dev->config + PCI_INTERRUPT_LINE,
> +                                 pci_get_word(dev->wmask + PCI_INTERRUPT_LINE) |
> +                                 pci_get_word(dev->w1cmask + PCI_INTERRUPT_LINE));
>     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
> -    dev->config[PCI_INTERRUPT_LINE] = 0x0;
>     for (r = 0; r < PCI_NUM_REGIONS; ++r) {
>         PCIIORegion *region = &dev->io_regions[r];
>         if (!region->size) {
>
> If this works, it will help simplify your patch even further.

This seems to work but originally I did not want to touch anything that 
could break other parts so I did not investigate this further. Thanks for 
the suggestion, I've added this patch in v3 and removed workaround from 
via-ide.

>>  static void via_ide_reset(DeviceState *dev)
>>  {
>>      PCIIDEState *d = PCI_IDE(dev);
>> @@ -169,7 +198,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;
>
> Possibly a separate patch for PCI_CLASS_PROG change? Although with the latest version
> of the patchset is it even still needed?

I don't see a need to add another one line patch for this here unless 
someone authorative asks for that. This is needed to allow Linux on 
pegasos2 to use this reg to signal its driver that it should use legacy 
interrupts. That is, while everything else is using native mode and this 
reg should also say that the pegasos2 fixup function of Linux sets this 
reg to legacy mode so it knows later that legacy interrupts should be used 
on this board. Therefore we need to allow this change.

Regards,
BALATON Zoltan

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

[Mark Cave-Ayland]

On 09/03/2020 20:17, BALATON Zoltan wrote:

> On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
>> On 09/03/2020 00:42, 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>
>>> ---
>>> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
>>>
>>>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>>>  hw/mips/mips_fulong2e.c |  2 +-
>>>  include/hw/ide.h        |  3 ++-
>>>  3 files changed, 52 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 096de8dba0..44ecc2af29 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,40 @@ 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 {
>>> +        qemu_set_irq(isa_get_irq(NULL, 14), level);
>>>      }
>>>  }
>>
>> There's still the need to convert this to qdev gpio, but for now I'll review the
>> updated version of this patch (and provide an example once the rest of the patchset
>> is okay).
> 
> There's no need to do that now. I think it only makes sense to do that when the 686B
> and VT8231 models are also qdevified (which I'll plan to do when cleaning up my
> pegasos2 patches later) since that may change how this should be qdevified. But I
> don't have time to fully do it now so don't even ask. This will have to do for now as
> it's not worse than it is already and does fix clients so I see no immediate need to
> force more clean ups upon me.
> 
>> This looks much closer to what I was expecting with the fixed IRQs, and in fact
>> doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG bit is now
>> the single source of truth as to whether native or legacy IRQs are used, and the code
>> routes the IRQ accordingly.
> 
> No, the feature bit is still needed to flag if this device should work with 100%
> native mode as on fulong2e or with forced legacy IRQ non-100% native mode as on
> pegasos2. In both cases PCI_CLASS_PROG is actually set to native mode (most of the
> time, Linux fixes this up on pegasos2 for it's own convenience but other OSes don't
> care about it but we still need to know to use legacy interrupts) so the feature bit
> is needed to know when to use legacy and when native interrupts.
> 
> The hardcoded IRQ14 in native mode is also wrong, If you check VT8231 datasheet it
> clearly says that the IRQ raised is selected by PCI_INTERRUPT_LINE so I think my
> previous version was correct but this still works because we fixed PCI_INTERRUPT_LINE
> to 14 so we know here it's 14 without looking at the config reg that you forbid me to
> do. But due to coincidence it still works and matches your ideals about PCI specs
> that I don't think apply for this device but I could not convince you about that so
> if this is what it takes then so be it. As long as it works with clients I don't
> care, we can always clean this up later.

Actually I've just realised that the conversion to qdev solves all these issues,
because qdev has the concept of connecting outputs to inputs. The way to do this is
to define the VIA(PCI)IDEState with an array of two name legacy IRQs (as detailed in
my previous email), plus the PCI IRQ.

Then all via_ide_set_irq() has to do is set each qdev gpio level accordingly; if a
board isn't interested in an IRQ it just doesn't wire it up. The final piece of the
puzzle is what do we do in PCI native mode, but that is simple: always drive the
legacy IRQs if there is no PCI IRQ connected.

>>> +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);
>>> +}
>>
>> The comment here is interesting so I had a quick look at pci_do_device_reset() to see
>> what is happening there, and to me it seems that the real bug is
>> pci_do_device_reset() doesn't honour wmask for PCI_INTERRUPT_LINE. This is because
>> normally the BIOS would write this value long after the PCI bus has been physically
>> reset and since via-ide is the first device to hardwire a value here, this wouldn't
>> have been needed up until now.
>>
>> Fortunately it seems that there is already precedent for this so does the following
>> diff work?
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index e1ed6677e1..4ae0e0e90f 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -302,8 +302,10 @@ static void pci_do_device_reset(PCIDevice *dev)
>>     pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
>>                                  pci_get_word(dev->wmask + PCI_STATUS) |
>>                                  pci_get_word(dev->w1cmask + PCI_STATUS));
>> +    pci_word_test_and_clear_mask(dev->config + PCI_INTERRUPT_LINE,
>> +                                 pci_get_word(dev->wmask + PCI_INTERRUPT_LINE) |
>> +                                 pci_get_word(dev->w1cmask + PCI_INTERRUPT_LINE));
>>     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
>> -    dev->config[PCI_INTERRUPT_LINE] = 0x0;
>>     for (r = 0; r < PCI_NUM_REGIONS; ++r) {
>>         PCIIORegion *region = &dev->io_regions[r];
>>         if (!region->size) {
>>
>> If this works, it will help simplify your patch even further.
> 
> This seems to work but originally I did not want to touch anything that could break
> other parts so I did not investigate this further. Thanks for the suggestion, I've
> added this patch in v3 and removed workaround from via-ide.
> 
>>>  static void via_ide_reset(DeviceState *dev)
>>>  {
>>>      PCIIDEState *d = PCI_IDE(dev);
>>> @@ -169,7 +198,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;
>>
>> Possibly a separate patch for PCI_CLASS_PROG change? Although with the latest version
>> of the patchset is it even still needed?
> 
> I don't see a need to add another one line patch for this here unless someone
> authorative asks for that. This is needed to allow Linux on pegasos2 to use this reg
> to signal its driver that it should use legacy interrupts. That is, while everything
> else is using native mode and this reg should also say that the pegasos2 fixup
> function of Linux sets this reg to legacy mode so it knows later that legacy
> interrupts should be used on this board. Therefore we need to allow this change.


ATB,

Mark.

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

[BALATON Zoltan]

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

On Tue, 10 Mar 2020, Mark Cave-Ayland wrote:
> On 09/03/2020 20:17, BALATON Zoltan wrote:
>> On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
>>> On 09/03/2020 00:42, 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>
>>>> ---
>>>> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
>>>>
>>>>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>>>>  hw/mips/mips_fulong2e.c |  2 +-
>>>>  include/hw/ide.h        |  3 ++-
>>>>  3 files changed, 52 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index 096de8dba0..44ecc2af29 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,40 @@ 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 {
>>>> +        qemu_set_irq(isa_get_irq(NULL, 14), level);
>>>>      }
>>>>  }
>>>
>>> There's still the need to convert this to qdev gpio, but for now I'll review the
>>> updated version of this patch (and provide an example once the rest of the patchset
>>> is okay).
>>
>> There's no need to do that now. I think it only makes sense to do that when the 686B
>> and VT8231 models are also qdevified (which I'll plan to do when cleaning up my
>> pegasos2 patches later) since that may change how this should be qdevified. But I
>> don't have time to fully do it now so don't even ask. This will have to do for now as
>> it's not worse than it is already and does fix clients so I see no immediate need to
>> force more clean ups upon me.
>>
>>> This looks much closer to what I was expecting with the fixed IRQs, and in fact
>>> doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG bit is now
>>> the single source of truth as to whether native or legacy IRQs are used, and the code
>>> routes the IRQ accordingly.
>>
>> No, the feature bit is still needed to flag if this device should work with 100%
>> native mode as on fulong2e or with forced legacy IRQ non-100% native mode as on
>> pegasos2. In both cases PCI_CLASS_PROG is actually set to native mode (most of the
>> time, Linux fixes this up on pegasos2 for it's own convenience but other OSes don't
>> care about it but we still need to know to use legacy interrupts) so the feature bit
>> is needed to know when to use legacy and when native interrupts.
>>
>> The hardcoded IRQ14 in native mode is also wrong, If you check VT8231 datasheet it
>> clearly says that the IRQ raised is selected by PCI_INTERRUPT_LINE so I think my
>> previous version was correct but this still works because we fixed PCI_INTERRUPT_LINE
>> to 14 so we know here it's 14 without looking at the config reg that you forbid me to
>> do. But due to coincidence it still works and matches your ideals about PCI specs
>> that I don't think apply for this device but I could not convince you about that so
>> if this is what it takes then so be it. As long as it works with clients I don't
>> care, we can always clean this up later.
>
> Actually I've just realised that the conversion to qdev solves all these issues,
> because qdev has the concept of connecting outputs to inputs. The way to do this is
> to define the VIA(PCI)IDEState with an array of two name legacy IRQs (as detailed in
> my previous email), plus the PCI IRQ.

This device (being part of an integrated southbridge chip) has no PCI IRQ 
at all, stop talking about it, that only may apply to CMD646. Once you 
accept this it may be easier to actually get to a solution. The via-ide 
either drives the legacy interrupts in legacy mode and on pegasos2 always, 
regardless of mode. Or in true native mode it drives one ISA interrupt for 
both channels selected by PCI_INTERRUPT_LINE config reg. This is clear 
from the VT8231 datasheet and my test with guests.

> Then all via_ide_set_irq() has to do is set each qdev gpio level accordingly; if a
> board isn't interested in an IRQ it just doesn't wire it up. The final piece of the
> puzzle is what do we do in PCI native mode, but that is simple: always drive the
> legacy IRQs if there is no PCI IRQ connected.

How does the device code know if there's anything connected to the gpio it 
defines? I don't see your proposal would be simpler or even easier to 
understand (aka cleaner) than what I've implemented. You'll need to 
explain a bit more but I think the real solution is to model VT82C686B and 
VT8231 and not parts of it in qdev. We don't want to model the inside of 
these chips.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

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

On Tue, 10 Mar 2020, BALATON Zoltan wrote:
> On Tue, 10 Mar 2020, Mark Cave-Ayland wrote:
>> On 09/03/2020 20:17, BALATON Zoltan wrote:
>>> On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 09/03/2020 00:42, 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>
>>>>> ---
>>>>> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
>>>>> 
>>>>>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>>>>>  hw/mips/mips_fulong2e.c |  2 +-
>>>>>  include/hw/ide.h        |  3 ++-
>>>>>  3 files changed, 52 insertions(+), 10 deletions(-)
>>>>> 
>>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>>> index 096de8dba0..44ecc2af29 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,40 @@ 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 {
>>>>> +        qemu_set_irq(isa_get_irq(NULL, 14), level);
>>>>>      }
>>>>>  }
>>>> 
>>>> There's still the need to convert this to qdev gpio, but for now I'll 
>>>> review the
>>>> updated version of this patch (and provide an example once the rest of 
>>>> the patchset
>>>> is okay).
>>> 
>>> There's no need to do that now. I think it only makes sense to do that 
>>> when the 686B
>>> and VT8231 models are also qdevified (which I'll plan to do when cleaning 
>>> up my
>>> pegasos2 patches later) since that may change how this should be 
>>> qdevified. But I
>>> don't have time to fully do it now so don't even ask. This will have to do 
>>> for now as
>>> it's not worse than it is already and does fix clients so I see no 
>>> immediate need to
>>> force more clean ups upon me.
>>> 
>>>> This looks much closer to what I was expecting with the fixed IRQs, and 
>>>> in fact
>>>> doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG 
>>>> bit is now
>>>> the single source of truth as to whether native or legacy IRQs are used, 
>>>> and the code
>>>> routes the IRQ accordingly.
>>> 
>>> No, the feature bit is still needed to flag if this device should work 
>>> with 100%
>>> native mode as on fulong2e or with forced legacy IRQ non-100% native mode 
>>> as on
>>> pegasos2. In both cases PCI_CLASS_PROG is actually set to native mode 
>>> (most of the
>>> time, Linux fixes this up on pegasos2 for it's own convenience but other 
>>> OSes don't
>>> care about it but we still need to know to use legacy interrupts) so the 
>>> feature bit
>>> is needed to know when to use legacy and when native interrupts.
>>> 
>>> The hardcoded IRQ14 in native mode is also wrong, If you check VT8231 
>>> datasheet it
>>> clearly says that the IRQ raised is selected by PCI_INTERRUPT_LINE so I 
>>> think my
>>> previous version was correct but this still works because we fixed 
>>> PCI_INTERRUPT_LINE
>>> to 14 so we know here it's 14 without looking at the config reg that you 
>>> forbid me to
>>> do. But due to coincidence it still works and matches your ideals about 
>>> PCI specs
>>> that I don't think apply for this device but I could not convince you 
>>> about that so
>>> if this is what it takes then so be it. As long as it works with clients I 
>>> don't
>>> care, we can always clean this up later.
>> 
>> Actually I've just realised that the conversion to qdev solves all these issues,
>> because qdev has the concept of connecting outputs to inputs. The way to do this is
>> to define the VIA(PCI)IDEState with an array of two name legacy IRQs (as detailed in
>> my previous email), plus the PCI IRQ.
>
> This device (being part of an integrated southbridge chip) has no PCI IRQ at 
> all, stop talking about it, that only may apply to CMD646. Once you accept 
> this it may be easier to actually get to a solution. The via-ide either 
> drives the legacy interrupts in legacy mode and on pegasos2 always, 
> regardless of mode. Or in true native mode it drives one ISA interrupt for 
> both channels selected by PCI_INTERRUPT_LINE config reg. This is clear from 
> the VT8231 datasheet and my test with guests.
>
>> Then all via_ide_set_irq() has to do is set each qdev gpio level accordingly; if a
>> board isn't interested in an IRQ it just doesn't wire it up. The final piece of the
>> puzzle is what do we do in PCI native mode, but that is simple: always drive the
>> legacy IRQs if there is no PCI IRQ connected.

Also even though we don't model that but real hardware can switch between 
native and legacy mode so in reality all boards need all irqs as they 
could be used depending on device setting. I think this legacy-irq thing 
is something peculiar to pegasos2 so using a feature but only set by that 
board is appropriate here. Unless you can propose something simpler you 
could just accept it at least for now and maybe we can try to clean it up 
later when other parts of the southbridge are qdevified.

> How does the device code know if there's anything connected to the gpio it 
> defines? I don't see your proposal would be simpler or even easier to 
> understand (aka cleaner) than what I've implemented. You'll need to explain a 
> bit more but I think the real solution is to model VT82C686B and VT8231 and 
> not parts of it in qdev. We don't want to model the inside of these chips.
>
> Regards,
> BALATON Zoltan

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

[Mark Cave-Ayland]

On 09/03/2020 00:42, 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>
> ---
> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
> 
>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>  hw/mips/mips_fulong2e.c |  2 +-
>  include/hw/ide.h        |  3 ++-
>  3 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 096de8dba0..44ecc2af29 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,40 @@ 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)) ||

One other thing whilst I'm here: the above line is quite cryptic to read unless
you're very familiar with the PCI IDE specifications. How about something like this
towards the top of the function:

int native = n ? pci_get_byte(d->config + PCI_CLASS_PROG) & 4 :
                 pci_get_byte(d->config + PCI_CLASS_PROG) & 1;

and change the if() accordingly:

if (native) {
   ...
} else {
   ...
}

With that you can could probably drop the comment since it's really obvious what the
code is doing when reading it against the datasheet.

> +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
> +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
> +    } else {
> +        qemu_set_irq(isa_get_irq(NULL, 14), 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 +198,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 +243,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 +268,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 v2 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
> On 09/03/2020 00:42, 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>
>> ---
>> v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
>>
>>  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
>>  hw/mips/mips_fulong2e.c |  2 +-
>>  include/hw/ide.h        |  3 ++-
>>  3 files changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 096de8dba0..44ecc2af29 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,40 @@ 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)) ||
>
> One other thing whilst I'm here: the above line is quite cryptic to read unless
> you're very familiar with the PCI IDE specifications. How about something like this
> towards the top of the function:
>
> int native = n ? pci_get_byte(d->config + PCI_CLASS_PROG) & 4 :
>                 pci_get_byte(d->config + PCI_CLASS_PROG) & 1;
>
> and change the if() accordingly:
>
> if (native) {
>   ...
> } else {
>   ...
> }
>
> With that you can could probably drop the comment since it's really obvious what the
> code is doing when reading it against the datasheet.

Too late, already sent v3, may consider later but the comment is actually 
for the line after the || not the one checking CLASS_PROG.

Regards,
BALATON Zoltan