[PATCH v2 0/9] A collection of RISC-V cleanups and improvements

[PATCH v2 0/9] A collection of RISC-V cleanups and improvements

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

This is a few patches to cleanup some RISC-V hardware and mark the
Hyperisor extension as non experimental.

v2:
 - Add some more fixes
 - Address review comments

Alistair Francis (9):
  hw/intc: sifive_plic: Add a reset function
  hw/intc: sifive_plic: Cleanup the write function
  hw/intc: sifive_plic: Cleanup the read function
  hw/intc: sifive_plic: Cleanup remaining functions
  target/riscv: Mark the Hypervisor extension as non experimental
  target/riscv: Enable the Hypervisor extension by default
  hw/riscv: Use error_fatal for SoC realisation
  hw/riscv: virt: Allow support for 32 cores
  hw/riscv: virt: Set the clock-frequency

 include/hw/riscv/virt.h    |   2 +-
 hw/intc/sifive_plic.c      | 254 +++++++++++--------------------------
 hw/riscv/microchip_pfsoc.c |   2 +-
 hw/riscv/opentitan.c       |   2 +-
 hw/riscv/sifive_e.c        |   2 +-
 hw/riscv/sifive_u.c        |   2 +-
 hw/riscv/virt.c            |   1 +
 target/riscv/cpu.c         |   2 +-
 8 files changed, 83 insertions(+), 184 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] hw/intc: sifive_plic: Add a reset function

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/intc/sifive_plic.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 877e76877c..a9f7a1bfb0 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -355,6 +355,23 @@ static const MemoryRegionOps sifive_plic_ops = {
     }
 };
 
+static void sifive_plic_reset(DeviceState *dev)
+{
+    SiFivePLICState *s = SIFIVE_PLIC(dev);
+    int i;
+
+    memset(s->source_priority, 0, sizeof(uint32_t) * s->num_sources);
+    memset(s->target_priority, 0, sizeof(uint32_t) * s->num_addrs);
+    memset(s->pending, 0, sizeof(uint32_t) * s->bitfield_words);
+    memset(s->claimed, 0, sizeof(uint32_t) * s->bitfield_words);
+    memset(s->enable, 0, sizeof(uint32_t) * s->num_enables);
+
+    for (i = 0; i < s->num_harts; i++) {
+        qemu_set_irq(s->m_external_irqs[i], 0);
+        qemu_set_irq(s->s_external_irqs[i], 0);
+    }
+}
+
 /*
  * parse PLIC hart/mode address offset config
  *
@@ -501,6 +518,7 @@ static void sifive_plic_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = sifive_plic_reset;
     device_class_set_props(dc, sifive_plic_properties);
     dc->realize = sifive_plic_realize;
     dc->vmsd = &vmstate_sifive_plic;
-- 
2.31.1

Re: [PATCH v2 1/9] hw/intc: sifive_plic: Add a reset function

[Philippe Mathieu-Daudé]

On 12/16/21 05:54, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

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

Re: [PATCH v2 1/9] hw/intc: sifive_plic: Add a reset function

[Philippe Mathieu-Daudé]

On 12/16/21 05:54, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

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

Re: [PATCH v2 1/9] hw/intc: sifive_plic: Add a reset function

[Bin Meng]

On Thu, Dec 16, 2021 at 12:54 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 1/9] hw/intc: sifive_plic: Add a reset function

[Bin Meng]

On Thu, Dec 16, 2021 at 12:54 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 2/9] hw/intc: sifive_plic: Cleanup the write function

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/intc/sifive_plic.c | 76 +++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 49 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index a9f7a1bfb0..698492ce77 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -33,6 +33,11 @@
 
 #define RISCV_DEBUG_PLIC 0
 
+static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
+{
+    return addr >= base && addr - base < num;
+}
+
 static PLICMode char_to_mode(char c)
 {
     switch (c) {
@@ -269,80 +274,53 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
 {
     SiFivePLICState *plic = opaque;
 
-    /* writes must be 4 byte words */
-    if ((addr & 0x3) != 0) {
-        goto err;
-    }
-
-    if (addr >= plic->priority_base && /* 4 bytes per source */
-        addr < plic->priority_base + (plic->num_sources << 2))
-    {
+    if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
         uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
+
         plic->source_priority[irq] = value & 7;
-        if (RISCV_DEBUG_PLIC) {
-            qemu_log("plic: write priority: irq=%d priority=%d\n",
-                irq, plic->source_priority[irq]);
-        }
         sifive_plic_update(plic);
-        return;
-    } else if (addr >= plic->pending_base && /* 1 bit per source */
-               addr < plic->pending_base + (plic->num_sources >> 3))
-    {
+    } else if (addr_between(addr, plic->pending_base,
+                            plic->num_sources >> 3)) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid pending write: 0x%" HWADDR_PRIx "",
                       __func__, addr);
-        return;
-    } else if (addr >= plic->enable_base && /* 1 bit per source */
-        addr < plic->enable_base + plic->num_addrs * plic->enable_stride)
-    {
+    } else if (addr_between(addr, plic->enable_base,
+                            plic->num_addrs * plic->enable_stride)) {
         uint32_t addrid = (addr - plic->enable_base) / plic->enable_stride;
         uint32_t wordid = (addr & (plic->enable_stride - 1)) >> 2;
+
         if (wordid < plic->bitfield_words) {
             plic->enable[addrid * plic->bitfield_words + wordid] = value;
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: write enable: hart%d-%c word=%d value=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode), wordid,
-                    plic->enable[addrid * plic->bitfield_words + wordid]);
-            }
-            return;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Invalid enable write 0x%" HWADDR_PRIx "\n",
+                          __func__, addr);
         }
-    } else if (addr >= plic->context_base && /* 4 bytes per reg */
-        addr < plic->context_base + plic->num_addrs * plic->context_stride)
-    {
+    } else if (addr_between(addr, plic->context_base,
+                            plic->num_addrs * plic->context_stride)) {
         uint32_t addrid = (addr - plic->context_base) / plic->context_stride;
         uint32_t contextid = (addr & (plic->context_stride - 1));
+
         if (contextid == 0) {
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: write priority: hart%d-%c priority=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode),
-                    plic->target_priority[addrid]);
-            }
             if (value <= plic->num_priorities) {
                 plic->target_priority[addrid] = value;
                 sifive_plic_update(plic);
             }
-            return;
         } else if (contextid == 4) {
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: write claim: hart%d-%c irq=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode),
-                    (uint32_t)value);
-            }
             if (value < plic->num_sources) {
                 sifive_plic_set_claimed(plic, value, false);
                 sifive_plic_update(plic);
             }
-            return;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Invalid context write 0x%" HWADDR_PRIx "\n",
+                          __func__, addr);
         }
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
+                      __func__, addr);
     }
-
-err:
-    qemu_log_mask(LOG_GUEST_ERROR,
-                  "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
-                  __func__, addr);
 }
 
 static const MemoryRegionOps sifive_plic_ops = {
-- 
2.31.1

[PATCH v2 3/9] hw/intc: sifive_plic: Cleanup the read function

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/intc/sifive_plic.c | 55 +++++++++----------------------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 698492ce77..44d24b3c59 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -199,70 +199,37 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
 {
     SiFivePLICState *plic = opaque;
 
-    /* writes must be 4 byte words */
-    if ((addr & 0x3) != 0) {
-        goto err;
-    }
-
-    if (addr >= plic->priority_base && /* 4 bytes per source */
-        addr < plic->priority_base + (plic->num_sources << 2))
-    {
+    if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
         uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
-        if (RISCV_DEBUG_PLIC) {
-            qemu_log("plic: read priority: irq=%d priority=%d\n",
-                irq, plic->source_priority[irq]);
-        }
+
         return plic->source_priority[irq];
-    } else if (addr >= plic->pending_base && /* 1 bit per source */
-               addr < plic->pending_base + (plic->num_sources >> 3))
-    {
+    } else if (addr_between(addr, plic->pending_base, plic->num_sources >> 3)) {
         uint32_t word = (addr - plic->pending_base) >> 2;
-        if (RISCV_DEBUG_PLIC) {
-            qemu_log("plic: read pending: word=%d value=%d\n",
-                word, plic->pending[word]);
-        }
+
         return plic->pending[word];
-    } else if (addr >= plic->enable_base && /* 1 bit per source */
-             addr < plic->enable_base + plic->num_addrs * plic->enable_stride)
-    {
+    } else if (addr_between(addr, plic->enable_base,
+                            plic->num_addrs * plic->enable_stride)) {
         uint32_t addrid = (addr - plic->enable_base) / plic->enable_stride;
         uint32_t wordid = (addr & (plic->enable_stride - 1)) >> 2;
+
         if (wordid < plic->bitfield_words) {
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: read enable: hart%d-%c word=%d value=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode), wordid,
-                    plic->enable[addrid * plic->bitfield_words + wordid]);
-            }
             return plic->enable[addrid * plic->bitfield_words + wordid];
         }
-    } else if (addr >= plic->context_base && /* 1 bit per source */
-             addr < plic->context_base + plic->num_addrs * plic->context_stride)
-    {
+    } else if (addr_between(addr, plic->context_base,
+                            plic->num_addrs * plic->context_stride)) {
         uint32_t addrid = (addr - plic->context_base) / plic->context_stride;
         uint32_t contextid = (addr & (plic->context_stride - 1));
+
         if (contextid == 0) {
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: read priority: hart%d-%c priority=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode),
-                    plic->target_priority[addrid]);
-            }
             return plic->target_priority[addrid];
         } else if (contextid == 4) {
             uint32_t value = sifive_plic_claim(plic, addrid);
-            if (RISCV_DEBUG_PLIC) {
-                qemu_log("plic: read claim: hart%d-%c irq=%x\n",
-                    plic->addr_config[addrid].hartid,
-                    mode_to_char(plic->addr_config[addrid].mode),
-                    value);
-            }
+
             sifive_plic_update(plic);
             return value;
         }
     }
 
-err:
     qemu_log_mask(LOG_GUEST_ERROR,
                   "%s: Invalid register read 0x%" HWADDR_PRIx "\n",
                   __func__, addr);
-- 
2.31.1

[PATCH v2 4/9] hw/intc: sifive_plic: Cleanup remaining functions

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

We can remove the original sifive_plic_irqs_pending() function and
instead just use the sifive_plic_claim() function (renamed to
sifive_plic_claimed()) to determine if any interrupts are pending.

This requires move the side effects outside of sifive_plic_claimed(),
but as they are only invoked once that isn't a problem.

We have also removed all of the old #ifdef debugging logs, so let's
cleanup the last remaining debug function while we are here.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/intc/sifive_plic.c | 109 +++++++++---------------------------------
 1 file changed, 22 insertions(+), 87 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 44d24b3c59..746c0f0343 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -31,8 +31,6 @@
 #include "migration/vmstate.h"
 #include "hw/irq.h"
 
-#define RISCV_DEBUG_PLIC 0
-
 static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
 {
     return addr >= base && addr - base < num;
@@ -51,47 +49,6 @@ static PLICMode char_to_mode(char c)
     }
 }
 
-static char mode_to_char(PLICMode m)
-{
-    switch (m) {
-    case PLICMode_U: return 'U';
-    case PLICMode_S: return 'S';
-    case PLICMode_H: return 'H';
-    case PLICMode_M: return 'M';
-    default: return '?';
-    }
-}
-
-static void sifive_plic_print_state(SiFivePLICState *plic)
-{
-    int i;
-    int addrid;
-
-    /* pending */
-    qemu_log("pending       : ");
-    for (i = plic->bitfield_words - 1; i >= 0; i--) {
-        qemu_log("%08x", plic->pending[i]);
-    }
-    qemu_log("\n");
-
-    /* pending */
-    qemu_log("claimed       : ");
-    for (i = plic->bitfield_words - 1; i >= 0; i--) {
-        qemu_log("%08x", plic->claimed[i]);
-    }
-    qemu_log("\n");
-
-    for (addrid = 0; addrid < plic->num_addrs; addrid++) {
-        qemu_log("hart%d-%c enable: ",
-            plic->addr_config[addrid].hartid,
-            mode_to_char(plic->addr_config[addrid].mode));
-        for (i = plic->bitfield_words - 1; i >= 0; i--) {
-            qemu_log("%08x", plic->enable[addrid * plic->bitfield_words + i]);
-        }
-        qemu_log("\n");
-    }
-}
-
 static uint32_t atomic_set_masked(uint32_t *a, uint32_t mask, uint32_t value)
 {
     uint32_t old, new, cmp = qatomic_read(a);
@@ -115,26 +72,34 @@ static void sifive_plic_set_claimed(SiFivePLICState *plic, int irq, bool level)
     atomic_set_masked(&plic->claimed[irq >> 5], 1 << (irq & 31), -!!level);
 }
 
-static int sifive_plic_irqs_pending(SiFivePLICState *plic, uint32_t addrid)
+static uint32_t sifive_plic_claimed(SiFivePLICState *plic, uint32_t addrid)
 {
+    uint32_t max_irq = 0;
+    uint32_t max_prio = plic->target_priority[addrid];
     int i, j;
+
     for (i = 0; i < plic->bitfield_words; i++) {
         uint32_t pending_enabled_not_claimed =
-            (plic->pending[i] & ~plic->claimed[i]) &
-            plic->enable[addrid * plic->bitfield_words + i];
+                        (plic->pending[i] & ~plic->claimed[i]) &
+                            plic->enable[addrid * plic->bitfield_words + i];
+
         if (!pending_enabled_not_claimed) {
             continue;
         }
+
         for (j = 0; j < 32; j++) {
             int irq = (i << 5) + j;
             uint32_t prio = plic->source_priority[irq];
             int enabled = pending_enabled_not_claimed & (1 << j);
-            if (enabled && prio > plic->target_priority[addrid]) {
-                return 1;
+
+            if (enabled && prio > max_prio) {
+                max_irq = irq;
+                max_prio = prio;
             }
         }
     }
-    return 0;
+
+    return max_irq;
 }
 
 static void sifive_plic_update(SiFivePLICState *plic)
@@ -145,7 +110,7 @@ static void sifive_plic_update(SiFivePLICState *plic)
     for (addrid = 0; addrid < plic->num_addrs; addrid++) {
         uint32_t hartid = plic->addr_config[addrid].hartid;
         PLICMode mode = plic->addr_config[addrid].mode;
-        int level = sifive_plic_irqs_pending(plic, addrid);
+        bool level = !!sifive_plic_claimed(plic, addrid);
 
         switch (mode) {
         case PLICMode_M:
@@ -158,41 +123,6 @@ static void sifive_plic_update(SiFivePLICState *plic)
             break;
         }
     }
-
-    if (RISCV_DEBUG_PLIC) {
-        sifive_plic_print_state(plic);
-    }
-}
-
-static uint32_t sifive_plic_claim(SiFivePLICState *plic, uint32_t addrid)
-{
-    int i, j;
-    uint32_t max_irq = 0;
-    uint32_t max_prio = plic->target_priority[addrid];
-
-    for (i = 0; i < plic->bitfield_words; i++) {
-        uint32_t pending_enabled_not_claimed =
-            (plic->pending[i] & ~plic->claimed[i]) &
-            plic->enable[addrid * plic->bitfield_words + i];
-        if (!pending_enabled_not_claimed) {
-            continue;
-        }
-        for (j = 0; j < 32; j++) {
-            int irq = (i << 5) + j;
-            uint32_t prio = plic->source_priority[irq];
-            int enabled = pending_enabled_not_claimed & (1 << j);
-            if (enabled && prio > max_prio) {
-                max_irq = irq;
-                max_prio = prio;
-            }
-        }
-    }
-
-    if (max_irq) {
-        sifive_plic_set_pending(plic, max_irq, false);
-        sifive_plic_set_claimed(plic, max_irq, true);
-    }
-    return max_irq;
 }
 
 static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
@@ -223,10 +153,15 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
         if (contextid == 0) {
             return plic->target_priority[addrid];
         } else if (contextid == 4) {
-            uint32_t value = sifive_plic_claim(plic, addrid);
+            uint32_t max_irq = sifive_plic_claimed(plic, addrid);
+
+            if (max_irq) {
+                sifive_plic_set_pending(plic, max_irq, false);
+                sifive_plic_set_claimed(plic, max_irq, true);
+            }
 
             sifive_plic_update(plic);
-            return value;
+            return max_irq;
         }
     }
 
-- 
2.31.1

Re: [PATCH v2 4/9] hw/intc: sifive_plic: Cleanup remaining functions

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We can remove the original sifive_plic_irqs_pending() function and
> instead just use the sifive_plic_claim() function (renamed to
> sifive_plic_claimed()) to determine if any interrupts are pending.
>
> This requires move the side effects outside of sifive_plic_claimed(),
> but as they are only invoked once that isn't a problem.
>
> We have also removed all of the old #ifdef debugging logs, so let's
> cleanup the last remaining debug function while we are here.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 109 +++++++++---------------------------------
>  1 file changed, 22 insertions(+), 87 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 4/9] hw/intc: sifive_plic: Cleanup remaining functions

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> We can remove the original sifive_plic_irqs_pending() function and
> instead just use the sifive_plic_claim() function (renamed to
> sifive_plic_claimed()) to determine if any interrupts are pending.
>
> This requires move the side effects outside of sifive_plic_claimed(),
> but as they are only invoked once that isn't a problem.
>
> We have also removed all of the old #ifdef debugging logs, so let's
> cleanup the last remaining debug function while we are here.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/intc/sifive_plic.c | 109 +++++++++---------------------------------
>  1 file changed, 22 insertions(+), 87 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 5/9] target/riscv: Mark the Hypervisor extension as non experimental

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

The Hypervisor spec is now frozen, so remove the experimental tag.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f812998123..1edb2771b4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -626,6 +626,7 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
     DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
     DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
+    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
     DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
     DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
     DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
@@ -639,7 +640,6 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("x-zbb", RISCVCPU, cfg.ext_zbb, false),
     DEFINE_PROP_BOOL("x-zbc", RISCVCPU, cfg.ext_zbc, false),
     DEFINE_PROP_BOOL("x-zbs", RISCVCPU, cfg.ext_zbs, false),
-    DEFINE_PROP_BOOL("x-h", RISCVCPU, cfg.ext_h, false),
     DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
     DEFINE_PROP_BOOL("x-v", RISCVCPU, cfg.ext_v, false),
     DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
-- 
2.31.1

Re: [PATCH v2 5/9] target/riscv: Mark the Hypervisor extension as non experimental

[Anup Patel]

On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The Hypervisor spec is now frozen, so remove the experimental tag.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup

> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f812998123..1edb2771b4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -626,6 +626,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
>      DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
>      DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> +    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
>      DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> @@ -639,7 +640,6 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("x-zbb", RISCVCPU, cfg.ext_zbb, false),
>      DEFINE_PROP_BOOL("x-zbc", RISCVCPU, cfg.ext_zbc, false),
>      DEFINE_PROP_BOOL("x-zbs", RISCVCPU, cfg.ext_zbs, false),
> -    DEFINE_PROP_BOOL("x-h", RISCVCPU, cfg.ext_h, false),
>      DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
>      DEFINE_PROP_BOOL("x-v", RISCVCPU, cfg.ext_v, false),
>      DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
> --
> 2.31.1
>
>

Re: [PATCH v2 5/9] target/riscv: Mark the Hypervisor extension as non experimental

[Anup Patel]

On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The Hypervisor spec is now frozen, so remove the experimental tag.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup

> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f812998123..1edb2771b4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -626,6 +626,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
>      DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
>      DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> +    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
>      DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> @@ -639,7 +640,6 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("x-zbb", RISCVCPU, cfg.ext_zbb, false),
>      DEFINE_PROP_BOOL("x-zbc", RISCVCPU, cfg.ext_zbc, false),
>      DEFINE_PROP_BOOL("x-zbs", RISCVCPU, cfg.ext_zbs, false),
> -    DEFINE_PROP_BOOL("x-h", RISCVCPU, cfg.ext_h, false),
>      DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
>      DEFINE_PROP_BOOL("x-v", RISCVCPU, cfg.ext_v, false),
>      DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
> --
> 2.31.1
>
>

Re: [PATCH v2 5/9] target/riscv: Mark the Hypervisor extension as non experimental

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The Hypervisor spec is now frozen, so remove the experimental tag.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 5/9] target/riscv: Mark the Hypervisor extension as non experimental

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The Hypervisor spec is now frozen, so remove the experimental tag.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 6/9] target/riscv: Enable the Hypervisor extension by default

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Let's enable the Hypervisor extension by default. This doesn't affect
named CPUs (such as lowrisc-ibex or sifive-u54) but does enable the
Hypervisor extensions by default for the virt machine.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1edb2771b4..013a8760b5 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -626,7 +626,7 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
     DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
     DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
-    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
+    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
     DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
     DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
     DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
-- 
2.31.1

Re: [PATCH v2 6/9] target/riscv: Enable the Hypervisor extension by default

[Anup Patel]

On Thu, Dec 16, 2021 at 10:29 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Let's enable the Hypervisor extension by default. This doesn't affect
> named CPUs (such as lowrisc-ibex or sifive-u54) but does enable the
> Hypervisor extensions by default for the virt machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1edb2771b4..013a8760b5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -626,7 +626,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
>      DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
>      DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> -    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
> +    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
>      DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> --
> 2.31.1
>
>

Re: [PATCH v2 6/9] target/riscv: Enable the Hypervisor extension by default

[Anup Patel]

On Thu, Dec 16, 2021 at 10:29 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Let's enable the Hypervisor extension by default. This doesn't affect
> named CPUs (such as lowrisc-ibex or sifive-u54) but does enable the
> Hypervisor extensions by default for the virt machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1edb2771b4..013a8760b5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -626,7 +626,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
>      DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
>      DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> -    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, false),
> +    DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
>      DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> --
> 2.31.1
>
>

Re: [PATCH v2 6/9] target/riscv: Enable the Hypervisor extension by default

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Let's enable the Hypervisor extension by default. This doesn't affect
> named CPUs (such as lowrisc-ibex or sifive-u54) but does enable the
> Hypervisor extensions by default for the virt machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 6/9] target/riscv: Enable the Hypervisor extension by default

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Let's enable the Hypervisor extension by default. This doesn't affect
> named CPUs (such as lowrisc-ibex or sifive-u54) but does enable the
> Hypervisor extensions by default for the virt machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 7/9] hw/riscv: Use error_fatal for SoC realisation

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

When realising the SoC use error_fatal instead of error_abort as the
process can fail and report useful information to the user.

Currently a user can see this:

   $ ../qemu/bld/qemu-system-riscv64 -M sifive_u -S -monitor stdio -display none -drive if=pflash
    QEMU 6.1.93 monitor - type 'help' for more information
    (qemu) Unexpected error in sifive_u_otp_realize() at ../hw/misc/sifive_u_otp.c:229:
    qemu-system-riscv64: OTP drive size < 16K
    Aborted (core dumped)

Which this patch addresses

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reported-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/riscv/microchip_pfsoc.c | 2 +-
 hw/riscv/opentitan.c       | 2 +-
 hw/riscv/sifive_e.c        | 2 +-
 hw/riscv/sifive_u.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 57d779fb55..f16e4d10eb 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -471,7 +471,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
     /* Initialize SoC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc,
                             TYPE_MICROCHIP_PFSOC);
-    qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
+    qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* Split RAM into low and high regions using aliases to machine->ram */
     mem_low_size = memmap[MICROCHIP_PFSOC_DRAM_LO].size;
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index c531450b9f..0856c347e8 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -80,7 +80,7 @@ static void opentitan_board_init(MachineState *machine)
     /* Initialize SoC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc,
                             TYPE_RISCV_IBEX_SOC);
-    qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
+    qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     memory_region_add_subregion(sys_mem,
         memmap[IBEX_DEV_RAM].base, machine->ram);
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 9b206407a6..dcb87b6cfd 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -88,7 +88,7 @@ static void sifive_e_machine_init(MachineState *machine)
 
     /* Initialize SoC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc, TYPE_RISCV_E_SOC);
-    qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
+    qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* Data Tightly Integrated Memory */
     memory_region_add_subregion(sys_mem,
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 589ae72a59..d576484851 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -545,7 +545,7 @@ static void sifive_u_machine_init(MachineState *machine)
                              &error_abort);
     object_property_set_str(OBJECT(&s->soc), "cpu-type", machine->cpu_type,
                              &error_abort);
-    qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
+    qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* register RAM */
     memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DEV_DRAM].base,
-- 
2.31.1

Re: [PATCH v2 7/9] hw/riscv: Use error_fatal for SoC realisation

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> When realising the SoC use error_fatal instead of error_abort as the
> process can fail and report useful information to the user.
>
> Currently a user can see this:
>
>    $ ../qemu/bld/qemu-system-riscv64 -M sifive_u -S -monitor stdio -display none -drive if=pflash
>     QEMU 6.1.93 monitor - type 'help' for more information
>     (qemu) Unexpected error in sifive_u_otp_realize() at ../hw/misc/sifive_u_otp.c:229:
>     qemu-system-riscv64: OTP drive size < 16K
>     Aborted (core dumped)
>
> Which this patch addresses
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/riscv/microchip_pfsoc.c | 2 +-
>  hw/riscv/opentitan.c       | 2 +-
>  hw/riscv/sifive_e.c        | 2 +-
>  hw/riscv/sifive_u.c        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 7/9] hw/riscv: Use error_fatal for SoC realisation

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> When realising the SoC use error_fatal instead of error_abort as the
> process can fail and report useful information to the user.
>
> Currently a user can see this:
>
>    $ ../qemu/bld/qemu-system-riscv64 -M sifive_u -S -monitor stdio -display none -drive if=pflash
>     QEMU 6.1.93 monitor - type 'help' for more information
>     (qemu) Unexpected error in sifive_u_otp_realize() at ../hw/misc/sifive_u_otp.c:229:
>     qemu-system-riscv64: OTP drive size < 16K
>     Aborted (core dumped)
>
> Which this patch addresses
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/riscv/microchip_pfsoc.c | 2 +-
>  hw/riscv/opentitan.c       | 2 +-
>  hw/riscv/sifive_e.c        | 2 +-
>  hw/riscv/sifive_u.c        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
let's set that as the maximum for the virt board.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/hw/riscv/virt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index b8ef99f348..6e9f61ccd9 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -24,7 +24,7 @@
 #include "hw/block/flash.h"
 #include "qom/object.h"
 
-#define VIRT_CPUS_MAX 8
+#define VIRT_CPUS_MAX 32
 #define VIRT_SOCKETS_MAX 8
 
 #define TYPE_RISCV_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
-- 
2.31.1

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Anup Patel]

On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> let's set that as the maximum for the virt board.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Overall, no issues with this patch. I have added PATCH23 in
the AIA v5 series which allows upto 512 CPUs . This is a hard
limit based on address space based on PLIC, ACLINT, IMSIC,
and APLIC address utilization and is also suitable for both RV32
and RV64.

IMO, we should keep QEMU VIRT_CPUS_MAX as high as
possible to allow any kind of software Linux, OpenSBI, FreeBSD,
Xvisor, Xen, etc. Let the guest software decide it's own limit (such
as NR_CPUS of Linux).

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup

> ---
>  include/hw/riscv/virt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index b8ef99f348..6e9f61ccd9 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -24,7 +24,7 @@
>  #include "hw/block/flash.h"
>  #include "qom/object.h"
>
> -#define VIRT_CPUS_MAX 8
> +#define VIRT_CPUS_MAX 32
>  #define VIRT_SOCKETS_MAX 8
>
>  #define TYPE_RISCV_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> --
> 2.31.1
>
>

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Anup Patel]

On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> let's set that as the maximum for the virt board.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Overall, no issues with this patch. I have added PATCH23 in
the AIA v5 series which allows upto 512 CPUs . This is a hard
limit based on address space based on PLIC, ACLINT, IMSIC,
and APLIC address utilization and is also suitable for both RV32
and RV64.

IMO, we should keep QEMU VIRT_CPUS_MAX as high as
possible to allow any kind of software Linux, OpenSBI, FreeBSD,
Xvisor, Xen, etc. Let the guest software decide it's own limit (such
as NR_CPUS of Linux).

Reviewed-by: Anup Patel <anup.patel@wdc.com>

Regards,
Anup

> ---
>  include/hw/riscv/virt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index b8ef99f348..6e9f61ccd9 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -24,7 +24,7 @@
>  #include "hw/block/flash.h"
>  #include "qom/object.h"
>
> -#define VIRT_CPUS_MAX 8
> +#define VIRT_CPUS_MAX 32
>  #define VIRT_SOCKETS_MAX 8
>
>  #define TYPE_RISCV_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> --
> 2.31.1
>
>

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Philippe Mathieu-Daudé]

On 12/16/21 06:58, Anup Patel wrote:
> On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
>>
>> From: Alistair Francis <alistair.francis@wdc.com>
>>
>> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
>> let's set that as the maximum for the virt board.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

> IMO, we should keep QEMU VIRT_CPUS_MAX as high as
> possible to allow any kind of software Linux, OpenSBI, FreeBSD,
> Xvisor, Xen, etc. Let the guest software decide it's own limit (such
> as NR_CPUS of Linux).

Agreed.

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

> 
> Reviewed-by: Anup Patel <anup.patel@wdc.com>
> 
> Regards,
> Anup

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Alistair Francis]

On Thu, Dec 16, 2021 at 6:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 12/16/21 06:58, Anup Patel wrote:
> > On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> >>
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> >> let's set that as the maximum for the virt board.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> > IMO, we should keep QEMU VIRT_CPUS_MAX as high as
> > possible to allow any kind of software Linux, OpenSBI, FreeBSD,
> > Xvisor, Xen, etc. Let the guest software decide it's own limit (such
> > as NR_CPUS of Linux).
>
> Agreed.

I agree as well. I'm happy to increase this in the future, the problem
is that I am only able to test it with 32 cores on Linux.

Alistair

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Alistair Francis]

On Thu, Dec 16, 2021 at 6:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 12/16/21 06:58, Anup Patel wrote:
> > On Thu, Dec 16, 2021 at 10:27 AM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> >>
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> >> let's set that as the maximum for the virt board.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>
> > IMO, we should keep QEMU VIRT_CPUS_MAX as high as
> > possible to allow any kind of software Linux, OpenSBI, FreeBSD,
> > Xvisor, Xen, etc. Let the guest software decide it's own limit (such
> > as NR_CPUS of Linux).
>
> Agreed.

I agree as well. I'm happy to increase this in the future, the problem
is that I am only able to test it with 32 cores on Linux.

Alistair

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> let's set that as the maximum for the virt board.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  include/hw/riscv/virt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v2 8/9] hw/riscv: virt: Allow support for 32 cores

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Linux supports up to 32 cores for both 32-bit and 64-bit RISC-V, so
> let's set that as the maximum for the virt board.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/435
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  include/hw/riscv/virt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Alistair Francis]

From: Alistair Francis <alistair.francis@wdc.com>

As per the device tree specification let's set the clock-frequency for
the virt CPUs.

QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 3af074148e..41a85cfc60 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
         qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
             s->soc[socket].hartid_base + cpu);
         qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
+        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);
         riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
         qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);
 
-- 
2.31.1

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Anup Patel]

On Thu, Dec 16, 2021 at 10:31 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> As per the device tree specification let's set the clock-frequency for
> the virt CPUs.
>
> QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3af074148e..41a85cfc60 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);

Instead of hard-coding, please use the same define used for timebase-frequency
(i.e. RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ)

Regards,
Anup

>          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);
>
> --
> 2.31.1
>
>

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Anup Patel]

On Thu, Dec 16, 2021 at 10:31 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> As per the device tree specification let's set the clock-frequency for
> the virt CPUs.
>
> QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3af074148e..41a85cfc60 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);

Instead of hard-coding, please use the same define used for timebase-frequency
(i.e. RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ)

Regards,
Anup

>          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);
>
> --
> 2.31.1
>
>

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> As per the device tree specification let's set the clock-frequency for
> the virt CPUs.
>
> QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)

I tend to think this issue is not valid, as the comment in the issue
says the value should reflect the running frequency.

Note the "clock-frequency" was once in the codes but was intentionally
removed before because it is not needed:

See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
of cpu nodes")

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3af074148e..41a85cfc60 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);
>          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);

Regards,
Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Bin Meng]

On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> As per the device tree specification let's set the clock-frequency for
> the virt CPUs.
>
> QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)

I tend to think this issue is not valid, as the comment in the issue
says the value should reflect the running frequency.

Note the "clock-frequency" was once in the codes but was intentionally
removed before because it is not needed:

See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
of cpu nodes")

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3af074148e..41a85cfc60 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);
>          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
>          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);

Regards,
Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Alistair Francis]

On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > As per the device tree specification let's set the clock-frequency for
> > the virt CPUs.
> >
> > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
>
> I tend to think this issue is not valid, as the comment in the issue
> says the value should reflect the running frequency.
>
> Note the "clock-frequency" was once in the codes but was intentionally
> removed before because it is not needed:
>
> See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> of cpu nodes")

Hmmm... Good point.

But if the device tree spec says it's required then I think we should
include it. Even if it isn't super relevant for us

Alistair

>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 3af074148e..41a85cfc60 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
> >              s->soc[socket].hartid_base + cpu);
> >          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> > +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);
> >          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
> >          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);
>
> Regards,
> Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Alistair Francis]

On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > As per the device tree specification let's set the clock-frequency for
> > the virt CPUs.
> >
> > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
>
> I tend to think this issue is not valid, as the comment in the issue
> says the value should reflect the running frequency.
>
> Note the "clock-frequency" was once in the codes but was intentionally
> removed before because it is not needed:
>
> See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> of cpu nodes")

Hmmm... Good point.

But if the device tree spec says it's required then I think we should
include it. Even if it isn't super relevant for us

Alistair

>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 3af074148e..41a85cfc60 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -202,6 +202,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg",
> >              s->soc[socket].hartid_base + cpu);
> >          qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu");
> > +        qemu_fdt_setprop_cell(mc->fdt, cpu_name, "clock-frequency", 1000000);
> >          riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket);
> >          qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle);
>
> Regards,
> Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Bin Meng]

On Tue, Dec 21, 2021 at 2:32 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > As per the device tree specification let's set the clock-frequency for
> > > the virt CPUs.
> > >
> > > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> >
> > I tend to think this issue is not valid, as the comment in the issue
> > says the value should reflect the running frequency.
> >
> > Note the "clock-frequency" was once in the codes but was intentionally
> > removed before because it is not needed:
> >
> > See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> > of cpu nodes")
>
> Hmmm... Good point.
>
> But if the device tree spec says it's required then I think we should
> include it. Even if it isn't super relevant for us

The Linux kernel upstream RISC-V DTS files also don't have
"clock-frequency" properties. I doubt there is benefit to provide one.

Regards,
Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Bin Meng]

On Tue, Dec 21, 2021 at 2:32 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > As per the device tree specification let's set the clock-frequency for
> > > the virt CPUs.
> > >
> > > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> >
> > I tend to think this issue is not valid, as the comment in the issue
> > says the value should reflect the running frequency.
> >
> > Note the "clock-frequency" was once in the codes but was intentionally
> > removed before because it is not needed:
> >
> > See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> > of cpu nodes")
>
> Hmmm... Good point.
>
> But if the device tree spec says it's required then I think we should
> include it. Even if it isn't super relevant for us

The Linux kernel upstream RISC-V DTS files also don't have
"clock-frequency" properties. I doubt there is benefit to provide one.

Regards,
Bin

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Peter Maydell]

On Tue, 21 Dec 2021 at 06:35, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > As per the device tree specification let's set the clock-frequency for
> > > the virt CPUs.
> > >
> > > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> >
> > I tend to think this issue is not valid, as the comment in the issue
> > says the value should reflect the running frequency.
> >
> > Note the "clock-frequency" was once in the codes but was intentionally
> > removed before because it is not needed:
> >
> > See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> > of cpu nodes")
>
> Hmmm... Good point.
>
> But if the device tree spec says it's required then I think we should
> include it. Even if it isn't super relevant for us

I talked with one of the device tree spec maintainers, and he said
that the "required" note on this property in the spec is a leftover
from the spec text for PPC ePAPR, and it isn't actually required
these days. Many DTs for real-hardware Arm boards don't set the
property, and it's not marked as required in the devicetree
yaml schema for the cpu node:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpus.yaml#L63

thanks
-- PMM

Re: [PATCH v2 9/9] hw/riscv: virt: Set the clock-frequency

[Peter Maydell]

On Tue, 21 Dec 2021 at 06:35, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 5:52 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 12:55 PM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > As per the device tree specification let's set the clock-frequency for
> > > the virt CPUs.
> > >
> > > QEMU doesn't really have an exact clock, so let's just 1000000 as it's a
> > > nice round number and matches the sifive_u CLINT_TIMEBASE_FREQ.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/472 (RISC-V virt)
> >
> > I tend to think this issue is not valid, as the comment in the issue
> > says the value should reflect the running frequency.
> >
> > Note the "clock-frequency" was once in the codes but was intentionally
> > removed before because it is not needed:
> >
> > See commit 7ae05377b85f (" riscv: hw: Drop "clock-frequency" property
> > of cpu nodes")
>
> Hmmm... Good point.
>
> But if the device tree spec says it's required then I think we should
> include it. Even if it isn't super relevant for us

I talked with one of the device tree spec maintainers, and he said
that the "required" note on this property in the spec is a leftover
from the spec text for PPC ePAPR, and it isn't actually required
these days. Many DTs for real-hardware Arm boards don't set the
property, and it's not marked as required in the devicetree
yaml schema for the cpu node:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpus.yaml#L63

thanks
-- PMM