* [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses
@ 2019-03-21 0:45 Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation Alistair Francis
` (6 more replies)
0 siblings, 7 replies; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:45 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
This series updates the PLIC address to match the documentation.
This fixes: https://github.com/riscv/opensbi/issues/97
Alistair Francis (5):
riscv: plic: Fix incorrect irq calculation
riscv: sifive_u: Fix PLIC priority base offset and numbering
riscv: sifive_e: Fix PLIC priority base offset
riscv: virt: Fix PLIC priority base offset
riscv: plic: Log guest errors
hw/riscv/sifive_plic.c | 16 +++++++++++-----
hw/riscv/sifive_u.c | 2 +-
include/hw/riscv/sifive_e.h | 2 +-
include/hw/riscv/sifive_u.h | 4 ++--
include/hw/riscv/virt.h | 2 +-
5 files changed, 16 insertions(+), 10 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
@ 2019-03-21 0:46 ` Alistair Francis
2019-03-27 10:29 ` Palmer Dabbelt
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 2/5] riscv: sifive_u: Fix PLIC priority base offset and numbering Alistair Francis
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
The irq is incorrectly calculated to be off by one. It has worked in the
past as the priority_base offset has also been set incorrectly. We are
about to fix the priority_base offset so first first the irq
calculation.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_plic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
index 1c703e1a37..70a85cd075 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
if (addr >= plic->priority_base && /* 4 bytes per source */
addr < plic->priority_base + (plic->num_sources << 2))
{
- uint32_t irq = (addr - plic->priority_base) >> 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]);
@@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
if (addr >= plic->priority_base && /* 4 bytes per source */
addr < plic->priority_base + (plic->num_sources << 2))
{
- uint32_t irq = (addr - plic->priority_base) >> 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",
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-riscv] [PATCH for 4.0 v1 2/5] riscv: sifive_u: Fix PLIC priority base offset and numbering
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation Alistair Francis
@ 2019-03-21 0:46 ` Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 3/5] riscv: sifive_e: Fix PLIC priority base offset Alistair Francis
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
According to the FU540 manual the PLIC source priority address starts at
an offset of 0x04 and not 0x00. The same manual also specifies that the
PLIC only has 53 source priorities. Fix these two incorrect header
files.
We also need to over extend the plic_gpios[] array as the PLIC sources
count from 1 and not 0.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_u.c | 2 +-
include/hw/riscv/sifive_u.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 5ecc47cea3..88381a7507 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -340,7 +340,7 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
const struct MemmapEntry *memmap = sifive_u_memmap;
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
- qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES];
+ qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES + 1];
int i;
Error *err = NULL;
NICInfo *nd = &nd_table[0];
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index be13cc1304..d859ea20f6 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -68,9 +68,9 @@ enum {
};
#define SIFIVE_U_PLIC_HART_CONFIG "MS"
-#define SIFIVE_U_PLIC_NUM_SOURCES 127
+#define SIFIVE_U_PLIC_NUM_SOURCES 53
#define SIFIVE_U_PLIC_NUM_PRIORITIES 7
-#define SIFIVE_U_PLIC_PRIORITY_BASE 0x0
+#define SIFIVE_U_PLIC_PRIORITY_BASE 0x04
#define SIFIVE_U_PLIC_PENDING_BASE 0x1000
#define SIFIVE_U_PLIC_ENABLE_BASE 0x2000
#define SIFIVE_U_PLIC_ENABLE_STRIDE 0x80
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-riscv] [PATCH for 4.0 v1 3/5] riscv: sifive_e: Fix PLIC priority base offset
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 2/5] riscv: sifive_u: Fix PLIC priority base offset and numbering Alistair Francis
@ 2019-03-21 0:46 ` Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 4/5] riscv: virt: " Alistair Francis
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
According to the FE31 manual the PLIC source priority address starts at
an offset of 0x04 and not 0x00.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
include/hw/riscv/sifive_e.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 7b6d8aed96..f715f8606f 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -70,7 +70,7 @@ enum {
#define SIFIVE_E_PLIC_HART_CONFIG "M"
#define SIFIVE_E_PLIC_NUM_SOURCES 127
#define SIFIVE_E_PLIC_NUM_PRIORITIES 7
-#define SIFIVE_E_PLIC_PRIORITY_BASE 0x0
+#define SIFIVE_E_PLIC_PRIORITY_BASE 0x04
#define SIFIVE_E_PLIC_PENDING_BASE 0x1000
#define SIFIVE_E_PLIC_ENABLE_BASE 0x2000
#define SIFIVE_E_PLIC_ENABLE_STRIDE 0x80
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-riscv] [PATCH for 4.0 v1 4/5] riscv: virt: Fix PLIC priority base offset
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
` (2 preceding siblings ...)
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 3/5] riscv: sifive_e: Fix PLIC priority base offset Alistair Francis
@ 2019-03-21 0:46 ` Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors Alistair Francis
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
Update the virt offsets based on the newly updated SiFive U and SiFive E
offsets.
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 f12deaebd6..568764b570 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -59,7 +59,7 @@ enum {
#define VIRT_PLIC_HART_CONFIG "MS"
#define VIRT_PLIC_NUM_SOURCES 127
#define VIRT_PLIC_NUM_PRIORITIES 7
-#define VIRT_PLIC_PRIORITY_BASE 0x0
+#define VIRT_PLIC_PRIORITY_BASE 0x04
#define VIRT_PLIC_PENDING_BASE 0x1000
#define VIRT_PLIC_ENABLE_BASE 0x2000
#define VIRT_PLIC_ENABLE_STRIDE 0x80
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-riscv] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
` (3 preceding siblings ...)
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 4/5] riscv: virt: " Alistair Francis
@ 2019-03-21 0:46 ` Alistair Francis
2019-03-26 22:19 ` [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
[not found] ` <5c92e23e.1c69fb81.aa92f.dd74SMTPIN_ADDED_BROKEN@mx.google.com>
6 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2019-03-21 0:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, Alistair Francis, alistair23
Instead of using error_report() to print guest errors let's use
qemu_log_mask(LOG_GUEST_ERROR,...) to log the error.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/sifive_plic.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
index 70a85cd075..7f373d6c9d 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -262,7 +262,9 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
}
err:
- error_report("plic: invalid register read: %08x", (uint32_t)addr);
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid register read 0x%" HWADDR_PRIx "\n",
+ __func__, addr);
return 0;
}
@@ -289,7 +291,9 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
} else if (addr >= plic->pending_base && /* 1 bit per source */
addr < plic->pending_base + (plic->num_sources >> 3))
{
- error_report("plic: invalid pending write: %08x", (uint32_t)addr);
+ 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)
@@ -339,7 +343,9 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
}
err:
- error_report("plic: invalid register write: %08x", (uint32_t)addr);
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
+ __func__, addr);
}
static const MemoryRegionOps sifive_plic_ops = {
--
2.21.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
` (4 preceding siblings ...)
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors Alistair Francis
@ 2019-03-26 22:19 ` Alistair Francis
2019-03-27 6:14 ` Palmer Dabbelt
[not found] ` <5c92e23e.1c69fb81.aa92f.dd74SMTPIN_ADDED_BROKEN@mx.google.com>
6 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2019-03-26 22:19 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, qemu-riscv, palmer
On Wed, Mar 20, 2019 at 5:45 PM Alistair Francis
<Alistair.Francis@wdc.com> wrote:
>
> This series updates the PLIC address to match the documentation.
>
> This fixes: https://github.com/riscv/opensbi/issues/97
>
> Alistair Francis (5):
> riscv: plic: Fix incorrect irq calculation
> riscv: sifive_u: Fix PLIC priority base offset and numbering
> riscv: sifive_e: Fix PLIC priority base offset
> riscv: virt: Fix PLIC priority base offset
> riscv: plic: Log guest errors
Ping! Can this make it into 4.0?
Alistair
>
> hw/riscv/sifive_plic.c | 16 +++++++++++-----
> hw/riscv/sifive_u.c | 2 +-
> include/hw/riscv/sifive_e.h | 2 +-
> include/hw/riscv/sifive_u.h | 4 ++--
> include/hw/riscv/virt.h | 2 +-
> 5 files changed, 16 insertions(+), 10 deletions(-)
>
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors
[not found] ` <5c92e23e.1c69fb81.aa92f.dd74SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2019-03-26 23:23 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-26 23:23 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, qemu-riscv, alistair23, palmer
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
Le jeu. 21 mars 2019 02:00, Alistair Francis <Alistair.Francis@wdc.com> a
écrit :
> Instead of using error_report() to print guest errors let's use
> qemu_log_mask(LOG_GUEST_ERROR,...) to log the error.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_plic.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index 70a85cd075..7f373d6c9d 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -262,7 +262,9 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr
> addr, unsigned size)
> }
>
> err:
> - error_report("plic: invalid register read: %08x", (uint32_t)addr);
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid register read 0x%" HWADDR_PRIx "\n",
> + __func__, addr);
> return 0;
> }
>
> @@ -289,7 +291,9 @@ static void sifive_plic_write(void *opaque, hwaddr
> addr, uint64_t value,
> } else if (addr >= plic->pending_base && /* 1 bit per source */
> addr < plic->pending_base + (plic->num_sources >> 3))
> {
> - error_report("plic: invalid pending write: %08x", (uint32_t)addr);
> + 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)
> @@ -339,7 +343,9 @@ static void sifive_plic_write(void *opaque, hwaddr
> addr, uint64_t value,
> }
>
> err:
> - error_report("plic: invalid register write: %08x", (uint32_t)addr);
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
> + __func__, addr);
> }
>
> static const MemoryRegionOps sifive_plic_ops = {
> --
> 2.21.0
>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
[-- Attachment #2: Type: text/html, Size: 3031 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses
2019-03-26 22:19 ` [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
@ 2019-03-27 6:14 ` Palmer Dabbelt
0 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2019-03-27 6:14 UTC (permalink / raw)
To: alistair23; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Tue, 26 Mar 2019 15:19:25 PDT (-0700), alistair23@gmail.com wrote:
> On Wed, Mar 20, 2019 at 5:45 PM Alistair Francis
> <Alistair.Francis@wdc.com> wrote:
>>
>> This series updates the PLIC address to match the documentation.
>>
>> This fixes: https://github.com/riscv/opensbi/issues/97
>>
>> Alistair Francis (5):
>> riscv: plic: Fix incorrect irq calculation
>> riscv: sifive_u: Fix PLIC priority base offset and numbering
>> riscv: sifive_e: Fix PLIC priority base offset
>> riscv: virt: Fix PLIC priority base offset
>> riscv: plic: Log guest errors
>
> Ping! Can this make it into 4.0?
Sorry, I missed this one. I'll take a look.
>
> Alistair
>
>>
>> hw/riscv/sifive_plic.c | 16 +++++++++++-----
>> hw/riscv/sifive_u.c | 2 +-
>> include/hw/riscv/sifive_e.h | 2 +-
>> include/hw/riscv/sifive_u.h | 4 ++--
>> include/hw/riscv/virt.h | 2 +-
>> 5 files changed, 16 insertions(+), 10 deletions(-)
>>
>> --
>> 2.21.0
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation Alistair Francis
@ 2019-03-27 10:29 ` Palmer Dabbelt
2019-03-27 16:29 ` Alistair Francis
0 siblings, 1 reply; 14+ messages in thread
From: Palmer Dabbelt @ 2019-03-27 10:29 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, qemu-riscv, Alistair Francis, alistair23
On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
> The irq is incorrectly calculated to be off by one. It has worked in the
> past as the priority_base offset has also been set incorrectly. We are
> about to fix the priority_base offset so first first the irq
> calculation.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/sifive_plic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index 1c703e1a37..70a85cd075 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
> if (addr >= plic->priority_base && /* 4 bytes per source */
> addr < plic->priority_base + (plic->num_sources << 2))
> {
> - uint32_t irq = (addr - plic->priority_base) >> 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]);
> @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
> if (addr >= plic->priority_base && /* 4 bytes per source */
> addr < plic->priority_base + (plic->num_sources << 2))
> {
> - uint32_t irq = (addr - plic->priority_base) >> 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",
I think this breaks bisectability and should be merged with the
*_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority being
set for the brong IRQ.
I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far as I
can understand it, all this does is ensure that source 0 is actually treated as
illegal (and shrinks the number of sources to match what's actually there, but
that shouldn't fix the bug). That looks more like a cleanup than an actual fix.
Am I missing something?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-27 10:29 ` Palmer Dabbelt
@ 2019-03-27 16:29 ` Alistair Francis
2019-03-28 3:15 ` Palmer Dabbelt
0 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2019-03-27 16:29 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, open list:RISC-V
On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
> > The irq is incorrectly calculated to be off by one. It has worked in the
> > past as the priority_base offset has also been set incorrectly. We are
> > about to fix the priority_base offset so first first the irq
> > calculation.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/sifive_plic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> > index 1c703e1a37..70a85cd075 100644
> > --- a/hw/riscv/sifive_plic.c
> > +++ b/hw/riscv/sifive_plic.c
> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
> > if (addr >= plic->priority_base && /* 4 bytes per source */
> > addr < plic->priority_base + (plic->num_sources << 2))
> > {
> > - uint32_t irq = (addr - plic->priority_base) >> 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]);
> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
> > if (addr >= plic->priority_base && /* 4 bytes per source */
> > addr < plic->priority_base + (plic->num_sources << 2))
> > {
> > - uint32_t irq = (addr - plic->priority_base) >> 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",
>
> I think this breaks bisectability and should be merged with the
> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority being
> set for the brong IRQ.
Good point, I can merge them together.
>
> I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far as I
> can understand it, all this does is ensure that source 0 is actually treated as
> illegal (and shrinks the number of sources to match what's actually there, but
> that shouldn't fix the bug). That looks more like a cleanup than an actual fix.
The problem is that before we where off by one. We supported 0 - (n -
1) and this patch set changes QEMU to support 1 - n. This is because
of the "addr < plic->priority_base + (plic->num_sources << 2)"
comparison. As priority_base is now 0x04 instead of 0x00 the
comparison will work correctly.
Alistair
>
> Am I missing something?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-27 16:29 ` Alistair Francis
@ 2019-03-28 3:15 ` Palmer Dabbelt
2019-03-28 17:34 ` Alistair Francis
0 siblings, 1 reply; 14+ messages in thread
From: Palmer Dabbelt @ 2019-03-28 3:15 UTC (permalink / raw)
To: alistair23; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote:
> On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
>> > The irq is incorrectly calculated to be off by one. It has worked in the
>> > past as the priority_base offset has also been set incorrectly. We are
>> > about to fix the priority_base offset so first first the irq
>> > calculation.
>> >
>> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> > ---
>> > hw/riscv/sifive_plic.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
>> > index 1c703e1a37..70a85cd075 100644
>> > --- a/hw/riscv/sifive_plic.c
>> > +++ b/hw/riscv/sifive_plic.c
>> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
>> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> > addr < plic->priority_base + (plic->num_sources << 2))
>> > {
>> > - uint32_t irq = (addr - plic->priority_base) >> 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]);
>> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
>> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> > addr < plic->priority_base + (plic->num_sources << 2))
>> > {
>> > - uint32_t irq = (addr - plic->priority_base) >> 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",
>>
>> I think this breaks bisectability and should be merged with the
>> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority being
>> set for the brong IRQ.
>
> Good point, I can merge them together.
>
>>
>> I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far as I
>> can understand it, all this does is ensure that source 0 is actually treated as
>> illegal (and shrinks the number of sources to match what's actually there, but
>> that shouldn't fix the bug). That looks more like a cleanup than an actual fix.
>
> The problem is that before we where off by one. We supported 0 - (n -
> 1) and this patch set changes QEMU to support 1 - n. This is because
> of the "addr < plic->priority_base + (plic->num_sources << 2)"
> comparison. As priority_base is now 0x04 instead of 0x00 the
> comparison will work correctly.
So something in OpenSBI is going through the entire set of sources and
initializing the priorities? That makes sense for the off-by-one, I was just
wondering because the array shrank so the specific offset in that error would
still be invalid with the new patch set.
>
> Alistair
>
>>
>> Am I missing something?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-28 3:15 ` Palmer Dabbelt
@ 2019-03-28 17:34 ` Alistair Francis
2019-03-29 5:54 ` Palmer Dabbelt
0 siblings, 1 reply; 14+ messages in thread
From: Alistair Francis @ 2019-03-28 17:34 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, open list:RISC-V
On Wed, Mar 27, 2019 at 8:15 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote:
> > On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
> >> > The irq is incorrectly calculated to be off by one. It has worked in the
> >> > past as the priority_base offset has also been set incorrectly. We are
> >> > about to fix the priority_base offset so first first the irq
> >> > calculation.
> >> >
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > ---
> >> > hw/riscv/sifive_plic.c | 4 ++--
> >> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> >> > index 1c703e1a37..70a85cd075 100644
> >> > --- a/hw/riscv/sifive_plic.c
> >> > +++ b/hw/riscv/sifive_plic.c
> >> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
> >> > if (addr >= plic->priority_base && /* 4 bytes per source */
> >> > addr < plic->priority_base + (plic->num_sources << 2))
> >> > {
> >> > - uint32_t irq = (addr - plic->priority_base) >> 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]);
> >> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
> >> > if (addr >= plic->priority_base && /* 4 bytes per source */
> >> > addr < plic->priority_base + (plic->num_sources << 2))
> >> > {
> >> > - uint32_t irq = (addr - plic->priority_base) >> 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",
> >>
> >> I think this breaks bisectability and should be merged with the
> >> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority being
> >> set for the brong IRQ.
> >
> > Good point, I can merge them together.
> >
> >>
> >> I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far as I
> >> can understand it, all this does is ensure that source 0 is actually treated as
> >> illegal (and shrinks the number of sources to match what's actually there, but
> >> that shouldn't fix the bug). That looks more like a cleanup than an actual fix.
> >
> > The problem is that before we where off by one. We supported 0 - (n -
> > 1) and this patch set changes QEMU to support 1 - n. This is because
> > of the "addr < plic->priority_base + (plic->num_sources << 2)"
> > comparison. As priority_base is now 0x04 instead of 0x00 the
> > comparison will work correctly.
>
> So something in OpenSBI is going through the entire set of sources and
> initializing the priorities? That makes sense for the off-by-one, I was just
> wondering because the array shrank so the specific offset in that error would
> still be invalid with the new patch set.
Yep, that is what caught this. The array only shrinks for the FU540
board, the virt board doesn't shrink and that is the board that
OpenSBI caught the bug with.
Alistair
>
> >
> > Alistair
> >
> >>
> >> Am I missing something?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation
2019-03-28 17:34 ` Alistair Francis
@ 2019-03-29 5:54 ` Palmer Dabbelt
0 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2019-03-29 5:54 UTC (permalink / raw)
To: alistair23; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Thu, 28 Mar 2019 10:34:08 PDT (-0700), alistair23@gmail.com wrote:
> On Wed, Mar 27, 2019 at 8:15 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote:
>> > On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>> >>
>> >> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
>> >> > The irq is incorrectly calculated to be off by one. It has worked in the
>> >> > past as the priority_base offset has also been set incorrectly. We are
>> >> > about to fix the priority_base offset so first first the irq
>> >> > calculation.
>> >> >
>> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> >> > ---
>> >> > hw/riscv/sifive_plic.c | 4 ++--
>> >> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
>> >> > index 1c703e1a37..70a85cd075 100644
>> >> > --- a/hw/riscv/sifive_plic.c
>> >> > +++ b/hw/riscv/sifive_plic.c
>> >> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
>> >> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> >> > addr < plic->priority_base + (plic->num_sources << 2))
>> >> > {
>> >> > - uint32_t irq = (addr - plic->priority_base) >> 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]);
>> >> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
>> >> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> >> > addr < plic->priority_base + (plic->num_sources << 2))
>> >> > {
>> >> > - uint32_t irq = (addr - plic->priority_base) >> 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",
>> >>
>> >> I think this breaks bisectability and should be merged with the
>> >> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority being
>> >> set for the brong IRQ.
>> >
>> > Good point, I can merge them together.
>> >
>> >>
>> >> I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far as I
>> >> can understand it, all this does is ensure that source 0 is actually treated as
>> >> illegal (and shrinks the number of sources to match what's actually there, but
>> >> that shouldn't fix the bug). That looks more like a cleanup than an actual fix.
>> >
>> > The problem is that before we where off by one. We supported 0 - (n -
>> > 1) and this patch set changes QEMU to support 1 - n. This is because
>> > of the "addr < plic->priority_base + (plic->num_sources << 2)"
>> > comparison. As priority_base is now 0x04 instead of 0x00 the
>> > comparison will work correctly.
>>
>> So something in OpenSBI is going through the entire set of sources and
>> initializing the priorities? That makes sense for the off-by-one, I was just
>> wondering because the array shrank so the specific offset in that error would
>> still be invalid with the new patch set.
>
> Yep, that is what caught this. The array only shrinks for the FU540
> board, the virt board doesn't shrink and that is the board that
> OpenSBI caught the bug with.
OK, that makes sense.
>
> Alistair
>
>>
>> >
>> > Alistair
>> >
>> >>
>> >> Am I missing something?
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-03-29 5:54 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 0:45 [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation Alistair Francis
2019-03-27 10:29 ` Palmer Dabbelt
2019-03-27 16:29 ` Alistair Francis
2019-03-28 3:15 ` Palmer Dabbelt
2019-03-28 17:34 ` Alistair Francis
2019-03-29 5:54 ` Palmer Dabbelt
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 2/5] riscv: sifive_u: Fix PLIC priority base offset and numbering Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 3/5] riscv: sifive_e: Fix PLIC priority base offset Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 4/5] riscv: virt: " Alistair Francis
2019-03-21 0:46 ` [Qemu-riscv] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors Alistair Francis
2019-03-26 22:19 ` [Qemu-riscv] [PATCH for 4.0 v1 0/5] Update the QEMU PLIC addresses Alistair Francis
2019-03-27 6:14 ` Palmer Dabbelt
[not found] ` <5c92e23e.1c69fb81.aa92f.dd74SMTPIN_ADDED_BROKEN@mx.google.com>
2019-03-26 23:23 ` [Qemu-riscv] [Qemu-devel] [PATCH for 4.0 v1 5/5] riscv: plic: Log guest errors Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.