[Qemu-devel] [PULL v3 00/10] target-arm queue

[Qemu-devel] [PULL v3 00/10] target-arm queue

[Peter Maydell]

This is a respin of my pull request from earlier this week:
 * versal board compile failure fixed
 * a few new patches:
  - MAINTAINERS file fix
  - use ARRAY_SIZE macro in xilinx_zynq
  - avoid an array overrun in strongarm GPIO irq handling
  - fix an assert running KVM on an aarch64-only host

The following changes since commit 69e2d03843412b9c076515b3aa9a71db161b6a1a:

  Merge remote-tracking branch 'remotes/riscv/tags/riscv-for-master-3.1-sf1' into staging (2018-11-02 13:16:13 +0000)

are available in the Git repository at:

  https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20181102

for you to fetch changes up to 6f16da53ffe4567c0353f85055df04860eb4e6fc:

  hw/arm: versal: Add a virtual Xilinx Versal board (2018-11-02 14:11:31 +0000)

----------------------------------------------------------------
target-arm queue:
 * microbit: Add the UART to our nRF51 SoC model
 * Add a virtual Xilinx Versal board "xlnx-versal-virt"
 * hw/arm/virt: Set VIRT_COMPAT_3_0 compat
 * MAINTAINERS: Remove bouncing email in ARM ACPI
 * strongarm: mask off high[31:28] bits from dir and state registers
 * target/arm: Conditionalize some asserts on aarch32 support
 * hw/arm/xilinx_zynq: Use the ARRAY_SIZE macro

----------------------------------------------------------------
Edgar E. Iglesias (2):
      hw/arm: versal: Add a model of Xilinx Versal SoC
      hw/arm: versal: Add a virtual Xilinx Versal board

Eric Auger (1):
      hw/arm/virt: Set VIRT_COMPAT_3_0 compat

Julia Suvorova (3):
      hw/char: Implement nRF51 SoC UART
      hw/arm/nrf51_soc: Connect UART to nRF51 SoC
      tests/boot-serial-test: Add microbit board testcase

Philippe Mathieu-Daudé (2):
      MAINTAINERS: Remove bouncing email in ARM ACPI
      hw/arm/xilinx_zynq: Use the ARRAY_SIZE macro

Prasad J Pandit (1):
      strongarm: mask off high[31:28] bits from dir and state registers

Richard Henderson (1):
      target/arm: Conditionalize some asserts on aarch32 support

 hw/arm/Makefile.objs                |   1 +
 hw/char/Makefile.objs               |   1 +
 include/hw/arm/nrf51_soc.h          |   3 +
 include/hw/arm/xlnx-versal.h        | 122 +++++++++
 include/hw/char/nrf51_uart.h        |  78 ++++++
 target/arm/cpu.h                    |   5 +
 hw/arm/microbit.c                   |   2 +
 hw/arm/nrf51_soc.c                  |  20 ++
 hw/arm/strongarm.c                  |   4 +-
 hw/arm/virt.c                       |   4 +
 hw/arm/xilinx_zynq.c                |   2 +-
 hw/arm/xlnx-versal-virt.c           | 494 ++++++++++++++++++++++++++++++++++++
 hw/arm/xlnx-versal.c                | 323 +++++++++++++++++++++++
 hw/char/nrf51_uart.c                | 330 ++++++++++++++++++++++++
 target/arm/cpu.c                    |  15 +-
 tests/boot-serial-test.c            |  19 ++
 MAINTAINERS                         |   1 -
 default-configs/aarch64-softmmu.mak |   1 +
 hw/char/trace-events                |   4 +
 19 files changed, 1423 insertions(+), 6 deletions(-)
 create mode 100644 include/hw/arm/xlnx-versal.h
 create mode 100644 include/hw/char/nrf51_uart.h
 create mode 100644 hw/arm/xlnx-versal-virt.c
 create mode 100644 hw/arm/xlnx-versal.c
 create mode 100644 hw/char/nrf51_uart.c

[Qemu-devel] [PULL 01/10] hw/arm/virt: Set VIRT_COMPAT_3_0 compat

[Peter Maydell]

From: Eric Auger <eric.auger@redhat.com>

We are missing the VIRT_COMPAT_3_0 definition and setting.
Let's add them.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Message-id: 20181024085602.16611-1-eric.auger@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9f677825f9f..a2b8d8f7c2c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1871,6 +1871,9 @@ static void virt_machine_3_1_options(MachineClass *mc)
 }
 DEFINE_VIRT_MACHINE_AS_LATEST(3, 1)
 
+#define VIRT_COMPAT_3_0 \
+    HW_COMPAT_3_0
+
 static void virt_3_0_instance_init(Object *obj)
 {
     virt_3_1_instance_init(obj);
@@ -1879,6 +1882,7 @@ static void virt_3_0_instance_init(Object *obj)
 static void virt_machine_3_0_options(MachineClass *mc)
 {
     virt_machine_3_1_options(mc);
+    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_3_0);
 }
 DEFINE_VIRT_MACHINE(3, 0)
 
-- 
2.19.1

[Qemu-devel] [PULL 02/10] hw/char: Implement nRF51 SoC UART

[Peter Maydell]

From: Julia Suvorova <jusual@mail.ru>

Not implemented: CTS/NCTS, PSEL*.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/Makefile.objs        |   1 +
 include/hw/char/nrf51_uart.h |  78 +++++++++
 hw/char/nrf51_uart.c         | 330 +++++++++++++++++++++++++++++++++++
 hw/char/trace-events         |   4 +
 4 files changed, 413 insertions(+)
 create mode 100644 include/hw/char/nrf51_uart.h
 create mode 100644 hw/char/nrf51_uart.c

diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index b5705312910..c4947d7ae7b 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -1,5 +1,6 @@
 common-obj-$(CONFIG_IPACK) += ipoctal232.o
 common-obj-$(CONFIG_ESCC) += escc.o
+common-obj-$(CONFIG_NRF51_SOC) += nrf51_uart.o
 common-obj-$(CONFIG_PARALLEL) += parallel.o
 common-obj-$(CONFIG_PARALLEL) += parallel-isa.o
 common-obj-$(CONFIG_PL011) += pl011.o
diff --git a/include/hw/char/nrf51_uart.h b/include/hw/char/nrf51_uart.h
new file mode 100644
index 00000000000..e3ecb7c81c2
--- /dev/null
+++ b/include/hw/char/nrf51_uart.h
@@ -0,0 +1,78 @@
+/*
+ * nRF51 SoC UART emulation
+ *
+ * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#ifndef NRF51_UART_H
+#define NRF51_UART_H
+
+#include "hw/sysbus.h"
+#include "chardev/char-fe.h"
+#include "hw/registerfields.h"
+
+#define UART_FIFO_LENGTH 6
+#define UART_BASE 0x40002000
+#define UART_SIZE 0x1000
+
+#define TYPE_NRF51_UART "nrf51_soc.uart"
+#define NRF51_UART(obj) OBJECT_CHECK(NRF51UARTState, (obj), TYPE_NRF51_UART)
+
+REG32(UART_STARTRX, 0x000)
+REG32(UART_STOPRX, 0x004)
+REG32(UART_STARTTX, 0x008)
+REG32(UART_STOPTX, 0x00C)
+REG32(UART_SUSPEND, 0x01C)
+
+REG32(UART_CTS, 0x100)
+REG32(UART_NCTS, 0x104)
+REG32(UART_RXDRDY, 0x108)
+REG32(UART_TXDRDY, 0x11C)
+REG32(UART_ERROR, 0x124)
+REG32(UART_RXTO, 0x144)
+
+REG32(UART_INTEN, 0x300)
+    FIELD(UART_INTEN, CTS, 0, 1)
+    FIELD(UART_INTEN, NCTS, 1, 1)
+    FIELD(UART_INTEN, RXDRDY, 2, 1)
+    FIELD(UART_INTEN, TXDRDY, 7, 1)
+    FIELD(UART_INTEN, ERROR, 9, 1)
+    FIELD(UART_INTEN, RXTO, 17, 1)
+REG32(UART_INTENSET, 0x304)
+REG32(UART_INTENCLR, 0x308)
+REG32(UART_ERRORSRC, 0x480)
+REG32(UART_ENABLE, 0x500)
+REG32(UART_PSELRTS, 0x508)
+REG32(UART_PSELTXD, 0x50C)
+REG32(UART_PSELCTS, 0x510)
+REG32(UART_PSELRXD, 0x514)
+REG32(UART_RXD, 0x518)
+REG32(UART_TXD, 0x51C)
+REG32(UART_BAUDRATE, 0x524)
+REG32(UART_CONFIG, 0x56C)
+
+typedef struct NRF51UARTState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    CharBackend chr;
+    qemu_irq irq;
+    guint watch_tag;
+
+    uint8_t rx_fifo[UART_FIFO_LENGTH];
+    unsigned int rx_fifo_pos;
+    unsigned int rx_fifo_len;
+
+    uint32_t reg[0x56C];
+
+    bool rx_started;
+    bool tx_started;
+    bool pending_tx_byte;
+    bool enabled;
+} NRF51UARTState;
+
+#endif
diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
new file mode 100644
index 00000000000..2f5fae61671
--- /dev/null
+++ b/hw/char/nrf51_uart.c
@@ -0,0 +1,330 @@
+/*
+ * nRF51 SoC UART emulation
+ *
+ * See nRF51 Series Reference Manual, "29 Universal Asynchronous
+ * Receiver/Transmitter" for hardware specifications:
+ * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/char/nrf51_uart.h"
+#include "trace.h"
+
+static void nrf51_uart_update_irq(NRF51UARTState *s)
+{
+    bool irq = false;
+
+    irq |= (s->reg[R_UART_RXDRDY] &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_RXDRDY_MASK));
+    irq |= (s->reg[R_UART_TXDRDY] &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_TXDRDY_MASK));
+    irq |= (s->reg[R_UART_ERROR]  &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_ERROR_MASK));
+    irq |= (s->reg[R_UART_RXTO]   &&
+            (s->reg[R_UART_INTEN] & R_UART_INTEN_RXTO_MASK));
+
+    qemu_set_irq(s->irq, irq);
+}
+
+static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+    uint64_t r;
+
+    if (!s->enabled) {
+        return 0;
+    }
+
+    switch (addr) {
+    case A_UART_RXD:
+        r = s->rx_fifo[s->rx_fifo_pos];
+        if (s->rx_started && s->rx_fifo_len) {
+            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
+            s->rx_fifo_len--;
+            if (s->rx_fifo_len) {
+                s->reg[R_UART_RXDRDY] = 1;
+                nrf51_uart_update_irq(s);
+            }
+            qemu_chr_fe_accept_input(&s->chr);
+        }
+        break;
+    case A_UART_INTENSET:
+    case A_UART_INTENCLR:
+    case A_UART_INTEN:
+        r = s->reg[R_UART_INTEN];
+        break;
+    default:
+        r = s->reg[addr / 4];
+        break;
+    }
+
+    trace_nrf51_uart_read(addr, r, size);
+
+    return r;
+}
+
+static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+    int r;
+    uint8_t c = s->reg[R_UART_TXD];
+
+    s->watch_tag = 0;
+
+    r = qemu_chr_fe_write(&s->chr, &c, 1);
+    if (r <= 0) {
+        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                             uart_transmit, s);
+        if (!s->watch_tag) {
+            /* The hardware has no transmit error reporting,
+             * so silently drop the byte
+             */
+            goto buffer_drained;
+        }
+        return FALSE;
+    }
+
+buffer_drained:
+    s->reg[R_UART_TXDRDY] = 1;
+    s->pending_tx_byte = false;
+    return FALSE;
+}
+
+static void uart_cancel_transmit(NRF51UARTState *s)
+{
+    if (s->watch_tag) {
+        g_source_remove(s->watch_tag);
+        s->watch_tag = 0;
+    }
+}
+
+static void uart_write(void *opaque, hwaddr addr,
+                       uint64_t value, unsigned int size)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    trace_nrf51_uart_write(addr, value, size);
+
+    if (!s->enabled && (addr != A_UART_ENABLE)) {
+        return;
+    }
+
+    switch (addr) {
+    case A_UART_TXD:
+        if (!s->pending_tx_byte && s->tx_started) {
+            s->reg[R_UART_TXD] = value;
+            s->pending_tx_byte = true;
+            uart_transmit(NULL, G_IO_OUT, s);
+        }
+        break;
+    case A_UART_INTEN:
+        s->reg[R_UART_INTEN] = value;
+        break;
+    case A_UART_INTENSET:
+        s->reg[R_UART_INTEN] |= value;
+        break;
+    case A_UART_INTENCLR:
+        s->reg[R_UART_INTEN] &= ~value;
+        break;
+    case A_UART_TXDRDY ... A_UART_RXTO:
+        s->reg[addr / 4] = value;
+        break;
+    case A_UART_ERRORSRC:
+        s->reg[addr / 4] &= ~value;
+        break;
+    case A_UART_RXD:
+        break;
+    case A_UART_RXDRDY:
+        if (value == 0) {
+            s->reg[R_UART_RXDRDY] = 0;
+        }
+        break;
+    case A_UART_STARTTX:
+        if (value == 1) {
+            s->tx_started = true;
+        }
+        break;
+    case A_UART_STARTRX:
+        if (value == 1) {
+            s->rx_started = true;
+        }
+        break;
+    case A_UART_ENABLE:
+        if (value) {
+            if (value == 4) {
+                s->enabled = true;
+            }
+            break;
+        }
+        s->enabled = false;
+        value = 1;
+        /* fall through */
+    case A_UART_SUSPEND:
+    case A_UART_STOPTX:
+        if (value == 1) {
+            s->tx_started = false;
+        }
+        /* fall through */
+    case A_UART_STOPRX:
+        if (addr != A_UART_STOPTX && value == 1) {
+            s->rx_started = false;
+            s->reg[R_UART_RXTO] = 1;
+        }
+        break;
+    default:
+        s->reg[addr / 4] = value;
+        break;
+    }
+    nrf51_uart_update_irq(s);
+}
+
+static const MemoryRegionOps uart_ops = {
+    .read =  uart_read,
+    .write = uart_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_uart_reset(DeviceState *dev)
+{
+    NRF51UARTState *s = NRF51_UART(dev);
+
+    s->pending_tx_byte = 0;
+
+    uart_cancel_transmit(s);
+
+    memset(s->reg, 0, sizeof(s->reg));
+
+    s->reg[R_UART_PSELRTS] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELTXD] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELCTS] = 0xFFFFFFFF;
+    s->reg[R_UART_PSELRXD] = 0xFFFFFFFF;
+    s->reg[R_UART_BAUDRATE] = 0x4000000;
+
+    s->rx_fifo_len = 0;
+    s->rx_fifo_pos = 0;
+    s->rx_started = false;
+    s->tx_started = false;
+    s->enabled = false;
+}
+
+static void uart_receive(void *opaque, const uint8_t *buf, int size)
+{
+
+    NRF51UARTState *s = NRF51_UART(opaque);
+    int i;
+
+    if (size == 0 || s->rx_fifo_len >= UART_FIFO_LENGTH) {
+        return;
+    }
+
+    for (i = 0; i < size; i++) {
+        uint32_t pos = (s->rx_fifo_pos + s->rx_fifo_len) % UART_FIFO_LENGTH;
+        s->rx_fifo[pos] = buf[i];
+        s->rx_fifo_len++;
+    }
+
+    s->reg[R_UART_RXDRDY] = 1;
+    nrf51_uart_update_irq(s);
+}
+
+static int uart_can_receive(void *opaque)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    return s->rx_started ? (UART_FIFO_LENGTH - s->rx_fifo_len) : 0;
+}
+
+static void uart_event(void *opaque, int event)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    if (event == CHR_EVENT_BREAK) {
+        s->reg[R_UART_ERRORSRC] |= 3;
+        s->reg[R_UART_ERROR] = 1;
+        nrf51_uart_update_irq(s);
+    }
+}
+
+static void nrf51_uart_realize(DeviceState *dev, Error **errp)
+{
+    NRF51UARTState *s = NRF51_UART(dev);
+
+    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
+                             uart_event, NULL, s, NULL, true);
+}
+
+static void nrf51_uart_init(Object *obj)
+{
+    NRF51UARTState *s = NRF51_UART(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->iomem, obj, &uart_ops, s,
+                          "nrf51_soc.uart", UART_SIZE);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+static int nrf51_uart_post_load(void *opaque, int version_id)
+{
+    NRF51UARTState *s = NRF51_UART(opaque);
+
+    if (s->pending_tx_byte) {
+        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                             uart_transmit, s);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription nrf51_uart_vmstate = {
+    .name = "nrf51_soc.uart",
+    .post_load = nrf51_uart_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, NRF51UARTState, 0x56C),
+        VMSTATE_UINT8_ARRAY(rx_fifo, NRF51UARTState, UART_FIFO_LENGTH),
+        VMSTATE_UINT32(rx_fifo_pos, NRF51UARTState),
+        VMSTATE_UINT32(rx_fifo_len, NRF51UARTState),
+        VMSTATE_BOOL(rx_started, NRF51UARTState),
+        VMSTATE_BOOL(tx_started, NRF51UARTState),
+        VMSTATE_BOOL(pending_tx_byte, NRF51UARTState),
+        VMSTATE_BOOL(enabled, NRF51UARTState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property nrf51_uart_properties[] = {
+    DEFINE_PROP_CHR("chardev", NRF51UARTState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nrf51_uart_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = nrf51_uart_reset;
+    dc->realize = nrf51_uart_realize;
+    dc->props = nrf51_uart_properties;
+    dc->vmsd = &nrf51_uart_vmstate;
+}
+
+static const TypeInfo nrf51_uart_info = {
+    .name = TYPE_NRF51_UART,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NRF51UARTState),
+    .instance_init = nrf51_uart_init,
+    .class_init = nrf51_uart_class_init
+};
+
+static void nrf51_uart_register_types(void)
+{
+    type_register_static(&nrf51_uart_info);
+}
+
+type_init(nrf51_uart_register_types)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index b64213d4dd1..de34a74399b 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -73,3 +73,7 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got character 0x%x from backe
 cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend pending"
 cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to backend"
 cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
+
+# hw/char/nrf51_uart.c
+nrf51_uart_read(uint64_t addr, uint64_t r, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
+nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
-- 
2.19.1

[Qemu-devel] [PULL 03/10] hw/arm/nrf51_soc: Connect UART to nRF51 SoC

[Peter Maydell]

From: Julia Suvorova <jusual@mail.ru>

Wire up nRF51 UART in the corresponding SoC.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/nrf51_soc.h |  3 +++
 hw/arm/microbit.c          |  2 ++
 hw/arm/nrf51_soc.c         | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index f4e092b554e..73fc92e9a8d 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -12,6 +12,7 @@
 
 #include "hw/sysbus.h"
 #include "hw/arm/armv7m.h"
+#include "hw/char/nrf51_uart.h"
 
 #define TYPE_NRF51_SOC "nrf51-soc"
 #define NRF51_SOC(obj) \
@@ -24,6 +25,8 @@ typedef struct NRF51State {
     /*< public >*/
     ARMv7MState cpu;
 
+    NRF51UARTState uart;
+
     MemoryRegion iomem;
     MemoryRegion sram;
     MemoryRegion flash;
diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index e7d74116a50..a734e7f650e 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -12,6 +12,7 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "sysemu/sysemu.h"
 #include "exec/address-spaces.h"
 
 #include "hw/arm/nrf51_soc.h"
@@ -35,6 +36,7 @@ static void microbit_init(MachineState *machine)
 
     sysbus_init_child_obj(OBJECT(machine), "nrf51", soc, sizeof(s->nrf51),
                           TYPE_NRF51_SOC);
+    qdev_prop_set_chr(DEVICE(&s->nrf51), "serial0", serial_hd(0));
     object_property_set_link(soc, OBJECT(system_memory), "memory",
                              &error_fatal);
     object_property_set_bool(soc, true, "realized", &error_fatal);
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 1a59ef45525..b89c1bdea08 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -43,9 +43,12 @@
 #define NRF51822_FLASH_SIZE     (256 * 1024)
 #define NRF51822_SRAM_SIZE      (16 * 1024)
 
+#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
+    MemoryRegion *mr;
     Error *err = NULL;
 
     if (!s->board_memory) {
@@ -82,6 +85,18 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
     memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
 
+    /* UART */
+    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
+    memory_region_add_subregion_overlap(&s->container, UART_BASE, mr, 0);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
+                       qdev_get_gpio_in(DEVICE(&s->cpu),
+                       BASE_TO_IRQ(UART_BASE)));
+
     create_unimplemented_device("nrf51_soc.io", IOMEM_BASE, IOMEM_SIZE);
     create_unimplemented_device("nrf51_soc.ficr", FICR_BASE, FICR_SIZE);
     create_unimplemented_device("nrf51_soc.private",
@@ -99,6 +114,11 @@ static void nrf51_soc_init(Object *obj)
     qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
                          ARM_CPU_TYPE_NAME("cortex-m0"));
     qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
+
+    sysbus_init_child_obj(obj, "uart", &s->uart, sizeof(s->uart),
+                           TYPE_NRF51_UART);
+    object_property_add_alias(obj, "serial0", OBJECT(&s->uart), "chardev",
+                              &error_abort);
 }
 
 static Property nrf51_soc_properties[] = {
-- 
2.19.1

[Qemu-devel] [PULL 04/10] tests/boot-serial-test: Add microbit board testcase

[Peter Maydell]

From: Julia Suvorova <jusual@mail.ru>

New mini-kernel test for nRF51 SoC UART.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
Acked-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/boot-serial-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index f865822e32f..8ec6aed35d2 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -62,6 +62,24 @@ static const uint8_t kernel_aarch64[] = {
     0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
 };
 
+static const uint8_t kernel_nrf51[] = {
+    0x00, 0x00, 0x00, 0x00,                 /* Stack top address */
+    0x09, 0x00, 0x00, 0x00,                 /* Reset handler address */
+    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get ENABLE */
+    0x04, 0x21,                             /* movs r1, #4 */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0x04, 0x4a,                             /* ldr  r2, [pc, #16] Get STARTTX */
+    0x01, 0x21,                             /* movs r1, #1 */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0x03, 0x4a,                             /* ldr  r2, [pc, #12] Get TXD */
+    0x54, 0x21,                             /* movs r1, 'T' */
+    0x11, 0x60,                             /* str  r1, [r2] */
+    0xfe, 0xe7,                             /* b    . */
+    0x00, 0x25, 0x00, 0x40,                 /* 0x40002500 = UART ENABLE */
+    0x08, 0x20, 0x00, 0x40,                 /* 0x40002008 = UART STARTTX */
+    0x1c, 0x25, 0x00, 0x40                  /* 0x4000251c = UART TXD */
+};
+
 typedef struct testdef {
     const char *arch;       /* Target architecture */
     const char *machine;    /* Name of the machine */
@@ -105,6 +123,7 @@ static testdef_t tests[] = {
     { "hppa", "hppa", "", "SeaBIOS wants SYSTEM HALT" },
     { "aarch64", "virt", "-cpu cortex-a57", "TT", sizeof(kernel_aarch64),
       kernel_aarch64 },
+    { "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
 
     { NULL }
 };
-- 
2.19.1

[Qemu-devel] [PULL 05/10] MAINTAINERS: Remove bouncing email in ARM ACPI

[Peter Maydell]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Shannon Zhao's email at Huawei is bouncing: remove it.

    X-Failed-Recipients: zhaoshenglong@huawei.com
    ** Address not found **
    Your message wasn't delivered to zhaoshenglong@huawei.com because the address couldn't be found, or is unable to receive mail.

Note that the section still contains his personal email (see e59f13d76bb).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Shannon Zhao <shannon.zhaosl@gmail.com>
Message-id: 20181029195931.8747-1-philmd@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 85f19f569ff..98a1856afc0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -627,7 +627,6 @@ F: hw/*/xlnx*.c
 F: include/hw/*/xlnx*.h
 
 ARM ACPI Subsystem
-M: Shannon Zhao <zhaoshenglong@huawei.com>
 M: Shannon Zhao <shannon.zhaosl@gmail.com>
 L: qemu-arm@nongnu.org
 S: Maintained
-- 
2.19.1

[Qemu-devel] [PULL 06/10] strongarm: mask off high[31:28] bits from dir and state registers

[Peter Maydell]

From: Prasad J Pandit <pjp@fedoraproject.org>

The high[31:28] bits of 'direction' and 'state' registers of
SA-1100/SA-1110 device are reserved. Setting them may lead to
OOB 's->handler[]' array access issue. Mask off [31:28] bits to
avoid it.

Reported-by: Moguofang <moguofang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Message-id: 20181030114635.31232-1-ppandit@redhat.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/strongarm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index ec2627374d0..644a9c45b4e 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -587,12 +587,12 @@ static void strongarm_gpio_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case GPDR:        /* GPIO Pin-Direction registers */
-        s->dir = value;
+        s->dir = value & 0x0fffffff;
         strongarm_gpio_handler_update(s);
         break;
 
     case GPSR:        /* GPIO Pin-Output Set registers */
-        s->olevel |= value;
+        s->olevel |= value & 0x0fffffff;
         strongarm_gpio_handler_update(s);
         break;
 
-- 
2.19.1

[Qemu-devel] [PULL 07/10] hw/arm/xilinx_zynq: Use the ARRAY_SIZE macro

[Peter Maydell]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/xilinx_zynq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index f1496d29273..57497b0c4d3 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -294,7 +294,7 @@ static void zynq_init(MachineState *machine)
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, 0xF8003000);
     sysbus_connect_irq(busdev, 0, pic[45-IRQ_OFFSET]); /* abort irq line */
-    for (n = 0; n < 8; ++n) { /* event irqs */
+    for (n = 0; n < ARRAY_SIZE(dma_irqs); ++n) { /* event irqs */
         sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
     }
 
-- 
2.19.1

[Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

From: Richard Henderson <richard.henderson@linaro.org>

When populating id registers from kvm, on a host that doesn't support
aarch32 mode at all, neither arm_div nor jazelle will be supported either.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20181102102025.3546-1-richard.henderson@linaro.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h |  5 +++++
 target/arm/cpu.c | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8e6779936eb..b5eff79f73b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3296,6 +3296,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
 }
 
+static inline bool isar_feature_aa64_aa32(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) >= 2;
+}
+
 static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 8f16e96b6c8..784a4c2dfcc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -774,6 +774,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUARMState *env = &cpu->env;
     int pagebits;
     Error *local_err = NULL;
+    bool no_aa32 = false;
 
     /* If we needed to query the host kernel for the CPU features
      * then it's possible that might have failed in the initfn, but
@@ -820,6 +821,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             set_feature(env, ARM_FEATURE_V7VE);
         }
     }
+
+    /*
+     * There exist AArch64 cpus without AArch32 support.  When KVM
+     * queries ID_ISAR0_EL1 on such a host, the value is UNKNOWN.
+     * Similarly, we cannot check ID_AA64PFR0 without AArch64 support.
+     */
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        no_aa32 = !cpu_isar_feature(aa64_aa32, cpu);
+    }
+
     if (arm_feature(env, ARM_FEATURE_V7VE)) {
         /* v7 Virtualization Extensions. In real hardware this implies
          * EL2 and also the presence of the Security Extensions.
@@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
          * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
          * Security Extensions is ARM_FEATURE_EL3.
          */
-        assert(cpu_isar_feature(arm_div, cpu));
+        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
         set_feature(env, ARM_FEATURE_LPAE);
         set_feature(env, ARM_FEATURE_V7);
     }
@@ -855,7 +866,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (arm_feature(env, ARM_FEATURE_V6)) {
         set_feature(env, ARM_FEATURE_V5);
         if (!arm_feature(env, ARM_FEATURE_M)) {
-            assert(cpu_isar_feature(jazelle, cpu));
+            assert(no_aa32 || cpu_isar_feature(jazelle, cpu));
             set_feature(env, ARM_FEATURE_AUXCR);
         }
     }
-- 
2.19.1

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

Hi,

On 11/02/18 18:16, Peter Maydell wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> When populating id registers from kvm, on a host that doesn't support
> aarch32 mode at all, neither arm_div nor jazelle will be supported either.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Message-id: 20181102102025.3546-1-richard.henderson@linaro.org
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h |  5 +++++
>  target/arm/cpu.c | 15 +++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8e6779936eb..b5eff79f73b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3296,6 +3296,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>  }
>  
> +static inline bool isar_feature_aa64_aa32(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) >= 2;
> +}
> +
>  static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 8f16e96b6c8..784a4c2dfcc 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -774,6 +774,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUARMState *env = &cpu->env;
>      int pagebits;
>      Error *local_err = NULL;
> +    bool no_aa32 = false;
>  
>      /* If we needed to query the host kernel for the CPU features
>       * then it's possible that might have failed in the initfn, but
> @@ -820,6 +821,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>              set_feature(env, ARM_FEATURE_V7VE);
>          }
>      }
> +
> +    /*
> +     * There exist AArch64 cpus without AArch32 support.  When KVM
> +     * queries ID_ISAR0_EL1 on such a host, the value is UNKNOWN.
> +     * Similarly, we cannot check ID_AA64PFR0 without AArch64 support.
> +     */
> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +        no_aa32 = !cpu_isar_feature(aa64_aa32, cpu);
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_V7VE)) {
>          /* v7 Virtualization Extensions. In real hardware this implies
>           * EL2 and also the presence of the Security Extensions.
> @@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>           * Security Extensions is ARM_FEATURE_EL3.
>           */
> -        assert(cpu_isar_feature(arm_div, cpu));
> +        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));

The assertion above fails on my AArch64 host (APM Mustang A3). Meaning
that my host CPU supports AArch32, but lacks "arm_div".

(My understanding is that this commit, i.e., 0f8d06f16c9d, relaxed the
assert originally added in commit 7e0cf8b47f0e ("target/arm: Convert
division from feature bits to isar0 tests", 2018-10-24). Can we relax it
even further?

Better yet: can we rework the code to emit a warning, rather than
aborting QEMU? Assertions are not the best tool IMHO for catching
unusual (or slightly non-conformant / early) hardware.)

Thanks,
Laszlo

>          set_feature(env, ARM_FEATURE_LPAE);
>          set_feature(env, ARM_FEATURE_V7);
>      }
> @@ -855,7 +866,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (arm_feature(env, ARM_FEATURE_V6)) {
>          set_feature(env, ARM_FEATURE_V5);
>          if (!arm_feature(env, ARM_FEATURE_M)) {
> -            assert(cpu_isar_feature(jazelle, cpu));
> +            assert(no_aa32 || cpu_isar_feature(jazelle, cpu));
>              set_feature(env, ARM_FEATURE_AUXCR);
>          }
>      }
> 

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 05/24/19 14:33, Laszlo Ersek wrote:
> Hi,
>
> On 11/02/18 18:16, Peter Maydell wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> When populating id registers from kvm, on a host that doesn't support
>> aarch32 mode at all, neither arm_div nor jazelle will be supported either.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-id: 20181102102025.3546-1-richard.henderson@linaro.org
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/cpu.h |  5 +++++
>>  target/arm/cpu.c | 15 +++++++++++++--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 8e6779936eb..b5eff79f73b 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -3296,6 +3296,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>>  }
>>
>> +static inline bool isar_feature_aa64_aa32(const ARMISARegisters *id)
>> +{
>> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) >= 2;
>> +}
>> +
>>  static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
>>  {
>>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 8f16e96b6c8..784a4c2dfcc 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -774,6 +774,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>      CPUARMState *env = &cpu->env;
>>      int pagebits;
>>      Error *local_err = NULL;
>> +    bool no_aa32 = false;
>>
>>      /* If we needed to query the host kernel for the CPU features
>>       * then it's possible that might have failed in the initfn, but
>> @@ -820,6 +821,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>              set_feature(env, ARM_FEATURE_V7VE);
>>          }
>>      }
>> +
>> +    /*
>> +     * There exist AArch64 cpus without AArch32 support.  When KVM
>> +     * queries ID_ISAR0_EL1 on such a host, the value is UNKNOWN.
>> +     * Similarly, we cannot check ID_AA64PFR0 without AArch64 support.
>> +     */
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +        no_aa32 = !cpu_isar_feature(aa64_aa32, cpu);
>> +    }
>> +
>>      if (arm_feature(env, ARM_FEATURE_V7VE)) {
>>          /* v7 Virtualization Extensions. In real hardware this implies
>>           * EL2 and also the presence of the Security Extensions.
>> @@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>           * Security Extensions is ARM_FEATURE_EL3.
>>           */
>> -        assert(cpu_isar_feature(arm_div, cpu));
>> +        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>
> The assertion above fails on my AArch64 host (APM Mustang A3). Meaning
> that my host CPU supports AArch32, but lacks "arm_div".
>
> (My understanding is that this commit, i.e., 0f8d06f16c9d, relaxed the
> assert originally added in commit 7e0cf8b47f0e ("target/arm: Convert
> division from feature bits to isar0 tests", 2018-10-24). Can we relax it
> even further?
>
> Better yet: can we rework the code to emit a warning, rather than
> aborting QEMU? Assertions are not the best tool IMHO for catching
> unusual (or slightly non-conformant / early) hardware.)

To clarify, I intended to launch a 32-bit ARM guest (with KVM
acceleration) on my AArch64 host.

Libvirt generated the following QEMU command line:

LC_ALL=C \
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin \
QEMU_AUDIO_DRV=none \
/opt/qemu-installed-optimized/bin/qemu-system-aarch64 \
  -name guest=f28.32bit,debug-threads=on \
  -S \
  -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-2-f28.32bit/master-key.aes \
  -machine virt-4.1,accel=kvm,usb=off,dump-guest-core=off,gic-version=2 \
  -cpu host,aarch64=off \
  -drive file=/root/QEMU_EFI.fd.padded,if=pflash,format=raw,unit=0,readonly=on \
  -drive file=/var/lib/libvirt/qemu/nvram/f28.32bit_VARS.fd,if=pflash,format=raw,unit=1 \
  -m 8192 \
  -realtime mlock=off \
  -smp 8,sockets=8,cores=1,threads=1 \
  -uuid d525042e-1b37-4058-86ca-c6a2086e8485 \
  -no-user-config \
  -nodefaults \
  -chardev socket,id=charmonitor,fd=27,server,nowait \
  -mon chardev=charmonitor,id=monitor,mode=control \
  -rtc base=utc \
  -no-shutdown \
  -boot strict=on \
  -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1 \
  -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
  -device pcie-root-port,port=0xa,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \
  -device pcie-root-port,port=0xb,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \
  -device pcie-root-port,port=0xc,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \
  -device pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
  -device qemu-xhci,id=usb,bus=pci.1,addr=0x0 \
  -device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x0 \
  -device virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 \
  -drive file=/var/lib/libvirt/images/f28.32bit.root.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,werror=enospc,cache=writeback,discard=unmap \
  -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1,write-cache=on \
  -drive file=/var/lib/libvirt/images/f28.32bit.home.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-1,werror=enospc,cache=writeback,discard=unmap \
  -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,write-cache=on \
  -netdev tap,fd=29,id=hostnet0,vhost=on,vhostfd=30 \
  -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:6f:d1:c8,bus=pci.4,addr=0x0,romfile= \
  -chardev pty,id=charserial0 \
  -serial chardev:charserial0 \
  -chardev socket,id=charchannel0,fd=31,server,nowait \
  -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 \
  -device usb-tablet,id=input0,bus=usb.0,port=1 \
  -device usb-kbd,id=input1,bus=usb.0,port=2 \
  -vnc 127.0.0.1:0 \
  -device virtio-gpu-pci,id=video0,max_outputs=1,bus=pci.5,addr=0x0 \
  -object rng-random,id=objrng0,filename=/dev/urandom \
  -device virtio-rng-pci,rng=objrng0,id=rng0,max-bytes=1048576,period=1000,bus=pci.6,addr=0x0 \
  -msg timestamp=on

and then I got:

> qemu-system-aarch64: /root/src/upstream/qemu/target/arm/cpu.c:986:
> arm_cpu_realizefn: Assertion `no_aa32 || ({ ARMCPU *cpu_ = (cpu);
> isar_feature_arm_div(&cpu_->isar); })' failed.

QEMU was built at commit 8dc7fd56dd4f ("Merge remote-tracking branch
'remotes/philmd-gitlab/tags/fw_cfg-20190523-pull-request' into staging",
2019-05-23).

Thanks
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Philippe Mathieu-Daudé]

On 5/24/19 2:33 PM, Laszlo Ersek wrote:
> Hi,
> 
> On 11/02/18 18:16, Peter Maydell wrote:
>> From: Richard Henderson <richard.henderson@linaro.org>
>>
>> When populating id registers from kvm, on a host that doesn't support
>> aarch32 mode at all, neither arm_div nor jazelle will be supported either.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-id: 20181102102025.3546-1-richard.henderson@linaro.org
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/cpu.h |  5 +++++
>>  target/arm/cpu.c | 15 +++++++++++++--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 8e6779936eb..b5eff79f73b 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -3296,6 +3296,11 @@ static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
>>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>>  }
>>  
>> +static inline bool isar_feature_aa64_aa32(const ARMISARegisters *id)
>> +{
>> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL0) >= 2;

FWIW here we check EL0, ...

>> +}
>> +
>>  static inline bool isar_feature_aa64_sve(const ARMISARegisters *id)
>>  {
>>      return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 8f16e96b6c8..784a4c2dfcc 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -774,6 +774,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>      CPUARMState *env = &cpu->env;
>>      int pagebits;
>>      Error *local_err = NULL;
>> +    bool no_aa32 = false;
>>  
>>      /* If we needed to query the host kernel for the CPU features
>>       * then it's possible that might have failed in the initfn, but
>> @@ -820,6 +821,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>              set_feature(env, ARM_FEATURE_V7VE);
>>          }
>>      }
>> +
>> +    /*
>> +     * There exist AArch64 cpus without AArch32 support.  When KVM
>> +     * queries ID_ISAR0_EL1 on such a host, the value is UNKNOWN.

... then here the comment is about EL1.

>> +     * Similarly, we cannot check ID_AA64PFR0 without AArch64 support.
>> +     */
>> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>> +        no_aa32 = !cpu_isar_feature(aa64_aa32, cpu);
>> +    }
>> +
>>      if (arm_feature(env, ARM_FEATURE_V7VE)) {
>>          /* v7 Virtualization Extensions. In real hardware this implies
>>           * EL2 and also the presence of the Security Extensions.
>> @@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>           * Security Extensions is ARM_FEATURE_EL3.
>>           */
>> -        assert(cpu_isar_feature(arm_div, cpu));
>> +        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> 
> The assertion above fails on my AArch64 host (APM Mustang A3). Meaning
> that my host CPU supports AArch32, but lacks "arm_div".
> 
> (My understanding is that this commit, i.e., 0f8d06f16c9d, relaxed the
> assert originally added in commit 7e0cf8b47f0e ("target/arm: Convert
> division from feature bits to isar0 tests", 2018-10-24). Can we relax it
> even further?
> 
> Better yet: can we rework the code to emit a warning, rather than
> aborting QEMU? Assertions are not the best tool IMHO for catching
> unusual (or slightly non-conformant / early) hardware.)
> 
> Thanks,
> Laszlo
> 
>>          set_feature(env, ARM_FEATURE_LPAE);
>>          set_feature(env, ARM_FEATURE_V7);
>>      }
>> @@ -855,7 +866,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>      if (arm_feature(env, ARM_FEATURE_V6)) {
>>          set_feature(env, ARM_FEATURE_V5);
>>          if (!arm_feature(env, ARM_FEATURE_M)) {
>> -            assert(cpu_isar_feature(jazelle, cpu));
>> +            assert(no_aa32 || cpu_isar_feature(jazelle, cpu));
>>              set_feature(env, ARM_FEATURE_AUXCR);
>>          }
>>      }
>>
> 

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Fri, 24 May 2019 at 13:33, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/02/18 18:16, Peter Maydell wrote:
> > @@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
> >           * Security Extensions is ARM_FEATURE_EL3.
> >           */
> > -        assert(cpu_isar_feature(arm_div, cpu));
> > +        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>
> The assertion above fails on my AArch64 host (APM Mustang A3). Meaning
> that my host CPU supports AArch32, but lacks "arm_div".

Hi; I just realized we left this assertion-failure bug report
unaddressed, so I had a look at it.

I tried to repro on my Mustang, but this works for me.
A CPU with AArch32 but without the Arm-mode division instructions
would be non-compliant (and very obviously so if tested), so
I suspect the actual problem is not with the hardware but with
the kernel not correctly reporting the ID registers to QEMU.
What kernel version are you using?

> Better yet: can we rework the code to emit a warning, rather than
> aborting QEMU? Assertions are not the best tool IMHO for catching
> unusual (or slightly non-conformant / early) hardware.)

The intention of the assertion really is to catch QEMU bugs
where we got the ID register values wrong in our emulated
CPUs. Perhaps we should relax all these assertions to only
testing if we're using TCG, not KVM ?

thanks
-- PMM

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Richard Henderson]

On 7/16/19 12:03 PM, Peter Maydell wrote:
> The intention of the assertion really is to catch QEMU bugs
> where we got the ID register values wrong in our emulated
> CPUs. Perhaps we should relax all these assertions to only
> testing if we're using TCG, not KVM ?

Perhaps.  In some instances if ID register values are wrong we would then not
migrate properly, but none of the checks we're currently doing of this sort
would catch those particular cases.


r~

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Tue, 16 Jul 2019 at 15:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/16/19 12:03 PM, Peter Maydell wrote:
> > The intention of the assertion really is to catch QEMU bugs
> > where we got the ID register values wrong in our emulated
> > CPUs. Perhaps we should relax all these assertions to only
> > testing if we're using TCG, not KVM ?
>
> Perhaps.  In some instances if ID register values are wrong we would then not
> migrate properly, but none of the checks we're currently doing of this sort
> would catch those particular cases.

In those cases we should probably print a warning and install
a migration-blocker, rather than asserting...

thanks
-- PMM

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Richard Henderson]

On 7/16/19 2:18 PM, Peter Maydell wrote:
> On Tue, 16 Jul 2019 at 15:02, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 7/16/19 12:03 PM, Peter Maydell wrote:
>>> The intention of the assertion really is to catch QEMU bugs
>>> where we got the ID register values wrong in our emulated
>>> CPUs. Perhaps we should relax all these assertions to only
>>> testing if we're using TCG, not KVM ?
>>
>> Perhaps.  In some instances if ID register values are wrong we would then not
>> migrate properly, but none of the checks we're currently doing of this sort
>> would catch those particular cases.
> 
> In those cases we should probably print a warning and install
> a migration-blocker, rather than asserting...

Ok, but I'm saying we don't assert for those either, at the moment.


r~

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/16/19 14:03, Peter Maydell wrote:
> On Fri, 24 May 2019 at 13:33, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 11/02/18 18:16, Peter Maydell wrote:
>>> @@ -829,7 +840,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>>           * Security Extensions is ARM_FEATURE_EL3.
>>>           */
>>> -        assert(cpu_isar_feature(arm_div, cpu));
>>> +        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>
>> The assertion above fails on my AArch64 host (APM Mustang A3). Meaning
>> that my host CPU supports AArch32, but lacks "arm_div".
> 
> Hi; I just realized we left this assertion-failure bug report
> unaddressed, so I had a look at it.

Yes, it's also tracked under LP#1830864; thanks for looking into it.

> 
> I tried to repro on my Mustang, but this works for me.
> A CPU with AArch32 but without the Arm-mode division instructions
> would be non-compliant (and very obviously so if tested), so
> I suspect the actual problem is not with the hardware but with
> the kernel not correctly reporting the ID registers to QEMU.
> What kernel version are you using?

So, I've just retested, with the QEMU binary I've left around from last
time. (This QEMU binary was built at upstream commit d247c8e7f4fc, with
Phil's v2 series applied on top, for regression testing:

[PATCH v2 0/9] hw/block/pflash_cfi01: Add DeviceReset() handler

http://mid.mail-archive.com/38281fa7-30f4-60ec-3357-3e1613c44dbe@redhat.com
)

The issue still reproduces, so it makes sense for me to look at the host
kernel version... Well, I'm afraid it won't help much, for an upstream
investigation:

  4.14.0-115.8.2.el7a.aarch64

This is the latest released kernel from "Red Hat Enterprise Linux for
ARM 64 7".

Thanks!
Laszlo

>> Better yet: can we rework the code to emit a warning, rather than
>> aborting QEMU? Assertions are not the best tool IMHO for catching
>> unusual (or slightly non-conformant / early) hardware.)
> 
> The intention of the assertion really is to catch QEMU bugs
> where we got the ID register values wrong in our emulated
> CPUs. Perhaps we should relax all these assertions to only
> testing if we're using TCG, not KVM ?
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
> The issue still reproduces, so it makes sense for me to look at the host
> kernel version... Well, I'm afraid it won't help much, for an upstream
> investigation:
>
>   4.14.0-115.8.2.el7a.aarch64
>
> This is the latest released kernel from "Red Hat Enterprise Linux for
> ARM 64 7".

OK. (I'm using 4.15.0-51-generic from ubuntu).

Could you run with QEMU under gdb, and when it hits the
assertion go back up a stack frame to the arm_cpu_realizefn()
frame, and then "print /x cpu->isar" ? That should show us
what we think we've got as ID registers from the kernel.
(You might need to build QEMU with --enable-debug to get
useful enough debug info to do that, not sure.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/16/19 18:59, Peter Maydell wrote:
> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>> The issue still reproduces, so it makes sense for me to look at the host
>> kernel version... Well, I'm afraid it won't help much, for an upstream
>> investigation:
>>
>>   4.14.0-115.8.2.el7a.aarch64
>>
>> This is the latest released kernel from "Red Hat Enterprise Linux for
>> ARM 64 7".
> 
> OK. (I'm using 4.15.0-51-generic from ubuntu).
> 
> Could you run with QEMU under gdb, and when it hits the
> assertion go back up a stack frame to the arm_cpu_realizefn()
> frame, and then "print /x cpu->isar" ? That should show us
> what we think we've got as ID registers from the kernel.
> (You might need to build QEMU with --enable-debug to get
> useful enough debug info to do that, not sure.)

(My qemu build script always builds QEMU in two configs, the difference
being --prefix and --enable-debug.)

This is what I got:

(gdb) frame 4
#4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
    errp=0xffffffffe540)
    at .../qemu/target/arm/cpu.c:1159
1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
(gdb) print /x cpu->isar
$1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
  id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
  mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
  id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
  id_aa64mmfr1 = 0x0}

Thanks!
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Philippe Mathieu-Daudé]

On 7/16/19 8:42 PM, Laszlo Ersek wrote:
> On 07/16/19 18:59, Peter Maydell wrote:
>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>>> The issue still reproduces, so it makes sense for me to look at the host
>>> kernel version... Well, I'm afraid it won't help much, for an upstream
>>> investigation:
>>>
>>>   4.14.0-115.8.2.el7a.aarch64
>>>
>>> This is the latest released kernel from "Red Hat Enterprise Linux for
>>> ARM 64 7".
>>
>> OK. (I'm using 4.15.0-51-generic from ubuntu).
>>
>> Could you run with QEMU under gdb, and when it hits the
>> assertion go back up a stack frame to the arm_cpu_realizefn()
>> frame, and then "print /x cpu->isar" ? That should show us
>> what we think we've got as ID registers from the kernel.
>> (You might need to build QEMU with --enable-debug to get
>> useful enough debug info to do that, not sure.)
> 
> (My qemu build script always builds QEMU in two configs, the difference
> being --prefix and --enable-debug.)
> 
> This is what I got:
> 
> (gdb) frame 4
> #4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
>     errp=0xffffffffe540)
>     at .../qemu/target/arm/cpu.c:1159
> 1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> (gdb) print /x cpu->isar
> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
>   id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
>   mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
>   id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
>   id_aa64mmfr1 = 0x0}

For ISAR0, DIVIDE=0

so cpu_isar_feature(arm_div, cpu)=false

For AA64PFR0, EL0=1, EL1=1.

EL0 = 1: EL0 can be executed in AArch64 state only.
EL1 = 1: EL1 can be executed in AArch64 state only.

so cpu_isar_feature(aa64_aa32, cpu)=false
then no_aa32=true

The commit description is "on a host that doesn't support aarch32 mode
at all, neither arm_div nor jazelle will be supported either."

Shouldn't we use a slighly different logic? Such:

-    assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
+    assert(no_aa32 && !cpu_isar_feature(arm_div, cpu));

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/16/19 22:10, Philippe Mathieu-Daudé wrote:
> On 7/16/19 8:42 PM, Laszlo Ersek wrote:
>> On 07/16/19 18:59, Peter Maydell wrote:
>>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> The issue still reproduces, so it makes sense for me to look at the host
>>>> kernel version... Well, I'm afraid it won't help much, for an upstream
>>>> investigation:
>>>>
>>>>   4.14.0-115.8.2.el7a.aarch64
>>>>
>>>> This is the latest released kernel from "Red Hat Enterprise Linux for
>>>> ARM 64 7".
>>>
>>> OK. (I'm using 4.15.0-51-generic from ubuntu).
>>>
>>> Could you run with QEMU under gdb, and when it hits the
>>> assertion go back up a stack frame to the arm_cpu_realizefn()
>>> frame, and then "print /x cpu->isar" ? That should show us
>>> what we think we've got as ID registers from the kernel.
>>> (You might need to build QEMU with --enable-debug to get
>>> useful enough debug info to do that, not sure.)
>>
>> (My qemu build script always builds QEMU in two configs, the difference
>> being --prefix and --enable-debug.)
>>
>> This is what I got:
>>
>> (gdb) frame 4
>> #4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
>>     errp=0xffffffffe540)
>>     at .../qemu/target/arm/cpu.c:1159
>> 1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>> (gdb) print /x cpu->isar
>> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
>>   id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
>>   mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
>>   id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
>>   id_aa64mmfr1 = 0x0}
> 
> For ISAR0, DIVIDE=0
> 
> so cpu_isar_feature(arm_div, cpu)=false
> 
> For AA64PFR0, EL0=1, EL1=1.
> 
> EL0 = 1: EL0 can be executed in AArch64 state only.
> EL1 = 1: EL1 can be executed in AArch64 state only.
> 
> so cpu_isar_feature(aa64_aa32, cpu)=false
> then no_aa32=true
> 
> The commit description is "on a host that doesn't support aarch32 mode
> at all, neither arm_div nor jazelle will be supported either."
> 
> Shouldn't we use a slighly different logic? Such:
> 
> -    assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> +    assert(no_aa32 && !cpu_isar_feature(arm_div, cpu));
> 

I'm unsure. The current formula seems to match the commit description.
Implication -- that is, "A implies B", (A-->B) -- is equivalent to (!A
|| B).

We have "no_aa32 || arm_div", which corresponds to "aa32 implies
arm_div" (aa32-->arm_div). And that seems to match exactly what Peter said.

The assert you suggest would fire on a host that supports at least one
of aa32 and arm_div (= the assertion would fail if (aa32 || arm_div)).
That would break on my host (hw+kernel) just the same, in the end. To
substitute the boolean values:

-    assert(false || false)
+    assert(false && true)

Thanks
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/17/19 10:36, Laszlo Ersek wrote:
> On 07/16/19 22:10, Philippe Mathieu-Daudé wrote:
>> On 7/16/19 8:42 PM, Laszlo Ersek wrote:
>>> On 07/16/19 18:59, Peter Maydell wrote:
>>>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com>
>>>> wrote:
>>>>> The issue still reproduces, so it makes sense for me to look at
>>>>> the host kernel version... Well, I'm afraid it won't help much,
>>>>> for an upstream investigation:
>>>>>
>>>>>   4.14.0-115.8.2.el7a.aarch64
>>>>>
>>>>> This is the latest released kernel from "Red Hat Enterprise Linux
>>>>> for ARM 64 7".
>>>>
>>>> OK. (I'm using 4.15.0-51-generic from ubuntu).
>>>>
>>>> Could you run with QEMU under gdb, and when it hits the
>>>> assertion go back up a stack frame to the arm_cpu_realizefn()
>>>> frame, and then "print /x cpu->isar" ? That should show us
>>>> what we think we've got as ID registers from the kernel.
>>>> (You might need to build QEMU with --enable-debug to get
>>>> useful enough debug info to do that, not sure.)
>>>
>>> (My qemu build script always builds QEMU in two configs, the
>>> difference being --prefix and --enable-debug.)
>>>
>>> This is what I got:
>>>
>>> (gdb) frame 4
>>> #4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
>>>     errp=0xffffffffe540)
>>>     at .../qemu/target/arm/cpu.c:1159
>>> 1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>> (gdb) print /x cpu->isar
>>> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
>>>   id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
>>>   mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
>>>   id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
>>>   id_aa64mmfr1 = 0x0}
>>
>> For ISAR0, DIVIDE=0
>>
>> so cpu_isar_feature(arm_div, cpu)=false
>>
>> For AA64PFR0, EL0=1, EL1=1.
>>
>> EL0 = 1: EL0 can be executed in AArch64 state only.
>> EL1 = 1: EL1 can be executed in AArch64 state only.
>>
>> so cpu_isar_feature(aa64_aa32, cpu)=false
>> then no_aa32=true
>>
>> The commit description is "on a host that doesn't support aarch32
>> mode at all, neither arm_div nor jazelle will be supported either."
>>
>> Shouldn't we use a slighly different logic? Such:
>>
>> -    assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>> +    assert(no_aa32 && !cpu_isar_feature(arm_div, cpu));
>>
>
> I'm unsure. The current formula seems to match the commit description.
> Implication -- that is, "A implies B", (A-->B) -- is equivalent to (!A
> || B).
>
> We have "no_aa32 || arm_div", which corresponds to "aa32 implies
> arm_div" (aa32-->arm_div). And that seems to match exactly what Peter
> said.
>
> The assert you suggest would fire on a host that supports at least one
> of aa32 and arm_div (= the assertion would fail if (aa32 || arm_div)).
> That would break on my host (hw+kernel) just the same, in the end. To
> substitute the boolean values:
>
> -    assert(false || false)
> +    assert(false && true)

Hmmm wait a second. The ARMv8 ARM says, about ID_ISAR0_EL1:

> Divide, bits [27:24]
>
>     Indicates the implemented Divide instructions. Permitted values
>     are:
>     0000 None implemented.
>     0001 Adds SDIV and UDIV in the T32 instruction set.
>     0010 As for 0b0001, and adds SDIV and UDIV in the A32 instruction
>          set.
>     All other values are reserved.

So this means that (aa32 && !arm_div) *does* conform to the architecture
manual! And then, I understand where the bug is.

As I wrote above, the current C expression stands for:

  aa32 --> arm_div

which -- we see from the ARMv8 ARM -- is wrong.

Upon re-reading the commit message more carefully:

    on a host that doesn't support aarch32 mode at all, neither arm_div
    nor jazelle will be supported either

it's clear that the intent was *not* the implication encoded in the
source. Instead, the intent was the *reverse* implication, namely:

  !aa32 --> !arm_div    [1]

Or, equivalently (because, (A --> B) === (!A --> !B)):

  arm_div --> aa32      [2]

Now, if you encode any one of these (equivalent) formulae in C, with the
logical OR operator, you get:

- Starting from [1]:

  (A     --> B)        === (!A   || B)
  (!aa32 --> !arm_div) === (aa32 || !arm_div) === (!no_aa32 || !arm_div)

- Starting from [2]:

  (A       --> B)    === (!A       || B)
  (arm_div --> aa32) === (!arm_div || aa32) === (!arm_div || !no_aa32)

You can see that, regardless of whether we start with [1], or
equivalently, [2], we end up with the exact same predicate, logically
speaking. The final expressions only differ in C with regard to the
order of evaluation / shortcut behavior. We can pick whichever we prefer
(for whatever other reason).

FWIW, the language of the original commit message corresponds to [1].
So, if we want to stick with that, then the patch we need is:

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index e75a64a25a4b..ea84a3e11abb 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1382,8 +1382,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           * include the various other features that V7VE implies.
>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>           * Security Extensions is ARM_FEATURE_EL3.
> +         *
> +         * Lack of aa32 support excludes arm_div support:
> +         *   no_aa32 --> !arm_div
> +         * Using the logical OR operator, the same is expressed as:
> +         *   !no_aa32 || !arm_div
>           */
> -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));
>          set_feature(env, ARM_FEATURE_LPAE);
>          set_feature(env, ARM_FEATURE_V7);
>      }

If you guys agree, I can formally submit this patch.

Thanks
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/17/19 11:22, Laszlo Ersek wrote:
> On 07/17/19 10:36, Laszlo Ersek wrote:
>> On 07/16/19 22:10, Philippe Mathieu-Daudé wrote:
>>> On 7/16/19 8:42 PM, Laszlo Ersek wrote:
>>>> On 07/16/19 18:59, Peter Maydell wrote:
>>>>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com>
>>>>> wrote:
>>>>>> The issue still reproduces, so it makes sense for me to look at
>>>>>> the host kernel version... Well, I'm afraid it won't help much,
>>>>>> for an upstream investigation:
>>>>>>
>>>>>>   4.14.0-115.8.2.el7a.aarch64
>>>>>>
>>>>>> This is the latest released kernel from "Red Hat Enterprise Linux
>>>>>> for ARM 64 7".
>>>>>
>>>>> OK. (I'm using 4.15.0-51-generic from ubuntu).
>>>>>
>>>>> Could you run with QEMU under gdb, and when it hits the
>>>>> assertion go back up a stack frame to the arm_cpu_realizefn()
>>>>> frame, and then "print /x cpu->isar" ? That should show us
>>>>> what we think we've got as ID registers from the kernel.
>>>>> (You might need to build QEMU with --enable-debug to get
>>>>> useful enough debug info to do that, not sure.)
>>>>
>>>> (My qemu build script always builds QEMU in two configs, the
>>>> difference being --prefix and --enable-debug.)
>>>>
>>>> This is what I got:
>>>>
>>>> (gdb) frame 4
>>>> #4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
>>>>     errp=0xffffffffe540)
>>>>     at .../qemu/target/arm/cpu.c:1159
>>>> 1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>>> (gdb) print /x cpu->isar
>>>> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
>>>>   id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
>>>>   mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
>>>>   id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
>>>>   id_aa64mmfr1 = 0x0}
>>>
>>> For ISAR0, DIVIDE=0
>>>
>>> so cpu_isar_feature(arm_div, cpu)=false
>>>
>>> For AA64PFR0, EL0=1, EL1=1.
>>>
>>> EL0 = 1: EL0 can be executed in AArch64 state only.
>>> EL1 = 1: EL1 can be executed in AArch64 state only.
>>>
>>> so cpu_isar_feature(aa64_aa32, cpu)=false
>>> then no_aa32=true
>>>
>>> The commit description is "on a host that doesn't support aarch32
>>> mode at all, neither arm_div nor jazelle will be supported either."
>>>
>>> Shouldn't we use a slighly different logic? Such:
>>>
>>> -    assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>> +    assert(no_aa32 && !cpu_isar_feature(arm_div, cpu));
>>>
>>
>> I'm unsure. The current formula seems to match the commit description.
>> Implication -- that is, "A implies B", (A-->B) -- is equivalent to (!A
>> || B).
>>
>> We have "no_aa32 || arm_div", which corresponds to "aa32 implies
>> arm_div" (aa32-->arm_div). And that seems to match exactly what Peter
>> said.
>>
>> The assert you suggest would fire on a host that supports at least one
>> of aa32 and arm_div (= the assertion would fail if (aa32 || arm_div)).
>> That would break on my host (hw+kernel) just the same, in the end. To
>> substitute the boolean values:
>>
>> -    assert(false || false)
>> +    assert(false && true)
> 
> Hmmm wait a second. The ARMv8 ARM says, about ID_ISAR0_EL1:
> 
>> Divide, bits [27:24]
>>
>>     Indicates the implemented Divide instructions. Permitted values
>>     are:
>>     0000 None implemented.
>>     0001 Adds SDIV and UDIV in the T32 instruction set.
>>     0010 As for 0b0001, and adds SDIV and UDIV in the A32 instruction
>>          set.
>>     All other values are reserved.
> 
> So this means that (aa32 && !arm_div) *does* conform to the architecture
> manual! And then, I understand where the bug is.
> 
> As I wrote above, the current C expression stands for:
> 
>   aa32 --> arm_div
> 
> which -- we see from the ARMv8 ARM -- is wrong.
> 
> Upon re-reading the commit message more carefully:
> 
>     on a host that doesn't support aarch32 mode at all, neither arm_div
>     nor jazelle will be supported either
> 
> it's clear that the intent was *not* the implication encoded in the
> source. Instead, the intent was the *reverse* implication, namely:
> 
>   !aa32 --> !arm_div    [1]
> 
> Or, equivalently (because, (A --> B) === (!A --> !B)):
> 
>   arm_div --> aa32      [2]
> 
> Now, if you encode any one of these (equivalent) formulae in C, with the
> logical OR operator, you get:
> 
> - Starting from [1]:
> 
>   (A     --> B)        === (!A   || B)
>   (!aa32 --> !arm_div) === (aa32 || !arm_div) === (!no_aa32 || !arm_div)
> 
> - Starting from [2]:
> 
>   (A       --> B)    === (!A       || B)
>   (arm_div --> aa32) === (!arm_div || aa32) === (!arm_div || !no_aa32)
> 
> You can see that, regardless of whether we start with [1], or
> equivalently, [2], we end up with the exact same predicate, logically
> speaking. The final expressions only differ in C with regard to the
> order of evaluation / shortcut behavior. We can pick whichever we prefer
> (for whatever other reason).
> 
> FWIW, the language of the original commit message corresponds to [1].
> So, if we want to stick with that, then the patch we need is:
> 
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index e75a64a25a4b..ea84a3e11abb 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1382,8 +1382,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           * include the various other features that V7VE implies.
>>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>           * Security Extensions is ARM_FEATURE_EL3.
>> +         *
>> +         * Lack of aa32 support excludes arm_div support:
>> +         *   no_aa32 --> !arm_div
>> +         * Using the logical OR operator, the same is expressed as:
>> +         *   !no_aa32 || !arm_div
>>           */
>> -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>> +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));
>>          set_feature(env, ARM_FEATURE_LPAE);
>>          set_feature(env, ARM_FEATURE_V7);
>>      }
> 
> If you guys agree, I can formally submit this patch.

NB: the same might apply to the "jazelle" feature; I didn't check.

Thanks
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/17/19 11:22, Laszlo Ersek wrote:

> because, (A --> B) === (!A --> !B)

Obviously, it is impossible for me to write an email containing logical
formulae without at least one *crucial* typo.

The correct form of the above equivalence is:

  (A --> B) === (!B --> !A)

This typo does not affect the rest of my previous message -- while I
quoted the equivalence incorrectly, I did use it correctly.

Thanks & sorry
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/17/19 11:22, Laszlo Ersek wrote:
> On 07/17/19 10:36, Laszlo Ersek wrote:

>> -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>> +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));

BTW this can be pronounced in natural language as follows: "we either
have aa32 support, or if we don't, then we lack arm_div support too".

Thanks
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Philippe Mathieu-Daudé]

On 7/17/19 11:22 AM, Laszlo Ersek wrote:
> On 07/17/19 10:36, Laszlo Ersek wrote:
>> On 07/16/19 22:10, Philippe Mathieu-Daudé wrote:
>>> On 7/16/19 8:42 PM, Laszlo Ersek wrote:
>>>> On 07/16/19 18:59, Peter Maydell wrote:
>>>>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com>
>>>>> wrote:
>>>>>> The issue still reproduces, so it makes sense for me to look at
>>>>>> the host kernel version... Well, I'm afraid it won't help much,
>>>>>> for an upstream investigation:
>>>>>>
>>>>>>   4.14.0-115.8.2.el7a.aarch64
>>>>>>
>>>>>> This is the latest released kernel from "Red Hat Enterprise Linux
>>>>>> for ARM 64 7".
>>>>>
>>>>> OK. (I'm using 4.15.0-51-generic from ubuntu).
>>>>>
>>>>> Could you run with QEMU under gdb, and when it hits the
>>>>> assertion go back up a stack frame to the arm_cpu_realizefn()
>>>>> frame, and then "print /x cpu->isar" ? That should show us
>>>>> what we think we've got as ID registers from the kernel.
>>>>> (You might need to build QEMU with --enable-debug to get
>>>>> useful enough debug info to do that, not sure.)
>>>>
>>>> (My qemu build script always builds QEMU in two configs, the
>>>> difference being --prefix and --enable-debug.)
>>>>
>>>> This is what I got:
>>>>
>>>> (gdb) frame 4
>>>> #4  0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140,
>>>>     errp=0xffffffffe540)
>>>>     at .../qemu/target/arm/cpu.c:1159
>>>> 1159            assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>>> (gdb) print /x cpu->isar
>>>> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0,
>>>>   id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0,
>>>>   mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0,
>>>>   id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0,
>>>>   id_aa64mmfr1 = 0x0}
>>>
>>> For ISAR0, DIVIDE=0
>>>
>>> so cpu_isar_feature(arm_div, cpu)=false
>>>
>>> For AA64PFR0, EL0=1, EL1=1.
>>>
>>> EL0 = 1: EL0 can be executed in AArch64 state only.
>>> EL1 = 1: EL1 can be executed in AArch64 state only.
>>>
>>> so cpu_isar_feature(aa64_aa32, cpu)=false
>>> then no_aa32=true
>>>
>>> The commit description is "on a host that doesn't support aarch32
>>> mode at all, neither arm_div nor jazelle will be supported either."
>>>
>>> Shouldn't we use a slighly different logic? Such:
>>>
>>> -    assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>>> +    assert(no_aa32 && !cpu_isar_feature(arm_div, cpu));
>>>
>>
>> I'm unsure. The current formula seems to match the commit description.
>> Implication -- that is, "A implies B", (A-->B) -- is equivalent to (!A
>> || B).
>>
>> We have "no_aa32 || arm_div", which corresponds to "aa32 implies
>> arm_div" (aa32-->arm_div). And that seems to match exactly what Peter
>> said.
>>
>> The assert you suggest would fire on a host that supports at least one
>> of aa32 and arm_div (= the assertion would fail if (aa32 || arm_div)).
>> That would break on my host (hw+kernel) just the same, in the end. To
>> substitute the boolean values:
>>
>> -    assert(false || false)
>> +    assert(false && true)
> 
> Hmmm wait a second. The ARMv8 ARM says, about ID_ISAR0_EL1:
> 
>> Divide, bits [27:24]
>>
>>     Indicates the implemented Divide instructions. Permitted values
>>     are:
>>     0000 None implemented.
>>     0001 Adds SDIV and UDIV in the T32 instruction set.
>>     0010 As for 0b0001, and adds SDIV and UDIV in the A32 instruction
>>          set.
>>     All other values are reserved.
> 
> So this means that (aa32 && !arm_div) *does* conform to the architecture
> manual! And then, I understand where the bug is.
> 
> As I wrote above, the current C expression stands for:
> 
>   aa32 --> arm_div
> 
> which -- we see from the ARMv8 ARM -- is wrong.
> 
> Upon re-reading the commit message more carefully:
> 
>     on a host that doesn't support aarch32 mode at all, neither arm_div
>     nor jazelle will be supported either
> 
> it's clear that the intent was *not* the implication encoded in the
> source. Instead, the intent was the *reverse* implication, namely:
> 
>   !aa32 --> !arm_div    [1]
> 
> Or, equivalently (because, (A --> B) === (!A --> !B)):

[Laszlo corrected this as:   (A --> B) === (!B --> !A)]

>   arm_div --> aa32      [2]
> 
> Now, if you encode any one of these (equivalent) formulae in C, with the
> logical OR operator, you get:
> 
> - Starting from [1]:
> 
>   (A     --> B)        === (!A   || B)
>   (!aa32 --> !arm_div) === (aa32 || !arm_div) === (!no_aa32 || !arm_div)
> 
> - Starting from [2]:
> 
>   (A       --> B)    === (!A       || B)
>   (arm_div --> aa32) === (!arm_div || aa32) === (!arm_div || !no_aa32)
> 
> You can see that, regardless of whether we start with [1], or
> equivalently, [2], we end up with the exact same predicate, logically
> speaking. The final expressions only differ in C with regard to the
> order of evaluation / shortcut behavior. We can pick whichever we prefer
> (for whatever other reason).

This makes sense.

I still wonder why this didn't assert on Peter's setup.

> 
> FWIW, the language of the original commit message corresponds to [1].
> So, if we want to stick with that, then the patch we need is:
> 
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index e75a64a25a4b..ea84a3e11abb 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1382,8 +1382,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           * include the various other features that V7VE implies.
>>           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>           * Security Extensions is ARM_FEATURE_EL3.
>> +         *
>> +         * Lack of aa32 support excludes arm_div support:
>> +         *   no_aa32 --> !arm_div
>> +         * Using the logical OR operator, the same is expressed as:
>> +         *   !no_aa32 || !arm_div
>>           */
>> -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
>> +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));
>>          set_feature(env, ARM_FEATURE_LPAE);
>>          set_feature(env, ARM_FEATURE_V7);
>>      }
> 
> If you guys agree, I can formally submit this patch.

Worthwhile, so it could get in for the next release.

Regards,

Phil.

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Wed, 17 Jul 2019 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> I still wonder why this didn't assert on Peter's setup.

My setup does not assert because my host kernel correctly
provides the ID register values to QEMU. Laszlo's appears
to be providing all-zeroes, which then obviously breaks
assertions being made about the sanity of those ID register
values...

thanks
-- PMM

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/17/19 15:46, Peter Maydell wrote:
> On Wed, 17 Jul 2019 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> I still wonder why this didn't assert on Peter's setup.
> 
> My setup does not assert because my host kernel correctly
> provides the ID register values to QEMU. Laszlo's appears
> to be providing all-zeroes, which then obviously breaks
> assertions being made about the sanity of those ID register
> values...

OK. Can you suggest a location that I should check in the host kernel?

Thanks!
Laszlo

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Wed, 17 Jul 2019 at 16:08, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 07/17/19 15:46, Peter Maydell wrote:
> > On Wed, 17 Jul 2019 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >> I still wonder why this didn't assert on Peter's setup.
> >
> > My setup does not assert because my host kernel correctly
> > provides the ID register values to QEMU. Laszlo's appears
> > to be providing all-zeroes, which then obviously breaks
> > assertions being made about the sanity of those ID register
> > values...
>
> OK. Can you suggest a location that I should check in the host kernel?

I was about to write out the process of how we get these values
from the kernel, but as the first step of that I read through
QEMU's target/arm/kvm64.c:kvm_arm_get_host_cpu_features(),
which is the function which reads these values using the
KVM_GET_ONE_REG ioctl. It starts with an attempt to read
ID_AA64PFR0, and has a comment for the error-handling case:

        /*
         * Before v4.15, the kernel only exposed a limited number of system
         * registers, not including any of the interesting AArch64 ID regs.
         * For the most part we could leave these fields as zero with minimal
         * effect, since this does not affect the values seen by the guest.
         *
         * However, it could cause problems down the line for QEMU,
         * so provide a minimal v8.0 default.
         *
         * ??? Could read MIDR and use knowledge from cpu64.c.
         * ??? Could map a page of memory into our temp guest and
         *     run the tiniest of hand-crafted kernels to extract
         *     the values seen by the guest.
         * ??? Either of these sounds like too much effort just
         *     to work around running a modern host kernel.
         */

I have 4.15, and don't hit this assert; you have 4.14 and do,
so I think you're going to be going through this codepath which
currently sets only ahcf->isar.id_aa64pfr0 and none of the other
ID register fields in the isar struct.

I'm not sure exactly which kernel commits added the ID register
reading support. (The relevant kernel code is in
arch/arm64/kvm/sys_regs.c I think.)

Anyway, I think we need to do at least one of:
 * enhance the "provide a minimal v8.0 default" code in this
   condition in kvm_arm_get_host_cpu_features() so that it
   populates the ID registers sufficiently to avoid asserts
   and other bad things
 * make the asserts on ID register oddnesses be only for TCG
   (ie where QEMU controls the values) and not for KVM

thanks
-- PMM

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Laszlo Ersek]

On 07/18/19 14:30, Peter Maydell wrote:
> On Wed, 17 Jul 2019 at 16:08, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 07/17/19 15:46, Peter Maydell wrote:
>>> On Wed, 17 Jul 2019 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>> I still wonder why this didn't assert on Peter's setup.
>>>
>>> My setup does not assert because my host kernel correctly
>>> provides the ID register values to QEMU. Laszlo's appears
>>> to be providing all-zeroes, which then obviously breaks
>>> assertions being made about the sanity of those ID register
>>> values...
>>
>> OK. Can you suggest a location that I should check in the host kernel?
> 
> I was about to write out the process of how we get these values
> from the kernel, but as the first step of that I read through
> QEMU's target/arm/kvm64.c:kvm_arm_get_host_cpu_features(),
> which is the function which reads these values using the
> KVM_GET_ONE_REG ioctl. It starts with an attempt to read
> ID_AA64PFR0, and has a comment for the error-handling case:
> 
>         /*
>          * Before v4.15, the kernel only exposed a limited number of system
>          * registers, not including any of the interesting AArch64 ID regs.
>          * For the most part we could leave these fields as zero with minimal
>          * effect, since this does not affect the values seen by the guest.
>          *
>          * However, it could cause problems down the line for QEMU,
>          * so provide a minimal v8.0 default.
>          *
>          * ??? Could read MIDR and use knowledge from cpu64.c.
>          * ??? Could map a page of memory into our temp guest and
>          *     run the tiniest of hand-crafted kernels to extract
>          *     the values seen by the guest.
>          * ??? Either of these sounds like too much effort just
>          *     to work around running a modern host kernel.
>          */
> 
> I have 4.15, and don't hit this assert; you have 4.14 and do,
> so I think you're going to be going through this codepath which
> currently sets only ahcf->isar.id_aa64pfr0 and none of the other
> ID register fields in the isar struct.
> 
> I'm not sure exactly which kernel commits added the ID register
> reading support. (The relevant kernel code is in
> arch/arm64/kvm/sys_regs.c I think.)

I compared that file between the downstream kernel source and upstream v4.15, and it looks like the following series (indeed released as part of v4.15) is what's missing down-stream, for this particular use case:

     1  27e64b4be4b8 regset: Add support for dynamically sized regsets
     2  94ef7ecbdf6f arm64: fpsimd: Correctly annotate exception helpers called from asm
     3  abf73988a7c2 arm64: signal: Verify extra data is user-readable in sys_rt_sigreturn
     4  93390c0a1b20 arm64: KVM: Hide unsupported AArch64 CPU features from guests
     5  b472db6cf8c6 arm64: efi: Add missing Kconfig dependency on KERNEL_MODE_NEON
     6  38b9aeb32fa7 arm64: Port deprecated instruction emulation to new sysctl interface
     7  9cf5b54fafed arm64: fpsimd: Simplify uses of {set,clear}_ti_thread_flag()
     8  672365649cca arm64/sve: System register and exception syndrome definitions
     9  1fc5dce78ad1 arm64/sve: Low-level SVE architectural state manipulation functions
    10  ddd25ad1fde8 arm64/sve: Kconfig update and conditional compilation support
    11  d0b8cd318788 arm64/sve: Signal frame and context structure definition
    12  22043a3c082a arm64/sve: Low-level CPU setup
    13  bc0ee4760364 arm64/sve: Core task context handling
    14  79ab047c75d6 arm64/sve: Support vector length resetting for new processes
    15  8cd969d28fd2 arm64/sve: Signal handling support
    16  7582e22038a2 arm64/sve: Backend logic for setting the vector length
    17  8f1eec57cdcc arm64: cpufeature: Move sys_caps_initialised declarations
    18  2e0f2478ea37 arm64/sve: Probe SVE capabilities and usable vector lengths
    19  1bd3f93641ec arm64/sve: Preserve SVE registers around kernel-mode NEON use
    20  fdfa976cae5c arm64/sve: Preserve SVE registers around EFI runtime service calls
    21  43d4da2c45b2 arm64/sve: ptrace and ELF coredump support
    22  2d2123bc7c7f arm64/sve: Add prctl controls for userspace vector length management
    23  4ffa09a939ab arm64/sve: Add sysctl to set the default vector length for new processes
    24  17eed27b02da arm64/sve: KVM: Prevent guests from using SVE
    25  aac45ffd1f8e arm64/sve: KVM: Treat guest SVE use as undefined instruction execution
    26  07d79fe7c223 arm64/sve: KVM: Hide SVE from CPU features exposed to guests
    27  43994d824e84 arm64/sve: Detect SVE and activate runtime support
    28  ce6990813f15 arm64/sve: Add documentation

The differences found by the simple "diff" that I mention above are mainly due to commit #4 (93390c0a1b20, "arm64: KVM: Hide unsupported AArch64 CPU features from guests", 2017-11-03).

I found a (likely non-final) version of the cover letter too, here: https://www.spinics.net/lists/arm-kernel/msg599528.html

I guess I should convince myself to install RHEL8 on the Mustang sometime...

Thanks!
Laszlo

> Anyway, I think we need to do at least one of:
>  * enhance the "provide a minimal v8.0 default" code in this
>    condition in kvm_arm_get_host_cpu_features() so that it
>    populates the ID registers sufficiently to avoid asserts
>    and other bad things
>  * make the asserts on ID register oddnesses be only for TCG
>    (ie where QEMU controls the values) and not for KVM
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Wed, 17 Jul 2019 at 10:22, Laszlo Ersek <lersek@redhat.com> wrote:
> Hmmm wait a second. The ARMv8 ARM says, about ID_ISAR0_EL1:
>
> > Divide, bits [27:24]
> >
> >     Indicates the implemented Divide instructions. Permitted values
> >     are:
> >     0000 None implemented.
> >     0001 Adds SDIV and UDIV in the T32 instruction set.
> >     0010 As for 0b0001, and adds SDIV and UDIV in the A32 instruction
> >          set.
> >     All other values are reserved.
>
> So this means that (aa32 && !arm_div) *does* conform to the architecture
> manual!

The architecture manual also says
"In ARMv8-A the only permitted value is 0010.", and in
earlier versions of the manual it says also that ARMv7VE
implies that we must have all the integer divide instructions.
And this assert is guarded by "if (arm_feature(env, ARM_FEATURE_V7VE))".
(This is what we're trying to test, really: something that's
providing AArch32 v7VE-or-better must include the divide insns.)

> Upon re-reading the commit message more carefully:
>
>     on a host that doesn't support aarch32 mode at all, neither arm_div
>     nor jazelle will be supported either

The point of this text is that "v8 AArch64 with no AArch32" is
an exception to the previous version of the check, which was
just "v7VE must mean we have the divide insns".

Similarly with Jazelle: what we were testing before commit 0f8d06f
was "v6 must imply Jazelle", which is true for everything except
the special case of "an AArch64 CPU with no AArch32 support" (which
can only happen for a KVM setup), and the test we are trying to
check after the commit is "v6 aarch32 must imply Jazelle".

> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index e75a64a25a4b..ea84a3e11abb 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1382,8 +1382,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >           * include the various other features that V7VE implies.
> >           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
> >           * Security Extensions is ARM_FEATURE_EL3.
> > +         *
> > +         * Lack of aa32 support excludes arm_div support:
> > +         *   no_aa32 --> !arm_div
> > +         * Using the logical OR operator, the same is expressed as:
> > +         *   !no_aa32 || !arm_div
> >           */
> > -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> > +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));

This now fails to test what we wanted, because it will allow
an incorrect set of ID registers that define a v7VE CPU
without the arm_div feature to pass.

thanks
-- PMM

[Qemu-devel] [PULL 09/10] hw/arm: versal: Add a model of Xilinx Versal SoC

[Peter Maydell]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add a model of Xilinx Versal SoC.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Message-id: 20181102131913.1535-2-edgar.iglesias@xilinx.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/Makefile.objs                |   1 +
 include/hw/arm/xlnx-versal.h        | 122 +++++++++++
 hw/arm/xlnx-versal.c                | 323 ++++++++++++++++++++++++++++
 default-configs/aarch64-softmmu.mak |   1 +
 4 files changed, 447 insertions(+)
 create mode 100644 include/hw/arm/xlnx-versal.h
 create mode 100644 hw/arm/xlnx-versal.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 5f88062c666..ec21d9bc1f0 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -26,6 +26,7 @@ obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
 obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
 obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
 obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
+obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o
 obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
 obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
 obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
new file mode 100644
index 00000000000..9da621e4b68
--- /dev/null
+++ b/include/hw/arm/xlnx-versal.h
@@ -0,0 +1,122 @@
+/*
+ * Model of the Xilinx Versal
+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ * Written by Edgar E. Iglesias
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#ifndef XLNX_VERSAL_H
+#define XLNX_VERSAL_H
+
+#include "hw/sysbus.h"
+#include "hw/arm/arm.h"
+#include "hw/intc/arm_gicv3.h"
+
+#define TYPE_XLNX_VERSAL "xlnx-versal"
+#define XLNX_VERSAL(obj) OBJECT_CHECK(Versal, (obj), TYPE_XLNX_VERSAL)
+
+#define XLNX_VERSAL_NR_ACPUS   2
+#define XLNX_VERSAL_NR_UARTS   2
+#define XLNX_VERSAL_NR_GEMS    2
+#define XLNX_VERSAL_NR_IRQS    256
+
+typedef struct Versal {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    struct {
+        struct {
+            MemoryRegion mr;
+            ARMCPU *cpu[XLNX_VERSAL_NR_ACPUS];
+            GICv3State gic;
+        } apu;
+    } fpd;
+
+    MemoryRegion mr_ps;
+
+    struct {
+        /* 4 ranges to access DDR.  */
+        MemoryRegion mr_ddr_ranges[4];
+    } noc;
+
+    struct {
+        MemoryRegion mr_ocm;
+
+        struct {
+            SysBusDevice *uart[XLNX_VERSAL_NR_UARTS];
+            SysBusDevice *gem[XLNX_VERSAL_NR_GEMS];
+        } iou;
+    } lpd;
+
+    struct {
+        MemoryRegion *mr_ddr;
+        uint32_t psci_conduit;
+    } cfg;
+} Versal;
+
+/* Memory-map and IRQ definitions. Copied a subset from
+ * auto-generated files.  */
+
+#define VERSAL_GIC_MAINT_IRQ        9
+#define VERSAL_TIMER_VIRT_IRQ       11
+#define VERSAL_TIMER_S_EL1_IRQ      13
+#define VERSAL_TIMER_NS_EL1_IRQ     14
+#define VERSAL_TIMER_NS_EL2_IRQ     10
+
+#define VERSAL_UART0_IRQ_0         18
+#define VERSAL_UART1_IRQ_0         19
+#define VERSAL_GEM0_IRQ_0          56
+#define VERSAL_GEM0_WAKE_IRQ_0     57
+#define VERSAL_GEM1_IRQ_0          58
+#define VERSAL_GEM1_WAKE_IRQ_0     59
+
+/* Architecturally eserved IRQs suitable for virtualization.  */
+#define VERSAL_RSVD_HIGH_IRQ_FIRST 160
+#define VERSAL_RSVD_HIGH_IRQ_LAST  255
+
+#define MM_TOP_RSVD                 0xa0000000U
+#define MM_TOP_RSVD_SIZE            0x4000000
+#define MM_GIC_APU_DIST_MAIN        0xf9000000U
+#define MM_GIC_APU_DIST_MAIN_SIZE   0x10000
+#define MM_GIC_APU_REDIST_0         0xf9080000U
+#define MM_GIC_APU_REDIST_0_SIZE    0x80000
+
+#define MM_UART0                    0xff000000U
+#define MM_UART0_SIZE               0x10000
+#define MM_UART1                    0xff010000U
+#define MM_UART1_SIZE               0x10000
+
+#define MM_GEM0                     0xff0c0000U
+#define MM_GEM0_SIZE                0x10000
+#define MM_GEM1                     0xff0d0000U
+#define MM_GEM1_SIZE                0x10000
+
+#define MM_OCM                      0xfffc0000U
+#define MM_OCM_SIZE                 0x40000
+
+#define MM_TOP_DDR                  0x0
+#define MM_TOP_DDR_SIZE             0x80000000U
+#define MM_TOP_DDR_2                0x800000000ULL
+#define MM_TOP_DDR_2_SIZE           0x800000000ULL
+#define MM_TOP_DDR_3                0xc000000000ULL
+#define MM_TOP_DDR_3_SIZE           0x4000000000ULL
+#define MM_TOP_DDR_4                0x10000000000ULL
+#define MM_TOP_DDR_4_SIZE           0xb780000000ULL
+
+#define MM_PSM_START                0xffc80000U
+#define MM_PSM_END                  0xffcf0000U
+
+#define MM_CRL                      0xff5e0000U
+#define MM_CRL_SIZE                 0x300000
+#define MM_IOU_SCNTR                0xff130000U
+#define MM_IOU_SCNTR_SIZE           0x10000
+#define MM_IOU_SCNTRS               0xff140000U
+#define MM_IOU_SCNTRS_SIZE          0x10000
+#define MM_FPD_CRF                  0xfd1a0000U
+#define MM_FPD_CRF_SIZE             0x140000
+#endif
diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
new file mode 100644
index 00000000000..5ee58c09be8
--- /dev/null
+++ b/hw/arm/xlnx-versal.c
@@ -0,0 +1,323 @@
+/*
+ * Xilinx Versal SoC model.
+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ * Written by Edgar E. Iglesias
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "qemu/log.h"
+#include "hw/sysbus.h"
+#include "net/net.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/kvm.h"
+#include "hw/arm/arm.h"
+#include "kvm_arm.h"
+#include "hw/misc/unimp.h"
+#include "hw/intc/arm_gicv3_common.h"
+#include "hw/arm/xlnx-versal.h"
+
+#define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
+#define GEM_REVISION        0x40070106
+
+static void versal_create_apu_cpus(Versal *s)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->fpd.apu.cpu); i++) {
+        Object *obj;
+        char *name;
+
+        obj = object_new(XLNX_VERSAL_ACPU_TYPE);
+        if (!obj) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            error_report("Unable to create apu.cpu[%d] of type %s",
+                         i, XLNX_VERSAL_ACPU_TYPE);
+            exit(EXIT_FAILURE);
+        }
+
+        name = g_strdup_printf("apu-cpu[%d]", i);
+        object_property_add_child(OBJECT(s), name, obj, &error_fatal);
+        g_free(name);
+
+        object_property_set_int(obj, s->cfg.psci_conduit,
+                                "psci-conduit", &error_abort);
+        if (i) {
+            object_property_set_bool(obj, true,
+                                     "start-powered-off", &error_abort);
+        }
+
+        object_property_set_int(obj, ARRAY_SIZE(s->fpd.apu.cpu),
+                                "core-count", &error_abort);
+        object_property_set_link(obj, OBJECT(&s->fpd.apu.mr), "memory",
+                                 &error_abort);
+        object_property_set_bool(obj, true, "realized", &error_fatal);
+        s->fpd.apu.cpu[i] = ARM_CPU(obj);
+    }
+}
+
+static void versal_create_apu_gic(Versal *s, qemu_irq *pic)
+{
+    static const uint64_t addrs[] = {
+        MM_GIC_APU_DIST_MAIN,
+        MM_GIC_APU_REDIST_0
+    };
+    SysBusDevice *gicbusdev;
+    DeviceState *gicdev;
+    int nr_apu_cpus = ARRAY_SIZE(s->fpd.apu.cpu);
+    int i;
+
+    sysbus_init_child_obj(OBJECT(s), "apu-gic",
+                          &s->fpd.apu.gic, sizeof(s->fpd.apu.gic),
+                          gicv3_class_name());
+    gicbusdev = SYS_BUS_DEVICE(&s->fpd.apu.gic);
+    gicdev = DEVICE(&s->fpd.apu.gic);
+    qdev_prop_set_uint32(gicdev, "revision", 3);
+    qdev_prop_set_uint32(gicdev, "num-cpu", 2);
+    qdev_prop_set_uint32(gicdev, "num-irq", XLNX_VERSAL_NR_IRQS + 32);
+    qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
+    qdev_prop_set_uint32(gicdev, "redist-region-count[0]", 2);
+    qdev_prop_set_bit(gicdev, "has-security-extensions", true);
+
+    object_property_set_bool(OBJECT(&s->fpd.apu.gic), true, "realized",
+                                    &error_fatal);
+
+    for (i = 0; i < ARRAY_SIZE(addrs); i++) {
+        MemoryRegion *mr;
+
+        mr = sysbus_mmio_get_region(gicbusdev, i);
+        memory_region_add_subregion(&s->fpd.apu.mr, addrs[i], mr);
+    }
+
+    for (i = 0; i < nr_apu_cpus; i++) {
+        DeviceState *cpudev = DEVICE(s->fpd.apu.cpu[i]);
+        int ppibase = XLNX_VERSAL_NR_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS;
+        qemu_irq maint_irq;
+        int ti;
+        /* Mapping from the output timer irq lines from the CPU to the
+         * GIC PPI inputs.
+         */
+        const int timer_irq[] = {
+            [GTIMER_PHYS] = VERSAL_TIMER_NS_EL1_IRQ,
+            [GTIMER_VIRT] = VERSAL_TIMER_VIRT_IRQ,
+            [GTIMER_HYP]  = VERSAL_TIMER_NS_EL2_IRQ,
+            [GTIMER_SEC]  = VERSAL_TIMER_S_EL1_IRQ,
+        };
+
+        for (ti = 0; ti < ARRAY_SIZE(timer_irq); ti++) {
+            qdev_connect_gpio_out(cpudev, ti,
+                                  qdev_get_gpio_in(gicdev,
+                                                   ppibase + timer_irq[ti]));
+        }
+        maint_irq = qdev_get_gpio_in(gicdev,
+                                        ppibase + VERSAL_GIC_MAINT_IRQ);
+        qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt",
+                                    0, maint_irq);
+        sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+        sysbus_connect_irq(gicbusdev, i + nr_apu_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
+        sysbus_connect_irq(gicbusdev, i + 2 * nr_apu_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
+        sysbus_connect_irq(gicbusdev, i + 3 * nr_apu_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
+    }
+
+    for (i = 0; i < XLNX_VERSAL_NR_IRQS; i++) {
+        pic[i] = qdev_get_gpio_in(gicdev, i);
+    }
+}
+
+static void versal_create_uarts(Versal *s, qemu_irq *pic)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->lpd.iou.uart); i++) {
+        static const int irqs[] = { VERSAL_UART0_IRQ_0, VERSAL_UART1_IRQ_0};
+        static const uint64_t addrs[] = { MM_UART0, MM_UART1 };
+        char *name = g_strdup_printf("uart%d", i);
+        DeviceState *dev;
+        MemoryRegion *mr;
+
+        dev = qdev_create(NULL, "pl011");
+        s->lpd.iou.uart[i] = SYS_BUS_DEVICE(dev);
+        qdev_prop_set_chr(dev, "chardev", serial_hd(i));
+        object_property_add_child(OBJECT(s), name, OBJECT(dev), &error_fatal);
+        qdev_init_nofail(dev);
+
+        mr = sysbus_mmio_get_region(s->lpd.iou.uart[i], 0);
+        memory_region_add_subregion(&s->mr_ps, addrs[i], mr);
+
+        sysbus_connect_irq(s->lpd.iou.uart[i], 0, pic[irqs[i]]);
+        g_free(name);
+    }
+}
+
+static void versal_create_gems(Versal *s, qemu_irq *pic)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->lpd.iou.gem); i++) {
+        static const int irqs[] = { VERSAL_GEM0_IRQ_0, VERSAL_GEM1_IRQ_0};
+        static const uint64_t addrs[] = { MM_GEM0, MM_GEM1 };
+        char *name = g_strdup_printf("gem%d", i);
+        NICInfo *nd = &nd_table[i];
+        DeviceState *dev;
+        MemoryRegion *mr;
+
+        dev = qdev_create(NULL, "cadence_gem");
+        s->lpd.iou.gem[i] = SYS_BUS_DEVICE(dev);
+        object_property_add_child(OBJECT(s), name, OBJECT(dev), &error_fatal);
+        if (nd->used) {
+            qemu_check_nic_model(nd, "cadence_gem");
+            qdev_set_nic_properties(dev, nd);
+        }
+        object_property_set_int(OBJECT(s->lpd.iou.gem[i]),
+                                2, "num-priority-queues",
+                                &error_abort);
+        object_property_set_link(OBJECT(s->lpd.iou.gem[i]),
+                                 OBJECT(&s->mr_ps), "dma",
+                                 &error_abort);
+        qdev_init_nofail(dev);
+
+        mr = sysbus_mmio_get_region(s->lpd.iou.gem[i], 0);
+        memory_region_add_subregion(&s->mr_ps, addrs[i], mr);
+
+        sysbus_connect_irq(s->lpd.iou.gem[i], 0, pic[irqs[i]]);
+        g_free(name);
+    }
+}
+
+/* This takes the board allocated linear DDR memory and creates aliases
+ * for each split DDR range/aperture on the Versal address map.
+ */
+static void versal_map_ddr(Versal *s)
+{
+    uint64_t size = memory_region_size(s->cfg.mr_ddr);
+    /* Describes the various split DDR access regions.  */
+    static const struct {
+        uint64_t base;
+        uint64_t size;
+    } addr_ranges[] = {
+        { MM_TOP_DDR, MM_TOP_DDR_SIZE },
+        { MM_TOP_DDR_2, MM_TOP_DDR_2_SIZE },
+        { MM_TOP_DDR_3, MM_TOP_DDR_3_SIZE },
+        { MM_TOP_DDR_4, MM_TOP_DDR_4_SIZE }
+    };
+    uint64_t offset = 0;
+    int i;
+
+    assert(ARRAY_SIZE(addr_ranges) == ARRAY_SIZE(s->noc.mr_ddr_ranges));
+    for (i = 0; i < ARRAY_SIZE(addr_ranges) && size; i++) {
+        char *name;
+        uint64_t mapsize;
+
+        mapsize = size < addr_ranges[i].size ? size : addr_ranges[i].size;
+        name = g_strdup_printf("noc-ddr-range%d", i);
+        /* Create the MR alias.  */
+        memory_region_init_alias(&s->noc.mr_ddr_ranges[i], OBJECT(s),
+                                 name, s->cfg.mr_ddr,
+                                 offset, mapsize);
+
+        /* Map it onto the NoC MR.  */
+        memory_region_add_subregion(&s->mr_ps, addr_ranges[i].base,
+                                    &s->noc.mr_ddr_ranges[i]);
+        offset += mapsize;
+        size -= mapsize;
+        g_free(name);
+    }
+}
+
+static void versal_unimp_area(Versal *s, const char *name,
+                                MemoryRegion *mr,
+                                hwaddr base, hwaddr size)
+{
+    DeviceState *dev = qdev_create(NULL, TYPE_UNIMPLEMENTED_DEVICE);
+    MemoryRegion *mr_dev;
+
+    qdev_prop_set_string(dev, "name", name);
+    qdev_prop_set_uint64(dev, "size", size);
+    object_property_add_child(OBJECT(s), name, OBJECT(dev), &error_fatal);
+    qdev_init_nofail(dev);
+
+    mr_dev = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+    memory_region_add_subregion(mr, base, mr_dev);
+}
+
+static void versal_unimp(Versal *s)
+{
+    versal_unimp_area(s, "psm", &s->mr_ps,
+                        MM_PSM_START, MM_PSM_END - MM_PSM_START);
+    versal_unimp_area(s, "crl", &s->mr_ps,
+                        MM_CRL, MM_CRL_SIZE);
+    versal_unimp_area(s, "crf", &s->mr_ps,
+                        MM_FPD_CRF, MM_FPD_CRF_SIZE);
+    versal_unimp_area(s, "iou-scntr", &s->mr_ps,
+                        MM_IOU_SCNTR, MM_IOU_SCNTR_SIZE);
+    versal_unimp_area(s, "iou-scntr-seucre", &s->mr_ps,
+                        MM_IOU_SCNTRS, MM_IOU_SCNTRS_SIZE);
+}
+
+static void versal_realize(DeviceState *dev, Error **errp)
+{
+    Versal *s = XLNX_VERSAL(dev);
+    qemu_irq pic[XLNX_VERSAL_NR_IRQS];
+
+    versal_create_apu_cpus(s);
+    versal_create_apu_gic(s, pic);
+    versal_create_uarts(s, pic);
+    versal_create_gems(s, pic);
+    versal_map_ddr(s);
+    versal_unimp(s);
+
+    /* Create the On Chip Memory (OCM).  */
+    memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm",
+                           MM_OCM_SIZE, &error_fatal);
+
+    memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, 0);
+    memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0);
+}
+
+static void versal_init(Object *obj)
+{
+    Versal *s = XLNX_VERSAL(obj);
+
+    memory_region_init(&s->fpd.apu.mr, obj, "mr-apu", UINT64_MAX);
+    memory_region_init(&s->mr_ps, obj, "mr-ps-switch", UINT64_MAX);
+}
+
+static Property versal_properties[] = {
+    DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_UINT32("psci-conduit", Versal, cfg.psci_conduit, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void versal_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = versal_realize;
+    dc->props = versal_properties;
+    /* No VMSD since we haven't got any top-level SoC state to save.  */
+}
+
+static const TypeInfo versal_info = {
+    .name = TYPE_XLNX_VERSAL,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Versal),
+    .instance_init = versal_init,
+    .class_init = versal_class_init,
+};
+
+static void versal_register_types(void)
+{
+    type_register_static(&versal_info);
+}
+
+type_init(versal_register_types);
diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 6f790f061a1..4ea9add0034 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -8,4 +8,5 @@ CONFIG_DDC=y
 CONFIG_DPCD=y
 CONFIG_XLNX_ZYNQMP=y
 CONFIG_XLNX_ZYNQMP_ARM=y
+CONFIG_XLNX_VERSAL=y
 CONFIG_ARM_SMMUV3=y
-- 
2.19.1

[Qemu-devel] [PULL 10/10] hw/arm: versal: Add a virtual Xilinx Versal board

[Peter Maydell]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add a virtual Xilinx Versal board.

This board is based on the Xilinx Versal SoC. The exact
details of what peripherals are attached to this board
will remain in control of QEMU. QEMU will generate an
FDT on the fly for Linux and other software to auto-discover
peripherals.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Message-id: 20181102131913.1535-3-edgar.iglesias@xilinx.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/Makefile.objs      |   2 +-
 hw/arm/xlnx-versal-virt.c | 494 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 495 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/xlnx-versal-virt.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index ec21d9bc1f0..50c7b4a927d 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -26,7 +26,7 @@ obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
 obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
 obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
 obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
-obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o
+obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o xlnx-versal-virt.o
 obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
 obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
 obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
new file mode 100644
index 00000000000..1e31a3f4429
--- /dev/null
+++ b/hw/arm/xlnx-versal-virt.c
@@ -0,0 +1,494 @@
+/*
+ * Xilinx Versal Virtual board.
+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ * Written by Edgar E. Iglesias
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "sysemu/device_tree.h"
+#include "exec/address-spaces.h"
+#include "hw/boards.h"
+#include "hw/sysbus.h"
+#include "hw/arm/sysbus-fdt.h"
+#include "hw/arm/fdt.h"
+#include "cpu.h"
+#include "hw/arm/xlnx-versal.h"
+
+#define TYPE_XLNX_VERSAL_VIRT_MACHINE MACHINE_TYPE_NAME("xlnx-versal-virt")
+#define XLNX_VERSAL_VIRT_MACHINE(obj) \
+    OBJECT_CHECK(VersalVirt, (obj), TYPE_XLNX_VERSAL_VIRT_MACHINE)
+
+typedef struct VersalVirt {
+    MachineState parent_obj;
+
+    Versal soc;
+    MemoryRegion mr_ddr;
+
+    void *fdt;
+    int fdt_size;
+    struct {
+        uint32_t gic;
+        uint32_t ethernet_phy[2];
+        uint32_t clk_125Mhz;
+        uint32_t clk_25Mhz;
+    } phandle;
+    struct arm_boot_info binfo;
+
+    struct {
+        bool secure;
+    } cfg;
+} VersalVirt;
+
+static void fdt_create(VersalVirt *s)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(s);
+    int i;
+
+    s->fdt = create_device_tree(&s->fdt_size);
+    if (!s->fdt) {
+        error_report("create_device_tree() failed");
+        exit(1);
+    }
+
+    /* Allocate all phandles.  */
+    s->phandle.gic = qemu_fdt_alloc_phandle(s->fdt);
+    for (i = 0; i < ARRAY_SIZE(s->phandle.ethernet_phy); i++) {
+        s->phandle.ethernet_phy[i] = qemu_fdt_alloc_phandle(s->fdt);
+    }
+    s->phandle.clk_25Mhz = qemu_fdt_alloc_phandle(s->fdt);
+    s->phandle.clk_125Mhz = qemu_fdt_alloc_phandle(s->fdt);
+
+    /* Create /chosen node for load_dtb.  */
+    qemu_fdt_add_subnode(s->fdt, "/chosen");
+
+    /* Header */
+    qemu_fdt_setprop_cell(s->fdt, "/", "interrupt-parent", s->phandle.gic);
+    qemu_fdt_setprop_cell(s->fdt, "/", "#size-cells", 0x2);
+    qemu_fdt_setprop_cell(s->fdt, "/", "#address-cells", 0x2);
+    qemu_fdt_setprop_string(s->fdt, "/", "model", mc->desc);
+    qemu_fdt_setprop_string(s->fdt, "/", "compatible", "xlnx-versal-virt");
+}
+
+static void fdt_add_clk_node(VersalVirt *s, const char *name,
+                             unsigned int freq_hz, uint32_t phandle)
+{
+    qemu_fdt_add_subnode(s->fdt, name);
+    qemu_fdt_setprop_cell(s->fdt, name, "phandle", phandle);
+    qemu_fdt_setprop_cell(s->fdt, name, "clock-frequency", freq_hz);
+    qemu_fdt_setprop_cell(s->fdt, name, "#clock-cells", 0x0);
+    qemu_fdt_setprop_string(s->fdt, name, "compatible", "fixed-clock");
+    qemu_fdt_setprop(s->fdt, name, "u-boot,dm-pre-reloc", NULL, 0);
+}
+
+static void fdt_add_cpu_nodes(VersalVirt *s, uint32_t psci_conduit)
+{
+    int i;
+
+    qemu_fdt_add_subnode(s->fdt, "/cpus");
+    qemu_fdt_setprop_cell(s->fdt, "/cpus", "#size-cells", 0x0);
+    qemu_fdt_setprop_cell(s->fdt, "/cpus", "#address-cells", 1);
+
+    for (i = XLNX_VERSAL_NR_ACPUS - 1; i >= 0; i--) {
+        char *name = g_strdup_printf("/cpus/cpu@%d", i);
+        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
+
+        qemu_fdt_add_subnode(s->fdt, name);
+        qemu_fdt_setprop_cell(s->fdt, name, "reg", armcpu->mp_affinity);
+        if (psci_conduit != QEMU_PSCI_CONDUIT_DISABLED) {
+            qemu_fdt_setprop_string(s->fdt, name, "enable-method", "psci");
+        }
+        qemu_fdt_setprop_string(s->fdt, name, "device_type", "cpu");
+        qemu_fdt_setprop_string(s->fdt, name, "compatible",
+                                armcpu->dtb_compatible);
+        g_free(name);
+    }
+}
+
+static void fdt_add_gic_nodes(VersalVirt *s)
+{
+    char *nodename;
+
+    nodename = g_strdup_printf("/gic@%x", MM_GIC_APU_DIST_MAIN);
+    qemu_fdt_add_subnode(s->fdt, nodename);
+    qemu_fdt_setprop_cell(s->fdt, nodename, "phandle", s->phandle.gic);
+    qemu_fdt_setprop_cells(s->fdt, nodename, "interrupts",
+                           GIC_FDT_IRQ_TYPE_PPI, VERSAL_GIC_MAINT_IRQ,
+                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+    qemu_fdt_setprop(s->fdt, nodename, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
+                                 2, MM_GIC_APU_DIST_MAIN,
+                                 2, MM_GIC_APU_DIST_MAIN_SIZE,
+                                 2, MM_GIC_APU_REDIST_0,
+                                 2, MM_GIC_APU_REDIST_0_SIZE);
+    qemu_fdt_setprop_cell(s->fdt, nodename, "#interrupt-cells", 3);
+    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "arm,gic-v3");
+}
+
+static void fdt_add_timer_nodes(VersalVirt *s)
+{
+    const char compat[] = "arm,armv8-timer";
+    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
+
+    qemu_fdt_add_subnode(s->fdt, "/timer");
+    qemu_fdt_setprop_cells(s->fdt, "/timer", "interrupts",
+            GIC_FDT_IRQ_TYPE_PPI, VERSAL_TIMER_S_EL1_IRQ, irqflags,
+            GIC_FDT_IRQ_TYPE_PPI, VERSAL_TIMER_NS_EL1_IRQ, irqflags,
+            GIC_FDT_IRQ_TYPE_PPI, VERSAL_TIMER_VIRT_IRQ, irqflags,
+            GIC_FDT_IRQ_TYPE_PPI, VERSAL_TIMER_NS_EL2_IRQ, irqflags);
+    qemu_fdt_setprop(s->fdt, "/timer", "compatible",
+                     compat, sizeof(compat));
+}
+
+static void fdt_add_uart_nodes(VersalVirt *s)
+{
+    uint64_t addrs[] = { MM_UART1, MM_UART0 };
+    unsigned int irqs[] = { VERSAL_UART1_IRQ_0, VERSAL_UART0_IRQ_0 };
+    const char compat[] = "arm,pl011\0arm,sbsa-uart";
+    const char clocknames[] = "uartclk\0apb_pclk";
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(addrs); i++) {
+        char *name = g_strdup_printf("/uart@%" PRIx64, addrs[i]);
+        qemu_fdt_add_subnode(s->fdt, name);
+        qemu_fdt_setprop_cell(s->fdt, name, "current-speed", 115200);
+        qemu_fdt_setprop_cells(s->fdt, name, "clocks",
+                               s->phandle.clk_125Mhz, s->phandle.clk_125Mhz);
+        qemu_fdt_setprop(s->fdt, name, "clock-names",
+                         clocknames, sizeof(clocknames));
+
+        qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
+                               GIC_FDT_IRQ_TYPE_SPI, irqs[i],
+                               GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+        qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
+                                     2, addrs[i], 2, 0x1000);
+        qemu_fdt_setprop(s->fdt, name, "compatible",
+                         compat, sizeof(compat));
+        qemu_fdt_setprop(s->fdt, name, "u-boot,dm-pre-reloc", NULL, 0);
+
+        if (addrs[i] == MM_UART0) {
+            /* Select UART0.  */
+            qemu_fdt_setprop_string(s->fdt, "/chosen", "stdout-path", name);
+        }
+        g_free(name);
+    }
+}
+
+static void fdt_add_fixed_link_nodes(VersalVirt *s, char *gemname,
+                                     uint32_t phandle)
+{
+    char *name = g_strdup_printf("%s/fixed-link", gemname);
+
+    qemu_fdt_add_subnode(s->fdt, name);
+    qemu_fdt_setprop_cell(s->fdt, name, "phandle", phandle);
+    qemu_fdt_setprop(s->fdt, name, "full-duplex", NULL, 0);
+    qemu_fdt_setprop_cell(s->fdt, name, "speed", 1000);
+    g_free(name);
+}
+
+static void fdt_add_gem_nodes(VersalVirt *s)
+{
+    uint64_t addrs[] = { MM_GEM1, MM_GEM0 };
+    unsigned int irqs[] = { VERSAL_GEM1_IRQ_0, VERSAL_GEM0_IRQ_0 };
+    const char clocknames[] = "pclk\0hclk\0tx_clk\0rx_clk";
+    const char compat_gem[] = "cdns,zynqmp-gem\0cdns,gem";
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(addrs); i++) {
+        char *name = g_strdup_printf("/ethernet@%" PRIx64, addrs[i]);
+        qemu_fdt_add_subnode(s->fdt, name);
+
+        fdt_add_fixed_link_nodes(s, name, s->phandle.ethernet_phy[i]);
+        qemu_fdt_setprop_string(s->fdt, name, "phy-mode", "rgmii-id");
+        qemu_fdt_setprop_cell(s->fdt, name, "phy-handle",
+                              s->phandle.ethernet_phy[i]);
+        qemu_fdt_setprop_cells(s->fdt, name, "clocks",
+                               s->phandle.clk_25Mhz, s->phandle.clk_25Mhz,
+                               s->phandle.clk_25Mhz, s->phandle.clk_25Mhz);
+        qemu_fdt_setprop(s->fdt, name, "clock-names",
+                         clocknames, sizeof(clocknames));
+        qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
+                               GIC_FDT_IRQ_TYPE_SPI, irqs[i],
+                               GIC_FDT_IRQ_FLAGS_LEVEL_HI,
+                               GIC_FDT_IRQ_TYPE_SPI, irqs[i],
+                               GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+        qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
+                                     2, addrs[i], 2, 0x1000);
+        qemu_fdt_setprop(s->fdt, name, "compatible",
+                         compat_gem, sizeof(compat_gem));
+        qemu_fdt_setprop_cell(s->fdt, name, "#address-cells", 1);
+        qemu_fdt_setprop_cell(s->fdt, name, "#size-cells", 0);
+        g_free(name);
+    }
+}
+
+static void fdt_nop_memory_nodes(void *fdt, Error **errp)
+{
+    Error *err = NULL;
+    char **node_path;
+    int n = 0;
+
+    node_path = qemu_fdt_node_unit_path(fdt, "memory", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    while (node_path[n]) {
+        if (g_str_has_prefix(node_path[n], "/memory")) {
+            qemu_fdt_nop_node(fdt, node_path[n]);
+        }
+        n++;
+    }
+    g_strfreev(node_path);
+}
+
+static void fdt_add_memory_nodes(VersalVirt *s, void *fdt, uint64_t ram_size)
+{
+    /* Describes the various split DDR access regions.  */
+    static const struct {
+        uint64_t base;
+        uint64_t size;
+    } addr_ranges[] = {
+        { MM_TOP_DDR, MM_TOP_DDR_SIZE },
+        { MM_TOP_DDR_2, MM_TOP_DDR_2_SIZE },
+        { MM_TOP_DDR_3, MM_TOP_DDR_3_SIZE },
+        { MM_TOP_DDR_4, MM_TOP_DDR_4_SIZE }
+    };
+    uint64_t mem_reg_prop[8] = {0};
+    uint64_t size = ram_size;
+    Error *err = NULL;
+    char *name;
+    int i;
+
+    fdt_nop_memory_nodes(fdt, &err);
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+
+    name = g_strdup_printf("/memory@%x", MM_TOP_DDR);
+    for (i = 0; i < ARRAY_SIZE(addr_ranges) && size; i++) {
+        uint64_t mapsize;
+
+        mapsize = size < addr_ranges[i].size ? size : addr_ranges[i].size;
+
+        mem_reg_prop[i * 2] = addr_ranges[i].base;
+        mem_reg_prop[i * 2 + 1] = mapsize;
+        size -= mapsize;
+    }
+    qemu_fdt_add_subnode(fdt, name);
+    qemu_fdt_setprop_string(fdt, name, "device_type", "memory");
+
+    switch (i) {
+    case 1:
+        qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+                                     2, mem_reg_prop[0],
+                                     2, mem_reg_prop[1]);
+        break;
+    case 2:
+        qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+                                     2, mem_reg_prop[0],
+                                     2, mem_reg_prop[1],
+                                     2, mem_reg_prop[2],
+                                     2, mem_reg_prop[3]);
+        break;
+    case 3:
+        qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+                                     2, mem_reg_prop[0],
+                                     2, mem_reg_prop[1],
+                                     2, mem_reg_prop[2],
+                                     2, mem_reg_prop[3],
+                                     2, mem_reg_prop[4],
+                                     2, mem_reg_prop[5]);
+        break;
+    case 4:
+        qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+                                     2, mem_reg_prop[0],
+                                     2, mem_reg_prop[1],
+                                     2, mem_reg_prop[2],
+                                     2, mem_reg_prop[3],
+                                     2, mem_reg_prop[4],
+                                     2, mem_reg_prop[5],
+                                     2, mem_reg_prop[6],
+                                     2, mem_reg_prop[7]);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    g_free(name);
+}
+
+static void versal_virt_modify_dtb(const struct arm_boot_info *binfo,
+                                    void *fdt)
+{
+    VersalVirt *s = container_of(binfo, VersalVirt, binfo);
+
+    fdt_add_memory_nodes(s, fdt, binfo->ram_size);
+}
+
+static void *versal_virt_get_dtb(const struct arm_boot_info *binfo,
+                                  int *fdt_size)
+{
+    const VersalVirt *board = container_of(binfo, VersalVirt, binfo);
+
+    *fdt_size = board->fdt_size;
+    return board->fdt;
+}
+
+#define NUM_VIRTIO_TRANSPORT 32
+static void create_virtio_regions(VersalVirt *s)
+{
+    int virtio_mmio_size = 0x200;
+    int i;
+
+    for (i = 0; i < NUM_VIRTIO_TRANSPORT; i++) {
+        char *name = g_strdup_printf("virtio%d", i);;
+        hwaddr base = MM_TOP_RSVD + i * virtio_mmio_size;
+        int irq = VERSAL_RSVD_HIGH_IRQ_FIRST + i;
+        MemoryRegion *mr;
+        DeviceState *dev;
+        qemu_irq pic_irq;
+
+        pic_irq = qdev_get_gpio_in(DEVICE(&s->soc.fpd.apu.gic), irq);
+        dev = qdev_create(NULL, "virtio-mmio");
+        object_property_add_child(OBJECT(&s->soc), name, OBJECT(dev),
+                                  &error_fatal);
+        qdev_init_nofail(dev);
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic_irq);
+        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+        memory_region_add_subregion(&s->soc.mr_ps, base, mr);
+        sysbus_create_simple("virtio-mmio", base, pic_irq);
+    }
+
+    for (i = 0; i < NUM_VIRTIO_TRANSPORT; i++) {
+        hwaddr base = MM_TOP_RSVD + i * virtio_mmio_size;
+        int irq = VERSAL_RSVD_HIGH_IRQ_FIRST + i;
+        char *name = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
+
+        qemu_fdt_add_subnode(s->fdt, name);
+        qemu_fdt_setprop(s->fdt, name, "dma-coherent", NULL, 0);
+        qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
+                               GIC_FDT_IRQ_TYPE_SPI, irq,
+                               GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+        qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
+                                     2, base, 2, virtio_mmio_size);
+        qemu_fdt_setprop_string(s->fdt, name, "compatible", "virtio,mmio");
+        g_free(name);
+    }
+}
+
+static void versal_virt_init(MachineState *machine)
+{
+    VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(machine);
+    int psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
+
+    /*
+     * If the user provides an Operating System to be loaded, we expect them
+     * to use the -kernel command line option.
+     *
+     * Users can load firmware or boot-loaders with the -device loader options.
+     *
+     * When loading an OS, we generate a dtb and let arm_load_kernel() select
+     * where it gets loaded. This dtb will be passed to the kernel in x0.
+     *
+     * If there's no -kernel option, we generate a DTB and place it at 0x1000
+     * for the bootloaders or firmware to pick up.
+     *
+     * If users want to provide their own DTB, they can use the -dtb option.
+     * These dtb's will have their memory nodes modified to match QEMU's
+     * selected ram_size option before they get passed to the kernel or fw.
+     *
+     * When loading an OS, we turn on QEMU's PSCI implementation with SMC
+     * as the PSCI conduit. When there's no -kernel, we assume the user
+     * provides EL3 firmware to handle PSCI.
+     */
+    if (machine->kernel_filename) {
+        psci_conduit = QEMU_PSCI_CONDUIT_SMC;
+    }
+
+    memory_region_allocate_system_memory(&s->mr_ddr, NULL, "ddr",
+                                         machine->ram_size);
+
+    sysbus_init_child_obj(OBJECT(machine), "xlnx-ve", &s->soc,
+                          sizeof(s->soc), TYPE_XLNX_VERSAL);
+    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->mr_ddr),
+                             "ddr", &error_abort);
+    object_property_set_int(OBJECT(&s->soc), psci_conduit,
+                            "psci-conduit", &error_abort);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
+
+    fdt_create(s);
+    create_virtio_regions(s);
+    fdt_add_gem_nodes(s);
+    fdt_add_uart_nodes(s);
+    fdt_add_gic_nodes(s);
+    fdt_add_timer_nodes(s);
+    fdt_add_cpu_nodes(s, psci_conduit);
+    fdt_add_clk_node(s, "/clk125", 125000000, s->phandle.clk_125Mhz);
+    fdt_add_clk_node(s, "/clk25", 25000000, s->phandle.clk_25Mhz);
+
+    /* Make the APU cpu address space visible to virtio and other
+     * modules unaware of muliple address-spaces.  */
+    memory_region_add_subregion_overlap(get_system_memory(),
+                                        0, &s->soc.fpd.apu.mr, 0);
+
+    s->binfo.ram_size = machine->ram_size;
+    s->binfo.kernel_filename = machine->kernel_filename;
+    s->binfo.kernel_cmdline = machine->kernel_cmdline;
+    s->binfo.initrd_filename = machine->initrd_filename;
+    s->binfo.loader_start = 0x0;
+    s->binfo.get_dtb = versal_virt_get_dtb;
+    s->binfo.modify_dtb = versal_virt_modify_dtb;
+    if (machine->kernel_filename) {
+        arm_load_kernel(s->soc.fpd.apu.cpu[0], &s->binfo);
+    } else {
+        AddressSpace *as = arm_boot_address_space(s->soc.fpd.apu.cpu[0],
+                                                  &s->binfo);
+        /* Some boot-loaders (e.g u-boot) don't like blobs at address 0 (NULL).
+         * Offset things by 4K.  */
+        s->binfo.loader_start = 0x1000;
+        s->binfo.dtb_limit = 0x1000000;
+        if (arm_load_dtb(s->binfo.loader_start,
+                         &s->binfo, s->binfo.dtb_limit, as) < 0) {
+            exit(EXIT_FAILURE);
+        }
+    }
+}
+
+static void versal_virt_machine_instance_init(Object *obj)
+{
+}
+
+static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->desc = "Xilinx Versal Virtual development board";
+    mc->init = versal_virt_init;
+    mc->max_cpus = XLNX_VERSAL_NR_ACPUS;
+    mc->default_cpus = XLNX_VERSAL_NR_ACPUS;
+    mc->no_cdrom = true;
+}
+
+static const TypeInfo versal_virt_machine_init_typeinfo = {
+    .name       = TYPE_XLNX_VERSAL_VIRT_MACHINE,
+    .parent     = TYPE_MACHINE,
+    .class_init = versal_virt_machine_class_init,
+    .instance_init = versal_virt_machine_instance_init,
+    .instance_size = sizeof(VersalVirt),
+};
+
+static void versal_virt_machine_init_register_types(void)
+{
+    type_register_static(&versal_virt_machine_init_typeinfo);
+}
+
+type_init(versal_virt_machine_init_register_types)
+
-- 
2.19.1

Re: [Qemu-devel] [PULL 10/10] hw/arm: versal: Add a virtual Xilinx Versal board

[Peter Maydell]

On Fri, 2 Nov 2018 at 17:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add a virtual Xilinx Versal board.
>
> This board is based on the Xilinx Versal SoC. The exact
> details of what peripherals are attached to this board
> will remain in control of QEMU. QEMU will generate an
> FDT on the fly for Linux and other software to auto-discover
> peripherals.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Message-id: 20181102131913.1535-3-edgar.iglesias@xilinx.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi Edgar -- I've been running the clang-7 leak sanitizer
on "make check", and it spotted a couple of minor memory
leaks in the versal board code:

> +static void fdt_add_gic_nodes(VersalVirt *s)
> +{
> +    char *nodename;
> +
> +    nodename = g_strdup_printf("/gic@%x", MM_GIC_APU_DIST_MAIN);
> +    qemu_fdt_add_subnode(s->fdt, nodename);
> +    qemu_fdt_setprop_cell(s->fdt, nodename, "phandle", s->phandle.gic);
> +    qemu_fdt_setprop_cells(s->fdt, nodename, "interrupts",
> +                           GIC_FDT_IRQ_TYPE_PPI, VERSAL_GIC_MAINT_IRQ,
> +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +    qemu_fdt_setprop(s->fdt, nodename, "interrupt-controller", NULL, 0);
> +    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> +                                 2, MM_GIC_APU_DIST_MAIN,
> +                                 2, MM_GIC_APU_DIST_MAIN_SIZE,
> +                                 2, MM_GIC_APU_REDIST_0,
> +                                 2, MM_GIC_APU_REDIST_0_SIZE);
> +    qemu_fdt_setprop_cell(s->fdt, nodename, "#interrupt-cells", 3);
> +    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "arm,gic-v3");

nodename is allocated but never freed here.

> +}


> +#define NUM_VIRTIO_TRANSPORT 32
> +static void create_virtio_regions(VersalVirt *s)
> +{
> +    int virtio_mmio_size = 0x200;
> +    int i;
> +
> +    for (i = 0; i < NUM_VIRTIO_TRANSPORT; i++) {
> +        char *name = g_strdup_printf("virtio%d", i);;
> +        hwaddr base = MM_TOP_RSVD + i * virtio_mmio_size;
> +        int irq = VERSAL_RSVD_HIGH_IRQ_FIRST + i;
> +        MemoryRegion *mr;
> +        DeviceState *dev;
> +        qemu_irq pic_irq;
> +
> +        pic_irq = qdev_get_gpio_in(DEVICE(&s->soc.fpd.apu.gic), irq);
> +        dev = qdev_create(NULL, "virtio-mmio");
> +        object_property_add_child(OBJECT(&s->soc), name, OBJECT(dev),
> +                                  &error_fatal);
> +        qdev_init_nofail(dev);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic_irq);
> +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +        memory_region_add_subregion(&s->soc.mr_ps, base, mr);
> +        sysbus_create_simple("virtio-mmio", base, pic_irq);

The body of this loop allocates name but forgets to free it.

> +    }

thanks
-- PMM

Re: [Qemu-devel] [PULL 10/10] hw/arm: versal: Add a virtual Xilinx Versal board

[Edgar E. Iglesias]

Hi Peter,

Thanks for pointing this out. I'll post patches soon unless someone beats
me to it. I'm travelling, hence the slow and top posted reply....

Cheers,
Edgar
---
Sent from my phone

On Tue, Dec 4, 2018, 11:28 Peter Maydell <peter.maydell@linaro.org wrote:

> On Fri, 2 Nov 2018 at 17:24, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add a virtual Xilinx Versal board.
> >
> > This board is based on the Xilinx Versal SoC. The exact
> > details of what peripherals are attached to this board
> > will remain in control of QEMU. QEMU will generate an
> > FDT on the fly for Linux and other software to auto-discover
> > peripherals.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Message-id: 20181102131913.1535-3-edgar.iglesias@xilinx.com
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi Edgar -- I've been running the clang-7 leak sanitizer
> on "make check", and it spotted a couple of minor memory
> leaks in the versal board code:
>
> > +static void fdt_add_gic_nodes(VersalVirt *s)
> > +{
> > +    char *nodename;
> > +
> > +    nodename = g_strdup_printf("/gic@%x", MM_GIC_APU_DIST_MAIN);
> > +    qemu_fdt_add_subnode(s->fdt, nodename);
> > +    qemu_fdt_setprop_cell(s->fdt, nodename, "phandle", s->phandle.gic);
> > +    qemu_fdt_setprop_cells(s->fdt, nodename, "interrupts",
> > +                           GIC_FDT_IRQ_TYPE_PPI, VERSAL_GIC_MAINT_IRQ,
> > +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> > +    qemu_fdt_setprop(s->fdt, nodename, "interrupt-controller", NULL, 0);
> > +    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> > +                                 2, MM_GIC_APU_DIST_MAIN,
> > +                                 2, MM_GIC_APU_DIST_MAIN_SIZE,
> > +                                 2, MM_GIC_APU_REDIST_0,
> > +                                 2, MM_GIC_APU_REDIST_0_SIZE);
> > +    qemu_fdt_setprop_cell(s->fdt, nodename, "#interrupt-cells", 3);
> > +    qemu_fdt_setprop_string(s->fdt, nodename, "compatible",
> "arm,gic-v3");
>
> nodename is allocated but never freed here.
>
> > +}
>
>
> > +#define NUM_VIRTIO_TRANSPORT 32
> > +static void create_virtio_regions(VersalVirt *s)
> > +{
> > +    int virtio_mmio_size = 0x200;
> > +    int i;
> > +
> > +    for (i = 0; i < NUM_VIRTIO_TRANSPORT; i++) {
> > +        char *name = g_strdup_printf("virtio%d", i);;
> > +        hwaddr base = MM_TOP_RSVD + i * virtio_mmio_size;
> > +        int irq = VERSAL_RSVD_HIGH_IRQ_FIRST + i;
> > +        MemoryRegion *mr;
> > +        DeviceState *dev;
> > +        qemu_irq pic_irq;
> > +
> > +        pic_irq = qdev_get_gpio_in(DEVICE(&s->soc.fpd.apu.gic), irq);
> > +        dev = qdev_create(NULL, "virtio-mmio");
> > +        object_property_add_child(OBJECT(&s->soc), name, OBJECT(dev),
> > +                                  &error_fatal);
> > +        qdev_init_nofail(dev);
> > +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic_irq);
> > +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> > +        memory_region_add_subregion(&s->soc.mr_ps, base, mr);
> > +        sysbus_create_simple("virtio-mmio", base, pic_irq);
>
> The body of this loop allocates name but forgets to free it.
>
> > +    }
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PULL 10/10] hw/arm: versal: Add a virtual Xilinx Versal board

[Peter Maydell]

On Fri, 2 Nov 2018 at 17:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add a virtual Xilinx Versal board.
>
> This board is based on the Xilinx Versal SoC. The exact
> details of what peripherals are attached to this board
> will remain in control of QEMU. QEMU will generate an
> FDT on the fly for Linux and other software to auto-discover
> peripherals.

Hi Edgar; I was just looking at the Versal board code for
something else I was working on, and I noticed a bug that's been
there since it was added in this patch:

> +    s->binfo.ram_size = machine->ram_size;
> +    s->binfo.kernel_filename = machine->kernel_filename;
> +    s->binfo.kernel_cmdline = machine->kernel_cmdline;
> +    s->binfo.initrd_filename = machine->initrd_filename;
> +    s->binfo.loader_start = 0x0;
> +    s->binfo.get_dtb = versal_virt_get_dtb;
> +    s->binfo.modify_dtb = versal_virt_modify_dtb;
> +    if (machine->kernel_filename) {
> +        arm_load_kernel(s->soc.fpd.apu.cpu[0], &s->binfo);
> +    } else {
> +        AddressSpace *as = arm_boot_address_space(s->soc.fpd.apu.cpu[0],
> +                                                  &s->binfo);
> +        /* Some boot-loaders (e.g u-boot) don't like blobs at address 0 (NULL).
> +         * Offset things by 4K.  */
> +        s->binfo.loader_start = 0x1000;
> +        s->binfo.dtb_limit = 0x1000000;
> +        if (arm_load_dtb(s->binfo.loader_start,
> +                         &s->binfo, s->binfo.dtb_limit, as) < 0) {
> +            exit(EXIT_FAILURE);
> +        }
> +    }

The board init code only calls arm_load_kernel() if machine->kernel_filename
is set. This is a bug, because calling arm_load_kernel() is mandatory
for board code -- it is the place where we set up the reset handler
that resets the CPUs, so if you don't call it the CPU objects don't
get reset.

thanks
-- PMM

Re: [Qemu-devel] [PULL 10/10] hw/arm: versal: Add a virtual Xilinx Versal board

[Edgar E. Iglesias]

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

On Thu, Jan 27, 2022 at 2:10 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 2 Nov 2018 at 17:24, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add a virtual Xilinx Versal board.
> >
> > This board is based on the Xilinx Versal SoC. The exact
> > details of what peripherals are attached to this board
> > will remain in control of QEMU. QEMU will generate an
> > FDT on the fly for Linux and other software to auto-discover
> > peripherals.
>
> Hi Edgar; I was just looking at the Versal board code for
> something else I was working on, and I noticed a bug that's been
> there since it was added in this patch:
>
> > +    s->binfo.ram_size = machine->ram_size;
> > +    s->binfo.kernel_filename = machine->kernel_filename;
> > +    s->binfo.kernel_cmdline = machine->kernel_cmdline;
> > +    s->binfo.initrd_filename = machine->initrd_filename;
> > +    s->binfo.loader_start = 0x0;
> > +    s->binfo.get_dtb = versal_virt_get_dtb;
> > +    s->binfo.modify_dtb = versal_virt_modify_dtb;
> > +    if (machine->kernel_filename) {
> > +        arm_load_kernel(s->soc.fpd.apu.cpu[0], &s->binfo);
> > +    } else {
> > +        AddressSpace *as = arm_boot_address_space(s->soc.fpd.apu.cpu[0],
> > +                                                  &s->binfo);
> > +        /* Some boot-loaders (e.g u-boot) don't like blobs at address 0
> (NULL).
> > +         * Offset things by 4K.  */
> > +        s->binfo.loader_start = 0x1000;
> > +        s->binfo.dtb_limit = 0x1000000;
> > +        if (arm_load_dtb(s->binfo.loader_start,
> > +                         &s->binfo, s->binfo.dtb_limit, as) < 0) {
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
>
> The board init code only calls arm_load_kernel() if
> machine->kernel_filename
> is set. This is a bug, because calling arm_load_kernel() is mandatory
> for board code -- it is the place where we set up the reset handler
> that resets the CPUs, so if you don't call it the CPU objects don't
> get reset.
>
> thanks
> -- PMM
>

Thanks Peter,

I'll send a patch shortly.

Cheers,
Edgar

[-- Attachment #2: Type: text/html, Size: 3108 bytes --]

Re: [Qemu-devel] [PULL v3 00/10] target-arm queue

[Peter Maydell]

On 2 November 2018 at 17:16, Peter Maydell <peter.maydell@linaro.org> wrote:
> This is a respin of my pull request from earlier this week:
>  * versal board compile failure fixed
>  * a few new patches:
>   - MAINTAINERS file fix
>   - use ARRAY_SIZE macro in xilinx_zynq
>   - avoid an array overrun in strongarm GPIO irq handling
>   - fix an assert running KVM on an aarch64-only host
>
> The following changes since commit 69e2d03843412b9c076515b3aa9a71db161b6a1a:
>
>   Merge remote-tracking branch 'remotes/riscv/tags/riscv-for-master-3.1-sf1' into staging (2018-11-02 13:16:13 +0000)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20181102
>
> for you to fetch changes up to 6f16da53ffe4567c0353f85055df04860eb4e6fc:
>
>   hw/arm: versal: Add a virtual Xilinx Versal board (2018-11-02 14:11:31 +0000)
>
> ----------------------------------------------------------------
> target-arm queue:
>  * microbit: Add the UART to our nRF51 SoC model
>  * Add a virtual Xilinx Versal board "xlnx-versal-virt"
>  * hw/arm/virt: Set VIRT_COMPAT_3_0 compat
>  * MAINTAINERS: Remove bouncing email in ARM ACPI
>  * strongarm: mask off high[31:28] bits from dir and state registers
>  * target/arm: Conditionalize some asserts on aarch32 support
>  * hw/arm/xilinx_zynq: Use the ARRAY_SIZE macro
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM