[PULL 0/4] qemu-sparc queue 20210106

[PULL 0/4] qemu-sparc queue 20210106

[Mark Cave-Ayland]

The following changes since commit 2e0b5bbe813930021b2baab03c9d424c1c52d18b:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/mips-20210104' into staging (2021-01-05 21:06:42 +0000)

are available in the Git repository at:

  git://github.com/mcayland/qemu.git tags/qemu-sparc-20210106

for you to fetch changes up to a879306ca14de576d3a5dd51f830ebf89753e223:

  sun4m: don't connect two qemu_irqs directly to the same input (2021-01-06 11:41:37 +0000)

----------------------------------------------------------------
qemu-sparc queue

----------------------------------------------------------------
Mark Cave-Ayland (1):
      sun4m: don't connect two qemu_irqs directly to the same input

Peter Maydell (2):
      hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines
      include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

Philippe Mathieu-Daudé (1):
      hw/timer/slavio_timer: Allow 64-bit accesses

 hw/intc/grlib_irqmp.c    |  5 ++++-
 hw/sparc/Kconfig         |  1 +
 hw/sparc/leon3.c         | 21 +++++++++------------
 hw/sparc/sun4m.c         | 24 +++++++++++++++++++-----
 hw/timer/slavio_timer.c  |  4 ++++
 include/hw/sparc/grlib.h |  4 ----
 6 files changed, 37 insertions(+), 22 deletions(-)

[PULL 1/4] hw/timer/slavio_timer: Allow 64-bit accesses

[Mark Cave-Ayland]

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

Per the "NCR89C105 Chip Specification" referenced in the header:

                  Chip-level Address Map

  ------------------------------------------------------------------
  | 1D0 0000 ->   | Counter/Timers                        | W,D    |
  |   1DF FFFF    |                                       |        |
  ...

  The address map indicated the allowed accesses at each address.
  [...] W indicates a word access, and D indicates a double-word
  access.

The SLAVIO timer controller is implemented expecting 32-bit accesses.
Commit a3d12d073e1 restricted the memory accesses to 32-bit, while
the device allows 64-bit accesses.

This was not an issue until commit 5d971f9e67 which reverted
("memory: accept mismatching sizes in memory_region_access_valid").

Fix by renaming .valid MemoryRegionOps as .impl, and add the valid
access range (W -> 4, D -> 8).

Since commit 21786c7e598 ("memory: Log invalid memory accesses")
this class of bug can be quickly debugged displaying 'guest_errors'
accesses, as:

  $ qemu-system-sparc -M SS-20 -m 256 -bios ss20_v2.25_rom -serial stdio -d guest_errors

  Power-ON Reset
  Invalid access at addr 0x0, size 8, region 'timer-1', reason: invalid size (min:4 max:4)

  $ qemu-system-sparc -M SS-20 -m 256 -bios ss20_v2.25_rom -monitor stdio -S
  (qemu) info mtree
  address-space: memory
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      ...
      0000000ff1300000-0000000ff130000f (prio 0, i/o): timer-1
             ^^^^^^^^^                                 ^^^^^^^
                   \ memory region base address and name /

  (qemu) info qtree
  bus: main-system-bus
    dev: slavio_timer, id ""              <-- device type name
      gpio-out "sysbus-irq" 17
      num_cpus = 1 (0x1)
      mmio 0000000ff1310000/0000000000000014
      mmio 0000000ff1300000/0000000000000010 <--- base address
      mmio 0000000ff1301000/0000000000000010
      mmio 0000000ff1302000/0000000000000010
      ...

Reported-by: Yap KV <yapkv@yahoo.com>
Buglink: https://bugs.launchpad.net/bugs/1906905
Fixes: a3d12d073e1 ("slavio_timer: convert to memory API")
CC: qemu-stable@nongnu.org
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20201205150903.3062711-1-f4bug@amsat.org>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/timer/slavio_timer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
index 5b2d20cb6a..03e33fc592 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -331,6 +331,10 @@ static const MemoryRegionOps slavio_timer_mem_ops = {
     .write = slavio_timer_mem_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
-- 
2.20.1

[PULL 2/4] hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines

[Mark Cave-Ayland]

From: Peter Maydell <peter.maydell@linaro.org>

Currently the GRLIB_IRQMP device is used in one place (the leon3 board),
but instead of the device providing inbound gpio lines for the board
to wire up, the board code itself calls qemu_allocate_irqs() with
the handler function being a set_irq function defined in the code
for the device.

Refactor this into the standard setup of a device having input
gpio lines.

This fixes a trivial Coverity memory leak report (the leon3
board code leaks the IRQ array returned from qemu_allocate_irqs()).

Fixes: Coverity CID 1421922
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201212144134.29594-2-peter.maydell@linaro.org>
Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/grlib_irqmp.c    |  5 ++++-
 hw/sparc/leon3.c         | 21 +++++++++------------
 include/hw/sparc/grlib.h |  2 --
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index ffec4a07ee..984334fa7b 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -51,6 +51,8 @@
 #define FORCE_OFFSET     0x80
 #define EXTENDED_OFFSET  0xC0
 
+#define MAX_PILS 16
+
 OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
 
 typedef struct IRQMPState IRQMPState;
@@ -126,7 +128,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
     grlib_irqmp_ack_mask(state, mask);
 }
 
-void grlib_irqmp_set_irq(void *opaque, int irq, int level)
+static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
 {
     IRQMP      *irqmp = GRLIB_IRQMP(opaque);
     IRQMPState *s;
@@ -328,6 +330,7 @@ static void grlib_irqmp_init(Object *obj)
     IRQMP *irqmp = GRLIB_IRQMP(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
+    qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
     qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
     memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
                           "irqmp", IRQMP_REG_SIZE);
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 4bc4ebea84..7e16eea9e6 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -52,8 +52,6 @@
 #define LEON3_PROM_OFFSET    (0x00000000)
 #define LEON3_RAM_OFFSET     (0x40000000)
 
-#define MAX_PILS 16
-
 #define LEON3_UART_OFFSET  (0x80000100)
 #define LEON3_UART_IRQ     (3)
 
@@ -194,11 +192,10 @@ static void leon3_generic_hw_init(MachineState *machine)
     MemoryRegion *prom = g_new(MemoryRegion, 1);
     int         ret;
     char       *filename;
-    qemu_irq   *cpu_irqs = NULL;
     int         bios_size;
     int         prom_size;
     ResetData  *reset_info;
-    DeviceState *dev;
+    DeviceState *dev, *irqmpdev;
     int i;
     AHBPnp *ahb_pnp;
     APBPnp *apb_pnp;
@@ -230,16 +227,15 @@ static void leon3_generic_hw_init(MachineState *machine)
                             GRLIB_AHB_SLAVE, GRLIB_AHBMEM_AREA);
 
     /* Allocate IRQ manager */
-    dev = qdev_new(TYPE_GRLIB_IRQMP);
+    irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
     qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
                                         env, "pil", 1);
-    qdev_connect_gpio_out_named(dev, "grlib-irq", 0,
+    qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
                                 qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_IRQMP_OFFSET);
-    env->irq_manager = dev;
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
+    env->irq_manager = irqmpdev;
     env->qemu_irq_ack = leon3_irq_manager;
-    cpu_irqs = qemu_allocate_irqs(grlib_irqmp_set_irq, dev, MAX_PILS);
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
                             GRLIB_VENDOR_GAISLER, GRLIB_IRQMP_DEV,
                             2, 0, GRLIB_APBIO_AREA);
@@ -330,7 +326,7 @@ static void leon3_generic_hw_init(MachineState *machine)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_TIMER_OFFSET);
     for (i = 0; i < LEON3_TIMER_COUNT; i++) {
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
-                           cpu_irqs[LEON3_TIMER_IRQ + i]);
+                           qdev_get_gpio_in(irqmpdev, LEON3_TIMER_IRQ + i));
     }
 
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_TIMER_OFFSET, 0xFFF,
@@ -342,7 +338,8 @@ static void leon3_generic_hw_init(MachineState *machine)
     qdev_prop_set_chr(dev, "chrdev", serial_hd(0));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_UART_OFFSET);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irqs[LEON3_UART_IRQ]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
+                       qdev_get_gpio_in(irqmpdev, LEON3_UART_IRQ));
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_UART_OFFSET, 0xFFF,
                             GRLIB_VENDOR_GAISLER, GRLIB_APBUART_DEV, 1,
                             LEON3_UART_IRQ, GRLIB_APBIO_AREA);
diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
index 78b6178fcd..e1d1beaa73 100644
--- a/include/hw/sparc/grlib.h
+++ b/include/hw/sparc/grlib.h
@@ -36,8 +36,6 @@
 
 typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
 
-void grlib_irqmp_set_irq(void *opaque, int irq, int level);
-
 void grlib_irqmp_ack(DeviceState *dev, int intno);
 
 /* GPTimer */
-- 
2.20.1

[PULL 3/4] include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

[Mark Cave-Ayland]

From: Peter Maydell <peter.maydell@linaro.org>

The grlib.h header defines a set_pil_in_fn typedef which is never
used; remove it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201212144134.29594-3-peter.maydell@linaro.org>
Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/sparc/grlib.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
index e1d1beaa73..2104f493f3 100644
--- a/include/hw/sparc/grlib.h
+++ b/include/hw/sparc/grlib.h
@@ -34,8 +34,6 @@
 /* IRQMP */
 #define TYPE_GRLIB_IRQMP "grlib,irqmp"
 
-typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
-
 void grlib_irqmp_ack(DeviceState *dev, int intno);
 
 /* GPTimer */
-- 
2.20.1

