[Qemu-devel] [PATCH 0/8] ppc: more Mac-related fixups

[Qemu-devel] [PATCH 0/8] ppc: more Mac-related fixups

[Mark Cave-Ayland]

Here is the latest set of Mac-related PPC patches based upon Ben's earlier
work to improve MacOS compatibility, with various fixups and other odds and
ends from me.

Note this patch bumps the vmstate_openpic_timer version as it includes extra
fields, however I don't see this currently being an issue since migration of
the Mac machines is fairly unreliable right now.

With many thanks to Howard Spoelstra over at emaculation for helping test
against a larger set of images than I currently have available myself.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Benjamin Herrenschmidt (5):
  ppc/mac: Advertise a high clock frequency for NewWorld Macs
  ppc/ide/macio: Add missing registers
  ppc/mac: More rework of the DBDMA emulation
  ppc: Fix OpenPIC model
  openpic: Fix problem when IRQ transitions from edge to level

Mark Cave-Ayland (3):
  ppc: QOMify g3beige machine
  macio: convert pmac_ide_ops from old_mmio
  openpic: add missing timer fields to vmstate_openpic_timer

 hw/ide/macio.c            |  170 ++++++++++++++++++++--------------------
 hw/intc/openpic.c         |   76 ++++++++++++++++--
 hw/misc/macio/mac_dbdma.c |  191 +++++++++++++++++++++++++++++++++------------
 hw/ppc/mac.h              |    6 +-
 hw/ppc/mac_newworld.c     |    4 +-
 hw/ppc/mac_oldworld.c     |   17 +++-
 include/hw/ppc/openpic.h  |    1 +
 7 files changed, 313 insertions(+), 152 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/8] ppc: QOMify g3beige machine

[Mark Cave-Ayland]

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

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index fcac399..5d1171d 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -380,8 +380,10 @@ static int heathrow_kvm_type(const char *arg)
     return 2;
 }
 
-static void heathrow_machine_init(MachineClass *mc)
+static void heathrow_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->desc = "Heathrow based PowerMAC";
     mc->init = ppc_heathrow_init;
     mc->block_default_type = IF_IDE;
@@ -394,4 +396,15 @@ static void heathrow_machine_init(MachineClass *mc)
     mc->kvm_type = heathrow_kvm_type;
 }
 
-DEFINE_MACHINE("g3beige", heathrow_machine_init)
+static const TypeInfo ppc_heathrow_machine_info = {
+    .name          = MACHINE_TYPE_NAME("g3beige"),
+    .parent        = TYPE_MACHINE,
+    .class_init    = heathrow_class_init
+};
+
+static void ppc_heathrow_register_types(void)
+{
+    type_register_static(&ppc_heathrow_machine_info);
+}
+
+type_init(ppc_heathrow_register_types);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/8] ppc: QOMify g3beige machine

[Philippe Mathieu-Daudé]

On 09/17/2017 02:15 PM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   hw/ppc/mac_oldworld.c |   17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index fcac399..5d1171d 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -380,8 +380,10 @@ static int heathrow_kvm_type(const char *arg)
>       return 2;
>   }
>   
> -static void heathrow_machine_init(MachineClass *mc)
> +static void heathrow_class_init(ObjectClass *oc, void *data)
>   {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>       mc->desc = "Heathrow based PowerMAC";
>       mc->init = ppc_heathrow_init;
>       mc->block_default_type = IF_IDE;
> @@ -394,4 +396,15 @@ static void heathrow_machine_init(MachineClass *mc)
>       mc->kvm_type = heathrow_kvm_type;
>   }
>   
> -DEFINE_MACHINE("g3beige", heathrow_machine_init)
> +static const TypeInfo ppc_heathrow_machine_info = {
> +    .name          = MACHINE_TYPE_NAME("g3beige"),
> +    .parent        = TYPE_MACHINE,
> +    .class_init    = heathrow_class_init
> +};
> +
> +static void ppc_heathrow_register_types(void)
> +{
> +    type_register_static(&ppc_heathrow_machine_info);
> +}
> +
> +type_init(ppc_heathrow_register_types);
> 

Re: [Qemu-devel] [PATCH 1/8] ppc: QOMify g3beige machine

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:41PM +0100, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-2.11.

> ---
>  hw/ppc/mac_oldworld.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index fcac399..5d1171d 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -380,8 +380,10 @@ static int heathrow_kvm_type(const char *arg)
>      return 2;
>  }
>  
> -static void heathrow_machine_init(MachineClass *mc)
> +static void heathrow_class_init(ObjectClass *oc, void *data)
>  {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>      mc->desc = "Heathrow based PowerMAC";
>      mc->init = ppc_heathrow_init;
>      mc->block_default_type = IF_IDE;
> @@ -394,4 +396,15 @@ static void heathrow_machine_init(MachineClass *mc)
>      mc->kvm_type = heathrow_kvm_type;
>  }
>  
> -DEFINE_MACHINE("g3beige", heathrow_machine_init)
> +static const TypeInfo ppc_heathrow_machine_info = {
> +    .name          = MACHINE_TYPE_NAME("g3beige"),
> +    .parent        = TYPE_MACHINE,
> +    .class_init    = heathrow_class_init
> +};
> +
> +static void ppc_heathrow_register_types(void)
> +{
> +    type_register_static(&ppc_heathrow_machine_info);
> +}
> +
> +type_init(ppc_heathrow_register_types);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/8] ppc/mac: Advertise a high clock frequency for NewWorld Macs

[Mark Cave-Ayland]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

We use 900Mhz, otherwise MacOS X 10.5 refuses to install.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/ppc/mac_newworld.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index d466634..c581d96 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -77,7 +77,7 @@
 #define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
 #define TBFREQ (100UL * 1000UL * 1000UL)
-#define CLOCKFREQ (266UL * 1000UL * 1000UL)
+#define CLOCKFREQ (900UL * 1000UL * 1000UL)
 #define BUSFREQ (100UL * 1000UL * 1000UL)
 
 #define NDRV_VGA_FILENAME "qemu_vga.ndrv"
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/8] ppc/mac: Advertise a high clock frequency for NewWorld Macs

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:42PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> We use 900Mhz, otherwise MacOS X 10.5 refuses to install.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to ppc-for-2.11.

> ---
>  hw/ppc/mac_newworld.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index d466634..c581d96 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -77,7 +77,7 @@
>  #define MAX_IDE_BUS 2
>  #define CFG_ADDR 0xf0000510
>  #define TBFREQ (100UL * 1000UL * 1000UL)
> -#define CLOCKFREQ (266UL * 1000UL * 1000UL)
> +#define CLOCKFREQ (900UL * 1000UL * 1000UL)
>  #define BUSFREQ (100UL * 1000UL * 1000UL)
>  
>  #define NDRV_VGA_FILENAME "qemu_vga.ndrv"

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 3/8] ppc/ide/macio: Add missing registers

[Mark Cave-Ayland]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

The timing register exists on all variants of MacIO IDE, we just
store and return its value.

The interrupts register only exists on KeyLargo but it doesn't
hurt to have it. The lack of this register causes MacOS X to
hangs under some circumstances.

Both are 32-bit only. The HW might support smaller access sizes
but no known OS uses them.

Because the core IDE subsystem doesn't provide us with a way
to query the main (level) interrupt state, nor do we have a way
to know that DBDMA issued a (edge) interrupt, we reflect both
through a private pair of qirq's in order to maintain the
register state.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/ide/macio.c |   44 +++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/mac.h   |    6 +++++-
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 9742c00..db5db39 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -331,6 +331,12 @@ static void pmac_ide_writel (void *opaque,
     val = bswap32(val);
     if (addr == 0) {
         ide_data_writel(&d->bus, 0, val);
+    } else if (addr == 0x20) {
+        d->timing_reg = val;
+    } else if (addr == 0x30) {
+        if (val & 0x80000000u) {
+            d->irq_reg &= 0x7fffffff;
+        }
     }
 }
 
@@ -342,6 +348,17 @@ static uint32_t pmac_ide_readl (void *opaque,hwaddr addr)
     addr = (addr & 0xFFF) >> 4;
     if (addr == 0) {
         retval = ide_data_readl(&d->bus, 0);
+    } else if (addr == 0x20) {
+        retval = d->timing_reg;
+    } else if (addr == 0x30) {
+        /* This is an interrupt state register that only exists
+         * in the KeyLargo and later variants. Bit 0x8000_0000
+         * latches the DMA interrupt and has to be written to
+         * clear. Bit 0x4000_0000 is an image of the disk
+         * interrupt. MacOS X relies on this and will hang if
+         * we don't provide at least the disk interrupt
+         */
+        retval = d->irq_reg;
     } else {
         retval = 0xFFFFFFFF;
     }
@@ -426,13 +443,32 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
 {
     MACIOIDEState *s = MACIO_IDE(dev);
 
-    ide_init2(&s->bus, s->irq);
+    ide_init2(&s->bus, s->ide_irq);
 
     /* Register DMA callbacks */
     s->dma.ops = &dbdma_ops;
     s->bus.dma = &s->dma;
 }
 
+static void pmac_ide_irq(void *opaque, int n, int level)
+{
+    MACIOIDEState *s = opaque;
+    uint32_t mask = 0x80000000u >> n;
+
+    /* We need to reflect the IRQ state in the irq register */
+    if (level) {
+        s->irq_reg |= mask;
+    } else {
+        s->irq_reg &= ~mask;
+    }
+
+    if (n) {
+        qemu_set_irq(s->real_ide_irq, level);
+    } else {
+        qemu_set_irq(s->real_dma_irq, level);
+    }
+}
+
 static void macio_ide_initfn(Object *obj)
 {
     SysBusDevice *d = SYS_BUS_DEVICE(obj);
@@ -441,8 +477,10 @@ static void macio_ide_initfn(Object *obj)
     ide_bus_new(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2);
     memory_region_init_io(&s->mem, obj, &pmac_ide_ops, s, "pmac-ide", 0x1000);
     sysbus_init_mmio(d, &s->mem);
-    sysbus_init_irq(d, &s->irq);
-    sysbus_init_irq(d, &s->dma_irq);
+    sysbus_init_irq(d, &s->real_ide_irq);
+    sysbus_init_irq(d, &s->real_dma_irq);
+    s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
+    s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
 }
 
 static void macio_ide_class_init(ObjectClass *oc, void *data)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 20cbddb..300fc8a 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -132,7 +132,9 @@ typedef struct MACIOIDEState {
     SysBusDevice parent_obj;
     /*< public >*/
 
-    qemu_irq irq;
+    qemu_irq real_ide_irq;
+    qemu_irq real_dma_irq;
+    qemu_irq ide_irq;
     qemu_irq dma_irq;
 
     MemoryRegion mem;
@@ -140,6 +142,8 @@ typedef struct MACIOIDEState {
     IDEDMA dma;
     void *dbdma;
     bool dma_active;
+    uint32_t timing_reg;
+    uint32_t irq_reg;
 } MACIOIDEState;
 
 void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/8] ppc/ide/macio: Add missing registers

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:43PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> The timing register exists on all variants of MacIO IDE, we just
> store and return its value.
> 
> The interrupts register only exists on KeyLargo but it doesn't
> hurt to have it. The lack of this register causes MacOS X to
> hangs under some circumstances.
> 
> Both are 32-bit only. The HW might support smaller access sizes
> but no known OS uses them.
> 
> Because the core IDE subsystem doesn't provide us with a way
> to query the main (level) interrupt state, nor do we have a way
> to know that DBDMA issued a (edge) interrupt, we reflect both
> through a private pair of qirq's in order to maintain the
> register state.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to ppc-for-2.11.

> ---
>  hw/ide/macio.c |   44 +++++++++++++++++++++++++++++++++++++++++---
>  hw/ppc/mac.h   |    6 +++++-
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 9742c00..db5db39 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -331,6 +331,12 @@ static void pmac_ide_writel (void *opaque,
>      val = bswap32(val);
>      if (addr == 0) {
>          ide_data_writel(&d->bus, 0, val);
> +    } else if (addr == 0x20) {
> +        d->timing_reg = val;
> +    } else if (addr == 0x30) {
> +        if (val & 0x80000000u) {
> +            d->irq_reg &= 0x7fffffff;
> +        }
>      }
>  }
>  
> @@ -342,6 +348,17 @@ static uint32_t pmac_ide_readl (void *opaque,hwaddr addr)
>      addr = (addr & 0xFFF) >> 4;
>      if (addr == 0) {
>          retval = ide_data_readl(&d->bus, 0);
> +    } else if (addr == 0x20) {
> +        retval = d->timing_reg;
> +    } else if (addr == 0x30) {
> +        /* This is an interrupt state register that only exists
> +         * in the KeyLargo and later variants. Bit 0x8000_0000
> +         * latches the DMA interrupt and has to be written to
> +         * clear. Bit 0x4000_0000 is an image of the disk
> +         * interrupt. MacOS X relies on this and will hang if
> +         * we don't provide at least the disk interrupt
> +         */
> +        retval = d->irq_reg;
>      } else {
>          retval = 0xFFFFFFFF;
>      }
> @@ -426,13 +443,32 @@ static void macio_ide_realizefn(DeviceState *dev, Error **errp)
>  {
>      MACIOIDEState *s = MACIO_IDE(dev);
>  
> -    ide_init2(&s->bus, s->irq);
> +    ide_init2(&s->bus, s->ide_irq);
>  
>      /* Register DMA callbacks */
>      s->dma.ops = &dbdma_ops;
>      s->bus.dma = &s->dma;
>  }
>  
> +static void pmac_ide_irq(void *opaque, int n, int level)
> +{
> +    MACIOIDEState *s = opaque;
> +    uint32_t mask = 0x80000000u >> n;
> +
> +    /* We need to reflect the IRQ state in the irq register */
> +    if (level) {
> +        s->irq_reg |= mask;
> +    } else {
> +        s->irq_reg &= ~mask;
> +    }
> +
> +    if (n) {
> +        qemu_set_irq(s->real_ide_irq, level);
> +    } else {
> +        qemu_set_irq(s->real_dma_irq, level);
> +    }
> +}
> +
>  static void macio_ide_initfn(Object *obj)
>  {
>      SysBusDevice *d = SYS_BUS_DEVICE(obj);
> @@ -441,8 +477,10 @@ static void macio_ide_initfn(Object *obj)
>      ide_bus_new(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2);
>      memory_region_init_io(&s->mem, obj, &pmac_ide_ops, s, "pmac-ide", 0x1000);
>      sysbus_init_mmio(d, &s->mem);
> -    sysbus_init_irq(d, &s->irq);
> -    sysbus_init_irq(d, &s->dma_irq);
> +    sysbus_init_irq(d, &s->real_ide_irq);
> +    sysbus_init_irq(d, &s->real_dma_irq);
> +    s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
> +    s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
>  }
>  
>  static void macio_ide_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
> index 20cbddb..300fc8a 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -132,7 +132,9 @@ typedef struct MACIOIDEState {
>      SysBusDevice parent_obj;
>      /*< public >*/
>  
> -    qemu_irq irq;
> +    qemu_irq real_ide_irq;
> +    qemu_irq real_dma_irq;
> +    qemu_irq ide_irq;
>      qemu_irq dma_irq;
>  
>      MemoryRegion mem;
> @@ -140,6 +142,8 @@ typedef struct MACIOIDEState {
>      IDEDMA dma;
>      void *dbdma;
>      bool dma_active;
> +    uint32_t timing_reg;
> +    uint32_t irq_reg;
>  } MACIOIDEState;
>  
>  void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 4/8] macio: convert pmac_ide_ops from old_mmio

[Mark Cave-Ayland]

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

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index db5db39..428fbfc 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -255,131 +255,89 @@ static void pmac_ide_flush(DBDMA_io *io)
 }
 
 /* PowerMac IDE memory IO */
-static void pmac_ide_writeb (void *opaque,
-                             hwaddr addr, uint32_t val)
+static uint64_t pmac_ide_read(void *opaque, hwaddr addr, unsigned size)
 {
     MACIOIDEState *d = opaque;
+    uint64_t retval;
+    addr = (addr & 0xfff) >> 4;
 
-    addr = (addr & 0xFFF) >> 4;
     switch (addr) {
-    case 1 ... 7:
-        ide_ioport_write(&d->bus, addr, val);
-        break;
-    case 8:
-    case 22:
-        ide_cmd_write(&d->bus, 0, val);
-        break;
-    default:
+    case 0x0:
+        if (size == 2) {
+            retval = ide_data_readw(&d->bus, 0);
+        } else if (size == 4) {
+            retval = ide_data_readl(&d->bus, 0);
+        } else {
+            retval = 0xffffffff;
+        }
         break;
-    }
-}
-
-static uint32_t pmac_ide_readb (void *opaque,hwaddr addr)
-{
-    uint8_t retval;
-    MACIOIDEState *d = opaque;
-
-    addr = (addr & 0xFFF) >> 4;
-    switch (addr) {
-    case 1 ... 7:
+    case 0x1 ... 0x7:
         retval = ide_ioport_read(&d->bus, addr);
         break;
-    case 8:
-    case 22:
+    case 0x8:
+    case 0x16:
         retval = ide_status_read(&d->bus, 0);
         break;
+    case 0x20:
+        retval = d->timing_reg;
+        break;
+    case 0x30:
+        /* This is an interrupt state register that only exists
+         * in the KeyLargo and later variants. Bit 0x8000_0000
+         * latches the DMA interrupt and has to be written to
+         * clear. Bit 0x4000_0000 is an image of the disk
+         * interrupt. MacOS X relies on this and will hang if
+         * we don't provide at least the disk interrupt
+         */
+        retval = d->irq_reg;
+        break;
     default:
-        retval = 0xFF;
+        retval = 0xffffffff;
         break;
     }
-    return retval;
-}
-
-static void pmac_ide_writew (void *opaque,
-                             hwaddr addr, uint32_t val)
-{
-    MACIOIDEState *d = opaque;
-
-    addr = (addr & 0xFFF) >> 4;
-    val = bswap16(val);
-    if (addr == 0) {
-        ide_data_writew(&d->bus, 0, val);
-    }
-}
 
-static uint32_t pmac_ide_readw (void *opaque,hwaddr addr)
-{
-    uint16_t retval;
-    MACIOIDEState *d = opaque;
-
-    addr = (addr & 0xFFF) >> 4;
-    if (addr == 0) {
-        retval = ide_data_readw(&d->bus, 0);
-    } else {
-        retval = 0xFFFF;
-    }
-    retval = bswap16(retval);
     return retval;
 }
 
