From 0a9a6cba8b9cad8786670fd8c9fa1b0d39bd00e8 Mon Sep 17 00:00:00 2001 From: Frédéric Pétrot Date: Mon, 14 Nov 2022 14:51:22 +0100 Subject: hw/intc: sifive_plic: Renumber the S irqs for numa support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 40244040a7a changed the way the S irqs are numbered. This breaks when using numa configuration, e.g.: ./qemu-system-riscv64 -nographic -machine virt,dumpdtb=numa-tree.dtb \ -m 2G -smp cpus=16 \ -object memory-backend-ram,id=mem0,size=512M \ -object memory-backend-ram,id=mem1,size=512M \ -object memory-backend-ram,id=mem2,size=512M \ -object memory-backend-ram,id=mem3,size=512M \ -numa node,cpus=0-3,memdev=mem0,nodeid=0 \ -numa node,cpus=4-7,memdev=mem1,nodeid=1 \ -numa node,cpus=8-11,memdev=mem2,nodeid=2 \ -numa node,cpus=12-15,memdev=mem3,nodeid=3 leads to: Unexpected error in object_property_find_err() at ../qom/object.c:1304: qemu-system-riscv64: Property 'riscv.sifive.plic.unnamed-gpio-out[8]' not found This patch makes the nubering of the S irqs identical to what it was before. Reviewed-by: Alistair Francis Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Frédéric Pétrot Message-Id: <20221114135122.1668703-1-frederic.petrot@univ-grenoble-alpes.fr> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index c2dfacf028..b4949bef97 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -476,11 +476,11 @@ DeviceState *sifive_plic_create(hwaddr addr, char *hart_config, CPUState *cpu = qemu_get_cpu(cpu_num); if (plic->addr_config[i].mode == PLICMode_M) { - qdev_connect_gpio_out(dev, num_harts - plic->hartid_base + cpu_num, + qdev_connect_gpio_out(dev, cpu_num - hartid_base + num_harts, qdev_get_gpio_in(DEVICE(cpu), IRQ_M_EXT)); } if (plic->addr_config[i].mode == PLICMode_S) { - qdev_connect_gpio_out(dev, cpu_num, + qdev_connect_gpio_out(dev, cpu_num - hartid_base, qdev_get_gpio_in(DEVICE(cpu), IRQ_S_EXT)); } } -- cgit 1.4.1 From bb22d391121fc0de42a04d1ed99f602441ea70e1 Mon Sep 17 00:00:00 2001 From: Jim Shu Date: Sun, 27 Nov 2022 16:57:46 +0000 Subject: hw/intc: sifive_plic: fix out-of-bound access of source_priority array If the number of interrupt is not multiple of 32, PLIC will have out-of-bound access to source_priority array. Compute the number of interrupt in the last word to avoid this out-of-bound access of array. Signed-off-by: Jim Shu Reviewed-by: Bin Meng Message-Id: <20221127165753.30533-1-jim.shu@sifive.com> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index b4949bef97..0c7696520d 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -78,6 +78,7 @@ 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; + int num_irq_in_word = 32; for (i = 0; i < plic->bitfield_words; i++) { uint32_t pending_enabled_not_claimed = @@ -88,7 +89,16 @@ static uint32_t sifive_plic_claimed(SiFivePLICState *plic, uint32_t addrid) continue; } - for (j = 0; j < 32; j++) { + if (i == (plic->bitfield_words - 1)) { + /* + * If plic->num_sources is not multiple of 32, num-of-irq in last + * word is not 32. Compute the num-of-irq of last word to avoid + * out-of-bound access of source_priority array. + */ + num_irq_in_word = plic->num_sources - ((plic->bitfield_words - 1) << 5); + } + + for (j = 0; j < num_irq_in_word; j++) { int irq = (i << 5) + j; uint32_t prio = plic->source_priority[irq]; int enabled = pending_enabled_not_claimed & (1 << j); -- cgit 1.4.1 From 2904dc1c1e7895a65a2ac18ea4bb1042d6dced50 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:19 +0800 Subject: hw/intc: sifive_plic: Drop PLICMode_H H-mode has been removed since priv spec 1.10. Drop it. Signed-off-by: Bin Meng Reviewed-by: Wilfred Mallawa Reviewed-by: Alistair Francis Message-Id: <20221211030829.802437-6-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 1 - include/hw/intc/sifive_plic.h | 1 - 2 files changed, 2 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 0c7696520d..936dcf74bc 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -42,7 +42,6 @@ static PLICMode char_to_mode(char c) switch (c) { case 'U': return PLICMode_U; case 'S': return PLICMode_S; - case 'H': return PLICMode_H; case 'M': return PLICMode_M; default: error_report("plic: invalid mode '%c'", c); diff --git a/include/hw/intc/sifive_plic.h b/include/hw/intc/sifive_plic.h index 134cf39a96..d3f45ec248 100644 --- a/include/hw/intc/sifive_plic.h +++ b/include/hw/intc/sifive_plic.h @@ -33,7 +33,6 @@ DECLARE_INSTANCE_CHECKER(SiFivePLICState, SIFIVE_PLIC, typedef enum PLICMode { PLICMode_U, PLICMode_S, - PLICMode_H, PLICMode_M } PLICMode; -- cgit 1.4.1 From 7b0f26e420f7988ae8c257c1f4369ba92c9b1594 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:20 +0800 Subject: hw/intc: sifive_plic: Improve robustness of the PLIC config parser At present the PLIC config parser can only handle legal config string like "MS,MS". However if a config string like ",MS,MS,,MS,MS,," is given the parser won't get the correct configuration. This commit improves the config parser to make it more robust. Signed-off-by: Bin Meng Acked-by: Alistair Francis Message-Id: <20221211030829.802437-7-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 936dcf74bc..c9af94a888 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -290,7 +290,7 @@ static void sifive_plic_reset(DeviceState *dev) */ static void parse_hart_config(SiFivePLICState *plic) { - int addrid, hartid, modes; + int addrid, hartid, modes, m; const char *p; char c; @@ -299,11 +299,13 @@ static void parse_hart_config(SiFivePLICState *plic) p = plic->hart_config; while ((c = *p++)) { if (c == ',') { - addrid += ctpop8(modes); - modes = 0; - hartid++; + if (modes) { + addrid += ctpop8(modes); + hartid++; + modes = 0; + } } else { - int m = 1 << char_to_mode(c); + m = 1 << char_to_mode(c); if (modes == (modes | m)) { error_report("plic: duplicate mode '%c' in config: %s", c, plic->hart_config); @@ -314,8 +316,9 @@ static void parse_hart_config(SiFivePLICState *plic) } if (modes) { addrid += ctpop8(modes); + hartid++; + modes = 0; } - hartid++; plic->num_addrs = addrid; plic->num_harts = hartid; @@ -326,11 +329,16 @@ static void parse_hart_config(SiFivePLICState *plic) p = plic->hart_config; while ((c = *p++)) { if (c == ',') { - hartid++; + if (modes) { + hartid++; + modes = 0; + } } else { + m = char_to_mode(c); plic->addr_config[addrid].addrid = addrid; plic->addr_config[addrid].hartid = hartid; - plic->addr_config[addrid].mode = char_to_mode(c); + plic->addr_config[addrid].mode = m; + modes |= (1 << m); addrid++; } } -- cgit 1.4.1 From 35401578e25caca8b3d94accf3999abf36ea3a4b Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:21 +0800 Subject: hw/intc: sifive_plic: Use error_setg() to propagate the error up via errp in sifive_plic_realize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The realize() callback has an errp for us to propagate the error up. While we are here, correct the wrong multi-line comment format. Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20221211030829.802437-8-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index c9af94a888..9cb4c6d6d4 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -379,7 +379,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) s->m_external_irqs = g_malloc(sizeof(qemu_irq) * s->num_harts); qdev_init_gpio_out(dev, s->m_external_irqs, s->num_harts); - /* We can't allow the supervisor to control SEIP as this would allow the + /* + * We can't allow the supervisor to control SEIP as this would allow the * supervisor to clear a pending external interrupt which will result in * lost a interrupt in the case a PLIC is attached. The SEIP bit must be * hardware controlled when a PLIC is attached. @@ -387,8 +388,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) for (i = 0; i < s->num_harts; i++) { RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(s->hartid_base + i)); if (riscv_cpu_claim_interrupts(cpu, MIP_SEIP) < 0) { - error_report("SEIP already claimed"); - exit(1); + error_setg(errp, "SEIP already claimed"); + return; } } -- cgit 1.4.1 From e8fe2bc11713897453580aa36085556cade63826 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:22 +0800 Subject: hw/intc: sifive_plic: Update "num-sources" property default value At present the default value of "num-sources" property is zero, which does not make a lot of sense, as in sifive_plic_realize() we see s->bitfield_words is calculated by: s->bitfield_words = (s->num_sources + 31) >> 5; if the we don't configure "num-sources" property its default value zero makes s->bitfield_words zero too, which isn't true because interrupt source 0 still occupies one word. Let's change the default value to 1 meaning that only interrupt source 0 is supported by default and a sanity check in realize(). While we are here, add a comment to describe the exact meaning of this property that the number should include interrupt source 0. Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20221211030829.802437-9-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 9cb4c6d6d4..1edeb1e1ed 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -363,6 +363,11 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) parse_hart_config(s); + if (!s->num_sources) { + error_setg(errp, "plic: invalid number of interrupt sources"); + return; + } + s->bitfield_words = (s->num_sources + 31) >> 5; s->num_enables = s->bitfield_words * s->num_addrs; s->source_priority = g_new0(uint32_t, s->num_sources); @@ -420,7 +425,8 @@ static const VMStateDescription vmstate_sifive_plic = { static Property sifive_plic_properties[] = { DEFINE_PROP_STRING("hart-config", SiFivePLICState, hart_config), DEFINE_PROP_UINT32("hartid-base", SiFivePLICState, hartid_base, 0), - DEFINE_PROP_UINT32("num-sources", SiFivePLICState, num_sources, 0), + /* number of interrupt sources including interrupt source 0 */ + DEFINE_PROP_UINT32("num-sources", SiFivePLICState, num_sources, 1), DEFINE_PROP_UINT32("num-priorities", SiFivePLICState, num_priorities, 0), DEFINE_PROP_UINT32("priority-base", SiFivePLICState, priority_base, 0), DEFINE_PROP_UINT32("pending-base", SiFivePLICState, pending_base, 0), -- cgit 1.4.1 From 5decd2c5218379ad8362b932461d139eab7205fa Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:27 +0800 Subject: hw/intc: sifive_plic: Change "priority-base" to start from interrupt source 0 At present the SiFive PLIC model "priority-base" expects interrupt priority register base starting from source 1 instead source 0, that's why on most platforms "priority-base" is set to 0x04 except 'opentitan' machine. 'opentitan' should have set "priority-base" to 0x04 too. Note the irq number calculation in sifive_plic_{read,write} is correct as the codes make up for the irq number by adding 1. Let's simply update "priority-base" to start from interrupt source 0 and add a comment to make it crystal clear. Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Reviewed-by: Wilfred Mallawa Message-Id: <20221211030829.802437-14-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 5 +++-- include/hw/riscv/microchip_pfsoc.h | 2 +- include/hw/riscv/shakti_c.h | 2 +- include/hw/riscv/sifive_e.h | 2 +- include/hw/riscv/sifive_u.h | 2 +- include/hw/riscv/virt.h | 2 +- 6 files changed, 8 insertions(+), 7 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 1edeb1e1ed..1a792cc3f5 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -140,7 +140,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size) SiFivePLICState *plic = opaque; if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; + uint32_t irq = (addr - plic->priority_base) >> 2; return plic->source_priority[irq]; } else if (addr_between(addr, plic->pending_base, plic->num_sources >> 3)) { @@ -187,7 +187,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value, SiFivePLICState *plic = opaque; if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; + uint32_t irq = (addr - plic->priority_base) >> 2; if (((plic->num_priorities + 1) & plic->num_priorities) == 0) { /* @@ -428,6 +428,7 @@ static Property sifive_plic_properties[] = { /* number of interrupt sources including interrupt source 0 */ DEFINE_PROP_UINT32("num-sources", SiFivePLICState, num_sources, 1), DEFINE_PROP_UINT32("num-priorities", SiFivePLICState, num_priorities, 0), + /* interrupt priority register base starting from source 0 */ DEFINE_PROP_UINT32("priority-base", SiFivePLICState, priority_base, 0), DEFINE_PROP_UINT32("pending-base", SiFivePLICState, pending_base, 0), DEFINE_PROP_UINT32("enable-base", SiFivePLICState, enable_base, 0), diff --git a/include/hw/riscv/microchip_pfsoc.h b/include/hw/riscv/microchip_pfsoc.h index 577efad0c4..e65ffeb02d 100644 --- a/include/hw/riscv/microchip_pfsoc.h +++ b/include/hw/riscv/microchip_pfsoc.h @@ -155,7 +155,7 @@ enum { #define MICROCHIP_PFSOC_PLIC_NUM_SOURCES 187 #define MICROCHIP_PFSOC_PLIC_NUM_PRIORITIES 7 -#define MICROCHIP_PFSOC_PLIC_PRIORITY_BASE 0x04 +#define MICROCHIP_PFSOC_PLIC_PRIORITY_BASE 0x00 #define MICROCHIP_PFSOC_PLIC_PENDING_BASE 0x1000 #define MICROCHIP_PFSOC_PLIC_ENABLE_BASE 0x2000 #define MICROCHIP_PFSOC_PLIC_ENABLE_STRIDE 0x80 diff --git a/include/hw/riscv/shakti_c.h b/include/hw/riscv/shakti_c.h index daf0aae13f..539fe1156d 100644 --- a/include/hw/riscv/shakti_c.h +++ b/include/hw/riscv/shakti_c.h @@ -65,7 +65,7 @@ enum { #define SHAKTI_C_PLIC_NUM_SOURCES 28 /* Excluding Priority 0 */ #define SHAKTI_C_PLIC_NUM_PRIORITIES 2 -#define SHAKTI_C_PLIC_PRIORITY_BASE 0x04 +#define SHAKTI_C_PLIC_PRIORITY_BASE 0x00 #define SHAKTI_C_PLIC_PENDING_BASE 0x1000 #define SHAKTI_C_PLIC_ENABLE_BASE 0x2000 #define SHAKTI_C_PLIC_ENABLE_STRIDE 0x80 diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h index 9e58247fd8..b824a79e2d 100644 --- a/include/hw/riscv/sifive_e.h +++ b/include/hw/riscv/sifive_e.h @@ -89,7 +89,7 @@ enum { */ #define SIFIVE_E_PLIC_NUM_SOURCES 53 #define SIFIVE_E_PLIC_NUM_PRIORITIES 7 -#define SIFIVE_E_PLIC_PRIORITY_BASE 0x04 +#define SIFIVE_E_PLIC_PRIORITY_BASE 0x00 #define SIFIVE_E_PLIC_PENDING_BASE 0x1000 #define SIFIVE_E_PLIC_ENABLE_BASE 0x2000 #define SIFIVE_E_PLIC_ENABLE_STRIDE 0x80 diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h index 8f63a183c4..e680d61ece 100644 --- a/include/hw/riscv/sifive_u.h +++ b/include/hw/riscv/sifive_u.h @@ -158,7 +158,7 @@ enum { #define SIFIVE_U_PLIC_NUM_SOURCES 54 #define SIFIVE_U_PLIC_NUM_PRIORITIES 7 -#define SIFIVE_U_PLIC_PRIORITY_BASE 0x04 +#define SIFIVE_U_PLIC_PRIORITY_BASE 0x00 #define SIFIVE_U_PLIC_PENDING_BASE 0x1000 #define SIFIVE_U_PLIC_ENABLE_BASE 0x2000 #define SIFIVE_U_PLIC_ENABLE_STRIDE 0x80 diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h index e1ce0048af..3407c9e8dd 100644 --- a/include/hw/riscv/virt.h +++ b/include/hw/riscv/virt.h @@ -98,7 +98,7 @@ enum { #define VIRT_IRQCHIP_MAX_GUESTS_BITS 3 #define VIRT_IRQCHIP_MAX_GUESTS ((1U << VIRT_IRQCHIP_MAX_GUESTS_BITS) - 1U) -#define VIRT_PLIC_PRIORITY_BASE 0x04 +#define VIRT_PLIC_PRIORITY_BASE 0x00 #define VIRT_PLIC_PENDING_BASE 0x1000 #define VIRT_PLIC_ENABLE_BASE 0x2000 #define VIRT_PLIC_ENABLE_STRIDE 0x80 -- cgit 1.4.1 From bc92f261519d5c77c70cf2ebcf0a3b9a414d82d0 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sun, 11 Dec 2022 11:08:29 +0800 Subject: hw/intc: sifive_plic: Fix the pending register range check The pending register upper limit is currently set to plic->num_sources >> 3, which is wrong, e.g.: considering plic->num_sources is 7, the upper limit becomes 0 which fails the range check if reading the pending register at pending_base. Fixes: 1e24429e40df ("SiFive RISC-V PLIC Block") Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20221211030829.802437-16-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/intc/sifive_plic.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'hw/intc/sifive_plic.c') diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 1a792cc3f5..5522ede2cf 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -143,7 +143,8 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size) uint32_t irq = (addr - plic->priority_base) >> 2; return plic->source_priority[irq]; - } else if (addr_between(addr, plic->pending_base, plic->num_sources >> 3)) { + } else if (addr_between(addr, plic->pending_base, + (plic->num_sources + 31) >> 3)) { uint32_t word = (addr - plic->pending_base) >> 2; return plic->pending[word]; @@ -202,7 +203,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value, sifive_plic_update(plic); } } else if (addr_between(addr, plic->pending_base, - plic->num_sources >> 3)) { + (plic->num_sources + 31) >> 3)) { qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid pending write: 0x%" HWADDR_PRIx "", __func__, addr); -- cgit 1.4.1