[Qemu-devel] [PATCH 1/3] milkymist-pfpu: more accurate emulation

[Qemu-devel] [PATCH 1/3] milkymist-pfpu: more accurate emulation

[Michael Walle]

Emulate write collisions, stray writes and microcode which has no VECTOUT
opcode. Although the latter was supported before, the emulation was
incorrect.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/misc/milkymist-pfpu.c | 105 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 30 deletions(-)

diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
index 86f5e383b0..1548e054b7 100644
--- a/hw/misc/milkymist-pfpu.c
+++ b/hw/misc/milkymist-pfpu.c
@@ -1,7 +1,7 @@
 /*
  *  QEMU model of the Milkymist programmable FPU.
  *
- *  Copyright (c) 2010 Michael Walle <michael@walle.cc>
+ *  Copyright (c) 2010-2018 Michael Walle <michael@walle.cc>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -121,6 +121,12 @@ static const char *opcode_to_str[] = {
 #define MILKYMIST_PFPU(obj) \
     OBJECT_CHECK(MilkymistPFPUState, (obj), TYPE_MILKYMIST_PFPU)
 
+struct OutputQueueEntry {
+    bool valid;
+    uint32_t value;
+};
+typedef struct OutputQueueEntry OutputQueueEntry;
+
 struct MilkymistPFPUState {
     SysBusDevice parent_obj;
 
@@ -133,7 +139,7 @@ struct MilkymistPFPUState {
     uint32_t microcode[MICROCODE_WORDS];
 
     int output_queue_pos;
-    uint32_t output_queue[MAX_LATENCY];
+    OutputQueueEntry output_queue[MAX_LATENCY];
 };
 typedef struct MilkymistPFPUState MilkymistPFPUState;
 
@@ -146,26 +152,37 @@ get_dma_address(uint32_t base, uint32_t x, uint32_t y)
 static inline void
 output_queue_insert(MilkymistPFPUState *s, uint32_t val, int pos)
 {
-    s->output_queue[(s->output_queue_pos + pos) % MAX_LATENCY] = val;
+    pos = (s->output_queue_pos + pos) % MAX_LATENCY;
+
+    /* if this output is already set, we have a collision */
+    if (s->output_queue[pos].valid == true) {
+        s->regs[R_COLLISIONS]++;
+    }
+    s->output_queue[pos].value = val;
+    s->output_queue[pos].valid = true;
 }
 
 static inline uint32_t
-output_queue_remove(MilkymistPFPUState *s)
+output_queue_get(MilkymistPFPUState *s)
 {
-    return s->output_queue[s->output_queue_pos];
+    return s->output_queue[s->output_queue_pos].value;
+}
+
+static inline bool
+output_queue_valid(MilkymistPFPUState *s)
+{
+    return s->output_queue[s->output_queue_pos].valid;
 }
 
 static inline void
 output_queue_advance(MilkymistPFPUState *s)
 {
-    s->output_queue[s->output_queue_pos] = 0;
+    s->output_queue[s->output_queue_pos].valid = false;
     s->output_queue_pos = (s->output_queue_pos + 1) % MAX_LATENCY;
 }
 
-static int pfpu_decode_insn(MilkymistPFPUState *s)
+static int pfpu_decode_insn(MilkymistPFPUState *s, uint32_t insn)
 {
-    uint32_t pc = s->regs[R_PC];
-    uint32_t insn = s->microcode[pc];
     uint32_t reg_a = (insn >> 18) & 0x7f;
     uint32_t reg_b = (insn >> 11) & 0x7f;
     uint32_t op = (insn >> 7) & 0xf;
@@ -237,7 +254,8 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
         cpu_physical_memory_write(dma_ptr, &a, 4);
         cpu_physical_memory_write(dma_ptr + 4, &b, 4);
         s->regs[R_LASTDMA] = dma_ptr + 4;
-        D_EXEC(qemu_log("VECTOUT a=%08x b=%08x dma=%08x\n", a, b, dma_ptr));
+        D_EXEC(qemu_log("VECTOUT a=%08x b=%08x dma=%08" HWADDR_PRIx "\n",
+                    a, b, dma_ptr));
         trace_milkymist_pfpu_vectout(a, b, dma_ptr);
     } break;
     case OP_SIN:
@@ -327,10 +345,13 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
     }
 
     /* store output for this cycle */
-    if (reg_d) {
-        uint32_t val = output_queue_remove(s);
+    if (output_queue_valid(s)) {
+        uint32_t val = output_queue_get(s);
         D_EXEC(qemu_log("R%03d <- 0x%08x\n", reg_d, val));
         s->gp_regs[reg_d] = val;
+        if (!reg_d) {
+            s->regs[R_STRAYWRITES]++;
+        }
     }
 
     output_queue_advance(s);
@@ -340,16 +361,21 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
         output_queue_insert(s, r, latency-1);
     }
 
-    /* advance PC */
-    s->regs[R_PC]++;
-
     return 1;
 };
 
 static void pfpu_start(MilkymistPFPUState *s)
 {
     int x, y;
-    int i;
+    int running;
+
+    /* indicate we are running */
+    s->regs[R_CTL] = CTL_START_BUSY;
+
+    /* starting the PFPU also clears some registers */
+    s->regs[R_VERTICES] = 0;
+    s->regs[R_COLLISIONS] = 0;
+    s->regs[R_STRAYWRITES] = 0;
 
     for (y = 0; y <= s->regs[R_VMESHLAST]; y++) {
         for (x = 0; x <= s->regs[R_HMESHLAST]; x++) {
@@ -359,23 +385,30 @@ static void pfpu_start(MilkymistPFPUState *s)
             s->gp_regs[GPR_X] = x;
             s->gp_regs[GPR_Y] = y;
 
-            /* run microcode on this position */
-            i = 0;
-            while (pfpu_decode_insn(s)) {
-                /* decode at most MICROCODE_WORDS instructions */
-                if (++i >= MICROCODE_WORDS) {
-                    error_report("milkymist_pfpu: too many instructions "
-                            "executed in microcode. No VECTOUT?");
-                    break;
-                }
+            /* reset pc */
+            s->regs[R_PC] = 0;
+
+            /* Run microcode. We decode at most MICROCODE_WORDS instructions,
+             * because there are no branches. The real hardware will spin
+             * endlessly if there is no VECTOUT instruction. */
+            do {
+                uint32_t insn = s->microcode[s->regs[R_PC]];
+                running = pfpu_decode_insn(s, insn);
+                s->regs[R_PC]++;
+            } while (running && (s->regs[R_PC] < MICROCODE_WORDS));
+
+            /* if there was no VECTOUT instructions, we just return and thus
+             * keeping the busy flag set */
+            if (running) {
+                return;
             }
 
-            /* reset pc for next run */
-            s->regs[R_PC] = 0;
+            s->regs[R_VERTICES]++;
         }
     }
 
-    s->regs[R_VERTICES] = x * y;
+    /* we are done */
+    s->regs[R_CTL] = 0;
 
     trace_milkymist_pfpu_pulse_irq();
     qemu_irq_pulse(s->irq);
@@ -493,7 +526,7 @@ static void milkymist_pfpu_reset(DeviceState *d)
     }
     s->output_queue_pos = 0;
     for (i = 0; i < MAX_LATENCY; i++) {
-        s->output_queue[i] = 0;
+        s->output_queue[i].valid = false;
     }
 }
 
@@ -510,6 +543,17 @@ static int milkymist_pfpu_init(SysBusDevice *dev)
     return 0;
 }
 
+static const VMStateDescription vmstate_output_queue_entry = {
+    .name = "milkymist-pfpu-output-queue-entry",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(valid, OutputQueueEntry),
+        VMSTATE_UINT32(value, OutputQueueEntry),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_milkymist_pfpu = {
     .name = "milkymist-pfpu",
     .version_id = 1,
@@ -519,7 +563,8 @@ static const VMStateDescription vmstate_milkymist_pfpu = {
         VMSTATE_UINT32_ARRAY(gp_regs, MilkymistPFPUState, 128),
         VMSTATE_UINT32_ARRAY(microcode, MilkymistPFPUState, MICROCODE_WORDS),
         VMSTATE_INT32(output_queue_pos, MilkymistPFPUState),
-        VMSTATE_UINT32_ARRAY(output_queue, MilkymistPFPUState, MAX_LATENCY),
+        VMSTATE_STRUCT_ARRAY(output_queue, MilkymistPFPUState, MAX_LATENCY, 1,
+                vmstate_output_queue_entry, OutputQueueEntry),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Michael Walle]

It is only possible to retrieve the current state of an interrupt line. But
there are devices which just pulses the interrupt line. Introduce a latch
which is set by qtest and which can be cleared by the test case.

Signed-off-by: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Andreas Färber <afaerber@suse.de>
---
 tests/libqtest.c | 19 +++++++++++++++++++
 tests/libqtest.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..85e1f6f92a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -40,6 +40,7 @@ struct QTestState
     int fd;
     int qmp_fd;
     bool irq_level[MAX_IRQ];
+    bool irq_latch[MAX_IRQ];
     GString *rx;
     pid_t qemu_pid;  /* our child QEMU process */
     bool big_endian;
@@ -233,6 +234,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
     s->rx = g_string_new("");
     for (i = 0; i < MAX_IRQ; i++) {
         s->irq_level[i] = false;
+        s->irq_latch[i] = false;
     }
 
     if (getenv("QTEST_STOP")) {
@@ -386,6 +388,7 @@ redo:
 
         if (strcmp(words[1], "raise") == 0) {
             s->irq_level[irq] = true;
+            s->irq_latch[irq] = true;
         } else {
             s->irq_level[irq] = false;
         }
@@ -678,6 +681,22 @@ bool qtest_get_irq(QTestState *s, int num)
     return s->irq_level[num];
 }
 
+bool qtest_get_irq_latched(QTestState *s, int num)
+{
+    g_assert_cmpint(num, <, MAX_IRQ);
+
+    /* dummy operation in order to make sure irq is up to date */
+    qtest_inb(s, 0);
+
+    return s->irq_latch[num];
+}
+
+void qtest_clear_irq_latch(QTestState *s, int num)
+{
+    g_assert_cmpint(num, <, MAX_IRQ);
+    s->irq_latch[num] = false;
+}
+
 static int64_t qtest_clock_rsp(QTestState *s)
 {
     gchar **words;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872cbe..721dd50863 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -192,6 +192,24 @@ char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
 bool qtest_get_irq(QTestState *s, int num);
 
 /**
+ * qtest_get_irq_latched:
+ * @s: #QTestState instance to operate on.
+ * @num: Interrupt to observe.
+ *
+ * Returns: The latched state of the @num interrupt.
+ */
+bool qtest_get_irq_latched(QTestState *s, int num);
+
+/**
+ * qtest_clear_irq_latch:
+ * @s: #QTestState instance to operate on.
+ * @num: Interrupt to operate on.
+ *
+ * Clears the latch of the @num interrupt.
+ */
+void qtest_clear_irq_latch(QTestState *s, int num);
+
+/**
  * qtest_irq_intercept_in:
  * @s: #QTestState instance to operate on.
  * @string: QOM path of a device.
@@ -638,6 +656,28 @@ static inline bool get_irq(int num)
 }
 
 /**
+ * get_irq_latched:
+ * @num: Interrupt to observe.
+ *
+ * Returns: The latched level of the @num interrupt.
+ */
+static inline bool get_irq_latched(int num)
+{
+    return qtest_get_irq_latched(global_qtest, num);
+}
+
+/**
+ * clear_irq_latch:
+ * @num: Interrupt to operate on.
+ *
+ * Clears the latch of the @num interrupt.
+ */
+static inline void clear_irq_latch(int num)
+{
+    return qtest_clear_irq_latch(global_qtest, num);
+}
+
+/**
  * irq_intercept_in:
  * @string: QOM path of a device.
  *
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] milkymist-pfpu: add qtests

[Michael Walle]

Add initial tests which check basic computations and error cases on the
PFPU.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 MAINTAINERS                 |   1 +
 hw/lm32/lm32.h              |   2 +
 tests/Makefile.include      |   4 +
 tests/milkymist-pfpu-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 200 insertions(+)
 create mode 100644 tests/milkymist-pfpu-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 666e936812..bde0d132b9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -171,6 +171,7 @@ F: hw/*/milkymist-*
 F: include/hw/char/lm32_juart.h
 F: include/hw/lm32/
 F: tests/tcg/lm32/
+F: tests/milkymist-*
 
 M68K
 M: Laurent Vivier <laurent@vivier.eu>
diff --git a/hw/lm32/lm32.h b/hw/lm32/lm32.h
index d1514a61b3..b14f7ce158 100644
--- a/hw/lm32/lm32.h
+++ b/hw/lm32/lm32.h
@@ -9,6 +9,8 @@ static inline DeviceState *lm32_pic_init(qemu_irq cpu_irq)
     SysBusDevice *d;
 
     dev = qdev_create(NULL, "lm32-pic");
+    object_property_add_child(qdev_get_machine(), "lm32-pic", OBJECT(dev),
+            NULL);
     qdev_init_nofail(dev);
     d = SYS_BUS_DEVICE(dev);
     sysbus_connect_irq(d, 0, cpu_irq);
diff --git a/tests/Makefile.include b/tests/Makefile.include
index a49282704e..d02d673f7d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -406,6 +406,8 @@ check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
 
+check-qtest-lm32-y = tests/milkymist-pfpu-test$(EXESUF)
+
 check-qtest-generic-y += tests/machine-none-test$(EXESUF)
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
@@ -868,6 +870,8 @@ tests/migration/initrd-stress.img: tests/migration/stress$(EXESUF)
 	rm $(INITRD_WORK_DIR)/init
 	rmdir $(INITRD_WORK_DIR)
 
+tests/milkymist-pfpu-test$(EXESUF): tests/milkymist-pfpu-test.o
+
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil
 endif
diff --git a/tests/milkymist-pfpu-test.c b/tests/milkymist-pfpu-test.c
new file mode 100644
index 0000000000..da506bbdf8
--- /dev/null
+++ b/tests/milkymist-pfpu-test.c
@@ -0,0 +1,193 @@
+/*
+ * QTest testcase for the Milkymist PFPU
+ *
+ * Copyright (c) 2018 Michael Walle <michael@walle.cc>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqtest.h"
+
+#define PFPU_BASE 0x60006000
+#define PFPU_IRQ 8
+
+static void pfpu_load_microcode(uint32_t *insn, int len)
+{
+    uint32_t addr = PFPU_BASE + 0x800;
+    while (len) {
+        writel(addr, *insn);
+        insn++;
+        len--;
+        addr += 4;
+    }
+}
+
+static void pfpu_clear_microcode(void)
+{
+    uint32_t addr = PFPU_BASE + 0x800;
+    uint32_t len = 512;
+    while (len) {
+        writel(addr, 0);
+        len--;
+        addr += 4;
+    }
+}
+
+static uint32_t float_to_u32(float val)
+{
+    return (*((uint32_t *)&(val)));
+}
+
+static void pfpu_add(void)
+{
+    static uint32_t ucode[] = {
+        0x000c2080, /* FADD R3, R4 */
+        0x00000000, /* NOP */
+        0x00000000, /* NOP */
+        0x00000000, /* NOP */
+        0x00000000, /* NOP */
+        0x00000003, /* NOP | EXIT R3 */
+        0x000c2380  /* VECTOUT R3, R4 */
+    };
+
+    /* 2x1 mesh size */
+    writel(PFPU_BASE + 0x8, 1);
+    writel(PFPU_BASE + 0xc, 0);
+
+    /* write test operands to r3 and r4 */
+    writel(PFPU_BASE + 0x40c, float_to_u32(3.0f));
+    writel(PFPU_BASE + 0x410, float_to_u32(9.0f));
+
+    pfpu_load_microcode(ucode, ARRAY_SIZE(ucode));
+
+    /* dma base */
+    writel(PFPU_BASE + 0x4, 0x40000000);
+
+    /* start */
+    writel(PFPU_BASE, 1);
+
+    /* on a successful run, the busy flag should be cleared */
+    g_assert(readl(PFPU_BASE) == 0);
+
+    /* interrupt line should have been pulsed */
+    g_assert(get_irq_latched(PFPU_IRQ) == 1);
+    clear_irq_latch(PFPU_IRQ);
+    g_assert(get_irq_latched(PFPU_IRQ) == 0);
+
+    /* resulting vertices should be written to RAM */
+    g_assert(readl(0x40000000) == float_to_u32(12.0f));
+    g_assert(readl(0x40000004) == float_to_u32(9.0f));
+    g_assert(readl(0x40000008) == float_to_u32(21.0f));
+    g_assert(readl(0x4000000c) == float_to_u32(9.0f));
+
+    /* count of computed vertices */
+    g_assert(readl(PFPU_BASE + 0x14) == 2);
+
+    /* no collisions */
+    g_assert(readl(PFPU_BASE + 0x18) == 0);
+
+    /* no stray writes */
+    g_assert(readl(PFPU_BASE + 0x1c) == 0);
+}
+
+static void pfpu_microcode_overflow(void)
+{
+    /* 1x1 mesh size */
+    writel(PFPU_BASE + 0x8, 1);
+    writel(PFPU_BASE + 0xc, 0);
+
+    pfpu_clear_microcode();
+
+    /* start */
+    writel(PFPU_BASE, 1);
+
+    /* because there is no VECTOUT, the busy flag should not be cleared */
+    g_assert(readl(PFPU_BASE) == 1);
+
+    /* and there should be no pending interrupt */
+    g_assert(get_irq_latched(PFPU_IRQ) == 0);
+}
+
+static void pfpu_stray_writes(void)
+{
+    static uint32_t ucode[] = {
+        0x000c0600, /* COPY R3 */
+        0x00000000, /* NOP */
+        0x00000000, /* NOP */
+        0x000c2380  /* VECTOUT R3, R4 */
+    };
+
+    /* 1x1 mesh size */
+    writel(PFPU_BASE + 0x8, 0);
+    writel(PFPU_BASE + 0xc, 0);
+
+    /* write test operands to r3 and r4 */
+    writel(PFPU_BASE + 0x40c, float_to_u32(1.0f));
+    writel(PFPU_BASE + 0x410, float_to_u32(2.0f));
+
+    pfpu_load_microcode(ucode, ARRAY_SIZE(ucode));
+
+    /* dma base */
+    writel(PFPU_BASE + 0x4, 0x40000000);
+
+    /* start */
+    writel(PFPU_BASE, 1);
+    clear_irq_latch(PFPU_IRQ);
+
+    /* stray writes */
+    g_assert(readl(PFPU_BASE + 0x1c) == 1);
+}
+
+static void pfpu_collision(void)
+{
+    static uint32_t ucode[] = {
+        0x000c0300, /* I2F R3 */
+        0x000c0600, /* COPY R3 */
+        0x00000000, /* NOP */
+        0x00000000, /* NOP */
+        0x000c2380  /* VECTOUT R3, R4 */
+    };
+
+    /* 1x1 mesh size */
+    writel(PFPU_BASE + 0x8, 0);
+    writel(PFPU_BASE + 0xc, 0);
+
+    /* write test operands to r3 and r4 */
+    writel(PFPU_BASE + 0x40c, float_to_u32(1.0f));
+    writel(PFPU_BASE + 0x410, float_to_u32(2.0f));
+
+    pfpu_load_microcode(ucode, ARRAY_SIZE(ucode));
+
+    /* dma base */
+    writel(PFPU_BASE + 0x4, 0x40000000);
+
+    /* start */
+    writel(PFPU_BASE, 1);
+    clear_irq_latch(PFPU_IRQ);
+
+    /* collisions */
+    g_assert(readl(PFPU_BASE + 0x18) == 1);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_start("-machine milkymist");
+    irq_intercept_in("lm32-pic");
+    qtest_add_func("/milkymist-pfpu/add", pfpu_add);
+    qtest_add_func("/milkymist-pfpu/microcode-overflow", pfpu_microcode_overflow);
+    qtest_add_func("/milkymist-pfpu/collision", pfpu_collision);
+    qtest_add_func("/milkymist-pfpu/stray-writes", pfpu_stray_writes);
+
+    ret = g_test_run();
+
+    qtest_end();
+
+    return ret;
+}
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Michael Walle]

Am 2018-07-18 12:48, schrieb Michael Walle:
> It is only possible to retrieve the current state of an interrupt line. 
> But
> there are devices which just pulses the interrupt line. Introduce a 
> latch
> which is set by qtest and which can be cleared by the test case.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Andreas Färber <afaerber@suse.de>

Hi,

unfortunately, there was never any feedback. Are you ok with this patch 
in general? There is still one patch for an additional test case in my 
queue, which depends on this.

-michael

> ---
>  tests/libqtest.c | 19 +++++++++++++++++++
>  tests/libqtest.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 098af6aec4..85e1f6f92a 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -40,6 +40,7 @@ struct QTestState
>      int fd;
>      int qmp_fd;
>      bool irq_level[MAX_IRQ];
> +    bool irq_latch[MAX_IRQ];
>      GString *rx;
>      pid_t qemu_pid;  /* our child QEMU process */
>      bool big_endian;
> @@ -233,6 +234,7 @@ QTestState *qtest_init_without_qmp_handshake(bool 
> use_oob,
>      s->rx = g_string_new("");
>      for (i = 0; i < MAX_IRQ; i++) {
>          s->irq_level[i] = false;
> +        s->irq_latch[i] = false;
>      }
> 
>      if (getenv("QTEST_STOP")) {
> @@ -386,6 +388,7 @@ redo:
> 
>          if (strcmp(words[1], "raise") == 0) {
>              s->irq_level[irq] = true;
> +            s->irq_latch[irq] = true;
>          } else {
>              s->irq_level[irq] = false;
>          }
> @@ -678,6 +681,22 @@ bool qtest_get_irq(QTestState *s, int num)
>      return s->irq_level[num];
>  }
> 
> +bool qtest_get_irq_latched(QTestState *s, int num)
> +{
> +    g_assert_cmpint(num, <, MAX_IRQ);
> +
> +    /* dummy operation in order to make sure irq is up to date */
> +    qtest_inb(s, 0);
> +
> +    return s->irq_latch[num];
> +}
> +
> +void qtest_clear_irq_latch(QTestState *s, int num)
> +{
> +    g_assert_cmpint(num, <, MAX_IRQ);
> +    s->irq_latch[num] = false;
> +}
> +
>  static int64_t qtest_clock_rsp(QTestState *s)
>  {
>      gchar **words;
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index ac52872cbe..721dd50863 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -192,6 +192,24 @@ char *qtest_hmpv(QTestState *s, const char *fmt,
> va_list ap);
>  bool qtest_get_irq(QTestState *s, int num);
> 
>  /**
> + * qtest_get_irq_latched:
> + * @s: #QTestState instance to operate on.
> + * @num: Interrupt to observe.
> + *
> + * Returns: The latched state of the @num interrupt.
> + */
> +bool qtest_get_irq_latched(QTestState *s, int num);
> +
> +/**
> + * qtest_clear_irq_latch:
> + * @s: #QTestState instance to operate on.
> + * @num: Interrupt to operate on.
> + *
> + * Clears the latch of the @num interrupt.
> + */
> +void qtest_clear_irq_latch(QTestState *s, int num);
> +
> +/**
>   * qtest_irq_intercept_in:
>   * @s: #QTestState instance to operate on.
>   * @string: QOM path of a device.
> @@ -638,6 +656,28 @@ static inline bool get_irq(int num)
>  }
> 
>  /**
> + * get_irq_latched:
> + * @num: Interrupt to observe.
> + *
> + * Returns: The latched level of the @num interrupt.
> + */
> +static inline bool get_irq_latched(int num)
> +{
> +    return qtest_get_irq_latched(global_qtest, num);
> +}
> +
> +/**
> + * clear_irq_latch:
> + * @num: Interrupt to operate on.
> + *
> + * Clears the latch of the @num interrupt.
> + */
> +static inline void clear_irq_latch(int num)
> +{
> +    return qtest_clear_irq_latch(global_qtest, num);
> +}
> +
> +/**
>   * irq_intercept_in:
>   * @string: QOM path of a device.
>   *

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Andreas Färber]

Am 31.10.18 um 09:31 schrieb Michael Walle:
> Am 2018-07-18 12:48, schrieb Michael Walle:
>> It is only possible to retrieve the current state of an interrupt
>> line. But
>> there are devices which just pulses the interrupt line. Introduce a latch
>> which is set by qtest and which can be cleared by the test case.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Andreas Färber <afaerber@suse.de>
> 
> Hi,
> 
> unfortunately, there was never any feedback. Are you ok with this patch
> in general? There is still one patch for an additional test case in my
> queue, which depends on this.

No objection from my side, but Paolo may be a better reviewer for irq.

Regards,
Andreas

> 
> -michael
> 
>> ---
>>  tests/libqtest.c | 19 +++++++++++++++++++
>>  tests/libqtest.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 098af6aec4..85e1f6f92a 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -40,6 +40,7 @@ struct QTestState
>>      int fd;
>>      int qmp_fd;
>>      bool irq_level[MAX_IRQ];
>> +    bool irq_latch[MAX_IRQ];
>>      GString *rx;
>>      pid_t qemu_pid;  /* our child QEMU process */
>>      bool big_endian;
>> @@ -233,6 +234,7 @@ QTestState *qtest_init_without_qmp_handshake(bool
>> use_oob,
>>      s->rx = g_string_new("");
>>      for (i = 0; i < MAX_IRQ; i++) {
>>          s->irq_level[i] = false;
>> +        s->irq_latch[i] = false;
>>      }
>>
>>      if (getenv("QTEST_STOP")) {
>> @@ -386,6 +388,7 @@ redo:
>>
>>          if (strcmp(words[1], "raise") == 0) {
>>              s->irq_level[irq] = true;
>> +            s->irq_latch[irq] = true;
>>          } else {
>>              s->irq_level[irq] = false;
>>          }
>> @@ -678,6 +681,22 @@ bool qtest_get_irq(QTestState *s, int num)
>>      return s->irq_level[num];
>>  }
>>
>> +bool qtest_get_irq_latched(QTestState *s, int num)
>> +{
>> +    g_assert_cmpint(num, <, MAX_IRQ);
>> +
>> +    /* dummy operation in order to make sure irq is up to date */
>> +    qtest_inb(s, 0);
>> +
>> +    return s->irq_latch[num];
>> +}
>> +
>> +void qtest_clear_irq_latch(QTestState *s, int num)
>> +{
>> +    g_assert_cmpint(num, <, MAX_IRQ);
>> +    s->irq_latch[num] = false;
>> +}
>> +
>>  static int64_t qtest_clock_rsp(QTestState *s)
>>  {
>>      gchar **words;
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index ac52872cbe..721dd50863 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -192,6 +192,24 @@ char *qtest_hmpv(QTestState *s, const char *fmt,
>> va_list ap);
>>  bool qtest_get_irq(QTestState *s, int num);
>>
>>  /**
>> + * qtest_get_irq_latched:
>> + * @s: #QTestState instance to operate on.
>> + * @num: Interrupt to observe.
>> + *
>> + * Returns: The latched state of the @num interrupt.
>> + */
>> +bool qtest_get_irq_latched(QTestState *s, int num);
>> +
>> +/**
>> + * qtest_clear_irq_latch:
>> + * @s: #QTestState instance to operate on.
>> + * @num: Interrupt to operate on.
>> + *
>> + * Clears the latch of the @num interrupt.
>> + */
>> +void qtest_clear_irq_latch(QTestState *s, int num);
>> +
>> +/**
>>   * qtest_irq_intercept_in:
>>   * @s: #QTestState instance to operate on.
>>   * @string: QOM path of a device.
>> @@ -638,6 +656,28 @@ static inline bool get_irq(int num)
>>  }
>>
>>  /**
>> + * get_irq_latched:
>> + * @num: Interrupt to observe.
>> + *
>> + * Returns: The latched level of the @num interrupt.
>> + */
>> +static inline bool get_irq_latched(int num)
>> +{
>> +    return qtest_get_irq_latched(global_qtest, num);
>> +}
>> +
>> +/**
>> + * clear_irq_latch:
>> + * @num: Interrupt to operate on.
>> + *
>> + * Clears the latch of the @num interrupt.
>> + */
>> +static inline void clear_irq_latch(int num)
>> +{
>> +    return qtest_clear_irq_latch(global_qtest, num);
>> +}
>> +
>> +/**
>>   * irq_intercept_in:
>>   * @string: QOM path of a device.
>>   *


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Paolo Bonzini]

On 18/07/2018 12:48, Michael Walle wrote:
>  /**
> + * get_irq_latched:
> + * @num: Interrupt to observe.
> + *
> + * Returns: The latched level of the @num interrupt.
> + */
> +static inline bool get_irq_latched(int num)
> +{
> +    return qtest_get_irq_latched(global_qtest, num);
> +}
> +
> +/**
> + * clear_irq_latch:
> + * @num: Interrupt to operate on.
> + *
> + * Clears the latch of the @num interrupt.
> + */
> +static inline void clear_irq_latch(int num)
> +{
> +    return qtest_clear_irq_latch(global_qtest, num);
> +}
> +
> +/**
>   * irq_intercept_in:
>   * @string: QOM path of a device.
>   *
> 

Just one thing, I think an atomic test-and-clear is a better API.  I
understand that what you have works, because
get_irq_latched+clear_irq_latch don't have any intervening access to the
qtest socket.

Paolo

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Michael Walle]

Am 2018-10-31 11:19, schrieb Paolo Bonzini:
> On 18/07/2018 12:48, Michael Walle wrote:
>>  /**
>> + * get_irq_latched:
>> + * @num: Interrupt to observe.
>> + *
>> + * Returns: The latched level of the @num interrupt.
>> + */
>> +static inline bool get_irq_latched(int num)
>> +{
>> +    return qtest_get_irq_latched(global_qtest, num);
>> +}
>> +
>> +/**
>> + * clear_irq_latch:
>> + * @num: Interrupt to operate on.
>> + *
>> + * Clears the latch of the @num interrupt.
>> + */
>> +static inline void clear_irq_latch(int num)
>> +{
>> +    return qtest_clear_irq_latch(global_qtest, num);
>> +}
>> +
>> +/**
>>   * irq_intercept_in:
>>   * @string: QOM path of a device.
>>   *
>> 
> 
> Just one thing, I think an atomic test-and-clear is a better API.  I
> understand that what you have works, because
> get_irq_latched+clear_irq_latch don't have any intervening access to 
> the
> qtest socket.

Ok thanks, like get_and_clear_irq_latched()? Do you think of we still 
need the get_irq_latched() to get the state without actually clearing 
it? I mean my simple test case won't need it, but there might be some 
cases where you may want peek at the state.

-michael

Re: [Qemu-devel] [PATCH 2/3] qtest: new functions for pulsed IRQs

[Paolo Bonzini]

On 31/10/2018 15:30, Michael Walle wrote:
>>
>> Just one thing, I think an atomic test-and-clear is a better API.  I
>> understand that what you have works, because
>> get_irq_latched+clear_irq_latch don't have any intervening access to the
>> qtest socket.
> 
> Ok thanks, like get_and_clear_irq_latched()? Do you think of we still
> need the get_irq_latched() to get the state without actually clearing
> it? I mean my simple test case won't need it, but there might be some
> cases where you may want peek at the state.

Maybe - it's easy enough to add it, so if your test doesn't need it you
can certainly leave it out.

Paolo