-static void pmac_ide_writel (void *opaque,
-                             hwaddr addr, uint32_t val)
+
+static void pmac_ide_write(void *opaque, hwaddr addr, uint64_t val,
+                           unsigned size)
 {
     MACIOIDEState *d = opaque;
+    addr = (addr & 0xfff) >> 4;
 
-    addr = (addr & 0xFFF) >> 4;
-    val = bswap32(val);
-    if (addr == 0) {
-        ide_data_writel(&d->bus, 0, val);
-    } else if (addr == 0x20) {
+    switch (addr) {
+    case 0x0:
+        if (size == 2) {
+            ide_data_writew(&d->bus, 0, val);
+        } else if (size == 4) {
+            ide_data_writel(&d->bus, 0, val);
+        }
+        break;
+    case 0x1 ... 0x7:
+        ide_ioport_write(&d->bus, addr, val);
+        break;
+    case 0x8:
+    case 0x16:
+        ide_cmd_write(&d->bus, 0, val);
+        break;
+    case 0x20:
         d->timing_reg = val;
-    } else if (addr == 0x30) {
+        break;
+    case 0x30:
         if (val & 0x80000000u) {
             d->irq_reg &= 0x7fffffff;
         }
+        break;
     }
 }
 
-static uint32_t pmac_ide_readl (void *opaque,hwaddr addr)
-{
-    uint32_t retval;
-    MACIOIDEState *d = opaque;
-
-    addr = (addr & 0xFFF) >> 4;
-    if (addr == 0) {
-        retval = ide_data_readl(&d->bus, 0);
-    } else if (addr == 0x20) {
-        retval = d->timing_reg;
-    } else if (addr == 0x30) {
-        /* This is an interrupt state register that only exists
-         * in the KeyLargo and later variants. Bit 0x8000_0000
-         * latches the DMA interrupt and has to be written to
-         * clear. Bit 0x4000_0000 is an image of the disk
-         * interrupt. MacOS X relies on this and will hang if
-         * we don't provide at least the disk interrupt
-         */
-        retval = d->irq_reg;
-    } else {
-        retval = 0xFFFFFFFF;
-    }
-    retval = bswap32(retval);
-    return retval;
-}
-
 static const MemoryRegionOps pmac_ide_ops = {
-    .old_mmio = {
-        .write = {
-            pmac_ide_writeb,
-            pmac_ide_writew,
-            pmac_ide_writel,
-        },
-        .read = {
-            pmac_ide_readb,
-            pmac_ide_readw,
-            pmac_ide_readl,
-        },
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .read = pmac_ide_read,
+    .write = pmac_ide_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static const VMStateDescription vmstate_pmac = {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/8] macio: convert pmac_ide_ops from old_mmio

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:44PM +0100, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Nice, but there are some details I'm not sure about.

> ---
>  hw/ide/macio.c |  154 +++++++++++++++++++++-----------------------------------
>  1 file changed, 56 insertions(+), 98 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index db5db39..428fbfc 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -255,131 +255,89 @@ static void pmac_ide_flush(DBDMA_io *io)
>  }
>  
>  /* PowerMac IDE memory IO */
> -static void pmac_ide_writeb (void *opaque,
> -                             hwaddr addr, uint32_t val)
> +static uint64_t pmac_ide_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MACIOIDEState *d = opaque;
> +    uint64_t retval;
> +    addr = (addr & 0xfff) >> 4;

Do you need the mask?  Using the new mmio interfaces, the given
address should already be just an offset within the region.  I also
dislike re-using the 'addr' name when it's now essentially a register
index.

>  
> -    addr = (addr & 0xFFF) >> 4;
>      switch (addr) {
> -    case 1 ... 7:
> -        ide_ioport_write(&d->bus, addr, val);
> -        break;
> -    case 8:
> -    case 22:
> -        ide_cmd_write(&d->bus, 0, val);
> -        break;
> -    default:
> +    case 0x0:
> +        if (size == 2) {
> +            retval = ide_data_readw(&d->bus, 0);
> +        } else if (size == 4) {
> +            retval = ide_data_readl(&d->bus, 0);
> +        } else {
> +            retval = 0xffffffff;

Are single byte reads not permitted, or will they just return parts of
the data?

> +        }
>          break;
> -    }
> -}
> -
> -static uint32_t pmac_ide_readb (void *opaque,hwaddr addr)
> -{
> -    uint8_t retval;
> -    MACIOIDEState *d = opaque;
> -
> -    addr = (addr & 0xFFF) >> 4;
> -    switch (addr) {
> -    case 1 ... 7:
> +    case 0x1 ... 0x7:
>          retval = ide_ioport_read(&d->bus, addr);
>          break;
> -    case 8:
> -    case 22:
> +    case 0x8:
> +    case 0x16:

I think you need some sort of size check for these other registers -
you've declared yourself as both supporting and implementing 1..4 byte
accesses, but I don't think these make sense for size != 4.

>          retval = ide_status_read(&d->bus, 0);
>          break;
> +    case 0x20:
> +        retval = d->timing_reg;
> +        break;
> +    case 0x30:
> +        /* This is an interrupt state register that only exists
> +         * in the KeyLargo and later variants. Bit 0x8000_0000
> +         * latches the DMA interrupt and has to be written to
> +         * clear. Bit 0x4000_0000 is an image of the disk
> +         * interrupt. MacOS X relies on this and will hang if
> +         * we don't provide at least the disk interrupt
> +         */
> +        retval = d->irq_reg;
> +        break;
>      default:
> -        retval = 0xFF;
> +        retval = 0xffffffff;
>          break;
>      }
> -    return retval;
> -}
> -
> -static void pmac_ide_writew (void *opaque,
> -                             hwaddr addr, uint32_t val)
> -{
> -    MACIOIDEState *d = opaque;
> -
> -    addr = (addr & 0xFFF) >> 4;
> -    val = bswap16(val);
> -    if (addr == 0) {
> -        ide_data_writew(&d->bus, 0, val);
> -    }
> -}
>  
> -static uint32_t pmac_ide_readw (void *opaque,hwaddr addr)
> -{
> -    uint16_t retval;
> -    MACIOIDEState *d = opaque;
> -
> -    addr = (addr & 0xFFF) >> 4;
> -    if (addr == 0) {
> -        retval = ide_data_readw(&d->bus, 0);
> -    } else {
> -        retval = 0xFFFF;
> -    }
> -    retval = bswap16(retval);
>      return retval;
>  }
>  
> -static void pmac_ide_writel (void *opaque,
> -                             hwaddr addr, uint32_t val)
> +
> +static void pmac_ide_write(void *opaque, hwaddr addr, uint64_t val,
> +                           unsigned size)

Similar comments on the write path.

>  {
>      MACIOIDEState *d = opaque;
> +    addr = (addr & 0xfff) >> 4;
>  
> -    addr = (addr & 0xFFF) >> 4;
> -    val = bswap32(val);
> -    if (addr == 0) {
> -        ide_data_writel(&d->bus, 0, val);
> -    } else if (addr == 0x20) {
> +    switch (addr) {
> +    case 0x0:
> +        if (size == 2) {
> +            ide_data_writew(&d->bus, 0, val);
> +        } else if (size == 4) {
> +            ide_data_writel(&d->bus, 0, val);
> +        }
> +        break;
> +    case 0x1 ... 0x7:
> +        ide_ioport_write(&d->bus, addr, val);
> +        break;
> +    case 0x8:
> +    case 0x16:
> +        ide_cmd_write(&d->bus, 0, val);
> +        break;
> +    case 0x20:
>          d->timing_reg = val;
> -    } else if (addr == 0x30) {
> +        break;
> +    case 0x30:
>          if (val & 0x80000000u) {
>              d->irq_reg &= 0x7fffffff;
>          }
> +        break;
>      }
>  }
>  
> -static uint32_t pmac_ide_readl (void *opaque,hwaddr addr)
> -{
> -    uint32_t retval;
> -    MACIOIDEState *d = opaque;
> -
> -    addr = (addr & 0xFFF) >> 4;
> -    if (addr == 0) {
> -        retval = ide_data_readl(&d->bus, 0);
> -    } else if (addr == 0x20) {
> -        retval = d->timing_reg;
> -    } else if (addr == 0x30) {
> -        /* This is an interrupt state register that only exists
> -         * in the KeyLargo and later variants. Bit 0x8000_0000
> -         * latches the DMA interrupt and has to be written to
> -         * clear. Bit 0x4000_0000 is an image of the disk
> -         * interrupt. MacOS X relies on this and will hang if
> -         * we don't provide at least the disk interrupt
> -         */
> -        retval = d->irq_reg;
> -    } else {
> -        retval = 0xFFFFFFFF;
> -    }
> -    retval = bswap32(retval);
> -    return retval;
> -}
> -
>  static const MemoryRegionOps pmac_ide_ops = {
> -    .old_mmio = {
> -        .write = {
> -            pmac_ide_writeb,
> -            pmac_ide_writew,
> -            pmac_ide_writel,
> -        },
> -        .read = {
> -            pmac_ide_readb,
> -            pmac_ide_readw,
> -            pmac_ide_readl,
> -        },
> -    },
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .read = pmac_ide_read,
> +    .write = pmac_ide_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,

Remember you can set .impl. different from .valid if you want the mmio
core to do the work of splitting accesses up.

> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
>  static const VMStateDescription vmstate_pmac = {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/8] macio: convert pmac_ide_ops from old_mmio

[Mark Cave-Ayland]

On 18/09/17 21:36, David Gibson wrote:

> On Sun, Sep 17, 2017 at 06:15:44PM +0100, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Nice, but there are some details I'm not sure about.
> 
>> ---
>>  hw/ide/macio.c |  154 +++++++++++++++++++++-----------------------------------
>>  1 file changed, 56 insertions(+), 98 deletions(-)
>>
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index db5db39..428fbfc 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -255,131 +255,89 @@ static void pmac_ide_flush(DBDMA_io *io)
>>  }
>>  
>>  /* PowerMac IDE memory IO */
>> -static void pmac_ide_writeb (void *opaque,
>> -                             hwaddr addr, uint32_t val)
>> +static uint64_t pmac_ide_read(void *opaque, hwaddr addr, unsigned size)
>>  {
>>      MACIOIDEState *d = opaque;
>> +    uint64_t retval;
>> +    addr = (addr & 0xfff) >> 4;
> 
> Do you need the mask?  Using the new mmio interfaces, the given
> address should already be just an offset within the region.  I also
> dislike re-using the 'addr' name when it's now essentially a register
> index.

Good point, I don't think it is needed anymore since the MemoryRegion
size is 0x1000.

>>  
>> -    addr = (addr & 0xFFF) >> 4;
>>      switch (addr) {
>> -    case 1 ... 7:
>> -        ide_ioport_write(&d->bus, addr, val);
>> -        break;
>> -    case 8:
>> -    case 22:
>> -        ide_cmd_write(&d->bus, 0, val);
>> -        break;
>> -    default:
>> +    case 0x0:
>> +        if (size == 2) {
>> +            retval = ide_data_readw(&d->bus, 0);
>> +        } else if (size == 4) {
>> +            retval = ide_data_readl(&d->bus, 0);
>> +        } else {
>> +            retval = 0xffffffff;
> 
> Are single byte reads not permitted, or will they just return parts of
> the data?

>From what I can tell looking at the IDE core code, the accesses are
effectively the same except for the access size, but ide_portio_list[]
specifies the access sizes as being 2 and 4 respectively.

>> +        }
>>          break;
>> -    }
>> -}
>> -
>> -static uint32_t pmac_ide_readb (void *opaque,hwaddr addr)
>> -{
>> -    uint8_t retval;
>> -    MACIOIDEState *d = opaque;
>> -
>> -    addr = (addr & 0xFFF) >> 4;
>> -    switch (addr) {
>> -    case 1 ... 7:
>> +    case 0x1 ... 0x7:
>>          retval = ide_ioport_read(&d->bus, addr);
>>          break;
>> -    case 8:
>> -    case 22:
>> +    case 0x8:
>> +    case 0x16:
> 
> I think you need some sort of size check for these other registers -
> you've declared yourself as both supporting and implementing 1..4 byte
> accesses, but I don't think these make sense for size != 4.

Yes, agreed.

>>          retval = ide_status_read(&d->bus, 0);
>>          break;
>> +    case 0x20:
>> +        retval = d->timing_reg;
>> +        break;
>> +    case 0x30:
>> +        /* This is an interrupt state register that only exists
>> +         * in the KeyLargo and later variants. Bit 0x8000_0000
>> +         * latches the DMA interrupt and has to be written to
>> +         * clear. Bit 0x4000_0000 is an image of the disk
>> +         * interrupt. MacOS X relies on this and will hang if
>> +         * we don't provide at least the disk interrupt
>> +         */
>> +        retval = d->irq_reg;
>> +        break;
>>      default:
>> -        retval = 0xFF;
>> +        retval = 0xffffffff;
>>          break;
>>      }
>> -    return retval;
>> -}
>> -
>> -static void pmac_ide_writew (void *opaque,
>> -                             hwaddr addr, uint32_t val)
>> -{
>> -    MACIOIDEState *d = opaque;
>> -
>> -    addr = (addr & 0xFFF) >> 4;
>> -    val = bswap16(val);
>> -    if (addr == 0) {
>> -        ide_data_writew(&d->bus, 0, val);
>> -    }
>> -}
>>  
>> -static uint32_t pmac_ide_readw (void *opaque,hwaddr addr)
>> -{
>> -    uint16_t retval;
>> -    MACIOIDEState *d = opaque;
>> -
>> -    addr = (addr & 0xFFF) >> 4;
>> -    if (addr == 0) {
>> -        retval = ide_data_readw(&d->bus, 0);
>> -    } else {
>> -        retval = 0xFFFF;
>> -    }
>> -    retval = bswap16(retval);
>>      return retval;
>>  }
>>  
>> -static void pmac_ide_writel (void *opaque,
>> -                             hwaddr addr, uint32_t val)
>> +
>> +static void pmac_ide_write(void *opaque, hwaddr addr, uint64_t val,
>> +                           unsigned size)
> 
> Similar comments on the write path.
> 
>>  {
>>      MACIOIDEState *d = opaque;
>> +    addr = (addr & 0xfff) >> 4;
>>  
>> -    addr = (addr & 0xFFF) >> 4;
>> -    val = bswap32(val);
>> -    if (addr == 0) {
>> -        ide_data_writel(&d->bus, 0, val);
>> -    } else if (addr == 0x20) {
>> +    switch (addr) {
>> +    case 0x0:
>> +        if (size == 2) {
>> +            ide_data_writew(&d->bus, 0, val);
>> +        } else if (size == 4) {
>> +            ide_data_writel(&d->bus, 0, val);
>> +        }
>> +        break;
>> +    case 0x1 ... 0x7:
>> +        ide_ioport_write(&d->bus, addr, val);
>> +        break;
>> +    case 0x8:
>> +    case 0x16:
>> +        ide_cmd_write(&d->bus, 0, val);
>> +        break;
>> +    case 0x20:
>>          d->timing_reg = val;
>> -    } else if (addr == 0x30) {
>> +        break;
>> +    case 0x30:
>>          if (val & 0x80000000u) {
>>              d->irq_reg &= 0x7fffffff;
>>          }
>> +        break;
>>      }
>>  }
>>  
>> -static uint32_t pmac_ide_readl (void *opaque,hwaddr addr)
>> -{
>> -    uint32_t retval;
>> -    MACIOIDEState *d = opaque;
>> -
>> -    addr = (addr & 0xFFF) >> 4;
>> -    if (addr == 0) {
>> -        retval = ide_data_readl(&d->bus, 0);
>> -    } else if (addr == 0x20) {
>> -        retval = d->timing_reg;
>> -    } else if (addr == 0x30) {
>> -        /* This is an interrupt state register that only exists
>> -         * in the KeyLargo and later variants. Bit 0x8000_0000
>> -         * latches the DMA interrupt and has to be written to
>> -         * clear. Bit 0x4000_0000 is an image of the disk
>> -         * interrupt. MacOS X relies on this and will hang if
>> -         * we don't provide at least the disk interrupt
>> -         */
>> -        retval = d->irq_reg;
>> -    } else {
>> -        retval = 0xFFFFFFFF;
>> -    }
>> -    retval = bswap32(retval);
>> -    return retval;
>> -}
>> -
>>  static const MemoryRegionOps pmac_ide_ops = {
>> -    .old_mmio = {
>> -        .write = {
>> -            pmac_ide_writeb,
>> -            pmac_ide_writew,
>> -            pmac_ide_writel,
>> -        },
>> -        .read = {
>> -            pmac_ide_readb,
>> -            pmac_ide_readw,
>> -            pmac_ide_readl,
>> -        },
>> -    },
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .read = pmac_ide_read,
>> +    .write = pmac_ide_write,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 4,
> 
> Remember you can set .impl. different from .valid if you want the mmio
> core to do the work of splitting accesses up.

I'm not sure exactly sure what the behaviour for unaligned accesses is
(or indeed if they are valid), however I've just run an updated version
through my OpenBIOS boot tests and I don't see any regression so I'll
resubmit it shortly.


ATB,

Mark.

[Qemu-devel] [PATCH 5/8] ppc/mac: More rework of the DBDMA emulation

[Mark Cave-Ayland]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This completely reworks the handling of the control register
according to my understanding of the HW and the spec.

It should (hopefully ... still testing) fix a number of issues
most notably cases of MacOS hanging.

Also update dbdma_unassigned_rw() and dbdma_unassigned_flush() to
have the expected behaviour now that flush is handled slightly
differently.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/mac_dbdma.c |  191 +++++++++++++++++++++++++++++++++------------
 1 file changed, 139 insertions(+), 52 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 15452b9..3fe5073 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -96,9 +96,8 @@ static void dbdma_cmdptr_load(DBDMA_channel *ch)
 
 static void dbdma_cmdptr_save(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_save 0x%08x\n",
-                    ch->regs[DBDMA_CMDPTR_LO]);
-    DBDMA_DPRINTFCH(ch, "xfer_status 0x%08x res_count 0x%04x\n",
+    DBDMA_DPRINTFCH(ch, "-> update 0x%08x stat=0x%08x, res=0x%04x\n",
+                    ch->regs[DBDMA_CMDPTR_LO],
                     le16_to_cpu(ch->current.xfer_status),
                     le16_to_cpu(ch->current.res_count));
     dma_memory_write(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
@@ -166,15 +165,14 @@ static int conditional_wait(DBDMA_channel *ch)
     uint16_t sel_mask, sel_value;
     uint32_t status;
     int cond;
-
-    DBDMA_DPRINTFCH(ch, "conditional_wait\n");
+    int res = 0;
 
     wait = le16_to_cpu(current->command) & WAIT_MASK;
-
     switch(wait) {
     case WAIT_NEVER:  /* don't wait */
         return 0;
     case WAIT_ALWAYS: /* always wait */
+        DBDMA_DPRINTFCH(ch, "  [WAIT_ALWAYS]\n");
         return 1;
     }
 
@@ -187,15 +185,19 @@ static int conditional_wait(DBDMA_channel *ch)
 
     switch(wait) {
     case WAIT_IFSET:  /* wait if condition bit is 1 */
-        if (cond)
-            return 1;
-        return 0;
+        if (cond) {
+            res = 1;
+        }
+        DBDMA_DPRINTFCH(ch, "  [WAIT_IFSET=%d]\n", res);
+        break;
     case WAIT_IFCLR:  /* wait if condition bit is 0 */
-        if (!cond)
-            return 1;
-        return 0;
+        if (!cond) {
+            res = 1;
+        }
+        DBDMA_DPRINTFCH(ch, "  [WAIT_IFCLR=%d]\n", res);
+        break;
     }
-    return 0;
+    return res;
 }
 
 static void next(DBDMA_channel *ch)
@@ -226,8 +228,6 @@ static void conditional_branch(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTFCH(ch, "conditional_branch\n");
-
     /* check if we must branch */
 
     br = le16_to_cpu(current->command) & BR_MASK;
@@ -237,6 +237,7 @@ static void conditional_branch(DBDMA_channel *ch)
         next(ch);
         return;
     case BR_ALWAYS: /* always branch */
+        DBDMA_DPRINTFCH(ch, "  [BR_ALWAYS]\n");
         branch(ch);
         return;
     }
@@ -250,16 +251,22 @@ static void conditional_branch(DBDMA_channel *ch)
 
     switch(br) {
     case BR_IFSET:  /* branch if condition bit is 1 */
-        if (cond)
+        if (cond) {
+            DBDMA_DPRINTFCH(ch, "  [BR_IFSET = 1]\n");
             branch(ch);
-        else
+        } else {
+            DBDMA_DPRINTFCH(ch, "  [BR_IFSET = 0]\n");
             next(ch);
+        }
         return;
     case BR_IFCLR:  /* branch if condition bit is 0 */
-        if (!cond)
+        if (!cond) {
+            DBDMA_DPRINTFCH(ch, "  [BR_IFCLR = 1]\n");
             branch(ch);
-        else
+        } else {
+            DBDMA_DPRINTFCH(ch, "  [BR_IFCLR = 0]\n");
             next(ch);
+        }
         return;
     }
 }
@@ -428,7 +435,7 @@ wait:
 
 static void stop(DBDMA_channel *ch)
 {
-    ch->regs[DBDMA_STATUS] &= ~(ACTIVE|DEAD|FLUSH);
+    ch->regs[DBDMA_STATUS] &= ~(ACTIVE);
 
     /* the stop command does not increment command pointer */
 }
@@ -471,18 +478,22 @@ static void channel_run(DBDMA_channel *ch)
 
     switch (cmd) {
     case OUTPUT_MORE:
+        DBDMA_DPRINTFCH(ch, "* OUTPUT_MORE *\n");
         start_output(ch, key, phy_addr, req_count, 0);
         return;
 
     case OUTPUT_LAST:
+        DBDMA_DPRINTFCH(ch, "* OUTPUT_LAST *\n");
         start_output(ch, key, phy_addr, req_count, 1);
         return;
 
     case INPUT_MORE:
+        DBDMA_DPRINTFCH(ch, "* INPUT_MORE *\n");
         start_input(ch, key, phy_addr, req_count, 0);
         return;
 
     case INPUT_LAST:
+        DBDMA_DPRINTFCH(ch, "* INPUT_LAST *\n");
         start_input(ch, key, phy_addr, req_count, 1);
         return;
     }
@@ -508,10 +519,12 @@ static void channel_run(DBDMA_channel *ch)
 
     switch (cmd) {
     case LOAD_WORD:
+        DBDMA_DPRINTFCH(ch, "* LOAD_WORD *\n");
         load_word(ch, key, phy_addr, req_count);
         return;
 
     case STORE_WORD:
+        DBDMA_DPRINTFCH(ch, "* STORE_WORD *\n");
         store_word(ch, key, phy_addr, req_count);
         return;
     }
@@ -562,43 +575,117 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
     ch->io.opaque = opaque;
 }
 
-static void
-dbdma_control_write(DBDMA_channel *ch)
+static void dbdma_control_write(DBDMA_channel *ch)
 {
     uint16_t mask, value;
     uint32_t status;
+    bool do_flush = false;
 
     mask = (ch->regs[DBDMA_CONTROL] >> 16) & 0xffff;
     value = ch->regs[DBDMA_CONTROL] & 0xffff;
 
-    value &= (RUN | PAUSE | FLUSH | WAKE | DEVSTAT);
-
+    /* This is the status register which we'll update
+     * appropriately and store back
+     */
     status = ch->regs[DBDMA_STATUS];
 
-    status = (value & mask) | (status & ~mask);
+    /* RUN and PAUSE are bits under SW control only
+     * FLUSH and WAKE are set by SW and cleared by HW
+     * DEAD, ACTIVE and BT are only under HW control
+     *
+     * We handle ACTIVE separately at the end of the
+     * logic to ensure all cases are covered.
+     */
 
-    if (status & WAKE)
-        status |= ACTIVE;
-    if (status & RUN) {
-        status |= ACTIVE;
-        status &= ~DEAD;
+    /* Setting RUN will tentatively activate the channel
+     */
+    if ((mask & RUN) && (value & RUN)) {
+        status |= RUN;
+        DBDMA_DPRINTFCH(ch, " Setting RUN !\n");
+    }
+
+    /* Clearing RUN 1->0 will stop the channel */
+    if ((mask & RUN) && !(value & RUN)) {
+        /* This has the side effect of clearing the DEAD bit */
+        status &= ~(DEAD | RUN);
+        DBDMA_DPRINTFCH(ch, " Clearing RUN !\n");
+    }
+
+    /* Setting WAKE wakes up an idle channel if it's running
+     *
+     * Note: The doc doesn't say so but assume that only works
+     * on a channel whose RUN bit is set.
+     *
+     * We set WAKE in status, it's not terribly useful as it will
+     * be cleared on the next command fetch but it seems to mimmic
+     * the HW behaviour and is useful for the way we handle
+     * ACTIVE further down.
+     */
+    if ((mask & WAKE) && (value & WAKE) && (status & RUN)) {
+        status |= WAKE;
+        DBDMA_DPRINTFCH(ch, " Setting WAKE !\n");
+    }
+
+    /* PAUSE being set will deactivate (or prevent activation)
+     * of the channel. We just copy it over for now, ACTIVE will
+     * be re-evaluated later.
+     */
+    if (mask & PAUSE) {
+        status = (status & ~PAUSE) | (value & PAUSE);
+        DBDMA_DPRINTFCH(ch, " %sing PAUSE !\n",
+                        (value & PAUSE) ? "sett" : "clear");
+    }
+
+    /* FLUSH is its own thing */
+    if ((mask & FLUSH) && (value & FLUSH))  {
+        DBDMA_DPRINTFCH(ch, " Setting FLUSH !\n");
+        /* We set flush directly in the status register, we do *NOT*
+         * set it in "status" so that it gets naturally cleared when
+         * we update the status register further down. That way it
+         * will be set only during the HW flush operation so it is
+         * visible to any completions happening during that time.
+         */
+        ch->regs[DBDMA_STATUS] |= FLUSH;
+        do_flush = true;
     }
-    if (status & PAUSE)
+
+    /* If either RUN or PAUSE is clear, so should ACTIVE be,
+     * otherwise, ACTIVE will be set if we modified RUN, PAUSE or
+     * set WAKE. That means that PAUSE was just cleared, RUN was
+     * just set or WAKE was just set.
+     */
+    if ((status & PAUSE) || !(status & RUN)) {
         status &= ~ACTIVE;
-    if ((ch->regs[DBDMA_STATUS] & RUN) && !(status & RUN)) {
-        /* RUN is cleared */
-        status &= ~(ACTIVE|DEAD);
+        DBDMA_DPRINTFCH(ch, "  -> ACTIVE down !\n");
+
+        /* We stopped processing, we want the underlying HW command
+         * to complete *before* we clear the ACTIVE bit. Otherwise
+         * we can get into a situation where the command status will
+         * have RUN or ACTIVE not set which is going to confuse the
+         * MacOS driver.
+         */
+        do_flush = true;
+    } else if (mask & (RUN | PAUSE)) {
+        status |= ACTIVE;
+        DBDMA_DPRINTFCH(ch, " -> ACTIVE up !\n");
+    } else if ((mask & WAKE) && (value & WAKE)) {
+        status |= ACTIVE;
+        DBDMA_DPRINTFCH(ch, " -> ACTIVE up !\n");
     }
 
-    if ((status & FLUSH) && ch->flush) {
+    DBDMA_DPRINTFCH(ch, " new status=0x%08x\n", status);
+
+    /* If we need to flush the underlying HW, do it now, this happens
+     * both on FLUSH commands and when stopping the channel for safety.
+     */
+    if (do_flush && ch->flush) {
         ch->flush(&ch->io);
-        status &= ~FLUSH;
     }
 
-    DBDMA_DPRINTFCH(ch, "    status 0x%08x\n", status);
-
+    /* Finally update the status register image */
     ch->regs[DBDMA_STATUS] = status;
 
+    /* If active, make sure the BH gets to run */
     if (status & ACTIVE) {
         DBDMA_kick(dbdma_from_ch(ch));
     }
@@ -666,13 +753,9 @@ static uint64_t dbdma_read(void *opaque, hwaddr addr,
 
     value = ch->regs[reg];
 
-    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
-    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
-                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
-
     switch(reg) {
     case DBDMA_CONTROL:
-        value = 0;
+        value = ch->regs[DBDMA_STATUS];
         break;
     case DBDMA_STATUS:
     case DBDMA_CMDPTR_LO:
@@ -698,6 +781,10 @@ static uint64_t dbdma_read(void *opaque, hwaddr addr,
         break;
     }
 
+    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
+    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
+                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
+
     return value;
 }
 
@@ -776,28 +863,28 @@ static void dbdma_reset(void *opaque)
 static void dbdma_unassigned_rw(DBDMA_io *io)
 {
     DBDMA_channel *ch = io->channel;
-    qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
-                  __func__, ch->channel);
-    ch->io.processing = false;
-}
-
-static void dbdma_unassigned_flush(DBDMA_io *io)
-{
-    DBDMA_channel *ch = io->channel;
     dbdma_cmd *current = &ch->current;
     uint16_t cmd;
     qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
                   __func__, ch->channel);
+    ch->io.processing = false;
 
     cmd = le16_to_cpu(current->command) & COMMAND_MASK;
     if (cmd == OUTPUT_MORE || cmd == OUTPUT_LAST ||
         cmd == INPUT_MORE || cmd == INPUT_LAST) {
-        current->xfer_status = cpu_to_le16(ch->regs[DBDMA_STATUS] | FLUSH);
+        current->xfer_status = cpu_to_le16(ch->regs[DBDMA_STATUS]);
         current->res_count = cpu_to_le16(io->len);
         dbdma_cmdptr_save(ch);
     }
 }
 
+static void dbdma_unassigned_flush(DBDMA_io *io)
+{
+    DBDMA_channel *ch = io->channel;
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
+                  __func__, ch->channel);
+}
+
 void* DBDMA_init (MemoryRegion **dbdma_mem)
 {
     DBDMAState *s;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/8] ppc/mac: More rework of the DBDMA emulation

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:45PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This completely reworks the handling of the control register
> according to my understanding of the HW and the spec.
> 
> It should (hopefully ... still testing) fix a number of issues
> most notably cases of MacOS hanging.
> 
> Also update dbdma_unassigned_rw() and dbdma_unassigned_flush() to
> have the expected behaviour now that flush is handled slightly
> differently.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-2.11.

> ---
>  hw/misc/macio/mac_dbdma.c |  191 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 139 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 15452b9..3fe5073 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -96,9 +96,8 @@ static void dbdma_cmdptr_load(DBDMA_channel *ch)
>  
>  static void dbdma_cmdptr_save(DBDMA_channel *ch)
>  {
> -    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_save 0x%08x\n",
> -                    ch->regs[DBDMA_CMDPTR_LO]);
> -    DBDMA_DPRINTFCH(ch, "xfer_status 0x%08x res_count 0x%04x\n",
> +    DBDMA_DPRINTFCH(ch, "-> update 0x%08x stat=0x%08x, res=0x%04x\n",
> +                    ch->regs[DBDMA_CMDPTR_LO],
>                      le16_to_cpu(ch->current.xfer_status),
>                      le16_to_cpu(ch->current.res_count));
>      dma_memory_write(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
> @@ -166,15 +165,14 @@ static int conditional_wait(DBDMA_channel *ch)
>      uint16_t sel_mask, sel_value;
>      uint32_t status;
>      int cond;
> -
> -    DBDMA_DPRINTFCH(ch, "conditional_wait\n");
> +    int res = 0;
>  
>      wait = le16_to_cpu(current->command) & WAIT_MASK;
> -
>      switch(wait) {
>      case WAIT_NEVER:  /* don't wait */
>          return 0;
>      case WAIT_ALWAYS: /* always wait */
> +        DBDMA_DPRINTFCH(ch, "  [WAIT_ALWAYS]\n");
>          return 1;
>      }
>  
> @@ -187,15 +185,19 @@ static int conditional_wait(DBDMA_channel *ch)
>  
>      switch(wait) {
>      case WAIT_IFSET:  /* wait if condition bit is 1 */
> -        if (cond)
> -            return 1;
> -        return 0;
> +        if (cond) {
> +            res = 1;
> +        }
> +        DBDMA_DPRINTFCH(ch, "  [WAIT_IFSET=%d]\n", res);
> +        break;
>      case WAIT_IFCLR:  /* wait if condition bit is 0 */
> -        if (!cond)
> -            return 1;
> -        return 0;
> +        if (!cond) {
> +            res = 1;
> +        }
> +        DBDMA_DPRINTFCH(ch, "  [WAIT_IFCLR=%d]\n", res);
> +        break;
>      }
> -    return 0;
> +    return res;
>  }
>  
>  static void next(DBDMA_channel *ch)
> @@ -226,8 +228,6 @@ static void conditional_branch(DBDMA_channel *ch)
>      uint32_t status;
>      int cond;
>  
> -    DBDMA_DPRINTFCH(ch, "conditional_branch\n");
> -
>      /* check if we must branch */
>  
>      br = le16_to_cpu(current->command) & BR_MASK;
> @@ -237,6 +237,7 @@ static void conditional_branch(DBDMA_channel *ch)
>          next(ch);
>          return;
>      case BR_ALWAYS: /* always branch */
> +        DBDMA_DPRINTFCH(ch, "  [BR_ALWAYS]\n");
>          branch(ch);
>          return;
>      }
> @@ -250,16 +251,22 @@ static void conditional_branch(DBDMA_channel *ch)
>  
>      switch(br) {
>      case BR_IFSET:  /* branch if condition bit is 1 */
> -        if (cond)
> +        if (cond) {
> +            DBDMA_DPRINTFCH(ch, "  [BR_IFSET = 1]\n");
>              branch(ch);
> -        else
> +        } else {
> +            DBDMA_DPRINTFCH(ch, "  [BR_IFSET = 0]\n");
>              next(ch);
> +        }
>          return;
>      case BR_IFCLR:  /* branch if condition bit is 0 */
> -        if (!cond)
> +        if (!cond) {
> +            DBDMA_DPRINTFCH(ch, "  [BR_IFCLR = 1]\n");
>              branch(ch);
> -        else
> +        } else {
> +            DBDMA_DPRINTFCH(ch, "  [BR_IFCLR = 0]\n");
>              next(ch);
> +        }
>          return;
>      }
>  }
> @@ -428,7 +435,7 @@ wait:
>  
>  static void stop(DBDMA_channel *ch)
>  {
> -    ch->regs[DBDMA_STATUS] &= ~(ACTIVE|DEAD|FLUSH);
> +    ch->regs[DBDMA_STATUS] &= ~(ACTIVE);
>  
>      /* the stop command does not increment command pointer */
>  }
> @@ -471,18 +478,22 @@ static void channel_run(DBDMA_channel *ch)
>  
>      switch (cmd) {
>      case OUTPUT_MORE:
> +        DBDMA_DPRINTFCH(ch, "* OUTPUT_MORE *\n");
>          start_output(ch, key, phy_addr, req_count, 0);
>          return;
>  
>      case OUTPUT_LAST:
> +        DBDMA_DPRINTFCH(ch, "* OUTPUT_LAST *\n");
>          start_output(ch, key, phy_addr, req_count, 1);
>          return;
>  
>      case INPUT_MORE:
> +        DBDMA_DPRINTFCH(ch, "* INPUT_MORE *\n");
>          start_input(ch, key, phy_addr, req_count, 0);
>          return;
>  
>      case INPUT_LAST:
> +        DBDMA_DPRINTFCH(ch, "* INPUT_LAST *\n");
>          start_input(ch, key, phy_addr, req_count, 1);
>          return;
>      }
> @@ -508,10 +519,12 @@ static void channel_run(DBDMA_channel *ch)
>  
>      switch (cmd) {
>      case LOAD_WORD:
> +        DBDMA_DPRINTFCH(ch, "* LOAD_WORD *\n");
>          load_word(ch, key, phy_addr, req_count);
>          return;
>  
>      case STORE_WORD:
> +        DBDMA_DPRINTFCH(ch, "* STORE_WORD *\n");
>          store_word(ch, key, phy_addr, req_count);
>          return;
>      }
> @@ -562,43 +575,117 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
>      ch->io.opaque = opaque;
>  }
>  
> -static void
> -dbdma_control_write(DBDMA_channel *ch)
> +static void dbdma_control_write(DBDMA_channel *ch)
>  {
>      uint16_t mask, value;
>      uint32_t status;
> +    bool do_flush = false;
>  
>      mask = (ch->regs[DBDMA_CONTROL] >> 16) & 0xffff;
>      value = ch->regs[DBDMA_CONTROL] & 0xffff;
>  
> -    value &= (RUN | PAUSE | FLUSH | WAKE | DEVSTAT);
> -
> +    /* This is the status register which we'll update
> +     * appropriately and store back
> +     */
>      status = ch->regs[DBDMA_STATUS];
>  
> -    status = (value & mask) | (status & ~mask);
> +    /* RUN and PAUSE are bits under SW control only
> +     * FLUSH and WAKE are set by SW and cleared by HW
> +     * DEAD, ACTIVE and BT are only under HW control
> +     *
> +     * We handle ACTIVE separately at the end of the
> +     * logic to ensure all cases are covered.
> +     */
>  
> -    if (status & WAKE)
> -        status |= ACTIVE;
> -    if (status & RUN) {
> -        status |= ACTIVE;
> -        status &= ~DEAD;
> +    /* Setting RUN will tentatively activate the channel
> +     */
> +    if ((mask & RUN) && (value & RUN)) {
> +        status |= RUN;
> +        DBDMA_DPRINTFCH(ch, " Setting RUN !\n");
> +    }
> +
> +    /* Clearing RUN 1->0 will stop the channel */
> +    if ((mask & RUN) && !(value & RUN)) {
> +        /* This has the side effect of clearing the DEAD bit */
> +        status &= ~(DEAD | RUN);
> +        DBDMA_DPRINTFCH(ch, " Clearing RUN !\n");
> +    }
> +
> +    /* Setting WAKE wakes up an idle channel if it's running
> +     *
> +     * Note: The doc doesn't say so but assume that only works
> +     * on a channel whose RUN bit is set.
> +     *
> +     * We set WAKE in status, it's not terribly useful as it will
> +     * be cleared on the next command fetch but it seems to mimmic
> +     * the HW behaviour and is useful for the way we handle
> +     * ACTIVE further down.
> +     */
> +    if ((mask & WAKE) && (value & WAKE) && (status & RUN)) {
> +        status |= WAKE;
> +        DBDMA_DPRINTFCH(ch, " Setting WAKE !\n");
> +    }
> +
> +    /* PAUSE being set will deactivate (or prevent activation)
> +     * of the channel. We just copy it over for now, ACTIVE will
> +     * be re-evaluated later.
> +     */
> +    if (mask & PAUSE) {
> +        status = (status & ~PAUSE) | (value & PAUSE);
> +        DBDMA_DPRINTFCH(ch, " %sing PAUSE !\n",
> +                        (value & PAUSE) ? "sett" : "clear");
> +    }
> +
> +    /* FLUSH is its own thing */
> +    if ((mask & FLUSH) && (value & FLUSH))  {
> +        DBDMA_DPRINTFCH(ch, " Setting FLUSH !\n");
> +        /* We set flush directly in the status register, we do *NOT*
> +         * set it in "status" so that it gets naturally cleared when
> +         * we update the status register further down. That way it
> +         * will be set only during the HW flush operation so it is
> +         * visible to any completions happening during that time.
> +         */
> +        ch->regs[DBDMA_STATUS] |= FLUSH;
> +        do_flush = true;
>      }
> -    if (status & PAUSE)
> +
> +    /* If either RUN or PAUSE is clear, so should ACTIVE be,
> +     * otherwise, ACTIVE will be set if we modified RUN, PAUSE or
> +     * set WAKE. That means that PAUSE was just cleared, RUN was
> +     * just set or WAKE was just set.
> +     */
> +    if ((status & PAUSE) || !(status & RUN)) {
>          status &= ~ACTIVE;
> -    if ((ch->regs[DBDMA_STATUS] & RUN) && !(status & RUN)) {
> -        /* RUN is cleared */
> -        status &= ~(ACTIVE|DEAD);
> +        DBDMA_DPRINTFCH(ch, "  -> ACTIVE down !\n");
> +
> +        /* We stopped processing, we want the underlying HW command
> +         * to complete *before* we clear the ACTIVE bit. Otherwise
> +         * we can get into a situation where the command status will
> +         * have RUN or ACTIVE not set which is going to confuse the
> +         * MacOS driver.
> +         */
> +        do_flush = true;
> +    } else if (mask & (RUN | PAUSE)) {
> +        status |= ACTIVE;
> +        DBDMA_DPRINTFCH(ch, " -> ACTIVE up !\n");
> +    } else if ((mask & WAKE) && (value & WAKE)) {
> +        status |= ACTIVE;
> +        DBDMA_DPRINTFCH(ch, " -> ACTIVE up !\n");
>      }
>  
> -    if ((status & FLUSH) && ch->flush) {
> +    DBDMA_DPRINTFCH(ch, " new status=0x%08x\n", status);
> +
> +    /* If we need to flush the underlying HW, do it now, this happens
> +     * both on FLUSH commands and when stopping the channel for safety.
> +     */
> +    if (do_flush && ch->flush) {
>          ch->flush(&ch->io);
> -        status &= ~FLUSH;
>      }
>  
> -    DBDMA_DPRINTFCH(ch, "    status 0x%08x\n", status);
> -
> +    /* Finally update the status register image */
>      ch->regs[DBDMA_STATUS] = status;
>  
> +    /* If active, make sure the BH gets to run */
>      if (status & ACTIVE) {
>          DBDMA_kick(dbdma_from_ch(ch));
>      }
> @@ -666,13 +753,9 @@ static uint64_t dbdma_read(void *opaque, hwaddr addr,
>  
>      value = ch->regs[reg];
>  
> -    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
> -    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
> -                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
> -
>      switch(reg) {
>      case DBDMA_CONTROL:
> -        value = 0;
> +        value = ch->regs[DBDMA_STATUS];
>          break;
>      case DBDMA_STATUS:
>      case DBDMA_CMDPTR_LO:
> @@ -698,6 +781,10 @@ static uint64_t dbdma_read(void *opaque, hwaddr addr,
>          break;
>      }
>  
> +    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
> +    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
> +                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
> +
>      return value;
>  }
>  
> @@ -776,28 +863,28 @@ static void dbdma_reset(void *opaque)
>  static void dbdma_unassigned_rw(DBDMA_io *io)
>  {
>      DBDMA_channel *ch = io->channel;
> -    qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
> -                  __func__, ch->channel);
> -    ch->io.processing = false;
> -}
> -
> -static void dbdma_unassigned_flush(DBDMA_io *io)
> -{
> -    DBDMA_channel *ch = io->channel;
>      dbdma_cmd *current = &ch->current;
>      uint16_t cmd;
>      qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
>                    __func__, ch->channel);
> +    ch->io.processing = false;
>  
>      cmd = le16_to_cpu(current->command) & COMMAND_MASK;
>      if (cmd == OUTPUT_MORE || cmd == OUTPUT_LAST ||
>          cmd == INPUT_MORE || cmd == INPUT_LAST) {
> -        current->xfer_status = cpu_to_le16(ch->regs[DBDMA_STATUS] | FLUSH);
> +        current->xfer_status = cpu_to_le16(ch->regs[DBDMA_STATUS]);
>          current->res_count = cpu_to_le16(io->len);
>          dbdma_cmdptr_save(ch);
>      }
>  }
>  
> +static void dbdma_unassigned_flush(DBDMA_io *io)
> +{
> +    DBDMA_channel *ch = io->channel;
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
> +                  __func__, ch->channel);
> +}
> +
>  void* DBDMA_init (MemoryRegion **dbdma_mem)
>  {
>      DBDMAState *s;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 6/8] ppc: Fix OpenPIC model

[Mark Cave-Ayland]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Apple uses an IBM MPIC2A without timers, it has 64 sources.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/intc/openpic.c        |   35 +++++++++++++++++++++++++++++++++++
 hw/ppc/mac_newworld.c    |    2 +-
 include/hw/ppc/openpic.h |    1 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 9dd285b..10d6e87 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -92,6 +92,16 @@ static int get_current_cpu(void);
 #define RAVEN_MAX_TMR      OPENPIC_MAX_TMR
 #define RAVEN_MAX_IPI      OPENPIC_MAX_IPI
 
+/* KeyLargo */
+#define KEYLARGO_MAX_CPU  4
+#define KEYLARGO_MAX_EXT  64
+#define KEYLARGO_MAX_IPI  4
+#define KEYLARGO_MAX_IRQ  (64 + KEYLARGO_MAX_IPI)
+#define KEYLARGO_MAX_TMR  0
+#define KEYLARGO_IPI_IRQ  (KEYLARGO_MAX_EXT) /* First IPI IRQ */
+/* Timers don't exist but this makes the code happy... */
+#define KEYLARGO_TMR_IRQ  (KEYLARGO_IPI_IRQ + KEYLARGO_MAX_IPI)
+
 /* Interrupt definitions */
 #define RAVEN_FE_IRQ     (RAVEN_MAX_EXT)     /* Internal functional IRQ */
 #define RAVEN_ERR_IRQ    (RAVEN_MAX_EXT + 1) /* Error IRQ */
@@ -120,6 +130,7 @@ static FslMpicInfo fsl_mpic_42 = {
 #define VID_REVISION_1_3   3
 
 #define VIR_GENERIC      0x00000000 /* Generic Vendor ID */
+#define VIR_MPIC2A       0x00004614 /* IBM MPIC-2A */
 
 #define GCR_RESET        0x80000000
 #define GCR_MODE_PASS    0x00000000
@@ -329,6 +340,8 @@ typedef struct OpenPICState {
     uint32_t nb_cpus;
     /* Timer registers */
     OpenPICTimer timers[OPENPIC_MAX_TMR];
+    uint32_t max_tmr;
+
     /* Shared MSI registers */
     OpenPICMSI msi[MAX_MSI];
     uint32_t max_irq;
@@ -1717,6 +1730,28 @@ static void openpic_realize(DeviceState *dev, Error **errp)
 
         map_list(opp, list_le, &list_count);
         break;
+
+    case OPENPIC_MODEL_KEYLARGO:
+        opp->nb_irqs = KEYLARGO_MAX_EXT;
+        opp->vid = VID_REVISION_1_2;
+        opp->vir = VIR_GENERIC;
+        opp->vector_mask = 0xFF;
+        opp->tfrr_reset = 4160000;
+        opp->ivpr_reset = IVPR_MASK_MASK | IVPR_MODE_MASK;
+        opp->idr_reset = 0;
+        opp->max_irq = KEYLARGO_MAX_IRQ;
+        opp->irq_ipi0 = KEYLARGO_IPI_IRQ;
+        opp->irq_tim0 = KEYLARGO_TMR_IRQ;
+        opp->brr1 = -1;
+        opp->mpic_mode_mask = GCR_MODE_MIXED;
+
+        if (opp->nb_cpus != 1) {
+            error_setg(errp, "Only UP supported today");
+            return;
+        }
+
+        map_list(opp, list_le, &list_count);
+        break;
     }
 
     for (i = 0; i < opp->nb_cpus; i++) {
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c581d96..62bd8d3 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -351,7 +351,7 @@ static void ppc_core99_init(MachineState *machine)
     pic = g_new0(qemu_irq, 64);
 
     dev = qdev_create(NULL, TYPE_OPENPIC);
-    qdev_prop_set_uint32(dev, "model", OPENPIC_MODEL_RAVEN);
+    qdev_prop_set_uint32(dev, "model", OPENPIC_MODEL_KEYLARGO);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     pic_mem = s->mmio[0].memory;
diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
index 6137e2d..e55ce54 100644
--- a/include/hw/ppc/openpic.h
+++ b/include/hw/ppc/openpic.h
@@ -20,6 +20,7 @@ enum {
 #define OPENPIC_MODEL_RAVEN       0
 #define OPENPIC_MODEL_FSL_MPIC_20 1
 #define OPENPIC_MODEL_FSL_MPIC_42 2
+#define OPENPIC_MODEL_KEYLARGO    3
 
 #define OPENPIC_MAX_SRC     256
 #define OPENPIC_MAX_TMR     4
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 6/8] ppc: Fix OpenPIC model

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:46PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Apple uses an IBM MPIC2A without timers, it has 64 sources.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to ppc-for-2.11.

> ---
>  hw/intc/openpic.c        |   35 +++++++++++++++++++++++++++++++++++
>  hw/ppc/mac_newworld.c    |    2 +-
>  include/hw/ppc/openpic.h |    1 +
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 9dd285b..10d6e87 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -92,6 +92,16 @@ static int get_current_cpu(void);
>  #define RAVEN_MAX_TMR      OPENPIC_MAX_TMR
>  #define RAVEN_MAX_IPI      OPENPIC_MAX_IPI
>  
> +/* KeyLargo */
> +#define KEYLARGO_MAX_CPU  4
> +#define KEYLARGO_MAX_EXT  64
> +#define KEYLARGO_MAX_IPI  4
> +#define KEYLARGO_MAX_IRQ  (64 + KEYLARGO_MAX_IPI)
> +#define KEYLARGO_MAX_TMR  0
> +#define KEYLARGO_IPI_IRQ  (KEYLARGO_MAX_EXT) /* First IPI IRQ */
> +/* Timers don't exist but this makes the code happy... */
> +#define KEYLARGO_TMR_IRQ  (KEYLARGO_IPI_IRQ + KEYLARGO_MAX_IPI)
> +
>  /* Interrupt definitions */
>  #define RAVEN_FE_IRQ     (RAVEN_MAX_EXT)     /* Internal functional IRQ */
>  #define RAVEN_ERR_IRQ    (RAVEN_MAX_EXT + 1) /* Error IRQ */
> @@ -120,6 +130,7 @@ static FslMpicInfo fsl_mpic_42 = {
>  #define VID_REVISION_1_3   3
>  
>  #define VIR_GENERIC      0x00000000 /* Generic Vendor ID */
> +#define VIR_MPIC2A       0x00004614 /* IBM MPIC-2A */
>  
>  #define GCR_RESET        0x80000000
>  #define GCR_MODE_PASS    0x00000000
> @@ -329,6 +340,8 @@ typedef struct OpenPICState {
>      uint32_t nb_cpus;
>      /* Timer registers */
>      OpenPICTimer timers[OPENPIC_MAX_TMR];
> +    uint32_t max_tmr;
> +
>      /* Shared MSI registers */
>      OpenPICMSI msi[MAX_MSI];
>      uint32_t max_irq;
> @@ -1717,6 +1730,28 @@ static void openpic_realize(DeviceState *dev, Error **errp)
>  
>          map_list(opp, list_le, &list_count);
>          break;
> +
> +    case OPENPIC_MODEL_KEYLARGO:
> +        opp->nb_irqs = KEYLARGO_MAX_EXT;
> +        opp->vid = VID_REVISION_1_2;
> +        opp->vir = VIR_GENERIC;
> +        opp->vector_mask = 0xFF;
> +        opp->tfrr_reset = 4160000;
> +        opp->ivpr_reset = IVPR_MASK_MASK | IVPR_MODE_MASK;
> +        opp->idr_reset = 0;
> +        opp->max_irq = KEYLARGO_MAX_IRQ;
> +        opp->irq_ipi0 = KEYLARGO_IPI_IRQ;
> +        opp->irq_tim0 = KEYLARGO_TMR_IRQ;
> +        opp->brr1 = -1;
> +        opp->mpic_mode_mask = GCR_MODE_MIXED;
> +
> +        if (opp->nb_cpus != 1) {
> +            error_setg(errp, "Only UP supported today");
> +            return;
> +        }
> +
> +        map_list(opp, list_le, &list_count);
> +        break;
>      }
>  
>      for (i = 0; i < opp->nb_cpus; i++) {
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index c581d96..62bd8d3 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -351,7 +351,7 @@ static void ppc_core99_init(MachineState *machine)
>      pic = g_new0(qemu_irq, 64);
>  
>      dev = qdev_create(NULL, TYPE_OPENPIC);
> -    qdev_prop_set_uint32(dev, "model", OPENPIC_MODEL_RAVEN);
> +    qdev_prop_set_uint32(dev, "model", OPENPIC_MODEL_KEYLARGO);
>      qdev_init_nofail(dev);
>      s = SYS_BUS_DEVICE(dev);
>      pic_mem = s->mmio[0].memory;
> diff --git a/include/hw/ppc/openpic.h b/include/hw/ppc/openpic.h
> index 6137e2d..e55ce54 100644
> --- a/include/hw/ppc/openpic.h
> +++ b/include/hw/ppc/openpic.h
> @@ -20,6 +20,7 @@ enum {
>  #define OPENPIC_MODEL_RAVEN       0
>  #define OPENPIC_MODEL_FSL_MPIC_20 1
>  #define OPENPIC_MODEL_FSL_MPIC_42 2
> +#define OPENPIC_MODEL_KEYLARGO    3
>  
>  #define OPENPIC_MAX_SRC     256
>  #define OPENPIC_MAX_TMR     4

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 7/8] openpic: add missing timer fields to vmstate_openpic_timer

[Mark Cave-Ayland]

Observation of the code shows indicates that several timer fields are
missing from the migration stream.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/openpic.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 10d6e87..debfcbf 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1570,11 +1570,14 @@ static const VMStateDescription vmstate_openpic_irqsource = {
 
 static const VMStateDescription vmstate_openpic_timer = {
     .name = "openpic_timer",
-    .version_id = 0,
-    .minimum_version_id = 0,
+    .version_id = 1,
+    .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(tccr, OpenPICTimer),
         VMSTATE_UINT32(tbcr, OpenPICTimer),
+        VMSTATE_BOOL(qemu_timer_active, OpenPICTimer),
+        VMSTATE_TIMER_PTR(qemu_timer, OpenPICTimer),
+        VMSTATE_UINT64(origin_time, OpenPICTimer),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1620,7 +1623,7 @@ static const VMStateDescription vmstate_openpic = {
         VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState, NULL),
         VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
                                      vmstate_openpic_irqdest, IRQDest),
-        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
+        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 1,
                              vmstate_openpic_timer, OpenPICTimer),
         VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
                              vmstate_openpic_msi, OpenPICMSI),
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 7/8] openpic: add missing timer fields to vmstate_openpic_timer

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:47PM +0100, Mark Cave-Ayland wrote:
> Observation of the code shows indicates that several timer fields are
> missing from the migration stream.

So, adding new things to the migration stream always requires some
thought.  The commit message should contain a rationale for why the
proposed representation is a good one.  In particular is it one that's
unlikely to be difficult if the device changes internal details in
future.  When possible it's best to stick to architected documented
state of the modelled hardware, though I can see you're probably going
to need some extras in this case.

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/intc/openpic.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 10d6e87..debfcbf 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1570,11 +1570,14 @@ static const VMStateDescription vmstate_openpic_irqsource = {
>  
>  static const VMStateDescription vmstate_openpic_timer = {
>      .name = "openpic_timer",
> -    .version_id = 0,
> -    .minimum_version_id = 0,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(tccr, OpenPICTimer),
>          VMSTATE_UINT32(tbcr, OpenPICTimer),
> +        VMSTATE_BOOL(qemu_timer_active, OpenPICTimer),
> +        VMSTATE_TIMER_PTR(qemu_timer, OpenPICTimer),
> +        VMSTATE_UINT64(origin_time, OpenPICTimer),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -1620,7 +1623,7 @@ static const VMStateDescription vmstate_openpic = {
>          VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState, NULL),
>          VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
>                                       vmstate_openpic_irqdest, IRQDest),
> -        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
> +        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 1,
>                               vmstate_openpic_timer, OpenPICTimer),
>          VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
>                               vmstate_openpic_msi, OpenPICMSI),

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 7/8] openpic: add missing timer fields to vmstate_openpic_timer

[Mark Cave-Ayland]

On 18/09/17 23:44, David Gibson wrote:

> So, adding new things to the migration stream always requires some
> thought.  The commit message should contain a rationale for why the
> proposed representation is a good one.  In particular is it one that's
> unlikely to be difficult if the device changes internal details in
> future.  When possible it's best to stick to architected documented
> state of the modelled hardware, though I can see you're probably going
> to need some extras in this case.

Yes, I think that's the case here. Definitely the timer can't be armed
correctly without origin_time, and if qemu_timer_active isn't present
and migration occurs with an active timer then a subsequent call to
openpic_tmr_get_timer() will be incorrect. Does that sound right to you?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 7/8] openpic: add missing timer fields to vmstate_openpic_timer

[David Gibson]

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

On Tue, Sep 19, 2017 at 08:50:49PM +0100, Mark Cave-Ayland wrote:
> On 18/09/17 23:44, David Gibson wrote:
> 
> > So, adding new things to the migration stream always requires some
> > thought.  The commit message should contain a rationale for why the
> > proposed representation is a good one.  In particular is it one that's
> > unlikely to be difficult if the device changes internal details in
> > future.  When possible it's best to stick to architected documented
> > state of the modelled hardware, though I can see you're probably going
> > to need some extras in this case.
> 
> Yes, I think that's the case here. Definitely the timer can't be armed
> correctly without origin_time, and if qemu_timer_active isn't present
> and migration occurs with an active timer then a subsequent call to
> openpic_tmr_get_timer() will be incorrect. Does that sound right to
> you?

You'll certainly need some sort of time variable, which could be
expressed in various ways equivalently.  I don't know if you'll need
an explicit extra "active" boolean - are there any openpic registers
which would indicate if a timer is active?  It's generally best to
represent state using documented forms from the hardware whenever
possible.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 8/8] openpic: Fix problem when IRQ transitions from edge to level

[Mark Cave-Ayland]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Some interrupts get triggered before the OS has setup the
right interrupt type. If an edge interrupt is latched that
way, not delivered (still masked), then the interrupt is
changed to level and isn't asserted anymore, it will be
stuck "pending", causing an interrupt flood. This can happen
with the PMU GPIO interrupt for example.

There are a few other corner cases like this, so let's keep
track of the input "level" so we can properly re-evaluate
when the trigger type changes.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/intc/openpic.c |   32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index debfcbf..34749f8 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -236,6 +236,7 @@ typedef struct IRQSource {
     int last_cpu;
     int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
     int pending;    /* TRUE if IRQ is pending */
+    int cur_level;  /* Cache current level */
     IRQType type;
     bool level:1;   /* level-triggered */
     bool nomask:1;  /* critical interrupts ignore mask on some FSL MPICs */
@@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
     }
 
     src = &opp->src[n_IRQ];
-    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
-            n_IRQ, level, src->ivpr);
+    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n",
+            n_IRQ, level, src->ivpr, src->level, src->cur_level);
+
+    /* Keep track of the current input level in order to properly deal
+     * with the configuration of the source changing from edge to level
+     * after it's been latched. Otherwise the interrupt can get stuck.
+     */
+    src->cur_level = level;
+
     if (src->level) {
-        /* level-sensitive irq */
         src->pending = level;
         openpic_update_irq(opp, n_IRQ);
     } else {
-        /* edge-sensitive irq */
+        /* edge-sensitive irq
+         *
+         * In an ideal world we would only set pending on an "edge", ie
+         * if level is set and src->cur_level as clear. However that would
+         * require all the devices to properly send "pulses" rather than
+         * just "raise" which isn't the case today.
+         */
         if (level) {
             src->pending = 1;
             openpic_update_irq(opp, n_IRQ);
@@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
     switch (opp->src[n_IRQ].type) {
     case IRQ_TYPE_NORMAL:
         opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK);
+
+        /* If we switched to level we need to re-evaluate "pending" based
+         * on the actual input state.
+         */
+        if (opp->src[n_IRQ].level) {
+            opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level;
+        }
         break;
 
     case IRQ_TYPE_FSLINT:
@@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
         break;
     }
 
+    /* Re-evaluate a level irq */
     openpic_update_irq(opp, n_IRQ);
     DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
             opp->src[n_IRQ].ivpr);
@@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
     }
 
     if (!src->level) {
-        /* edge-sensitive IRQ */
+        /* edge-sensitive IRQ or level dropped */
         src->ivpr &= ~IVPR_ACTIVITY_MASK;
         src->pending = 0;
         IRQ_resetbit(&dst->raised, irq);
@@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
         VMSTATE_UINT32(destmask, IRQSource),
         VMSTATE_INT32(last_cpu, IRQSource),
         VMSTATE_INT32(pending, IRQSource),
+        VMSTATE_INT32(cur_level, IRQSource),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 8/8] openpic: Fix problem when IRQ transitions from edge to level

[David Gibson]

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

On Sun, Sep 17, 2017 at 06:15:48PM +0100, Mark Cave-Ayland wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Some interrupts get triggered before the OS has setup the
> right interrupt type. If an edge interrupt is latched that
> way, not delivered (still masked), then the interrupt is
> changed to level and isn't asserted anymore, it will be
> stuck "pending", causing an interrupt flood. This can happen
> with the PMU GPIO interrupt for example.
> 
> There are a few other corner cases like this, so let's keep
> track of the input "level" so we can properly re-evaluate
> when the trigger type changes.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/intc/openpic.c |   32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index debfcbf..34749f8 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -236,6 +236,7 @@ typedef struct IRQSource {
>      int last_cpu;
>      int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
>      int pending;    /* TRUE if IRQ is pending */
> +    int cur_level;  /* Cache current level */
>      IRQType type;
>      bool level:1;   /* level-triggered */
>      bool nomask:1;  /* critical interrupts ignore mask on some FSL MPICs */
> @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
>      }
>  
>      src = &opp->src[n_IRQ];
> -    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
> -            n_IRQ, level, src->ivpr);
> +    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n",
> +            n_IRQ, level, src->ivpr, src->level, src->cur_level);
> +
> +    /* Keep track of the current input level in order to properly deal
> +     * with the configuration of the source changing from edge to level
> +     * after it's been latched. Otherwise the interrupt can get stuck.
> +     */
> +    src->cur_level = level;
> +
>      if (src->level) {
> -        /* level-sensitive irq */
>          src->pending = level;
>          openpic_update_irq(opp, n_IRQ);
>      } else {
> -        /* edge-sensitive irq */
> +        /* edge-sensitive irq
> +         *
> +         * In an ideal world we would only set pending on an "edge", ie
> +         * if level is set and src->cur_level as clear. However that would
> +         * require all the devices to properly send "pulses" rather than
> +         * just "raise" which isn't the case today.
> +         */
>          if (level) {
>              src->pending = 1;
>              openpic_update_irq(opp, n_IRQ);
> @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>      switch (opp->src[n_IRQ].type) {
>      case IRQ_TYPE_NORMAL:
>          opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK);
> +
> +        /* If we switched to level we need to re-evaluate "pending" based
> +         * on the actual input state.
> +         */
> +        if (opp->src[n_IRQ].level) {
> +            opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level;
> +        }
>          break;
>  
>      case IRQ_TYPE_FSLINT:
> @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>          break;
>      }
>  
> +    /* Re-evaluate a level irq */
>      openpic_update_irq(opp, n_IRQ);
>      DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
>              opp->src[n_IRQ].ivpr);
> @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
>      }
>  
>      if (!src->level) {
> -        /* edge-sensitive IRQ */
> +        /* edge-sensitive IRQ or level dropped */
>          src->ivpr &= ~IVPR_ACTIVITY_MASK;
>          src->pending = 0;
>          IRQ_resetbit(&dst->raised, irq);
> @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
>          VMSTATE_UINT32(destmask, IRQSource),
>          VMSTATE_INT32(last_cpu, IRQSource),
>          VMSTATE_INT32(pending, IRQSource),
> +        VMSTATE_INT32(cur_level, IRQSource),
>          VMSTATE_END_OF_LIST()

This alters the migration stream without bumping the version number.
I suspect it will be best to move this hunk into your other patch
updating the migration of the openpic.

>      }
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 8/8] openpic: Fix problem when IRQ transitions from edge to level

[Mark Cave-Ayland]

On 18/09/17 21:39, David Gibson wrote:

> On Sun, Sep 17, 2017 at 06:15:48PM +0100, Mark Cave-Ayland wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> Some interrupts get triggered before the OS has setup the
>> right interrupt type. If an edge interrupt is latched that
>> way, not delivered (still masked), then the interrupt is
>> changed to level and isn't asserted anymore, it will be
>> stuck "pending", causing an interrupt flood. This can happen
>> with the PMU GPIO interrupt for example.
>>
>> There are a few other corner cases like this, so let's keep
>> track of the input "level" so we can properly re-evaluate
>> when the trigger type changes.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> ---
>>  hw/intc/openpic.c |   32 +++++++++++++++++++++++++++-----
>>  1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
>> index debfcbf..34749f8 100644
>> --- a/hw/intc/openpic.c
>> +++ b/hw/intc/openpic.c
>> @@ -236,6 +236,7 @@ typedef struct IRQSource {
>>      int last_cpu;
>>      int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
>>      int pending;    /* TRUE if IRQ is pending */
>> +    int cur_level;  /* Cache current level */
>>      IRQType type;
>>      bool level:1;   /* level-triggered */
>>      bool nomask:1;  /* critical interrupts ignore mask on some FSL MPICs */
>> @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
>>      }
>>  
>>      src = &opp->src[n_IRQ];
>> -    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n",
>> -            n_IRQ, level, src->ivpr);
>> +    DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n",
>> +            n_IRQ, level, src->ivpr, src->level, src->cur_level);
>> +
>> +    /* Keep track of the current input level in order to properly deal
>> +     * with the configuration of the source changing from edge to level
>> +     * after it's been latched. Otherwise the interrupt can get stuck.
>> +     */
>> +    src->cur_level = level;
>> +
>>      if (src->level) {
>> -        /* level-sensitive irq */
>>          src->pending = level;
>>          openpic_update_irq(opp, n_IRQ);
>>      } else {
>> -        /* edge-sensitive irq */
>> +        /* edge-sensitive irq
>> +         *
>> +         * In an ideal world we would only set pending on an "edge", ie
>> +         * if level is set and src->cur_level as clear. However that would
>> +         * require all the devices to properly send "pulses" rather than
>> +         * just "raise" which isn't the case today.
>> +         */
>>          if (level) {
>>              src->pending = 1;
>>              openpic_update_irq(opp, n_IRQ);
>> @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>>      switch (opp->src[n_IRQ].type) {
>>      case IRQ_TYPE_NORMAL:
>>          opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK);
>> +
>> +        /* If we switched to level we need to re-evaluate "pending" based
>> +         * on the actual input state.
>> +         */
>> +        if (opp->src[n_IRQ].level) {
>> +            opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level;
>> +        }
>>          break;
>>  
>>      case IRQ_TYPE_FSLINT:
>> @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
>>          break;
>>      }
>>  
>> +    /* Re-evaluate a level irq */
>>      openpic_update_irq(opp, n_IRQ);
>>      DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
>>              opp->src[n_IRQ].ivpr);
>> @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
>>      }
>>  
>>      if (!src->level) {
>> -        /* edge-sensitive IRQ */
>> +        /* edge-sensitive IRQ or level dropped */
>>          src->ivpr &= ~IVPR_ACTIVITY_MASK;
>>          src->pending = 0;
>>          IRQ_resetbit(&dst->raised, irq);
>> @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = {
>>          VMSTATE_UINT32(destmask, IRQSource),
>>          VMSTATE_INT32(last_cpu, IRQSource),
>>          VMSTATE_INT32(pending, IRQSource),
>> +        VMSTATE_INT32(cur_level, IRQSource),
>>          VMSTATE_END_OF_LIST()
> 
> This alters the migration stream without bumping the version number.
> I suspect it will be best to move this hunk into your other patch
> updating the migration of the openpic.

Yes, that's fine. I'll wait for your reply to the previous openpic patch
before resubmitting though.


ATB,

Mark.