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 <address@hidden>
> ---
> 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 <address@hidden>
> + * 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.