[PULL 4/4] sun4m: don't connect two qemu_irqs directly to the same input

[Mark Cave-Ayland]

The sun4m board code connects both of the IRQ outputs of each ESCC to the
same slavio input qemu_irq. Connecting two qemu_irqs outputs directly to the
same input is not valid as it produces subtly wrong behaviour (for instance
if both the IRQ lines are high, and then one goes low, the PIC input will see
this as a high-to-low transition even though the second IRQ line should still
be holding it high).

This kind of wiring needs an explicitly created OR gate; add one.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Message-Id: <20201219111934.5540-1-mark.cave-ayland@ilande.co.uk>
Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc/Kconfig |  1 +
 hw/sparc/sun4m.c | 24 +++++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/hw/sparc/Kconfig b/hw/sparc/Kconfig
index 91805afab6..8dcb10086f 100644
--- a/hw/sparc/Kconfig
+++ b/hw/sparc/Kconfig
@@ -14,6 +14,7 @@ config SUN4M
     select M48T59
     select STP2000
     select CHRP_NVRAM
+    select OR_IRQ
 
 config LEON3
     bool
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 8686371318..38ca1e33c7 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -50,6 +50,7 @@
 #include "hw/misc/empty_slot.h"
 #include "hw/misc/unimp.h"
 #include "hw/irq.h"
+#include "hw/or-irq.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "trace.h"
@@ -848,7 +849,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     uint32_t initrd_size;
     DriveInfo *fd[MAX_FD];
     FWCfgState *fw_cfg;
-    DeviceState *dev;
+    DeviceState *dev, *ms_kb_orgate, *serial_orgate;
     SysBusDevice *s;
     unsigned int smp_cpus = machine->smp.cpus;
     unsigned int max_cpus = machine->smp.max_cpus;
@@ -994,10 +995,16 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     qdev_prop_set_uint32(dev, "chnAtype", escc_kbd);
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
-    sysbus_connect_irq(s, 0, slavio_irq[14]);
-    sysbus_connect_irq(s, 1, slavio_irq[14]);
     sysbus_mmio_map(s, 0, hwdef->ms_kb_base);
 
+    /* Logically OR both its IRQs together */
+    ms_kb_orgate = DEVICE(object_new(TYPE_OR_IRQ));
+    object_property_set_int(OBJECT(ms_kb_orgate), "num-lines", 2, &error_fatal);
+    qdev_realize_and_unref(ms_kb_orgate, NULL, &error_fatal);
+    sysbus_connect_irq(s, 0, qdev_get_gpio_in(ms_kb_orgate, 0));
+    sysbus_connect_irq(s, 1, qdev_get_gpio_in(ms_kb_orgate, 1));
+    qdev_connect_gpio_out(DEVICE(ms_kb_orgate), 0, slavio_irq[14]);
+
     dev = qdev_new(TYPE_ESCC);
     qdev_prop_set_uint32(dev, "disabled", 0);
     qdev_prop_set_uint32(dev, "frequency", ESCC_CLOCK);
@@ -1009,10 +1016,17 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
 
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
-    sysbus_connect_irq(s, 0, slavio_irq[15]);
-    sysbus_connect_irq(s, 1,  slavio_irq[15]);
     sysbus_mmio_map(s, 0, hwdef->serial_base);
 
+    /* Logically OR both its IRQs together */
+    serial_orgate = DEVICE(object_new(TYPE_OR_IRQ));
+    object_property_set_int(OBJECT(serial_orgate), "num-lines", 2,
+                            &error_fatal);
+    qdev_realize_and_unref(serial_orgate, NULL, &error_fatal);
+    sysbus_connect_irq(s, 0, qdev_get_gpio_in(serial_orgate, 0));
+    sysbus_connect_irq(s, 1, qdev_get_gpio_in(serial_orgate, 1));
+    qdev_connect_gpio_out(DEVICE(serial_orgate), 0, slavio_irq[15]);
+
     if (hwdef->apc_base) {
         apc_init(hwdef->apc_base, qemu_allocate_irq(cpu_halt_signal, NULL, 0));
     }
-- 
2.20.1

Re: [PULL 0/4] qemu-sparc queue 20210106

[Peter Maydell]

On Wed, 6 Jan 2021 at 12:05, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> The following changes since commit 2e0b5bbe813930021b2baab03c9d424c1c52d18b:
>
>   Merge remote-tracking branch 'remotes/philmd-gitlab/tags/mips-20210104' into staging (2021-01-05 21:06:42 +0000)
>
> are available in the Git repository at:
>
>   git://github.com/mcayland/qemu.git tags/qemu-sparc-20210106
>
> for you to fetch changes up to a879306ca14de576d3a5dd51f830ebf89753e223:
>
>   sun4m: don't connect two qemu_irqs directly to the same input (2021-01-06 11:41:37 +0000)
>
> ----------------------------------------------------------------
> qemu-sparc queue
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM