[PULL 0/5] M68k for 6.0 patches

[PULL 0/5] M68k for 6.0 patches

[Laurent Vivier]

The following changes since commit e7c6a8cf9f5c82aa152273e1c9e80d07b1b0c32c:

  Merge remote-tracking branch 'remotes/philmd/tags/avr-20210315' into stagin=
g (2021-03-15 16:59:55 +0000)

are available in the Git repository at:

  git://github.com/vivier/qemu-m68k.git tags/m68k-for-6.0-pull-request

for you to fetch changes up to e1cecdca559d552bc5ab282696301858a97c3e8c:

  m68k: add Virtual M68k Machine (2021-03-15 21:03:06 +0100)

----------------------------------------------------------------
m68k pull request 20210315

Add m68k virt machine

----------------------------------------------------------------

Laurent Vivier (5):
  hw/char: add goldfish-tty
  hw/intc: add goldfish-pic
  m68k: add an interrupt controller
  m68k: add a system controller
  m68k: add Virtual M68k Machine

 docs/specs/virt-ctlr.txt                      |  26 ++
 default-configs/devices/m68k-softmmu.mak      |   1 +
 include/hw/char/goldfish_tty.h                |  35 ++
 include/hw/intc/goldfish_pic.h                |  33 ++
 include/hw/intc/m68k_irqc.h                   |  41 +++
 include/hw/misc/virt_ctrl.h                   |  22 ++
 .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
 hw/char/goldfish_tty.c                        | 285 ++++++++++++++++
 hw/intc/goldfish_pic.c                        | 219 ++++++++++++
 hw/intc/m68k_irqc.c                           | 119 +++++++
 hw/m68k/virt.c                                | 313 ++++++++++++++++++
 hw/misc/virt_ctrl.c                           | 151 +++++++++
 MAINTAINERS                                   |  13 +
 hw/char/Kconfig                               |   3 +
 hw/char/meson.build                           |   2 +
 hw/char/trace-events                          |  10 +
 hw/intc/Kconfig                               |   6 +
 hw/intc/meson.build                           |   2 +
 hw/intc/trace-events                          |   8 +
 hw/m68k/Kconfig                               |   9 +
 hw/m68k/meson.build                           |   1 +
 hw/misc/Kconfig                               |   3 +
 hw/misc/meson.build                           |   3 +
 hw/misc/trace-events                          |   7 +
 24 files changed, 1330 insertions(+)
 create mode 100644 docs/specs/virt-ctlr.txt
 create mode 100644 include/hw/char/goldfish_tty.h
 create mode 100644 include/hw/intc/goldfish_pic.h
 create mode 100644 include/hw/intc/m68k_irqc.h
 create mode 100644 include/hw/misc/virt_ctrl.h
 create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
 create mode 100644 hw/char/goldfish_tty.c
 create mode 100644 hw/intc/goldfish_pic.c
 create mode 100644 hw/intc/m68k_irqc.c
 create mode 100644 hw/m68k/virt.c
 create mode 100644 hw/misc/virt_ctrl.c

--=20
2.29.2

[PULL 1/5] hw/char: add goldfish-tty

[Laurent Vivier]

Implement the goldfish tty device as defined in

https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT

and based on the kernel driver code:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/goldfish.c

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210312214145.2936082-2-laurent@vivier.eu>
---
 include/hw/char/goldfish_tty.h |  35 ++++
 hw/char/goldfish_tty.c         | 285 +++++++++++++++++++++++++++++++++
 hw/char/Kconfig                |   3 +
 hw/char/meson.build            |   2 +
 hw/char/trace-events           |  10 ++
 5 files changed, 335 insertions(+)
 create mode 100644 include/hw/char/goldfish_tty.h
 create mode 100644 hw/char/goldfish_tty.c

diff --git a/include/hw/char/goldfish_tty.h b/include/hw/char/goldfish_tty.h
new file mode 100644
index 000000000000..b9dd67362a68
--- /dev/null
+++ b/include/hw/char/goldfish_tty.h
@@ -0,0 +1,35 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Goldfish TTY
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#ifndef HW_CHAR_GOLDFISH_TTY_H
+#define HW_CHAR_GOLDFISH_TTY_H
+
+#include "qemu/fifo8.h"
+#include "chardev/char-fe.h"
+
+#define TYPE_GOLDFISH_TTY "goldfish_tty"
+OBJECT_DECLARE_SIMPLE_TYPE(GoldfishTTYState, GOLDFISH_TTY)
+
+#define GOLFISH_TTY_BUFFER_SIZE 128
+
+struct GoldfishTTYState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+    CharBackend chr;
+
+    uint32_t data_len;
+    uint64_t data_ptr;
+    bool int_enabled;
+
+    Fifo8 rx_fifo;
+};
+
+#endif
diff --git a/hw/char/goldfish_tty.c b/hw/char/goldfish_tty.c
new file mode 100644
index 000000000000..8365a1876145
--- /dev/null
+++ b/hw/char/goldfish_tty.c
@@ -0,0 +1,285 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Goldfish TTY
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "chardev/char-fe.h"
+#include "qemu/log.h"
+#include "trace.h"
+#include "exec/address-spaces.h"
+#include "hw/char/goldfish_tty.h"
+
+#define GOLDFISH_TTY_VERSION 1
+
+/* registers */
+
+enum {
+    REG_PUT_CHAR      = 0x00,
+    REG_BYTES_READY   = 0x04,
+    REG_CMD           = 0x08,
+    REG_DATA_PTR      = 0x10,
+    REG_DATA_LEN      = 0x14,
+    REG_DATA_PTR_HIGH = 0x18,
+    REG_VERSION       = 0x20,
+};
+
+/* commands */
+
+enum {
+    CMD_INT_DISABLE   = 0x00,
+    CMD_INT_ENABLE    = 0x01,
+    CMD_WRITE_BUFFER  = 0x02,
+    CMD_READ_BUFFER   = 0x03,
+};
+
+static uint64_t goldfish_tty_read(void *opaque, hwaddr addr,
+                                  unsigned size)
+{
+    GoldfishTTYState *s = opaque;
+    uint64_t value = 0;
+
+    switch (addr) {
+    case REG_BYTES_READY:
+        value = fifo8_num_used(&s->rx_fifo);
+        break;
+    case REG_VERSION:
+        value = GOLDFISH_TTY_VERSION;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register read 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+
+    trace_goldfish_tty_read(s, addr, size, value);
+
+    return value;
+}
+
+static void goldfish_tty_cmd(GoldfishTTYState *s, uint32_t cmd)
+{
+    uint32_t to_copy;
+    uint8_t *buf;
+    uint8_t data_out[GOLFISH_TTY_BUFFER_SIZE];
+    int len;
+    uint64_t ptr;
+
+    switch (cmd) {
+    case CMD_INT_DISABLE:
+        if (s->int_enabled) {
+            if (!fifo8_is_empty(&s->rx_fifo)) {
+                qemu_set_irq(s->irq, 0);
+            }
+            s->int_enabled = false;
+        }
+        break;
+    case CMD_INT_ENABLE:
+        if (!s->int_enabled) {
+            if (!fifo8_is_empty(&s->rx_fifo)) {
+                qemu_set_irq(s->irq, 1);
+            }
+            s->int_enabled = true;
+        }
+        break;
+    case CMD_WRITE_BUFFER:
+        len = s->data_len;
+        ptr = s->data_ptr;
+        while (len) {
+            to_copy = MIN(GOLFISH_TTY_BUFFER_SIZE, len);
+
+            address_space_rw(&address_space_memory, ptr,
+                             MEMTXATTRS_UNSPECIFIED, data_out, to_copy, 0);
+            qemu_chr_fe_write_all(&s->chr, data_out, to_copy);
+
+            len -= to_copy;
+            ptr += to_copy;
+        }
+        break;
+    case CMD_READ_BUFFER:
+        len = s->data_len;
+        ptr = s->data_ptr;
+        while (len && !fifo8_is_empty(&s->rx_fifo)) {
+            buf = (uint8_t *)fifo8_pop_buf(&s->rx_fifo, len, &to_copy);
+            address_space_rw(&address_space_memory, ptr,
+                            MEMTXATTRS_UNSPECIFIED, buf, to_copy, 1);
+
+            len -= to_copy;
+            ptr += to_copy;
+        }
+        if (s->int_enabled && fifo8_is_empty(&s->rx_fifo)) {
+            qemu_set_irq(s->irq, 0);
+        }
+        break;
+    }
+}
+
+static void goldfish_tty_write(void *opaque, hwaddr addr,
+                               uint64_t value, unsigned size)
+{
+    GoldfishTTYState *s = opaque;
+    unsigned char c;
+
+    trace_goldfish_tty_write(s, addr, size, value);
+
+    switch (addr) {
+    case REG_PUT_CHAR:
+        c = value;
+        qemu_chr_fe_write_all(&s->chr, &c, sizeof(c));
+        break;
+    case REG_CMD:
+        goldfish_tty_cmd(s, value);
+        break;
+    case REG_DATA_PTR:
+        s->data_ptr = value;
+        break;
+    case REG_DATA_PTR_HIGH:
+        s->data_ptr = deposit64(s->data_ptr, 32, 32, value);
+        break;
+    case REG_DATA_LEN:
+        s->data_len = value;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register write 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+}
+
+static const MemoryRegionOps goldfish_tty_ops = {
+    .read = goldfish_tty_read,
+    .write = goldfish_tty_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.max_access_size = 4,
+    .impl.max_access_size = 4,
+    .impl.min_access_size = 4,
+};
+
+static int goldfish_tty_can_receive(void *opaque)
+{
+    GoldfishTTYState *s = opaque;
+    int available = fifo8_num_free(&s->rx_fifo);
+
+    trace_goldfish_tty_can_receive(s, available);
+
+    return available;
+}
+
+static void goldfish_tty_receive(void *opaque, const uint8_t *buffer, int size)
+{
+    GoldfishTTYState *s = opaque;
+
+    trace_goldfish_tty_receive(s, size);
+
+    g_assert(size <= fifo8_num_free(&s->rx_fifo));
+
+    fifo8_push_all(&s->rx_fifo, buffer, size);
+
+    if (s->int_enabled && !fifo8_is_empty(&s->rx_fifo)) {
+        qemu_set_irq(s->irq, 1);
+    }
+}
+
+static void goldfish_tty_reset(DeviceState *dev)
+{
+    GoldfishTTYState *s = GOLDFISH_TTY(dev);
+
+    trace_goldfish_tty_reset(s);
+
+    fifo8_reset(&s->rx_fifo);
+    s->int_enabled = false;
+    s->data_ptr = 0;
+    s->data_len = 0;
+}
+
+static void goldfish_tty_realize(DeviceState *dev, Error **errp)
+{
+    GoldfishTTYState *s = GOLDFISH_TTY(dev);
+
+    trace_goldfish_tty_realize(s);
+
+    fifo8_create(&s->rx_fifo, GOLFISH_TTY_BUFFER_SIZE);
+    memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_tty_ops, s,
+                          "goldfish_tty", 0x24);
+
+    if (qemu_chr_fe_backend_connected(&s->chr)) {
+        qemu_chr_fe_set_handlers(&s->chr, goldfish_tty_can_receive,
+                                 goldfish_tty_receive, NULL, NULL,
+                                 s, NULL, true);
+    }
+}
+
+static void goldfish_tty_unrealize(DeviceState *dev)
+{
+    GoldfishTTYState *s = GOLDFISH_TTY(dev);
+
+    trace_goldfish_tty_unrealize(s);
+
+    fifo8_destroy(&s->rx_fifo);
+}
+
+static const VMStateDescription vmstate_goldfish_tty = {
+    .name = "goldfish_tty",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(data_len, GoldfishTTYState),
+        VMSTATE_UINT64(data_ptr, GoldfishTTYState),
+        VMSTATE_BOOL(int_enabled, GoldfishTTYState),
+        VMSTATE_FIFO8(rx_fifo, GoldfishTTYState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property goldfish_tty_properties[] = {
+    DEFINE_PROP_CHR("chardev", GoldfishTTYState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void goldfish_tty_instance_init(Object *obj)
+{
+    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+    GoldfishTTYState *s = GOLDFISH_TTY(obj);
+
+    trace_goldfish_tty_instance_init(s);
+
+    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_irq(dev, &s->irq);
+}
+
+static void goldfish_tty_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    device_class_set_props(dc, goldfish_tty_properties);
+    dc->reset = goldfish_tty_reset;
+    dc->realize = goldfish_tty_realize;
+    dc->unrealize = goldfish_tty_unrealize;
+    dc->vmsd = &vmstate_goldfish_tty;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
+static const TypeInfo goldfish_tty_info = {
+    .name = TYPE_GOLDFISH_TTY,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .class_init = goldfish_tty_class_init,
+    .instance_init = goldfish_tty_instance_init,
+    .instance_size = sizeof(GoldfishTTYState),
+};
+
+static void goldfish_tty_register_types(void)
+{
+    type_register_static(&goldfish_tty_info);
+}
+
+type_init(goldfish_tty_register_types)
diff --git a/hw/char/Kconfig b/hw/char/Kconfig
index f6f4fffd1b7c..4cf36ac637ba 100644
--- a/hw/char/Kconfig
+++ b/hw/char/Kconfig
@@ -64,3 +64,6 @@ config MCHP_PFSOC_MMUART
 
 config SIFIVE_UART
     bool
+
+config GOLDFISH_TTY
+    bool
diff --git a/hw/char/meson.build b/hw/char/meson.build
index 7ba38dbd965f..da5bb8b762e0 100644
--- a/hw/char/meson.build
+++ b/hw/char/meson.build
@@ -39,3 +39,5 @@ specific_ss.add(when: 'CONFIG_HTIF', if_true: files('riscv_htif.c'))
 specific_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('terminal3270.c'))
 specific_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-serial-bus.c'))
 specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_vty.c'))
+
+specific_ss.add(when: 'CONFIG_GOLDFISH_TTY', if_true: files('goldfish_tty.c'))
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 81026f661277..76d52938ead3 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -20,6 +20,16 @@ virtio_console_flush_buf(unsigned int port, size_t len, ssize_t ret) "port %u, i
 virtio_console_chr_read(unsigned int port, int size) "port %u, size %d"
 virtio_console_chr_event(unsigned int port, int event) "port %u, event %d"
 
+# goldfish_tty.c
+goldfish_tty_read(void *dev, unsigned int addr, unsigned int size, uint64_t value) "tty: %p reg: 0x%02x size: %d value: 0x%"PRIx64
+goldfish_tty_write(void *dev, unsigned int addr, unsigned int size, uint64_t value) "tty: %p reg: 0x%02x size: %d value: 0x%"PRIx64
+goldfish_tty_can_receive(void *dev, unsigned int available) "tty: %p available: %u"
+goldfish_tty_receive(void *dev, unsigned int size) "tty: %p size: %u"
+goldfish_tty_reset(void *dev) "tty: %p"
+goldfish_tty_realize(void *dev) "tty: %p"
+goldfish_tty_unrealize(void *dev) "tty: %p"
+goldfish_tty_instance_init(void *dev) "tty: %p"
+
 # grlib_apbuart.c
 grlib_apbuart_event(int event) "event:%d"
 grlib_apbuart_writel_unknown(uint64_t addr, uint32_t value) "addr 0x%"PRIx64" value 0x%x"
-- 
2.29.2

[PULL 2/5] hw/intc: add goldfish-pic

[Laurent Vivier]

Implement the goldfish pic device as defined in

https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210312214145.2936082-3-laurent@vivier.eu>
---
 include/hw/intc/goldfish_pic.h |  33 +++++
 hw/intc/goldfish_pic.c         | 219 +++++++++++++++++++++++++++++++++
 hw/intc/Kconfig                |   3 +
 hw/intc/meson.build            |   1 +
 hw/intc/trace-events           |   8 ++
 5 files changed, 264 insertions(+)
 create mode 100644 include/hw/intc/goldfish_pic.h
 create mode 100644 hw/intc/goldfish_pic.c

diff --git a/include/hw/intc/goldfish_pic.h b/include/hw/intc/goldfish_pic.h
new file mode 100644
index 000000000000..ad13ab37fc3e
--- /dev/null
+++ b/include/hw/intc/goldfish_pic.h
@@ -0,0 +1,33 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Goldfish PIC
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#ifndef HW_INTC_GOLDFISH_PIC_H
+#define HW_INTC_GOLDFISH_PIC_H
+
+#define TYPE_GOLDFISH_PIC "goldfish_pic"
+OBJECT_DECLARE_SIMPLE_TYPE(GoldfishPICState, GOLDFISH_PIC)
+
+#define GOLDFISH_PIC_IRQ_NB 32
+
+struct GoldfishPICState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t pending;
+    uint32_t enabled;
+
+    /* statistics */
+    uint64_t stats_irq_count[32];
+    /* for tracing */
+    uint8_t idx;
+};
+
+#endif
diff --git a/hw/intc/goldfish_pic.c b/hw/intc/goldfish_pic.c
new file mode 100644
index 000000000000..e3b43a69f163
--- /dev/null
+++ b/hw/intc/goldfish_pic.c
@@ -0,0 +1,219 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Goldfish PIC
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "monitor/monitor.h"
+#include "qemu/log.h"
+#include "trace.h"
+#include "hw/intc/intc.h"
+#include "hw/intc/goldfish_pic.h"
+
+/* registers */
+
+enum {
+    REG_STATUS          = 0x00,
+    REG_IRQ_PENDING     = 0x04,
+    REG_IRQ_DISABLE_ALL = 0x08,
+    REG_DISABLE         = 0x0c,
+    REG_ENABLE          = 0x10,
+};
+
+static bool goldfish_pic_get_statistics(InterruptStatsProvider *obj,
+                                        uint64_t **irq_counts,
+                                        unsigned int *nb_irqs)
+{
+    GoldfishPICState *s = GOLDFISH_PIC(obj);
+
+    *irq_counts = s->stats_irq_count;
+    *nb_irqs = ARRAY_SIZE(s->stats_irq_count);
+    return true;
+}
+
+static void goldfish_pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
+{
+    GoldfishPICState *s = GOLDFISH_PIC(obj);
+    monitor_printf(mon, "goldfish-pic.%d: pending=0x%08x enabled=0x%08x\n",
+                   s->idx, s->pending, s->enabled);
+}
+
+static void goldfish_pic_update(GoldfishPICState *s)
+{
+    if (s->pending & s->enabled) {
+        qemu_irq_raise(s->irq);
+    } else {
+        qemu_irq_lower(s->irq);
+    }
+}
+
+static void goldfish_irq_request(void *opaque, int irq, int level)
+{
+    GoldfishPICState *s = opaque;
+
+    trace_goldfish_irq_request(s, s->idx, irq, level);
+
+    if (level) {
+        s->pending |= 1 << irq;
+        s->stats_irq_count[irq]++;
+    } else {
+        s->pending &= ~(1 << irq);
+    }
+    goldfish_pic_update(s);
+}
+
+static uint64_t goldfish_pic_read(void *opaque, hwaddr addr,
+                                  unsigned size)
+{
+    GoldfishPICState *s = opaque;
+    uint64_t value = 0;
+
+    switch (addr) {
+    case REG_STATUS:
+        /* The number of pending interrupts (0 to 32) */
+        value = ctpop32(s->pending & s->enabled);
+        break;
+    case REG_IRQ_PENDING:
+        /* The pending interrupt mask */
+        value = s->pending & s->enabled;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register read 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+
+    trace_goldfish_pic_read(s, s->idx, addr, size, value);
+
+    return value;
+}
+
+static void goldfish_pic_write(void *opaque, hwaddr addr,
+                               uint64_t value, unsigned size)
+{
+    GoldfishPICState *s = opaque;
+
+    trace_goldfish_pic_write(s, s->idx, addr, size, value);
+
+    switch (addr) {
+    case REG_IRQ_DISABLE_ALL:
+        s->enabled = 0;
+        s->pending = 0;
+        break;
+    case REG_DISABLE:
+        s->enabled &= ~value;
+        break;
+    case REG_ENABLE:
+        s->enabled |= value;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register write 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+    goldfish_pic_update(s);
+}
+
+static const MemoryRegionOps goldfish_pic_ops = {
+    .read = goldfish_pic_read,
+    .write = goldfish_pic_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+};
+
+static void goldfish_pic_reset(DeviceState *dev)
+{
+    GoldfishPICState *s = GOLDFISH_PIC(dev);
+    int i;
+
+    trace_goldfish_pic_reset(s, s->idx);
+    s->pending = 0;
+    s->enabled = 0;
+
+    for (i = 0; i < ARRAY_SIZE(s->stats_irq_count); i++) {
+        s->stats_irq_count[i] = 0;
+    }
+}
+
+static void goldfish_pic_realize(DeviceState *dev, Error **errp)
+{
+    GoldfishPICState *s = GOLDFISH_PIC(dev);
+
+    trace_goldfish_pic_realize(s, s->idx);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_pic_ops, s,
+                          "goldfish_pic", 0x24);
+}
+
+static const VMStateDescription vmstate_goldfish_pic = {
+    .name = "goldfish_pic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(pending, GoldfishPICState),
+        VMSTATE_UINT32(enabled, GoldfishPICState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void goldfish_pic_instance_init(Object *obj)
+{
+    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+    GoldfishPICState *s = GOLDFISH_PIC(obj);
+
+    trace_goldfish_pic_instance_init(s);
+
+    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_irq(dev, &s->irq);
+
+    qdev_init_gpio_in(DEVICE(obj), goldfish_irq_request, GOLDFISH_PIC_IRQ_NB);
+}
+
+static Property goldfish_pic_properties[] = {
+    DEFINE_PROP_UINT8("index", GoldfishPICState, idx, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void goldfish_pic_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc);
+
+    dc->reset = goldfish_pic_reset;
+    dc->realize = goldfish_pic_realize;
+    dc->vmsd = &vmstate_goldfish_pic;
+    ic->get_statistics = goldfish_pic_get_statistics;
+    ic->print_info = goldfish_pic_print_info;
+    device_class_set_props(dc, goldfish_pic_properties);
+}
+
+static const TypeInfo goldfish_pic_info = {
+    .name = TYPE_GOLDFISH_PIC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .class_init = goldfish_pic_class_init,
+    .instance_init = goldfish_pic_instance_init,
+    .instance_size = sizeof(GoldfishPICState),
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_INTERRUPT_STATS_PROVIDER },
+         { }
+    },
+};
+
+static void goldfish_pic_register_types(void)
+{
+    type_register_static(&goldfish_pic_info);
+}
+
+type_init(goldfish_pic_register_types)
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 66bf0b90b47a..186cb5daa0ff 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -67,3 +67,6 @@ config SIFIVE_CLINT
 
 config SIFIVE_PLIC
     bool
+
+config GOLDFISH_PIC
+    bool
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 8df3656419e3..5fcb923dd13e 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -57,3 +57,4 @@ specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('xics_spapr.c', 'spapr_xi
 specific_ss.add(when: 'CONFIG_XIVE', if_true: files('xive.c'))
 specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
 		if_true: files('spapr_xive_kvm.c'))
+specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 45ddaf48df8e..c9ab17234b44 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -239,3 +239,11 @@ xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END 0x%x
 
 # pnv_xive.c
 pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" val=0x%"PRIx64
+
+# goldfish_pic.c
+goldfish_irq_request(void *dev, int idx, int irq, int level) "pic: %p goldfish-irq.%d irq: %d level: %d"
+goldfish_pic_read(void *dev, int idx, unsigned int addr, unsigned int size, uint64_t value) "pic: %p goldfish-irq.%d reg: 0x%02x size: %d value: 0x%"PRIx64
+goldfish_pic_write(void *dev, int idx, unsigned int addr, unsigned int size, uint64_t value) "pic: %p goldfish-irq.%d reg: 0x%02x size: %d value: 0x%"PRIx64
+goldfish_pic_reset(void *dev, int idx) "pic: %p goldfish-irq.%d"
+goldfish_pic_realize(void *dev, int idx) "pic: %p goldfish-irq.%d"
+goldfish_pic_instance_init(void *dev) "pic: %p goldfish-irq"
-- 
2.29.2

[PULL 3/5] m68k: add an interrupt controller

[Laurent Vivier]

A (generic) copy of the GLUE device we already have for q800 to use with
the m68k-virt machine.
The q800 one would disappear in the future as q800 uses actually the djMEMC
controller.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210312214145.2936082-4-laurent@vivier.eu>
---
 include/hw/intc/m68k_irqc.h |  41 +++++++++++++
 hw/intc/m68k_irqc.c         | 119 ++++++++++++++++++++++++++++++++++++
 hw/intc/Kconfig             |   3 +
 hw/intc/meson.build         |   1 +
 4 files changed, 164 insertions(+)
 create mode 100644 include/hw/intc/m68k_irqc.h
 create mode 100644 hw/intc/m68k_irqc.c

diff --git a/include/hw/intc/m68k_irqc.h b/include/hw/intc/m68k_irqc.h
new file mode 100644
index 000000000000..dbcfcfc2e000
--- /dev/null
+++ b/include/hw/intc/m68k_irqc.h
@@ -0,0 +1,41 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * QEMU Motorola 680x0 IRQ Controller
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#ifndef M68K_IRQC_H
+#define M68K_IRQC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_M68K_IRQC "m68k-irq-controller"
+#define M68K_IRQC(obj) OBJECT_CHECK(M68KIRQCState, (obj), \
+                                    TYPE_M68K_IRQC)
+
+#define M68K_IRQC_AUTOVECTOR_BASE 25
+
+enum {
+    M68K_IRQC_LEVEL_1 = 0,
+    M68K_IRQC_LEVEL_2,
+    M68K_IRQC_LEVEL_3,
+    M68K_IRQC_LEVEL_4,
+    M68K_IRQC_LEVEL_5,
+    M68K_IRQC_LEVEL_6,
+    M68K_IRQC_LEVEL_7,
+};
+#define M68K_IRQC_LEVEL_NUM (M68K_IRQC_LEVEL_7 - M68K_IRQC_LEVEL_1 + 1)
+
+typedef struct M68KIRQCState {
+    SysBusDevice parent_obj;
+
+    uint8_t ipr;
+
+    /* statistics */
+    uint64_t stats_irq_count[M68K_IRQC_LEVEL_NUM];
+} M68KIRQCState;
+
+#endif
diff --git a/hw/intc/m68k_irqc.c b/hw/intc/m68k_irqc.c
new file mode 100644
index 000000000000..2133d2a698ab
--- /dev/null
+++ b/hw/intc/m68k_irqc.c
@@ -0,0 +1,119 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * QEMU Motorola 680x0 IRQ Controller
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "migration/vmstate.h"
+#include "monitor/monitor.h"
+#include "hw/nmi.h"
+#include "hw/intc/intc.h"
+#include "hw/intc/m68k_irqc.h"
+
+
+static bool m68k_irqc_get_statistics(InterruptStatsProvider *obj,
+                                     uint64_t **irq_counts, unsigned int *nb_irqs)
+{
+    M68KIRQCState *s = M68K_IRQC(obj);
+
+    *irq_counts = s->stats_irq_count;
+    *nb_irqs = ARRAY_SIZE(s->stats_irq_count);
+    return true;
+}
+
+static void m68k_irqc_print_info(InterruptStatsProvider *obj, Monitor *mon)
+{
+    M68KIRQCState *s = M68K_IRQC(obj);
+    monitor_printf(mon, "m68k-irqc: ipr=0x%x\n", s->ipr);
+}
+
+static void m68k_set_irq(void *opaque, int irq, int level)
+{
+    M68KIRQCState *s = opaque;
+    M68kCPU *cpu = M68K_CPU(first_cpu);
+    int i;
+
+    if (level) {
+        s->ipr |= 1 << irq;
+        s->stats_irq_count[irq]++;
+    } else {
+        s->ipr &= ~(1 << irq);
+    }
+
+    for (i = M68K_IRQC_LEVEL_7; i >= M68K_IRQC_LEVEL_1; i--) {
+        if ((s->ipr >> i) & 1) {
+            m68k_set_irq_level(cpu, i + 1, i + M68K_IRQC_AUTOVECTOR_BASE);
+            return;
+        }
+    }
+    m68k_set_irq_level(cpu, 0, 0);
+}
+
+static void m68k_irqc_reset(DeviceState *d)
+{
+    M68KIRQCState *s = M68K_IRQC(d);
+    int i;
+
+    s->ipr = 0;
+    for (i = 0; i < ARRAY_SIZE(s->stats_irq_count); i++) {
+        s->stats_irq_count[i] = 0;
+    }
+}
+
+static void m68k_irqc_instance_init(Object *obj)
+{
+    qdev_init_gpio_in(DEVICE(obj), m68k_set_irq, M68K_IRQC_LEVEL_NUM);
+}
+
+static void m68k_nmi(NMIState *n, int cpu_index, Error **errp)
+{
+    m68k_set_irq(n, M68K_IRQC_LEVEL_7, 1);
+}
+
+static const VMStateDescription vmstate_m68k_irqc = {
+    .name = "m68k-irqc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(ipr, M68KIRQCState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void m68k_irqc_class_init(ObjectClass *oc, void *data)
+ {
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    NMIClass *nc = NMI_CLASS(oc);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc);
+
+    nc->nmi_monitor_handler = m68k_nmi;
+    dc->reset = m68k_irqc_reset;
+    dc->vmsd = &vmstate_m68k_irqc;
+    ic->get_statistics = m68k_irqc_get_statistics;
+    ic->print_info = m68k_irqc_print_info;
+}
+
+static const TypeInfo m68k_irqc_type_info = {
+    .name = TYPE_M68K_IRQC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(M68KIRQCState),
+    .instance_init = m68k_irqc_instance_init,
+    .class_init = m68k_irqc_class_init,
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_NMI },
+         { TYPE_INTERRUPT_STATS_PROVIDER },
+         { }
+    },
+};
+
+static void q800_irq_register_types(void)
+{
+    type_register_static(&m68k_irqc_type_info);
+}
+
+type_init(q800_irq_register_types);
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 186cb5daa0ff..f4694088a483 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -70,3 +70,6 @@ config SIFIVE_PLIC
 
 config GOLDFISH_PIC
     bool
+
+config M68K_IRQC
+    bool
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 5fcb923dd13e..1c299039f650 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -58,3 +58,4 @@ specific_ss.add(when: 'CONFIG_XIVE', if_true: files('xive.c'))
 specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
 		if_true: files('spapr_xive_kvm.c'))
 specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
+specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
-- 
2.29.2

[PULL 4/5] m68k: add a system controller

[Laurent Vivier]

Add a system controller for the m68k-virt machine.
This controller allows the kernel to power off or reset the machine.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210312214145.2936082-5-laurent@vivier.eu>
---
 docs/specs/virt-ctlr.txt    |  26 +++++++
 include/hw/misc/virt_ctrl.h |  22 ++++++
 hw/misc/virt_ctrl.c         | 151 ++++++++++++++++++++++++++++++++++++
 hw/misc/Kconfig             |   3 +
 hw/misc/meson.build         |   3 +
 hw/misc/trace-events        |   7 ++
 6 files changed, 212 insertions(+)
 create mode 100644 docs/specs/virt-ctlr.txt
 create mode 100644 include/hw/misc/virt_ctrl.h
 create mode 100644 hw/misc/virt_ctrl.c

diff --git a/docs/specs/virt-ctlr.txt b/docs/specs/virt-ctlr.txt
new file mode 100644
index 000000000000..24d38084f7fd
--- /dev/null
+++ b/docs/specs/virt-ctlr.txt
@@ -0,0 +1,26 @@
+Virtual System Controller
+=========================
+
+This device is a simple interface defined for the pure virtual machine with no
+hardware reference implementation to allow the guest kernel to send command
+to the host hypervisor.
+
+The specification can evolve, the current state is defined as below.
+
+This is a MMIO mapped device using 256 bytes.
+
+Two 32bit registers are defined:
+
+1- the features register (read-only, address 0x00)
+
+   This register allows the device to report features supported by the
+   controller.
+   The only feature supported for the moment is power control (0x01).
+
+2- the command register (write-only, address 0x04)
+
+   This register allows the kernel to send the commands to the hypervisor.
+   The implemented commands are part of the power control feature and
+   are reset (1), halt (2) and panic (3).
+   A basic command, no-op (0), is always present and can be used to test the
+   register access. This command has no effect.
diff --git a/include/hw/misc/virt_ctrl.h b/include/hw/misc/virt_ctrl.h
new file mode 100644
index 000000000000..edfadc469505
--- /dev/null
+++ b/include/hw/misc/virt_ctrl.h
@@ -0,0 +1,22 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Virt system Controller
+ */
+
+#ifndef VIRT_CTRL_H
+#define VIRT_CTRL_H
+
+#define TYPE_VIRT_CTRL "virt-ctrl"
+OBJECT_DECLARE_SIMPLE_TYPE(VirtCtrlState, VIRT_CTRL)
+
+struct VirtCtrlState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t irq_enabled;
+};
+
+#endif
diff --git a/hw/misc/virt_ctrl.c b/hw/misc/virt_ctrl.c
new file mode 100644
index 000000000000..2ea01bd7a1f0
--- /dev/null
+++ b/hw/misc/virt_ctrl.c
@@ -0,0 +1,151 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * Virt system Controller
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "trace.h"
+#include "sysemu/runstate.h"
+#include "hw/misc/virt_ctrl.h"
+
+enum {
+    REG_FEATURES = 0x00,
+    REG_CMD      = 0x04,
+};
+
+#define FEAT_POWER_CTRL 0x00000001
+
+enum {
+    CMD_NOOP,
+    CMD_RESET,
+    CMD_HALT,
+    CMD_PANIC,
+};
+
+static uint64_t virt_ctrl_read(void *opaque, hwaddr addr, unsigned size)
+{
+    VirtCtrlState *s = opaque;
+    uint64_t value = 0;
+
+    switch (addr) {
+    case REG_FEATURES:
+        value = FEAT_POWER_CTRL;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register read 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+
+    trace_virt_ctrl_write(s, addr, size, value);
+
+    return value;
+}
+
+static void virt_ctrl_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{
+    VirtCtrlState *s = opaque;
+
+    trace_virt_ctrl_write(s, addr, size, value);
+
+    switch (addr) {
+    case REG_CMD:
+        switch (value) {
+        case CMD_NOOP:
+            break;
+        case CMD_RESET:
+            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            break;
+        case CMD_HALT:
+            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+            break;
+        case CMD_PANIC:
+            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_PANIC);
+            break;
+        }
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: unimplemented register write 0x%02"HWADDR_PRIx"\n",
+                      __func__, addr);
+        break;
+    }
+}
+
+static const MemoryRegionOps virt_ctrl_ops = {
+    .read = virt_ctrl_read,
+    .write = virt_ctrl_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.max_access_size = 4,
+    .impl.max_access_size = 4,
+};
+
+static void virt_ctrl_reset(DeviceState *dev)
+{
+    VirtCtrlState *s = VIRT_CTRL(dev);
+
+    trace_virt_ctrl_reset(s);
+}
+
+static void virt_ctrl_realize(DeviceState *dev, Error **errp)
+{
+    VirtCtrlState *s = VIRT_CTRL(dev);
+
+    trace_virt_ctrl_instance_init(s);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &virt_ctrl_ops, s,
+                          "virt-ctrl", 0x100);
+}
+
+static const VMStateDescription vmstate_virt_ctrl = {
+    .name = "virt-ctrl",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(irq_enabled, VirtCtrlState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void virt_ctrl_instance_init(Object *obj)
+{
+    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+    VirtCtrlState *s = VIRT_CTRL(obj);
+
+    trace_virt_ctrl_instance_init(s);
+
+    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_irq(dev, &s->irq);
+}
+
+static void virt_ctrl_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->reset = virt_ctrl_reset;
+    dc->realize = virt_ctrl_realize;
+    dc->vmsd = &vmstate_virt_ctrl;
+}
+
+static const TypeInfo virt_ctrl_info = {
+    .name = TYPE_VIRT_CTRL,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .class_init = virt_ctrl_class_init,
+    .instance_init = virt_ctrl_instance_init,
+    .instance_size = sizeof(VirtCtrlState),
+};
+
+static void virt_ctrl_register_types(void)
+{
+    type_register_static(&virt_ctrl_info);
+}
+
+type_init(virt_ctrl_register_types)
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 5426b9b1a1ad..c71ed2582046 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -183,4 +183,7 @@ config SIFIVE_U_OTP
 config SIFIVE_U_PRCI
     bool
 
+config VIRT_CTRL
+    bool
+
 source macio/Kconfig
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 7a2b0d031a78..21034dc60a81 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -24,6 +24,9 @@ softmmu_ss.add(when: 'CONFIG_ARM11SCU', if_true: files('arm11scu.c'))
 # Mac devices
 softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
 
+# virt devices
+softmmu_ss.add(when: 'CONFIG_VIRT_CTRL', if_true: files('virt_ctrl.c'))
+
 # RISC-V devices
 softmmu_ss.add(when: 'CONFIG_MCHP_PFSOC_DMC', if_true: files('mchp_pfsoc_dmc.c'))
 softmmu_ss.add(when: 'CONFIG_MCHP_PFSOC_IOSCB', if_true: files('mchp_pfsoc_ioscb.c'))
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index b87d0b4c906a..3d44978cca36 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -255,3 +255,10 @@ pca955x_gpio_change(const char *description, unsigned id, unsigned prev_state, u
 bcm2835_cprman_read(uint64_t offset, uint64_t value) "offset:0x%" PRIx64 " value:0x%" PRIx64
 bcm2835_cprman_write(uint64_t offset, uint64_t value) "offset:0x%" PRIx64 " value:0x%" PRIx64
 bcm2835_cprman_write_invalid_magic(uint64_t offset, uint64_t value) "offset:0x%" PRIx64 " value:0x%" PRIx64
+
+# virt_ctrl.c
+virt_ctrl_read(void *dev, unsigned int addr, unsigned int size, uint64_t value) "ctrl: %p reg: 0x%02x size: %d value: 0x%"PRIx64
+virt_ctrl_write(void *dev, unsigned int addr, unsigned int size, uint64_t value) "ctrl: %p reg: 0x%02x size: %d value: 0x%"PRIx64
+virt_ctrl_reset(void *dev) "ctrl: %p"
+virt_ctrl_realize(void *dev) "ctrl: %p"
+virt_ctrl_instance_init(void *dev) "ctrl: %p"
-- 
2.29.2

[PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

The machine is based on Goldfish interfaces defined by Google
for Android simulator. It uses Goldfish-rtc (timer and RTC),
Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).

The machine is created with 128 virtio-mmio bus, and they can
be used to use serial console, GPU, disk, NIC, HID, ...

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
---
 default-configs/devices/m68k-softmmu.mak      |   1 +
 .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
 hw/m68k/virt.c                                | 313 ++++++++++++++++++
 MAINTAINERS                                   |  13 +
 hw/m68k/Kconfig                               |   9 +
 hw/m68k/meson.build                           |   1 +
 6 files changed, 355 insertions(+)
 create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
 create mode 100644 hw/m68k/virt.c

diff --git a/default-configs/devices/m68k-softmmu.mak b/default-configs/devices/m68k-softmmu.mak
index 6629fd2aa330..7f8619e42786 100644
--- a/default-configs/devices/m68k-softmmu.mak
+++ b/default-configs/devices/m68k-softmmu.mak
@@ -8,3 +8,4 @@ CONFIG_AN5206=y
 CONFIG_MCF5208=y
 CONFIG_NEXTCUBE=y
 CONFIG_Q800=y
+CONFIG_M68K_VIRT=y
diff --git a/include/standard-headers/asm-m68k/bootinfo-virt.h b/include/standard-headers/asm-m68k/bootinfo-virt.h
new file mode 100644
index 000000000000..81be1e092497
--- /dev/null
+++ b/include/standard-headers/asm-m68k/bootinfo-virt.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+** asm/bootinfo-virt.h -- Virtual-m68k-specific boot information definitions
+*/
+
+#ifndef _UAPI_ASM_M68K_BOOTINFO_VIRT_H
+#define _UAPI_ASM_M68K_BOOTINFO_VIRT_H
+
+#define BI_VIRT_QEMU_VERSION	0x8000
+#define BI_VIRT_GF_PIC_BASE	0x8001
+#define BI_VIRT_GF_RTC_BASE	0x8002
+#define BI_VIRT_GF_TTY_BASE	0x8003
+#define BI_VIRT_VIRTIO_BASE	0x8004
+#define BI_VIRT_CTRL_BASE	0x8005
+
+#define VIRT_BOOTI_VERSION	MK_BI_VERSION(2, 0)
+
+#endif /* _UAPI_ASM_M68K_BOOTINFO_MAC_H */
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
new file mode 100644
index 000000000000..e9a5d4c69b97
--- /dev/null
+++ b/hw/m68k/virt.c
@@ -0,0 +1,313 @@
+/*
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ *
+ * QEMU Vitual M68K Machine
+ *
+ * (c) 2020 Laurent Vivier <laurent@vivier.eu>
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu-common.h"
+#include "sysemu/sysemu.h"
+#include "cpu.h"
+#include "hw/hw.h"
+#include "hw/boards.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "elf.h"
+#include "hw/loader.h"
+#include "ui/console.h"
+#include "exec/address-spaces.h"
+#include "hw/sysbus.h"
+#include "standard-headers/asm-m68k/bootinfo.h"
+#include "standard-headers/asm-m68k/bootinfo-virt.h"
+#include "bootinfo.h"
+#include "net/net.h"
+#include "qapi/error.h"
+#include "sysemu/qtest.h"
+#include "sysemu/runstate.h"
+#include "sysemu/reset.h"
+
+#include "hw/intc/m68k_irqc.h"
+#include "hw/misc/virt_ctrl.h"
+#include "hw/char/goldfish_tty.h"
+#include "hw/rtc/goldfish_rtc.h"
+#include "hw/intc/goldfish_pic.h"
+#include "hw/virtio/virtio-mmio.h"
+#include "hw/virtio/virtio-blk.h"
+
+/*
+ * 6 goldfish-pic for CPU IRQ #1 to IRQ #6
+ * CPU IRQ #1 -> PIC #1
+ *               IRQ #1 to IRQ #31 -> unused
+ *               IRQ #32 -> goldfish-tty
+ * CPU IRQ #2 -> PIC #2
+ *               IRQ #1 to IRQ #32 -> virtio-mmio from 1 to 32
+ * CPU IRQ #3 -> PIC #3
+ *               IRQ #1 to IRQ #32 -> virtio-mmio from 33 to 64
+ * CPU IRQ #4 -> PIC #4
+ *               IRQ #1 to IRQ #32 -> virtio-mmio from 65 to 96
+ * CPU IRQ #5 -> PIC #5
+ *               IRQ #1 to IRQ #32 -> virtio-mmio from 97 to 128
+ * CPU IRQ #6 -> PIC #6
+ *               IRQ #1 -> goldfish-rtc
+ *               IRQ #2 to IRQ #32 -> unused
+ * CPU IRQ #7 -> NMI
+ */
+
+#define PIC_IRQ_BASE(num)     (8 + (num - 1) * 32)
+#define PIC_IRQ(num, irq)     (PIC_IRQ_BASE(num) + irq - 1)
+#define PIC_GPIO(pic_irq)     (qdev_get_gpio_in(pic_dev[(pic_irq - 8) / 32], \
+                                                (pic_irq - 8) % 32))
+
+#define VIRT_GF_PIC_MMIO_BASE 0xff000000     /* MMIO: 0xff000000 - 0xff005fff */
+#define VIRT_GF_PIC_IRQ_BASE  1              /* IRQ: #1 -> #6 */
+#define VIRT_GF_PIC_NB        6
+
+/* 2 goldfish-rtc (and timer) */
+#define VIRT_GF_RTC_MMIO_BASE 0xff006000     /* MMIO: 0xff006000 - 0xff007fff */
+#define VIRT_GF_RTC_IRQ_BASE  PIC_IRQ(6, 1)  /* PIC: #6, IRQ: #1 */
+#define VIRT_GF_RTC_NB        2
+
+/* 1 goldfish-tty */
+#define VIRT_GF_TTY_MMIO_BASE 0xff008000     /* MMIO: 0xff008000 - 0xff008fff */
+#define VIRT_GF_TTY_IRQ_BASE  PIC_IRQ(1, 32) /* PIC: #1, IRQ: #32 */
+
+/* 1 virt-ctrl */
+#define VIRT_CTRL_MMIO_BASE 0xff009000    /* MMIO: 0xff009000 - 0xff009fff */
+#define VIRT_CTRL_IRQ_BASE  PIC_IRQ(1, 1) /* PIC: #1, IRQ: #1 */
+
+/*
+ * virtio-mmio size is 0x200 bytes
+ * we use 4 goldfish-pic to attach them,
+ * we can attach 32 virtio devices / goldfish-pic
+ * -> we can manage 32 * 4 = 128 virtio devices
+ */
+#define VIRT_VIRTIO_MMIO_BASE 0xff010000     /* MMIO: 0xff010000 - 0xff01ffff */
+#define VIRT_VIRTIO_IRQ_BASE  PIC_IRQ(2, 1)  /* PIC: 2, 3, 4, 5, IRQ: ALL */
+
+static void main_cpu_reset(void *opaque)
+{
+    M68kCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+
+    cpu_reset(cs);
+    cpu->env.aregs[7] = ldl_phys(cs->as, 0);
+    cpu->env.pc = ldl_phys(cs->as, 4);
+}
+
+static void virt_init(MachineState *machine)
+{
+    M68kCPU *cpu = NULL;
+    int32_t kernel_size;
+    uint64_t elf_entry;
+    ram_addr_t initrd_base;
+    int32_t initrd_size;
+    ram_addr_t ram_size = machine->ram_size;
+    const char *kernel_filename = machine->kernel_filename;
+    const char *initrd_filename = machine->initrd_filename;
+    const char *kernel_cmdline = machine->kernel_cmdline;
+    hwaddr parameters_base;
+    DeviceState *dev;
+    DeviceState *irqc_dev;
+    DeviceState *pic_dev[VIRT_GF_PIC_NB];
+    SysBusDevice *sysbus;
+    hwaddr io_base;
+    int i;
+
+    if (ram_size > 3399672 * KiB) {
+        /*
+         * The physical memory can be up to 4 GiB - 16 MiB, but linux
+         * kernel crashes after this limit (~ 3.2 GiB)
+         */
+        error_report("Too much memory for this machine: %" PRId64 " KiB, "
+                     "maximum 3399672 KiB", ram_size / KiB);
+        exit(1);
+    }
+
+    /* init CPUs */
+    cpu = M68K_CPU(cpu_create(machine->cpu_type));
+    qemu_register_reset(main_cpu_reset, cpu);
+
+    /* RAM */
+    memory_region_add_subregion(get_system_memory(), 0, machine->ram);
+
+    /* IRQ Controller */
+
+    irqc_dev = qdev_new(TYPE_M68K_IRQC);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(irqc_dev), &error_fatal);
+
+    /*
+     * 6 goldfish-pic
+     *
+     * map: 0xff000000 - 0xff006fff = 28 KiB
+     * IRQ: #1 (lower priority) -> #6 (higher priority)
+     *
+     */
+    io_base = VIRT_GF_PIC_MMIO_BASE;
+    for (i = 0; i < VIRT_GF_PIC_NB; i++) {
+        pic_dev[i] = qdev_new(TYPE_GOLDFISH_PIC);
+        sysbus = SYS_BUS_DEVICE(pic_dev[i]);
+        qdev_prop_set_uint8(pic_dev[i], "index", i);
+        sysbus_realize_and_unref(sysbus, &error_fatal);
+
+        sysbus_mmio_map(sysbus, 0, io_base);
+        sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(irqc_dev, i));
+
+        io_base += 0x1000;
+    }
+
+    /* goldfish-rtc */
+    io_base = VIRT_GF_RTC_MMIO_BASE;
+    for (i = 0; i < VIRT_GF_RTC_NB; i++) {
+        dev = qdev_new(TYPE_GOLDFISH_RTC);
+        sysbus = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(sysbus, &error_fatal);
+        sysbus_mmio_map(sysbus, 0, io_base);
+        sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_GF_RTC_IRQ_BASE + i));
+
+        io_base += 0x1000;
+    }
+
+    /* goldfish-tty */
+    dev = qdev_new(TYPE_GOLDFISH_TTY);
+    sysbus = SYS_BUS_DEVICE(dev);
+    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    sysbus_realize_and_unref(sysbus, &error_fatal);
+    sysbus_mmio_map(sysbus, 0, VIRT_GF_TTY_MMIO_BASE);
+    sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_GF_TTY_IRQ_BASE));
+
+    /* virt controller */
+    dev = qdev_new(TYPE_VIRT_CTRL);
+    sysbus = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sysbus, &error_fatal);
+    sysbus_mmio_map(sysbus, 0, VIRT_CTRL_MMIO_BASE);
+    sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_CTRL_IRQ_BASE));
+
+    /* virtio-mmio */
+    io_base = VIRT_VIRTIO_MMIO_BASE;
+    for (i = 0; i < 128; i++) {
+        dev = qdev_new(TYPE_VIRTIO_MMIO);
+        qdev_prop_set_bit(dev, "force-legacy", false);
+        sysbus = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(sysbus, &error_fatal);
+        sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_VIRTIO_IRQ_BASE + i));
+        sysbus_mmio_map(sysbus, 0, io_base);
+        io_base += 0x200;
+    }
+
+    if (kernel_filename) {
+        CPUState *cs = CPU(cpu);
+        uint64_t high;
+
+        kernel_size = load_elf(kernel_filename, NULL, NULL, NULL,
+                               &elf_entry, NULL, &high, NULL, 1,
+                               EM_68K, 0, 0);
+        if (kernel_size < 0) {
+            error_report("could not load kernel '%s'", kernel_filename);
+            exit(1);
+        }
+        stl_phys(cs->as, 4, elf_entry); /* reset initial PC */
+        parameters_base = (high + 1) & ~1;
+
+        BOOTINFO1(cs->as, parameters_base, BI_MACHTYPE, MACH_VIRT);
+        BOOTINFO1(cs->as, parameters_base, BI_FPUTYPE, FPU_68040);
+        BOOTINFO1(cs->as, parameters_base, BI_MMUTYPE, MMU_68040);
+        BOOTINFO1(cs->as, parameters_base, BI_CPUTYPE, CPU_68040);
+        BOOTINFO2(cs->as, parameters_base, BI_MEMCHUNK, 0, ram_size);
+
+        BOOTINFO1(cs->as, parameters_base, BI_VIRT_QEMU_VERSION,
+                  ((QEMU_VERSION_MAJOR << 24) | (QEMU_VERSION_MINOR << 16) |
+                   (QEMU_VERSION_MICRO << 8)));
+        BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_PIC_BASE,
+                  VIRT_GF_PIC_MMIO_BASE, VIRT_GF_PIC_IRQ_BASE);
+        BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_RTC_BASE,
+                  VIRT_GF_RTC_MMIO_BASE, VIRT_GF_RTC_IRQ_BASE);
+        BOOTINFO2(cs->as, parameters_base, BI_VIRT_GF_TTY_BASE,
+                  VIRT_GF_TTY_MMIO_BASE, VIRT_GF_TTY_IRQ_BASE);
+        BOOTINFO2(cs->as, parameters_base, BI_VIRT_CTRL_BASE,
+                  VIRT_CTRL_MMIO_BASE, VIRT_CTRL_IRQ_BASE);
+        BOOTINFO2(cs->as, parameters_base, BI_VIRT_VIRTIO_BASE,
+                  VIRT_VIRTIO_MMIO_BASE, VIRT_VIRTIO_IRQ_BASE);
+
+        if (kernel_cmdline) {
+            BOOTINFOSTR(cs->as, parameters_base, BI_COMMAND_LINE,
+                        kernel_cmdline);
+        }
+
+        /* load initrd */
+        if (initrd_filename) {
+            initrd_size = get_image_size(initrd_filename);
+            if (initrd_size < 0) {
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
+                exit(1);
+            }
+
+            initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK;
+            load_image_targphys(initrd_filename, initrd_base,
+                                ram_size - initrd_base);
+            BOOTINFO2(cs->as, parameters_base, BI_RAMDISK, initrd_base,
+                      initrd_size);
+        } else {
+            initrd_base = 0;
+            initrd_size = 0;
+        }
+        BOOTINFO0(cs->as, parameters_base, BI_LAST);
+    }
+}
+
+static void virt_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    mc->desc = "QEMU M68K Virtual Machine";
+    mc->init = virt_init;
+    mc->default_cpu_type = M68K_CPU_TYPE_NAME("m68040");
+    mc->max_cpus = 1;
+    mc->no_floppy = 1;
+    mc->no_parallel = 1;
+    mc->default_ram_id = "m68k_virt.ram";
+}
+
+static const TypeInfo virt_machine_info = {
+    .name       = MACHINE_TYPE_NAME("virt"),
+    .parent     = TYPE_MACHINE,
+    .abstract   = true,
+    .class_init = virt_machine_class_init,
+};
+
+static void virt_machine_register_types(void)
+{
+    type_register_static(&virt_machine_info);
+}
+
+type_init(virt_machine_register_types)
+
+#define DEFINE_VIRT_MACHINE(major, minor, latest) \
+    static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
+                                                    void *data) \
+    { \
+        MachineClass *mc = MACHINE_CLASS(oc); \
+        virt_machine_##major##_##minor##_options(mc); \
+        mc->desc = "QEMU " # major "." # minor " M68K Virtual Machine"; \
+        if (latest) { \
+            mc->alias = "virt"; \
+        } \
+    } \
+    static const TypeInfo machvirt_##major##_##minor##_info = { \
+        .name = MACHINE_TYPE_NAME("virt-" # major "." # minor), \
+        .parent = MACHINE_TYPE_NAME("virt"), \
+        .class_init = virt_##major##_##minor##_class_init, \
+    }; \
+    static void machvirt_machine_##major##_##minor##_init(void) \
+    { \
+        type_register_static(&machvirt_##major##_##minor##_info); \
+    } \
+    type_init(machvirt_machine_##major##_##minor##_init);
+
+static void virt_machine_6_0_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE(6, 0, true)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5ca3c9f851f9..682e4ed48152 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1130,6 +1130,19 @@ F: include/hw/nubus/*
 F: include/hw/display/macfb.h
 F: include/hw/block/swim.h
 
+virt
+M: Laurent Vivier <laurent@vivier.eu>
+S: Maintained
+F: hw/m68k/virt.c
+F: hw/char/goldfish_tty.c
+F: hw/intc/goldfish_pic.c
+F: hw/intc/m68k_irqc.c
+F: hw/misc/virt_ctrl.c
+F: include/hw/char/goldfish_tty.h
+F: include/hw/intc/goldfish_pic.h
+F: include/hw/intc/m68k_irqc.h
+F: include/hw/misc/virt_ctrl.h
+
 MicroBlaze Machines
 -------------------
 petalogix_s3adsp1800
diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
index 60d7bcfb8f2b..f839f8a03064 100644
--- a/hw/m68k/Kconfig
+++ b/hw/m68k/Kconfig
@@ -23,3 +23,12 @@ config Q800
     select ESP
     select DP8393X
     select OR_IRQ
+
+config M68K_VIRT
+    bool
+    select M68K_IRQC
+    select VIRT_CTRL
+    select GOLDFISH_PIC
+    select GOLDFISH_TTY
+    select GOLDFISH_RTC
+    select VIRTIO_MMIO
diff --git a/hw/m68k/meson.build b/hw/m68k/meson.build
index ca0044c652d3..31248641d301 100644
--- a/hw/m68k/meson.build
+++ b/hw/m68k/meson.build
@@ -3,5 +3,6 @@ m68k_ss.add(when: 'CONFIG_AN5206', if_true: files('an5206.c', 'mcf5206.c'))
 m68k_ss.add(when: 'CONFIG_MCF5208', if_true: files('mcf5208.c', 'mcf_intc.c'))
 m68k_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-kbd.c', 'next-cube.c'))
 m68k_ss.add(when: 'CONFIG_Q800', if_true: files('q800.c'))
+m68k_ss.add(when: 'CONFIG_M68K_VIRT', if_true: files('virt.c'))
 
 hw_arch += {'m68k': m68k_ss}
-- 
2.29.2

Re: [PULL 0/5] M68k for 6.0 patches

[Peter Maydell]

On Mon, 15 Mar 2021 at 20:45, Laurent Vivier <laurent@vivier.eu> wrote:
>
> The following changes since commit e7c6a8cf9f5c82aa152273e1c9e80d07b1b0c32c:
>
>   Merge remote-tracking branch 'remotes/philmd/tags/avr-20210315' into stagin=
> g (2021-03-15 16:59:55 +0000)
>
> are available in the Git repository at:
>
>   git://github.com/vivier/qemu-m68k.git tags/m68k-for-6.0-pull-request
>
> for you to fetch changes up to e1cecdca559d552bc5ab282696301858a97c3e8c:
>
>   m68k: add Virtual M68k Machine (2021-03-15 21:03:06 +0100)
>
> ----------------------------------------------------------------
> m68k pull request 20210315
>
> Add m68k virt machine
>
> ----------------------------------------------------------------
>


Applied, thanks.

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

-- PMM

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Philippe Mathieu-Daudé]

Hi Laurent,

+Paolo / Thomas

On 3/15/21 9:42 PM, Laurent Vivier wrote:
> The machine is based on Goldfish interfaces defined by Google
> for Android simulator. It uses Goldfish-rtc (timer and RTC),
> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
> 
> The machine is created with 128 virtio-mmio bus, and they can
> be used to use serial console, GPU, disk, NIC, HID, ...
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
> ---
>  default-configs/devices/m68k-softmmu.mak      |   1 +
>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>  MAINTAINERS                                   |  13 +
>  hw/m68k/Kconfig                               |   9 +
>  hw/m68k/meson.build                           |   1 +
>  6 files changed, 355 insertions(+)
>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>  create mode 100644 hw/m68k/virt.c

> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
> index 60d7bcfb8f2b..f839f8a03064 100644
> --- a/hw/m68k/Kconfig
> +++ b/hw/m68k/Kconfig
> @@ -23,3 +23,12 @@ config Q800
>      select ESP
>      select DP8393X
>      select OR_IRQ
> +
> +config M68K_VIRT
> +    bool
> +    select M68K_IRQC
> +    select VIRT_CTRL
> +    select GOLDFISH_PIC
> +    select GOLDFISH_TTY
> +    select GOLDFISH_RTC
> +    select VIRTIO_MMIO

I had this error on gitlab:

(qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
'virtio-blk-pci' is not a valid device model name
job: check-system-fedora
https://gitlab.com/philmd/qemu/-/jobs/1106469724

I bisected locally to this commit.

check-system-fedora uses build-system-fedora:

build-system-fedora:
    CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
             --enable-fdt=system --enable-slirp=system
             --enable-capstone=system

I'm confused because the machine provides a VIRTIO bus
via MMIO:

config VIRTIO_MMIO
    bool
    select VIRTIO

I remember I tested your machine with virtio-blk-device.

config VIRTIO_BLK
    bool
    default y
    depends on VIRTIO

Ah, this is virtio-blk-pci, which has:

virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
files('virtio-blk-pci.c'))
virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)

And VIRTIO_PCI isn't selected...

Are the tests incorrect then?

libqos isn't restricted to PCI:

tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
"virtio-blk")) {
tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
in virtio-blk-device\n", interface);
tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
tests/qtest/libqos/virtio-blk.c:111:
qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
tests/qtest/libqos/virtio-blk.c:112:
qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
tests/qtest/libqos/virtio-blk.c:113:
qos_node_produces("virtio-blk-device", "virtio-blk");

But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
use a generic virtio-blk-device instead, hoping it get plugged correctly
to the virtio bus...

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,
> 
> +Paolo / Thomas
> 
> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>> The machine is based on Goldfish interfaces defined by Google
>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>
>> The machine is created with 128 virtio-mmio bus, and they can
>> be used to use serial console, GPU, disk, NIC, HID, ...
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>> ---
>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>  MAINTAINERS                                   |  13 +
>>  hw/m68k/Kconfig                               |   9 +
>>  hw/m68k/meson.build                           |   1 +
>>  6 files changed, 355 insertions(+)
>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>  create mode 100644 hw/m68k/virt.c
> 
>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>> index 60d7bcfb8f2b..f839f8a03064 100644
>> --- a/hw/m68k/Kconfig
>> +++ b/hw/m68k/Kconfig
>> @@ -23,3 +23,12 @@ config Q800
>>      select ESP
>>      select DP8393X
>>      select OR_IRQ
>> +
>> +config M68K_VIRT
>> +    bool
>> +    select M68K_IRQC
>> +    select VIRT_CTRL
>> +    select GOLDFISH_PIC
>> +    select GOLDFISH_TTY
>> +    select GOLDFISH_RTC
>> +    select VIRTIO_MMIO
> 
> I had this error on gitlab:
> 
> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
> 'virtio-blk-pci' is not a valid device model name
> job: check-system-fedora
> https://gitlab.com/philmd/qemu/-/jobs/1106469724
> 
> I bisected locally to this commit.
> 
> check-system-fedora uses build-system-fedora:
> 
> build-system-fedora:
>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>              --enable-fdt=system --enable-slirp=system
>              --enable-capstone=system
> 
> I'm confused because the machine provides a VIRTIO bus
> via MMIO:
> 
> config VIRTIO_MMIO
>     bool
>     select VIRTIO
> 
> I remember I tested your machine with virtio-blk-device.
> 
> config VIRTIO_BLK
>     bool
>     default y
>     depends on VIRTIO
> 
> Ah, this is virtio-blk-pci, which has:
> 
> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
> files('virtio-blk-pci.c'))
> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
> 
> And VIRTIO_PCI isn't selected...

This machine doesn't have virtio-pci, but only virtio-mmio buses.

> Are the tests incorrect then?
> 
> libqos isn't restricted to PCI:
> 
> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
> "virtio-blk")) {
> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
> in virtio-blk-device\n", interface);
> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
> tests/qtest/libqos/virtio-blk.c:111:
> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
> tests/qtest/libqos/virtio-blk.c:112:
> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
> tests/qtest/libqos/virtio-blk.c:113:
> qos_node_produces("virtio-blk-device", "virtio-blk");
> 
> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
> use a generic virtio-blk-device instead, hoping it get plugged correctly
> to the virtio bus...

Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
in the first free ones.

I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.

Why is it executed for now?

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Philippe Mathieu-Daudé]

On 3/18/21 10:52 AM, Laurent Vivier wrote:
> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>> Hi Laurent,
>>
>> +Paolo / Thomas
>>
>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>> The machine is based on Goldfish interfaces defined by Google
>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>
>>> The machine is created with 128 virtio-mmio bus, and they can
>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>> ---
>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>  MAINTAINERS                                   |  13 +
>>>  hw/m68k/Kconfig                               |   9 +
>>>  hw/m68k/meson.build                           |   1 +
>>>  6 files changed, 355 insertions(+)
>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>  create mode 100644 hw/m68k/virt.c
>>
>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>> --- a/hw/m68k/Kconfig
>>> +++ b/hw/m68k/Kconfig
>>> @@ -23,3 +23,12 @@ config Q800
>>>      select ESP
>>>      select DP8393X
>>>      select OR_IRQ
>>> +
>>> +config M68K_VIRT
>>> +    bool
>>> +    select M68K_IRQC
>>> +    select VIRT_CTRL
>>> +    select GOLDFISH_PIC
>>> +    select GOLDFISH_TTY
>>> +    select GOLDFISH_RTC
>>> +    select VIRTIO_MMIO
>>
>> I had this error on gitlab:
>>
>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>> 'virtio-blk-pci' is not a valid device model name
>> job: check-system-fedora
>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>
>> I bisected locally to this commit.
>>
>> check-system-fedora uses build-system-fedora:
>>
>> build-system-fedora:
>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>              --enable-fdt=system --enable-slirp=system
>>              --enable-capstone=system
>>
>> I'm confused because the machine provides a VIRTIO bus
>> via MMIO:
>>
>> config VIRTIO_MMIO
>>     bool
>>     select VIRTIO
>>
>> I remember I tested your machine with virtio-blk-device.
>>
>> config VIRTIO_BLK
>>     bool
>>     default y
>>     depends on VIRTIO
>>
>> Ah, this is virtio-blk-pci, which has:
>>
>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>> files('virtio-blk-pci.c'))
>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>
>> And VIRTIO_PCI isn't selected...
> 
> This machine doesn't have virtio-pci, but only virtio-mmio buses.

Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
for this machine". So the problem isn't the m68k-virt machine addition,
but it shows another problem elsewhere.

>> Are the tests incorrect then?
>>
>> libqos isn't restricted to PCI:
>>
>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>> "virtio-blk")) {
>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>> in virtio-blk-device\n", interface);
>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>> tests/qtest/libqos/virtio-blk.c:111:
>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>> tests/qtest/libqos/virtio-blk.c:112:
>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>> tests/qtest/libqos/virtio-blk.c:113:
>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>
>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>> to the virtio bus...
> 
> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
> in the first free ones.
> 
> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
> 
> Why is it executed for now?

This is probably the problem root cause.

Possible fix:

-->8 --
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 66ee9fbf450..d7f3fad51c1 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -217,13 +217,17 @@
   'emc141x-test.c',
   'usb-hcd-ohci-test.c',
   'virtio-test.c',
-  'virtio-blk-test.c',
-  'virtio-net-test.c',
-  'virtio-rng-test.c',
-  'virtio-scsi-test.c',
   'virtio-serial-test.c',
   'vmxnet3-test.c',
 )
+if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
+  qos_test_ss.add(
+    'virtio-blk-test.c',
+    'virtio-net-test.c',
+    'virtio-rng-test.c',
+    'virtio-scsi-test.c',
+  )
+endif
 if have_virtfs
   qos_test_ss.add(files('virtio-9p-test.c'))
 endif
---

I'll test that locally but not on Gitlab.

Thanks,

Phil.

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>> Hi Laurent,
>>>
>>> +Paolo / Thomas
>>>
>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>> The machine is based on Goldfish interfaces defined by Google
>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>
>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>> ---
>>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>  MAINTAINERS                                   |  13 +
>>>>  hw/m68k/Kconfig                               |   9 +
>>>>  hw/m68k/meson.build                           |   1 +
>>>>  6 files changed, 355 insertions(+)
>>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>  create mode 100644 hw/m68k/virt.c
>>>
>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>> --- a/hw/m68k/Kconfig
>>>> +++ b/hw/m68k/Kconfig
>>>> @@ -23,3 +23,12 @@ config Q800
>>>>      select ESP
>>>>      select DP8393X
>>>>      select OR_IRQ
>>>> +
>>>> +config M68K_VIRT
>>>> +    bool
>>>> +    select M68K_IRQC
>>>> +    select VIRT_CTRL
>>>> +    select GOLDFISH_PIC
>>>> +    select GOLDFISH_TTY
>>>> +    select GOLDFISH_RTC
>>>> +    select VIRTIO_MMIO
>>>
>>> I had this error on gitlab:
>>>
>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>> 'virtio-blk-pci' is not a valid device model name
>>> job: check-system-fedora
>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>
>>> I bisected locally to this commit.
>>>
>>> check-system-fedora uses build-system-fedora:
>>>
>>> build-system-fedora:
>>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>              --enable-fdt=system --enable-slirp=system
>>>              --enable-capstone=system
>>>
>>> I'm confused because the machine provides a VIRTIO bus
>>> via MMIO:
>>>
>>> config VIRTIO_MMIO
>>>     bool
>>>     select VIRTIO
>>>
>>> I remember I tested your machine with virtio-blk-device.
>>>
>>> config VIRTIO_BLK
>>>     bool
>>>     default y
>>>     depends on VIRTIO
>>>
>>> Ah, this is virtio-blk-pci, which has:
>>>
>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>> files('virtio-blk-pci.c'))
>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>
>>> And VIRTIO_PCI isn't selected...
>>
>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
> 
> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
> for this machine". So the problem isn't the m68k-virt machine addition,
> but it shows another problem elsewhere.
> 
>>> Are the tests incorrect then?
>>>
>>> libqos isn't restricted to PCI:
>>>
>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>> "virtio-blk")) {
>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>> in virtio-blk-device\n", interface);
>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>> tests/qtest/libqos/virtio-blk.c:111:
>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>> tests/qtest/libqos/virtio-blk.c:112:
>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>> tests/qtest/libqos/virtio-blk.c:113:
>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>
>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>> to the virtio bus...
>>
>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>> in the first free ones.
>>
>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>
>> Why is it executed for now?
> 
> This is probably the problem root cause.
> 
> Possible fix:
> 
> -->8 --
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 66ee9fbf450..d7f3fad51c1 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -217,13 +217,17 @@
>    'emc141x-test.c',
>    'usb-hcd-ohci-test.c',
>    'virtio-test.c',
> -  'virtio-blk-test.c',
> -  'virtio-net-test.c',
> -  'virtio-rng-test.c',
> -  'virtio-scsi-test.c',
>    'virtio-serial-test.c',
>    'vmxnet3-test.c',
>  )
> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
> +  qos_test_ss.add(
> +    'virtio-blk-test.c',
> +    'virtio-net-test.c',
> +    'virtio-rng-test.c',
> +    'virtio-scsi-test.c',
> +  )
> +endif
>  if have_virtfs
>    qos_test_ss.add(files('virtio-9p-test.c'))
>  endif
> ---
> 
> I'll test that locally but not on Gitlab.
> 

This also removes the virtio-devices test, I think we should keep the files, but in the files to
disable the PCI part when it is not available.

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Paolo Bonzini]

On 18/03/21 11:06, Laurent Vivier wrote:
> This also removes the virtio-devices test, I think we should keep the
> files, but in the files to disable the PCI part when it is not
> available.

I think we should just shuffle the targets in the gitlab YAML to bypass 
the issue.

Paolo

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Peter Maydell]

On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 18/03/21 11:06, Laurent Vivier wrote:
> > This also removes the virtio-devices test, I think we should keep the
> > files, but in the files to disable the PCI part when it is not
> > available.
>
> I think we should just shuffle the targets in the gitlab YAML to bypass
> the issue.

Then we'll hit it again later. I'm pretty sure this isn't the
first time we've run into "some test makes dubious assumptions"...

-- PMM

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Paolo Bonzini]

On 18/03/21 11:40, Peter Maydell wrote:
> On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 18/03/21 11:06, Laurent Vivier wrote:
>>> This also removes the virtio-devices test, I think we should keep the
>>> files, but in the files to disable the PCI part when it is not
>>> available.
>>
>> I think we should just shuffle the targets in the gitlab YAML to bypass
>> the issue.
> 
> Then we'll hit it again later. I'm pretty sure this isn't the
> first time we've run into "some test makes dubious assumptions"...

We can both fix qemu-iotests and CI configuration, but m68k is certainly 
not the culprit here.  And we are going to make more assumptions over 
time, not fewer, in order to keep the CI time at bay.

Paolo

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Peter Maydell]

On Thu, 18 Mar 2021 at 10:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 18/03/21 11:40, Peter Maydell wrote:
> > On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 18/03/21 11:06, Laurent Vivier wrote:
> >>> This also removes the virtio-devices test, I think we should keep the
> >>> files, but in the files to disable the PCI part when it is not
> >>> available.
> >>
> >> I think we should just shuffle the targets in the gitlab YAML to bypass
> >> the issue.
> >
> > Then we'll hit it again later. I'm pretty sure this isn't the
> > first time we've run into "some test makes dubious assumptions"...
>
> We can both fix qemu-iotests and CI configuration, but m68k is certainly
> not the culprit here.  And we are going to make more assumptions over
> time, not fewer, in order to keep the CI time at bay.

I don't see why CI time is relevant to whether the test says
"I require X,Y,Z, so don't run me on configs without those"
or whether it just randomly assumes X,Y,Z are always present
or that if it says "I require W" than W must imply X,Y,Z...

thanks
-- PMM

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Philippe Mathieu-Daudé]

On 3/18/21 12:10 PM, Peter Maydell wrote:
> On Thu, 18 Mar 2021 at 10:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 18/03/21 11:40, Peter Maydell wrote:
>>> On Thu, 18 Mar 2021 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>> On 18/03/21 11:06, Laurent Vivier wrote:
>>>>> This also removes the virtio-devices test, I think we should keep the
>>>>> files, but in the files to disable the PCI part when it is not
>>>>> available.
>>>>
>>>> I think we should just shuffle the targets in the gitlab YAML to bypass
>>>> the issue.
>>>
>>> Then we'll hit it again later. I'm pretty sure this isn't the
>>> first time we've run into "some test makes dubious assumptions"...
>>
>> We can both fix qemu-iotests and CI configuration, but m68k is certainly
>> not the culprit here.  And we are going to make more assumptions over
>> time, not fewer, in order to keep the CI time at bay.
> 
> I don't see why CI time is relevant to whether the test says
> "I require X,Y,Z, so don't run me on configs without those"
> or whether it just randomly assumes X,Y,Z are always present
> or that if it says "I require W" than W must imply X,Y,Z...

Recently we changed a bit our view and are trying to have smarter
tests. In particular building target/device agnostic tests, and
have the test queries the QEMU binary what features/devices are
available before running. This will take some time before we get
there, unlikely for the 6.0 release. For short term, Paolo's
"shuffle gitlab YAML" suggestion is simpler.

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Philippe Mathieu-Daudé]

On 3/18/21 11:06 AM, Laurent Vivier wrote:
> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
>> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>>> Hi Laurent,
>>>>
>>>> +Paolo / Thomas
>>>>
>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>>> The machine is based on Goldfish interfaces defined by Google
>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>>
>>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>>
>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>>> ---
>>>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>>  MAINTAINERS                                   |  13 +
>>>>>  hw/m68k/Kconfig                               |   9 +
>>>>>  hw/m68k/meson.build                           |   1 +
>>>>>  6 files changed, 355 insertions(+)
>>>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>>  create mode 100644 hw/m68k/virt.c
>>>>
>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>>> --- a/hw/m68k/Kconfig
>>>>> +++ b/hw/m68k/Kconfig
>>>>> @@ -23,3 +23,12 @@ config Q800
>>>>>      select ESP
>>>>>      select DP8393X
>>>>>      select OR_IRQ
>>>>> +
>>>>> +config M68K_VIRT
>>>>> +    bool
>>>>> +    select M68K_IRQC
>>>>> +    select VIRT_CTRL
>>>>> +    select GOLDFISH_PIC
>>>>> +    select GOLDFISH_TTY
>>>>> +    select GOLDFISH_RTC
>>>>> +    select VIRTIO_MMIO
>>>>
>>>> I had this error on gitlab:
>>>>
>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>>> 'virtio-blk-pci' is not a valid device model name
>>>> job: check-system-fedora
>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>>
>>>> I bisected locally to this commit.
>>>>
>>>> check-system-fedora uses build-system-fedora:
>>>>
>>>> build-system-fedora:
>>>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>>              --enable-fdt=system --enable-slirp=system
>>>>              --enable-capstone=system
>>>>
>>>> I'm confused because the machine provides a VIRTIO bus
>>>> via MMIO:
>>>>
>>>> config VIRTIO_MMIO
>>>>     bool
>>>>     select VIRTIO
>>>>
>>>> I remember I tested your machine with virtio-blk-device.
>>>>
>>>> config VIRTIO_BLK
>>>>     bool
>>>>     default y
>>>>     depends on VIRTIO
>>>>
>>>> Ah, this is virtio-blk-pci, which has:
>>>>
>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>>> files('virtio-blk-pci.c'))
>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>>
>>>> And VIRTIO_PCI isn't selected...
>>>
>>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
>>
>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
>> for this machine". So the problem isn't the m68k-virt machine addition,
>> but it shows another problem elsewhere.
>>
>>>> Are the tests incorrect then?
>>>>
>>>> libqos isn't restricted to PCI:
>>>>
>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>>> "virtio-blk")) {
>>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>>> in virtio-blk-device\n", interface);
>>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>>> tests/qtest/libqos/virtio-blk.c:111:
>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>>> tests/qtest/libqos/virtio-blk.c:112:
>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>>> tests/qtest/libqos/virtio-blk.c:113:
>>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>>
>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>>> to the virtio bus...
>>>
>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>>> in the first free ones.
>>>
>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>>
>>> Why is it executed for now?
>>
>> This is probably the problem root cause.
>>
>> Possible fix:
>>
>> -->8 --
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 66ee9fbf450..d7f3fad51c1 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -217,13 +217,17 @@
>>    'emc141x-test.c',
>>    'usb-hcd-ohci-test.c',
>>    'virtio-test.c',
>> -  'virtio-blk-test.c',
>> -  'virtio-net-test.c',
>> -  'virtio-rng-test.c',
>> -  'virtio-scsi-test.c',
>>    'virtio-serial-test.c',
>>    'vmxnet3-test.c',
>>  )
>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>> +  qos_test_ss.add(
>> +    'virtio-blk-test.c',
>> +    'virtio-net-test.c',
>> +    'virtio-rng-test.c',
>> +    'virtio-scsi-test.c',
>> +  )
>> +endif
>>  if have_virtfs
>>    qos_test_ss.add(files('virtio-9p-test.c'))
>>  endif
>> ---
>>
>> I'll test that locally but not on Gitlab.

This approach doesn't work for the iotests.

> This also removes the virtio-devices test, I think we should keep the files, but in the files to
> disable the PCI part when it is not available.
I don't understand how the virtio devices are created, it seems there
is an alias to generic virtio hw that map to the arch virtio bus.

I was not obvious to understand why start the virt machine with
"-device virtio-blk" returns "'virtio-blk-pci' is not a valid device
model name" at first, then I figured out the qdev_alias_table array.

Maybe you need to complete it for your arch? I've been using that:

-- >8 --
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 8dc656becca..b326bd76c2a 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = {
     { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
     { "virtio-balloon-pci", "virtio-balloon",
             QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K },
     { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
-    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
+                                      & ~(QEMU_ARCH_S390X |
QEMU_ARCH_M68K) },
     { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X },
     { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
     { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
@@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = {
     { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
     { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
     { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+    { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K },
     { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
-    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL &
~QEMU_ARCH_S390X },
+    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
+                                            & ~(QEMU_ARCH_S390X |
QEMU_ARCH_M68K)},
     { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
     { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL &
~QEMU_ARCH_S390X },
     { }
---

But this looks ugly, I don't think it should work that way (because
a machine could provide virtio buses over multiple transport, mmio
and pci...).

I'll ignore this problem and send my pull request with a red CI
as others seem to do.

Regards,

Phil.

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit :
> On 3/18/21 11:06 AM, Laurent Vivier wrote:
>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
>>> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>>>> Hi Laurent,
>>>>>
>>>>> +Paolo / Thomas
>>>>>
>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>>>> The machine is based on Goldfish interfaces defined by Google
>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>>>
>>>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>>>> ---
>>>>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>>>  MAINTAINERS                                   |  13 +
>>>>>>  hw/m68k/Kconfig                               |   9 +
>>>>>>  hw/m68k/meson.build                           |   1 +
>>>>>>  6 files changed, 355 insertions(+)
>>>>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>>>  create mode 100644 hw/m68k/virt.c
>>>>>
>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>>>> --- a/hw/m68k/Kconfig
>>>>>> +++ b/hw/m68k/Kconfig
>>>>>> @@ -23,3 +23,12 @@ config Q800
>>>>>>      select ESP
>>>>>>      select DP8393X
>>>>>>      select OR_IRQ
>>>>>> +
>>>>>> +config M68K_VIRT
>>>>>> +    bool
>>>>>> +    select M68K_IRQC
>>>>>> +    select VIRT_CTRL
>>>>>> +    select GOLDFISH_PIC
>>>>>> +    select GOLDFISH_TTY
>>>>>> +    select GOLDFISH_RTC
>>>>>> +    select VIRTIO_MMIO
>>>>>
>>>>> I had this error on gitlab:
>>>>>
>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>>>> 'virtio-blk-pci' is not a valid device model name
>>>>> job: check-system-fedora
>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>>>
>>>>> I bisected locally to this commit.
>>>>>
>>>>> check-system-fedora uses build-system-fedora:
>>>>>
>>>>> build-system-fedora:
>>>>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>>>              --enable-fdt=system --enable-slirp=system
>>>>>              --enable-capstone=system
>>>>>
>>>>> I'm confused because the machine provides a VIRTIO bus
>>>>> via MMIO:
>>>>>
>>>>> config VIRTIO_MMIO
>>>>>     bool
>>>>>     select VIRTIO
>>>>>
>>>>> I remember I tested your machine with virtio-blk-device.
>>>>>
>>>>> config VIRTIO_BLK
>>>>>     bool
>>>>>     default y
>>>>>     depends on VIRTIO
>>>>>
>>>>> Ah, this is virtio-blk-pci, which has:
>>>>>
>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>>>> files('virtio-blk-pci.c'))
>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>>>
>>>>> And VIRTIO_PCI isn't selected...
>>>>
>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
>>>
>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
>>> for this machine". So the problem isn't the m68k-virt machine addition,
>>> but it shows another problem elsewhere.
>>>
>>>>> Are the tests incorrect then?
>>>>>
>>>>> libqos isn't restricted to PCI:
>>>>>
>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>>>> "virtio-blk")) {
>>>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>>>> in virtio-blk-device\n", interface);
>>>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>>>> tests/qtest/libqos/virtio-blk.c:111:
>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>>>> tests/qtest/libqos/virtio-blk.c:112:
>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>>>> tests/qtest/libqos/virtio-blk.c:113:
>>>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>>>
>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>>>> to the virtio bus...
>>>>
>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>>>> in the first free ones.
>>>>
>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>>>
>>>> Why is it executed for now?
>>>
>>> This is probably the problem root cause.
>>>
>>> Possible fix:
>>>
>>> -->8 --
>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>> index 66ee9fbf450..d7f3fad51c1 100644
>>> --- a/tests/qtest/meson.build
>>> +++ b/tests/qtest/meson.build
>>> @@ -217,13 +217,17 @@
>>>    'emc141x-test.c',
>>>    'usb-hcd-ohci-test.c',
>>>    'virtio-test.c',
>>> -  'virtio-blk-test.c',
>>> -  'virtio-net-test.c',
>>> -  'virtio-rng-test.c',
>>> -  'virtio-scsi-test.c',
>>>    'virtio-serial-test.c',
>>>    'vmxnet3-test.c',
>>>  )
>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>>> +  qos_test_ss.add(
>>> +    'virtio-blk-test.c',
>>> +    'virtio-net-test.c',
>>> +    'virtio-rng-test.c',
>>> +    'virtio-scsi-test.c',
>>> +  )
>>> +endif
>>>  if have_virtfs
>>>    qos_test_ss.add(files('virtio-9p-test.c'))
>>>  endif
>>> ---
>>>
>>> I'll test that locally but not on Gitlab.
> 
> This approach doesn't work for the iotests.
> 
>> This also removes the virtio-devices test, I think we should keep the files, but in the files to
>> disable the PCI part when it is not available.
> I don't understand how the virtio devices are created, it seems there
> is an alias to generic virtio hw that map to the arch virtio bus.
> 
> I was not obvious to understand why start the virt machine with
> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device
> model name" at first, then I figured out the qdev_alias_table array.
> 
> Maybe you need to complete it for your arch? I've been using that:
> 
> -- >8 --
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 8dc656becca..b326bd76c2a 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = {
>      { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
>      { "virtio-balloon-pci", "virtio-balloon",
>              QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
> +    { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K },
>      { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
> -    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
> +    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
> +                                      & ~(QEMU_ARCH_S390X |
> QEMU_ARCH_M68K) },
>      { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X },
>      { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>      { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = {
>      { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>      { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
>      { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
> +    { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K },
>      { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
> -    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL &
> ~QEMU_ARCH_S390X },
> +    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
> +                                            & ~(QEMU_ARCH_S390X |
> QEMU_ARCH_M68K)},
>      { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
>      { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL &
> ~QEMU_ARCH_S390X },
>      { }
> ---
> 
> But this looks ugly, I don't think it should work that way (because
> a machine could provide virtio buses over multiple transport, mmio
> and pci...).

IMHO, this looks like the solution.

The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one.

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 18/03/2021 à 16:51, Laurent Vivier a écrit :
> Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit :
>> On 3/18/21 11:06 AM, Laurent Vivier wrote:
>>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
>>>> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>>>>> Hi Laurent,
>>>>>>
>>>>>> +Paolo / Thomas
>>>>>>
>>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>>>>> The machine is based on Goldfish interfaces defined by Google
>>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>>>>
>>>>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>>>>
>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>>>>> ---
>>>>>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>>>>  MAINTAINERS                                   |  13 +
>>>>>>>  hw/m68k/Kconfig                               |   9 +
>>>>>>>  hw/m68k/meson.build                           |   1 +
>>>>>>>  6 files changed, 355 insertions(+)
>>>>>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>>>>  create mode 100644 hw/m68k/virt.c
>>>>>>
>>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>>>>> --- a/hw/m68k/Kconfig
>>>>>>> +++ b/hw/m68k/Kconfig
>>>>>>> @@ -23,3 +23,12 @@ config Q800
>>>>>>>      select ESP
>>>>>>>      select DP8393X
>>>>>>>      select OR_IRQ
>>>>>>> +
>>>>>>> +config M68K_VIRT
>>>>>>> +    bool
>>>>>>> +    select M68K_IRQC
>>>>>>> +    select VIRT_CTRL
>>>>>>> +    select GOLDFISH_PIC
>>>>>>> +    select GOLDFISH_TTY
>>>>>>> +    select GOLDFISH_RTC
>>>>>>> +    select VIRTIO_MMIO
>>>>>>
>>>>>> I had this error on gitlab:
>>>>>>
>>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>>>>> 'virtio-blk-pci' is not a valid device model name
>>>>>> job: check-system-fedora
>>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>>>>
>>>>>> I bisected locally to this commit.
>>>>>>
>>>>>> check-system-fedora uses build-system-fedora:
>>>>>>
>>>>>> build-system-fedora:
>>>>>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>>>>              --enable-fdt=system --enable-slirp=system
>>>>>>              --enable-capstone=system
>>>>>>
>>>>>> I'm confused because the machine provides a VIRTIO bus
>>>>>> via MMIO:
>>>>>>
>>>>>> config VIRTIO_MMIO
>>>>>>     bool
>>>>>>     select VIRTIO
>>>>>>
>>>>>> I remember I tested your machine with virtio-blk-device.
>>>>>>
>>>>>> config VIRTIO_BLK
>>>>>>     bool
>>>>>>     default y
>>>>>>     depends on VIRTIO
>>>>>>
>>>>>> Ah, this is virtio-blk-pci, which has:
>>>>>>
>>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>>>>> files('virtio-blk-pci.c'))
>>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>>>>
>>>>>> And VIRTIO_PCI isn't selected...
>>>>>
>>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
>>>>
>>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
>>>> for this machine". So the problem isn't the m68k-virt machine addition,
>>>> but it shows another problem elsewhere.
>>>>
>>>>>> Are the tests incorrect then?
>>>>>>
>>>>>> libqos isn't restricted to PCI:
>>>>>>
>>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>>>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>>>>> "virtio-blk")) {
>>>>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>>>>> in virtio-blk-device\n", interface);
>>>>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>>>>> tests/qtest/libqos/virtio-blk.c:111:
>>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>>>>> tests/qtest/libqos/virtio-blk.c:112:
>>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>>>>> tests/qtest/libqos/virtio-blk.c:113:
>>>>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>>>>
>>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>>>>> to the virtio bus...
>>>>>
>>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>>>>> in the first free ones.
>>>>>
>>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>>>>
>>>>> Why is it executed for now?
>>>>
>>>> This is probably the problem root cause.
>>>>
>>>> Possible fix:
>>>>
>>>> -->8 --
>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>> index 66ee9fbf450..d7f3fad51c1 100644
>>>> --- a/tests/qtest/meson.build
>>>> +++ b/tests/qtest/meson.build
>>>> @@ -217,13 +217,17 @@
>>>>    'emc141x-test.c',
>>>>    'usb-hcd-ohci-test.c',
>>>>    'virtio-test.c',
>>>> -  'virtio-blk-test.c',
>>>> -  'virtio-net-test.c',
>>>> -  'virtio-rng-test.c',
>>>> -  'virtio-scsi-test.c',
>>>>    'virtio-serial-test.c',
>>>>    'vmxnet3-test.c',
>>>>  )
>>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>>>> +  qos_test_ss.add(
>>>> +    'virtio-blk-test.c',
>>>> +    'virtio-net-test.c',
>>>> +    'virtio-rng-test.c',
>>>> +    'virtio-scsi-test.c',
>>>> +  )
>>>> +endif
>>>>  if have_virtfs
>>>>    qos_test_ss.add(files('virtio-9p-test.c'))
>>>>  endif
>>>> ---
>>>>
>>>> I'll test that locally but not on Gitlab.
>>
>> This approach doesn't work for the iotests.
>>
>>> This also removes the virtio-devices test, I think we should keep the files, but in the files to
>>> disable the PCI part when it is not available.
>> I don't understand how the virtio devices are created, it seems there
>> is an alias to generic virtio hw that map to the arch virtio bus.
>>
>> I was not obvious to understand why start the virt machine with
>> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device
>> model name" at first, then I figured out the qdev_alias_table array.
>>
>> Maybe you need to complete it for your arch? I've been using that:
>>
>> -- >8 --
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 8dc656becca..b326bd76c2a 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = {
>>      { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
>>      { "virtio-balloon-pci", "virtio-balloon",
>>              QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>> +    { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K },
>>      { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
>> -    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>> +    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
>> +                                      & ~(QEMU_ARCH_S390X |
>> QEMU_ARCH_M68K) },
>>      { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X },
>>      { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>      { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
>> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = {
>>      { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>      { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
>>      { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>> +    { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K },
>>      { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
>> -    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL &
>> ~QEMU_ARCH_S390X },
>> +    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
>> +                                            & ~(QEMU_ARCH_S390X |
>> QEMU_ARCH_M68K)},
>>      { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
>>      { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL &
>> ~QEMU_ARCH_S390X },
>>      { }
>> ---
>>
>> But this looks ugly, I don't think it should work that way (because
>> a machine could provide virtio buses over multiple transport, mmio
>> and pci...).
> 
> IMHO, this looks like the solution.
> 
> The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one.

See:

commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53
Author: Alexander Graf <agraf@csgraf.de>
Date:   Fri May 18 02:36:26 2012 +0200

    s390x: fix s390 virtio aliases

    Some of the virtio devices have the same frontend name, but actually
    implement different devices behind the scenes through aliases.

    The indicator which device type to use is the architecture. On s390, we
    want s390 virtio devices. On everything else, we want PCI devices.

    Reflect this in the alias selection code. This way we fix commands like
    -device virtio-blk on s390x which with this patch applied select the
    correct virtio-blk-s390 device rather than virtio-blk-pci.

    Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
    Signed-off-by: Alexander Graf <agraf@suse.de>

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Philippe Mathieu-Daudé]

On 3/18/21 4:56 PM, Laurent Vivier wrote:
> Le 18/03/2021 à 16:51, Laurent Vivier a écrit :
>> Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit :
>>> On 3/18/21 11:06 AM, Laurent Vivier wrote:
>>>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
>>>>> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>>>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>>>>>> Hi Laurent,
>>>>>>>
>>>>>>> +Paolo / Thomas
>>>>>>>
>>>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>>>>>> The machine is based on Goldfish interfaces defined by Google
>>>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>>>>>
>>>>>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>>>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>>>>>
>>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>>>>>> ---
>>>>>>>>  default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>>>>>  .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>>>>>  hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>>>>>  MAINTAINERS                                   |  13 +
>>>>>>>>  hw/m68k/Kconfig                               |   9 +
>>>>>>>>  hw/m68k/meson.build                           |   1 +
>>>>>>>>  6 files changed, 355 insertions(+)
>>>>>>>>  create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>>>>>  create mode 100644 hw/m68k/virt.c
>>>>>>>
>>>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>>>>>> --- a/hw/m68k/Kconfig
>>>>>>>> +++ b/hw/m68k/Kconfig
>>>>>>>> @@ -23,3 +23,12 @@ config Q800
>>>>>>>>      select ESP
>>>>>>>>      select DP8393X
>>>>>>>>      select OR_IRQ
>>>>>>>> +
>>>>>>>> +config M68K_VIRT
>>>>>>>> +    bool
>>>>>>>> +    select M68K_IRQC
>>>>>>>> +    select VIRT_CTRL
>>>>>>>> +    select GOLDFISH_PIC
>>>>>>>> +    select GOLDFISH_TTY
>>>>>>>> +    select GOLDFISH_RTC
>>>>>>>> +    select VIRTIO_MMIO
>>>>>>>
>>>>>>> I had this error on gitlab:
>>>>>>>
>>>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>>>>>> 'virtio-blk-pci' is not a valid device model name
>>>>>>> job: check-system-fedora
>>>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>>>>>
>>>>>>> I bisected locally to this commit.
>>>>>>>
>>>>>>> check-system-fedora uses build-system-fedora:
>>>>>>>
>>>>>>> build-system-fedora:
>>>>>>>     CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>>>>>              --enable-fdt=system --enable-slirp=system
>>>>>>>              --enable-capstone=system
>>>>>>>
>>>>>>> I'm confused because the machine provides a VIRTIO bus
>>>>>>> via MMIO:
>>>>>>>
>>>>>>> config VIRTIO_MMIO
>>>>>>>     bool
>>>>>>>     select VIRTIO
>>>>>>>
>>>>>>> I remember I tested your machine with virtio-blk-device.
>>>>>>>
>>>>>>> config VIRTIO_BLK
>>>>>>>     bool
>>>>>>>     default y
>>>>>>>     depends on VIRTIO
>>>>>>>
>>>>>>> Ah, this is virtio-blk-pci, which has:
>>>>>>>
>>>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>>>>>> files('virtio-blk-pci.c'))
>>>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>>>>>
>>>>>>> And VIRTIO_PCI isn't selected...
>>>>>>
>>>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
>>>>>
>>>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
>>>>> for this machine". So the problem isn't the m68k-virt machine addition,
>>>>> but it shows another problem elsewhere.
>>>>>
>>>>>>> Are the tests incorrect then?
>>>>>>>
>>>>>>> libqos isn't restricted to PCI:
>>>>>>>
>>>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>>>>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>>>>>> "virtio-blk")) {
>>>>>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>>>>>> in virtio-blk-device\n", interface);
>>>>>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>>>>>> tests/qtest/libqos/virtio-blk.c:111:
>>>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>>>>>> tests/qtest/libqos/virtio-blk.c:112:
>>>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>>>>>> tests/qtest/libqos/virtio-blk.c:113:
>>>>>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>>>>>
>>>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>>>>>> to the virtio bus...
>>>>>>
>>>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>>>>>> in the first free ones.
>>>>>>
>>>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>>>>>
>>>>>> Why is it executed for now?
>>>>>
>>>>> This is probably the problem root cause.
>>>>>
>>>>> Possible fix:
>>>>>
>>>>> -->8 --
>>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>>> index 66ee9fbf450..d7f3fad51c1 100644
>>>>> --- a/tests/qtest/meson.build
>>>>> +++ b/tests/qtest/meson.build
>>>>> @@ -217,13 +217,17 @@
>>>>>    'emc141x-test.c',
>>>>>    'usb-hcd-ohci-test.c',
>>>>>    'virtio-test.c',
>>>>> -  'virtio-blk-test.c',
>>>>> -  'virtio-net-test.c',
>>>>> -  'virtio-rng-test.c',
>>>>> -  'virtio-scsi-test.c',
>>>>>    'virtio-serial-test.c',
>>>>>    'vmxnet3-test.c',
>>>>>  )
>>>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>>>>> +  qos_test_ss.add(
>>>>> +    'virtio-blk-test.c',
>>>>> +    'virtio-net-test.c',
>>>>> +    'virtio-rng-test.c',
>>>>> +    'virtio-scsi-test.c',
>>>>> +  )
>>>>> +endif
>>>>>  if have_virtfs
>>>>>    qos_test_ss.add(files('virtio-9p-test.c'))
>>>>>  endif
>>>>> ---
>>>>>
>>>>> I'll test that locally but not on Gitlab.
>>>
>>> This approach doesn't work for the iotests.
>>>
>>>> This also removes the virtio-devices test, I think we should keep the files, but in the files to
>>>> disable the PCI part when it is not available.
>>> I don't understand how the virtio devices are created, it seems there
>>> is an alias to generic virtio hw that map to the arch virtio bus.
>>>
>>> I was not obvious to understand why start the virt machine with
>>> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device
>>> model name" at first, then I figured out the qdev_alias_table array.
>>>
>>> Maybe you need to complete it for your arch? I've been using that:
>>>
>>> -- >8 --
>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index 8dc656becca..b326bd76c2a 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = {
>>>      { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
>>>      { "virtio-balloon-pci", "virtio-balloon",
>>>              QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>> +    { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K },
>>>      { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
>>> -    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>> +    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
>>> +                                      & ~(QEMU_ARCH_S390X |
>>> QEMU_ARCH_M68K) },
>>>      { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X },
>>>      { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>      { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
>>> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = {
>>>      { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>      { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
>>>      { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>> +    { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K },
>>>      { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
>>> -    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL &
>>> ~QEMU_ARCH_S390X },
>>> +    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
>>> +                                            & ~(QEMU_ARCH_S390X |
>>> QEMU_ARCH_M68K)},
>>>      { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
>>>      { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL &
>>> ~QEMU_ARCH_S390X },
>>>      { }
>>> ---
>>>
>>> But this looks ugly, I don't think it should work that way (because
>>> a machine could provide virtio buses over multiple transport, mmio
>>> and pci...).
>>
>> IMHO, this looks like the solution.
>>
>> The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one.
> 
> See:
> 
> commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53
> Author: Alexander Graf <agraf@csgraf.de>
> Date:   Fri May 18 02:36:26 2012 +0200
> 
>     s390x: fix s390 virtio aliases
> 
>     Some of the virtio devices have the same frontend name, but actually
>     implement different devices behind the scenes through aliases.
> 
>     The indicator which device type to use is the architecture. On s390, we
>     want s390 virtio devices. On everything else, we want PCI devices.
> 
>     Reflect this in the alias selection code. This way we fix commands like
>     -device virtio-blk on s390x which with this patch applied select the
>     correct virtio-blk-s390 device rather than virtio-blk-pci.
> 
>     Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>     Signed-off-by: Alexander Graf <agraf@suse.de>

So now than MMIO is available, we hit the "everything else" limit :)

The other function I had to modify is in tests/qemu-iotests/iotests.py:

  def get_virtio_scsi_device():
      if qemu_default_machine == 's390-ccw-virtio':
          return 'virtio-scsi-ccw'
      return 'virtio-scsi-pci'

But Max said there is no interest in testing the block devices here
(here = archs providing virtio via MMIO such ARM/m68k).

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Paolo Bonzini]

On 18/03/21 16:36, Philippe Mathieu-Daudé wrote:
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 66ee9fbf450..d7f3fad51c1 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -217,13 +217,17 @@
>     'emc141x-test.c',
>     'usb-hcd-ohci-test.c',
>     'virtio-test.c',
> -  'virtio-blk-test.c',
> -  'virtio-net-test.c',
> -  'virtio-rng-test.c',
> -  'virtio-scsi-test.c',
>     'virtio-serial-test.c',
>     'vmxnet3-test.c',
>   )
> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
> +  qos_test_ss.add(
> +    'virtio-blk-test.c',
> +    'virtio-net-test.c',
> +    'virtio-rng-test.c',
> +    'virtio-scsi-test.c',
> +  )
> +endif
>   if have_virtfs
>     qos_test_ss.add(files('virtio-9p-test.c'))
>   endif

I don't understand, what would this be trying to do?  (And besides, why 
would it work?  The CI failure is in qemu-iotests that has no connection 
to qtest at all).

> Maybe you need to complete it for your arch? I've been using that:

Instead of completing it, you can drop your arch from virtio-*-pci, so:

> +    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
> +                                      & ~(QEMU_ARCH_S390X |
> QEMU_ARCH_M68K) },

but do not add m68k anywhere.

Paolo

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Max Reitz]

On 18.03.21 17:25, Philippe Mathieu-Daudé wrote:
> On 3/18/21 4:56 PM, Laurent Vivier wrote:
>> Le 18/03/2021 à 16:51, Laurent Vivier a écrit :
>>> Le 18/03/2021 à 16:36, Philippe Mathieu-Daudé a écrit :
>>>> On 3/18/21 11:06 AM, Laurent Vivier wrote:
>>>>> Le 18/03/2021 à 11:02, Philippe Mathieu-Daudé a écrit :
>>>>>> On 3/18/21 10:52 AM, Laurent Vivier wrote:
>>>>>>> Le 18/03/2021 à 10:19, Philippe Mathieu-Daudé a écrit :
>>>>>>>> Hi Laurent,
>>>>>>>>
>>>>>>>> +Paolo / Thomas
>>>>>>>>
>>>>>>>> On 3/15/21 9:42 PM, Laurent Vivier wrote:
>>>>>>>>> The machine is based on Goldfish interfaces defined by Google
>>>>>>>>> for Android simulator. It uses Goldfish-rtc (timer and RTC),
>>>>>>>>> Goldfish-pic (PIC) and Goldfish-tty (for serial port and early tty).
>>>>>>>>>
>>>>>>>>> The machine is created with 128 virtio-mmio bus, and they can
>>>>>>>>> be used to use serial console, GPU, disk, NIC, HID, ...
>>>>>>>>>
>>>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>>>> Message-Id: <20210312214145.2936082-6-laurent@vivier.eu>
>>>>>>>>> ---
>>>>>>>>>   default-configs/devices/m68k-softmmu.mak      |   1 +
>>>>>>>>>   .../standard-headers/asm-m68k/bootinfo-virt.h |  18 +
>>>>>>>>>   hw/m68k/virt.c                                | 313 ++++++++++++++++++
>>>>>>>>>   MAINTAINERS                                   |  13 +
>>>>>>>>>   hw/m68k/Kconfig                               |   9 +
>>>>>>>>>   hw/m68k/meson.build                           |   1 +
>>>>>>>>>   6 files changed, 355 insertions(+)
>>>>>>>>>   create mode 100644 include/standard-headers/asm-m68k/bootinfo-virt.h
>>>>>>>>>   create mode 100644 hw/m68k/virt.c
>>>>>>>>
>>>>>>>>> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
>>>>>>>>> index 60d7bcfb8f2b..f839f8a03064 100644
>>>>>>>>> --- a/hw/m68k/Kconfig
>>>>>>>>> +++ b/hw/m68k/Kconfig
>>>>>>>>> @@ -23,3 +23,12 @@ config Q800
>>>>>>>>>       select ESP
>>>>>>>>>       select DP8393X
>>>>>>>>>       select OR_IRQ
>>>>>>>>> +
>>>>>>>>> +config M68K_VIRT
>>>>>>>>> +    bool
>>>>>>>>> +    select M68K_IRQC
>>>>>>>>> +    select VIRT_CTRL
>>>>>>>>> +    select GOLDFISH_PIC
>>>>>>>>> +    select GOLDFISH_TTY
>>>>>>>>> +    select GOLDFISH_RTC
>>>>>>>>> +    select VIRTIO_MMIO
>>>>>>>>
>>>>>>>> I had this error on gitlab:
>>>>>>>>
>>>>>>>> (qemu) QEMU_PROG: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio:
>>>>>>>> 'virtio-blk-pci' is not a valid device model name
>>>>>>>> job: check-system-fedora
>>>>>>>> https://gitlab.com/philmd/qemu/-/jobs/1106469724
>>>>>>>>
>>>>>>>> I bisected locally to this commit.
>>>>>>>>
>>>>>>>> check-system-fedora uses build-system-fedora:
>>>>>>>>
>>>>>>>> build-system-fedora:
>>>>>>>>      CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
>>>>>>>>               --enable-fdt=system --enable-slirp=system
>>>>>>>>               --enable-capstone=system
>>>>>>>>
>>>>>>>> I'm confused because the machine provides a VIRTIO bus
>>>>>>>> via MMIO:
>>>>>>>>
>>>>>>>> config VIRTIO_MMIO
>>>>>>>>      bool
>>>>>>>>      select VIRTIO
>>>>>>>>
>>>>>>>> I remember I tested your machine with virtio-blk-device.
>>>>>>>>
>>>>>>>> config VIRTIO_BLK
>>>>>>>>      bool
>>>>>>>>      default y
>>>>>>>>      depends on VIRTIO
>>>>>>>>
>>>>>>>> Ah, this is virtio-blk-pci, which has:
>>>>>>>>
>>>>>>>> virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
>>>>>>>> files('virtio-blk-pci.c'))
>>>>>>>> virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
>>>>>>>>
>>>>>>>> And VIRTIO_PCI isn't selected...
>>>>>>>
>>>>>>> This machine doesn't have virtio-pci, but only virtio-mmio buses.
>>>>>>
>>>>>> Yes. I meant "VIRTIO_PCI isn't selected, which is the correct config
>>>>>> for this machine". So the problem isn't the m68k-virt machine addition,
>>>>>> but it shows another problem elsewhere.
>>>>>>
>>>>>>>> Are the tests incorrect then?
>>>>>>>>
>>>>>>>> libqos isn't restricted to PCI:
>>>>>>>>
>>>>>>>> tests/qtest/libqos/virtio-blk.c:24:#include "virtio-blk.h"
>>>>>>>> tests/qtest/libqos/virtio-blk.c:29:/* virtio-blk-device */
>>>>>>>> tests/qtest/libqos/virtio-blk.c:33:    if (!g_strcmp0(interface,
>>>>>>>> "virtio-blk")) {
>>>>>>>> tests/qtest/libqos/virtio-blk.c:40:    fprintf(stderr, "%s not present
>>>>>>>> in virtio-blk-device\n", interface);
>>>>>>>> tests/qtest/libqos/virtio-blk.c:109:    /* virtio-blk-device */
>>>>>>>> tests/qtest/libqos/virtio-blk.c:111:
>>>>>>>> qos_node_create_driver("virtio-blk-device", virtio_blk_device_create);
>>>>>>>> tests/qtest/libqos/virtio-blk.c:112:
>>>>>>>> qos_node_consumes("virtio-blk-device", "virtio-bus", &opts);
>>>>>>>> tests/qtest/libqos/virtio-blk.c:113:
>>>>>>>> qos_node_produces("virtio-blk-device", "virtio-blk");
>>>>>>>>
>>>>>>>> But qemu-iotests / qtests do use virtio-blk-pci. Maybe they should
>>>>>>>> use a generic virtio-blk-device instead, hoping it get plugged correctly
>>>>>>>> to the virtio bus...
>>>>>>>
>>>>>>> Yes, it's how the machine work: it has 128 virtio-mmio buses and virtio-devices are plugged directly
>>>>>>> in the first free ones.
>>>>>>>
>>>>>>> I think the fix would be to disable the virtio-blk-pci test for the machines without PCI bus.
>>>>>>>
>>>>>>> Why is it executed for now?
>>>>>>
>>>>>> This is probably the problem root cause.
>>>>>>
>>>>>> Possible fix:
>>>>>>
>>>>>> -->8 --
>>>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>>>> index 66ee9fbf450..d7f3fad51c1 100644
>>>>>> --- a/tests/qtest/meson.build
>>>>>> +++ b/tests/qtest/meson.build
>>>>>> @@ -217,13 +217,17 @@
>>>>>>     'emc141x-test.c',
>>>>>>     'usb-hcd-ohci-test.c',
>>>>>>     'virtio-test.c',
>>>>>> -  'virtio-blk-test.c',
>>>>>> -  'virtio-net-test.c',
>>>>>> -  'virtio-rng-test.c',
>>>>>> -  'virtio-scsi-test.c',
>>>>>>     'virtio-serial-test.c',
>>>>>>     'vmxnet3-test.c',
>>>>>>   )
>>>>>> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>>>>>> +  qos_test_ss.add(
>>>>>> +    'virtio-blk-test.c',
>>>>>> +    'virtio-net-test.c',
>>>>>> +    'virtio-rng-test.c',
>>>>>> +    'virtio-scsi-test.c',
>>>>>> +  )
>>>>>> +endif
>>>>>>   if have_virtfs
>>>>>>     qos_test_ss.add(files('virtio-9p-test.c'))
>>>>>>   endif
>>>>>> ---
>>>>>>
>>>>>> I'll test that locally but not on Gitlab.
>>>>
>>>> This approach doesn't work for the iotests.
>>>>
>>>>> This also removes the virtio-devices test, I think we should keep the files, but in the files to
>>>>> disable the PCI part when it is not available.
>>>> I don't understand how the virtio devices are created, it seems there
>>>> is an alias to generic virtio hw that map to the arch virtio bus.
>>>>
>>>> I was not obvious to understand why start the virt machine with
>>>> "-device virtio-blk" returns "'virtio-blk-pci' is not a valid device
>>>> model name" at first, then I figured out the qdev_alias_table array.
>>>>
>>>> Maybe you need to complete it for your arch? I've been using that:
>>>>
>>>> -- >8 --
>>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>>> index 8dc656becca..b326bd76c2a 100644
>>>> --- a/softmmu/qdev-monitor.c
>>>> +++ b/softmmu/qdev-monitor.c
>>>> @@ -65,8 +65,10 @@ static const QDevAlias qdev_alias_table[] = {
>>>>       { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
>>>>       { "virtio-balloon-pci", "virtio-balloon",
>>>>               QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>> +    { "virtio-blk-device", "virtio-blk", QEMU_ARCH_M68K },
>>>>       { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
>>>> -    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>> +    { "virtio-blk-pci", "virtio-blk", QEMU_ARCH_ALL
>>>> +                                      & ~(QEMU_ARCH_S390X |
>>>> QEMU_ARCH_M68K) },
>>>>       { "virtio-gpu-ccw", "virtio-gpu", QEMU_ARCH_S390X },
>>>>       { "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>>       { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
>>>> @@ -84,8 +86,10 @@ static const QDevAlias qdev_alias_table[] = {
>>>>       { "virtio-rng-pci", "virtio-rng", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>>       { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
>>>>       { "virtio-scsi-pci", "virtio-scsi", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>>>> +    { "virtio-serial-device", "virtio-serial", QEMU_ARCH_M68K },
>>>>       { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
>>>> -    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL &
>>>> ~QEMU_ARCH_S390X },
>>>> +    { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
>>>> +                                            & ~(QEMU_ARCH_S390X |
>>>> QEMU_ARCH_M68K)},
>>>>       { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
>>>>       { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL &
>>>> ~QEMU_ARCH_S390X },
>>>>       { }
>>>> ---
>>>>
>>>> But this looks ugly, I don't think it should work that way (because
>>>> a machine could provide virtio buses over multiple transport, mmio
>>>> and pci...).
>>>
>>> IMHO, this looks like the solution.
>>>
>>> The alias is to define the prefered way, on PCI it's the -pci one otherwise it is the -device one.
>>
>> See:
>>
>> commit 5f629d943cb0b11c37a891cf4f40a9166aee6f53
>> Author: Alexander Graf <agraf@csgraf.de>
>> Date:   Fri May 18 02:36:26 2012 +0200
>>
>>      s390x: fix s390 virtio aliases
>>
>>      Some of the virtio devices have the same frontend name, but actually
>>      implement different devices behind the scenes through aliases.
>>
>>      The indicator which device type to use is the architecture. On s390, we
>>      want s390 virtio devices. On everything else, we want PCI devices.
>>
>>      Reflect this in the alias selection code. This way we fix commands like
>>      -device virtio-blk on s390x which with this patch applied select the
>>      correct virtio-blk-s390 device rather than virtio-blk-pci.
>>
>>      Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>      Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>      Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> So now than MMIO is available, we hit the "everything else" limit :)
> 
> The other function I had to modify is in tests/qemu-iotests/iotests.py:
> 
>    def get_virtio_scsi_device():
>        if qemu_default_machine == 's390-ccw-virtio':
>            return 'virtio-scsi-ccw'
>        return 'virtio-scsi-pci'
> 
> But Max said there is no interest in testing the block devices here
> (here = archs providing virtio via MMIO such ARM/m68k).

(To elaborate on my perspective)

I’m not exactly sure what you mean by this, i.e. whether you mean that 
we don’t want to test at all on that target, or that we don’t want to 
test specific devices.

The former is not entirely true (but in practice kind of), the latter I 
think is true.

I think we’d like to be able to provide developers the ability to run 
the iotests whatever target they’re working on, except where it’s just 
too complicated to do and nobody cares.  The obvious problem is that 
often nobody cares, so there’s a low threshold to disabling stuff.  (On 
s390, there was some pain, but people did care, so that’s a 
counter-story.)  The threshold is especially low if it’s just to silence 
CI, obviously.

The thing to note is that at least most iotests are not there to test 
the guest device, but rather the block layer; and the block layer is 
pretty much the same for every target, so there isn’t a big incentive 
for the block layer developers to have it run it on different targets. 
(I can’t rule out the possibility of a target-specific bug in how the 
block layer is used, which the iotests might reveal, but I don’t 
remember something like that to have happened so far.)

 From that it follows that I don’t see much use in testing specific 
devices either.  Say there’s a platform that provides both virtio-pci 
and virtio-mmio, the default (say virtio-pci) is fine for the iotests. 
I see little value in testing virtio-mmio as well.  (Perhaps I’m 
short-sighted, though.)

Max

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Thomas Huth]

On 18/03/2021 18.28, Max Reitz wrote:
[...]
>  From that it follows that I don’t see much use in testing specific devices 
> either.  Say there’s a platform that provides both virtio-pci and 
> virtio-mmio, the default (say virtio-pci) is fine for the iotests. I see 
> little value in testing virtio-mmio as well.  (Perhaps I’m short-sighted, 
> though.)

That's a fair point. But still, if someone compiled QEMU only with a target 
that only provided virtio-mmio, the iotests should not fail when running 
"make check".
To avoid that we continue playing whack-a-mole here in the future, maybe it 
would be better to restrict the iotests to the "main" targets only, e.g. 
modify check-block.sh so that the tests only run with x86, aarch64, s390x 
and ppc64 ?

  Thomas

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Max Reitz]

On 19.03.21 07:32, Thomas Huth wrote:
> On 18/03/2021 18.28, Max Reitz wrote:
> [...]
>>  From that it follows that I don’t see much use in testing specific 
>> devices either.  Say there’s a platform that provides both virtio-pci 
>> and virtio-mmio, the default (say virtio-pci) is fine for the iotests. 
>> I see little value in testing virtio-mmio as well.  (Perhaps I’m 
>> short-sighted, though.)
> 
> That's a fair point. But still, if someone compiled QEMU only with a 
> target that only provided virtio-mmio, the iotests should not fail when 
> running "make check".
> To avoid that we continue playing whack-a-mole here in the future, maybe 
> it would be better to restrict the iotests to the "main" targets only, 
> e.g. modify check-block.sh so that the tests only run with x86, aarch64, 
> s390x and ppc64 ?

Right, that would certainly be the simplest solution.

Max

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Paolo Bonzini]

On 19/03/21 10:20, Max Reitz wrote:
> On 19.03.21 07:32, Thomas Huth wrote:
>> On 18/03/2021 18.28, Max Reitz wrote:
>> [...]
>>>  From that it follows that I don’t see much use in testing specific 
>>> devices either.  Say there’s a platform that provides both virtio-pci 
>>> and virtio-mmio, the default (say virtio-pci) is fine for the 
>>> iotests. I see little value in testing virtio-mmio as well.  (Perhaps 
>>> I’m short-sighted, though.)
>>
>> That's a fair point. But still, if someone compiled QEMU only with a 
>> target that only provided virtio-mmio, the iotests should not fail 
>> when running "make check".
>> To avoid that we continue playing whack-a-mole here in the future, 
>> maybe it would be better to restrict the iotests to the "main" targets 
>> only, e.g. modify check-block.sh so that the tests only run with x86, 
>> aarch64, s390x and ppc64 ?
> 
> Right, that would certainly be the simplest solution.

It would also make the patches that Laurent sent this morning 
unnecessary, and avoid the use of aliases in the tests (so that it's 
clear what is tested).

Paolo

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 19/03/2021 à 10:20, Max Reitz a écrit :
> On 19.03.21 07:32, Thomas Huth wrote:
>> On 18/03/2021 18.28, Max Reitz wrote:
>> [...]
>>>  From that it follows that I don’t see much use in testing specific devices either.  Say there’s
>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine
>>> for the iotests. I see little value in testing virtio-mmio as well.  (Perhaps I’m short-sighted,
>>> though.)
>>
>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided
>> virtio-mmio, the iotests should not fail when running "make check".
>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to
>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only
>> run with x86, aarch64, s390x and ppc64 ?
> 
> Right, that would certainly be the simplest solution.
> 

The problem with that is we can't run the tests if target-list doesn't contain one of these targets.

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Laurent Vivier]

Le 19/03/2021 à 10:29, Paolo Bonzini a écrit :
> On 19/03/21 10:20, Max Reitz wrote:
>> On 19.03.21 07:32, Thomas Huth wrote:
>>> On 18/03/2021 18.28, Max Reitz wrote:
>>> [...]
>>>>  From that it follows that I don’t see much use in testing specific devices either.  Say there’s
>>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine
>>>> for the iotests. I see little value in testing virtio-mmio as well.  (Perhaps I’m short-sighted,
>>>> though.)
>>>
>>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided
>>> virtio-mmio, the iotests should not fail when running "make check".
>>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to
>>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests
>>> only run with x86, aarch64, s390x and ppc64 ?
>>
>> Right, that would certainly be the simplest solution.
> 
> It would also make the patches that Laurent sent this morning unnecessary, and avoid the use of
> aliases in the tests (so that it's clear what is tested).
>

We don't test the virtio frontend, but the blockdev backend, so we don't care what we use here.

Aliases simplify the code...

Thanks,
Laurent

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Max Reitz]

On 19.03.21 11:50, Laurent Vivier wrote:
> Le 19/03/2021 à 10:20, Max Reitz a écrit :
>> On 19.03.21 07:32, Thomas Huth wrote:
>>> On 18/03/2021 18.28, Max Reitz wrote:
>>> [...]
>>>>   From that it follows that I don’t see much use in testing specific devices either.  Say there’s
>>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine
>>>> for the iotests. I see little value in testing virtio-mmio as well.  (Perhaps I’m short-sighted,
>>>> though.)
>>>
>>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided
>>> virtio-mmio, the iotests should not fail when running "make check".
>>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to
>>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only
>>> run with x86, aarch64, s390x and ppc64 ?
>>
>> Right, that would certainly be the simplest solution.
>>
> 
> The problem with that is we can't run the tests if target-list doesn't contain one of these targets.

Yes, but is that really a problem?

Max

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Thomas Huth]

On 19/03/2021 11.50, Laurent Vivier wrote:
> Le 19/03/2021 à 10:20, Max Reitz a écrit :
>> On 19.03.21 07:32, Thomas Huth wrote:
>>> On 18/03/2021 18.28, Max Reitz wrote:
>>> [...]
>>>>   From that it follows that I don’t see much use in testing specific devices either.  Say there’s
>>>> a platform that provides both virtio-pci and virtio-mmio, the default (say virtio-pci) is fine
>>>> for the iotests. I see little value in testing virtio-mmio as well.  (Perhaps I’m short-sighted,
>>>> though.)
>>>
>>> That's a fair point. But still, if someone compiled QEMU only with a target that only provided
>>> virtio-mmio, the iotests should not fail when running "make check".
>>> To avoid that we continue playing whack-a-mole here in the future, maybe it would be better to
>>> restrict the iotests to the "main" targets only, e.g. modify check-block.sh so that the tests only
>>> run with x86, aarch64, s390x and ppc64 ?
>>
>> Right, that would certainly be the simplest solution.
>>
> 
> The problem with that is we can't run the tests if target-list doesn't contain one of these targets.

The iotests are skipped in quite a bunch of cases already anyway, e.g. when 
GNU sed or bash are not available in the right version. So I think it would 
be also ok to skip them in builds without one of the major architectures.
Anyway, since your patches are already ready, I think we also could simply 
go with those this time, and reconsider tweaking tests/check-block.sh the 
next time we run into this issue.

  Thomas

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Max Reitz]

On 19.03.21 11:51, Max Reitz wrote:
> On 19.03.21 11:50, Laurent Vivier wrote:
>> Le 19/03/2021 à 10:20, Max Reitz a écrit :
>>> On 19.03.21 07:32, Thomas Huth wrote:
>>>> On 18/03/2021 18.28, Max Reitz wrote:
>>>> [...]
>>>>>   From that it follows that I don’t see much use in testing 
>>>>> specific devices either.  Say there’s
>>>>> a platform that provides both virtio-pci and virtio-mmio, the 
>>>>> default (say virtio-pci) is fine
>>>>> for the iotests. I see little value in testing virtio-mmio as 
>>>>> well.  (Perhaps I’m short-sighted,
>>>>> though.)
>>>>
>>>> That's a fair point. But still, if someone compiled QEMU only with a 
>>>> target that only provided
>>>> virtio-mmio, the iotests should not fail when running "make check".
>>>> To avoid that we continue playing whack-a-mole here in the future, 
>>>> maybe it would be better to
>>>> restrict the iotests to the "main" targets only, e.g. modify 
>>>> check-block.sh so that the tests only
>>>> run with x86, aarch64, s390x and ppc64 ?
>>>
>>> Right, that would certainly be the simplest solution.
>>>
>>
>> The problem with that is we can't run the tests if target-list doesn't 
>> contain one of these targets.
> 
> Yes, but is that really a problem?

I should add: The thing is, I wouldn’t really call it a problem.  But 
still, as I said before somewhere in this thread, in theory we want to 
allow running the tests with every configuration.  It’s just that it’s a 
tradeoff between how much it helps and how much work it is to make them 
work.  (I gave s390 as an example, where effort was undertaken to make 
the iotests work.)

You’ve sent patches, so it seems you’re willing to invest the work. 
Sounds good to me, as long as we know it won’t rot.

Max

Re: [PULL 5/5] m68k: add Virtual M68k Machine

[Paolo Bonzini]

On 19/03/21 11:51, Laurent Vivier wrote:
>> It would also make the patches that Laurent sent this morning unnecessary, and avoid the use of
>> aliases in the tests (so that it's clear what is tested).
>
> We don't test the virtio frontend, but the blockdev backend, so we don't care what we use here.

Sort of, see for example the iothreads which, with your patch, would not 
be covered anymore on s390.

> Aliases simplify the code...

Aliases are not deprecated but... let's say despised, because of the 
special casing they add.  But yes, the code simplification from your 
patch is hard to brush away.

So I agree, since you have already mostly written the patches let's just 
complete them.

Paolo