Re: [PATCH 2/2] target/riscv: Make the "virt" register writable by GDB
On Mon, Mar 6, 2023 at 7:26 PM LIU Zhiwei wrote:
>
>
> On 2023/3/5 17:42, Jim Shu wrote:
> > This patch also enables debugger to set current privilege mode to
> > VU/VS-mode.
> >
> > Extend previous commit 81d2929c41d32af138f3562f5a7b309f6eac7ca7 to
> > support H-extension.
> >
> > Signed-off-by: Jim Shu
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/gdbstub.c | 18 --
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index 1755fd9d51..a7f234beaf 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -203,15 +203,29 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs,
> > GByteArray *buf, int n)
> >
> > static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int
> > n)
> > {
> > +#ifdef CONFIG_USER_ONLY
> > +if (n >= 0 && n <= 1) {
> > +return sizeof(target_ulong);
> > +}
> > +#else
> > +bool virt;
> > +
> > if (n == 0) {
> > -#ifndef CONFIG_USER_ONLY
> > cs->priv = ldtul_p(mem_buf) & 0x3;
> > if (cs->priv == PRV_H) {
> > cs->priv = PRV_S;
> > }
> > -#endif
> > +return sizeof(target_ulong);
> We should return according to the misa_mxl_max. And this is a bug before
> your commit.
Hi Zhiwei,
After reading other gdbstub.c code, I think it is OK to use
'sizeof(target_ulong)' as virtual register length.
Its length is 32-bit in RV32 and is 64-bit in RV64/RV128. We don't
need to handle RV128 specially.
Virtual register length is same as CSR length and
'riscv_gdb_set_csr()' also use 'sizeof(target_ulong)'.
Jim Shu
> > +} else if (n == 1) {
> > +virt = ldtul_p(mem_buf) & 0x1;
> > +if ((cs->priv == PRV_M) && (virt == true)) {
> > +/* M-mode only supports V=0. */
> > +virt = false;
> > +}
> > +riscv_cpu_set_virt_enabled(cs, virt);
> > return sizeof(target_ulong);
> Same error here. Otherwise,
>
> Reviewed-by: LIU Zhiwei
>
> Zhiwei
>
> > }
> > +#endif
> > return 0;
> > }
> >
Re: [PATCH 2/2] target/riscv: Make the "virt" register writable by GDB
Thanks for reviewing.
I'll fix this issue.
On Mon, Mar 6, 2023 at 7:26 PM LIU Zhiwei wrote:
>
>
> On 2023/3/5 17:42, Jim Shu wrote:
> > This patch also enables debugger to set current privilege mode to
> > VU/VS-mode.
> >
> > Extend previous commit 81d2929c41d32af138f3562f5a7b309f6eac7ca7 to
> > support H-extension.
> >
> > Signed-off-by: Jim Shu
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/gdbstub.c | 18 --
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index 1755fd9d51..a7f234beaf 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -203,15 +203,29 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs,
> > GByteArray *buf, int n)
> >
> > static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int
> > n)
> > {
> > +#ifdef CONFIG_USER_ONLY
> > +if (n >= 0 && n <= 1) {
> > +return sizeof(target_ulong);
> > +}
> > +#else
> > +bool virt;
> > +
> > if (n == 0) {
> > -#ifndef CONFIG_USER_ONLY
> > cs->priv = ldtul_p(mem_buf) & 0x3;
> > if (cs->priv == PRV_H) {
> > cs->priv = PRV_S;
> > }
> > -#endif
> > +return sizeof(target_ulong);
> We should return according to the misa_mxl_max. And this is a bug before
> your commit.
> > +} else if (n == 1) {
> > +virt = ldtul_p(mem_buf) & 0x1;
> > +if ((cs->priv == PRV_M) && (virt == true)) {
> > +/* M-mode only supports V=0. */
> > +virt = false;
> > +}
> > +riscv_cpu_set_virt_enabled(cs, virt);
> > return sizeof(target_ulong);
> Same error here. Otherwise,
>
> Reviewed-by: LIU Zhiwei
>
> Zhiwei
>
> > }
> > +#endif
> > return 0;
> > }
> >
[PATCH 1/2] target/riscv: Expose "virt" register for GDB for reads
This patch enables a debugger to read current virtualization mode via
virtual "virt" register. After it, we could get full current privilege
mode via both "priv" and "virt" register.
Extend previous commit ab9056ff9bdb3f95db6e7a666d10522d289f14ec to
support H-extension.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
gdb-xml/riscv-32bit-virtual.xml | 1 +
gdb-xml/riscv-64bit-virtual.xml | 1 +
target/riscv/gdbstub.c | 12
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
index 905f1c555d..d44b6ca2dc 100644
--- a/gdb-xml/riscv-32bit-virtual.xml
+++ b/gdb-xml/riscv-32bit-virtual.xml
@@ -8,4 +8,5 @@
+
diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
index 62d86c237b..7c9b63d5b6 100644
--- a/gdb-xml/riscv-64bit-virtual.xml
+++ b/gdb-xml/riscv-64bit-virtual.xml
@@ -8,4 +8,5 @@
+
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 6048541606..1755fd9d51 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -187,13 +187,17 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t
*mem_buf, int n)
static int riscv_gdb_get_virtual(CPURISCVState *cs, GByteArray *buf, int n)
{
-if (n == 0) {
#ifdef CONFIG_USER_ONLY
+if (n >= 0 && n <= 1) {
return gdb_get_regl(buf, 0);
+}
#else
+if (n == 0) {
return gdb_get_regl(buf, cs->priv);
-#endif
+} else if (n == 1) {
+return gdb_get_regl(buf, riscv_cpu_virt_enabled(cs));
}
+#endif
return 0;
}
@@ -328,13 +332,13 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState
*cs)
case MXL_RV32:
gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
riscv_gdb_set_virtual,
- 1, "riscv-32bit-virtual.xml", 0);
+ 2, "riscv-32bit-virtual.xml", 0);
break;
case MXL_RV64:
case MXL_RV128:
gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
riscv_gdb_set_virtual,
- 1, "riscv-64bit-virtual.xml", 0);
+ 2, "riscv-64bit-virtual.xml", 0);
break;
default:
g_assert_not_reached();
--
2.17.1
[PATCH 2/2] target/riscv: Make the "virt" register writable by GDB
This patch also enables debugger to set current privilege mode to
VU/VS-mode.
Extend previous commit 81d2929c41d32af138f3562f5a7b309f6eac7ca7 to
support H-extension.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
target/riscv/gdbstub.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 1755fd9d51..a7f234beaf 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -203,15 +203,29 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs,
GByteArray *buf, int n)
static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
{
+#ifdef CONFIG_USER_ONLY
+if (n >= 0 && n <= 1) {
+return sizeof(target_ulong);
+}
+#else
+bool virt;
+
if (n == 0) {
-#ifndef CONFIG_USER_ONLY
cs->priv = ldtul_p(mem_buf) & 0x3;
if (cs->priv == PRV_H) {
cs->priv = PRV_S;
}
-#endif
+return sizeof(target_ulong);
+} else if (n == 1) {
+virt = ldtul_p(mem_buf) & 0x1;
+if ((cs->priv == PRV_M) && (virt == true)) {
+/* M-mode only supports V=0. */
+virt = false;
+}
+riscv_cpu_set_virt_enabled(cs, virt);
return sizeof(target_ulong);
}
+#endif
return 0;
}
--
2.17.1
[PATCH] 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
---
hw/intc/sifive_plic.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index c2dfacf028..1cf156cf85 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);
--
2.17.1
[PATCH] target/riscv: support cache-related PMU events in virtual mode
let tlb_fill() function also increments PMU counter when it is from
two-stage translation, so QEMU could also monitor these PMU events when
CPU runs in VS/VU mode (like running guest OS).
Signed-off-by: Jim Shu
---
target/riscv/cpu_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..a52a9b14d7 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1248,6 +1248,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
}
}
+pmu_tlb_fill_incr_ctr(cpu, access_type);
if (riscv_cpu_virt_enabled(env) ||
((riscv_cpu_two_stage_lookup(mmu_idx) || two_stage_lookup) &&
access_type != MMU_INST_FETCH)) {
@@ -1311,7 +1312,6 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
}
}
} else {
-pmu_tlb_fill_incr_ctr(cpu, access_type);
/* Single stage lookup */
ret = get_physical_address(env, , , address, NULL,
access_type, mmu_idx, true, false, false);
--
2.17.1
Re: [PATCH v3 0/2] Enhance maximum priority support of PLIC
Gentle ping. It's a patch for fix and spec alignment of PLIC. On Mon, Oct 3, 2022 at 12:14 PM Jim Shu wrote: > > This patchset fixes hard-coded maximum priority of interrupt priority > register and also changes this register to WARL field to align the PLIC > spec. > > Changelog: > > v3: > * fix opposite of power-of-2 max priority checking expression. > > v2: > * change interrupt priority register to WARL field. > > Jim Shu (2): > hw/intc: sifive_plic: fix hard-coded max priority level > hw/intc: sifive_plic: change interrupt priority register to WARL field > > hw/intc/sifive_plic.c | 25 ++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > -- > 2.17.1 >
[PATCH v3 0/2] Enhance maximum priority support of PLIC
This patchset fixes hard-coded maximum priority of interrupt priority register and also changes this register to WARL field to align the PLIC spec. Changelog: v3: * fix opposite of power-of-2 max priority checking expression. v2: * change interrupt priority register to WARL field. Jim Shu (2): hw/intc: sifive_plic: fix hard-coded max priority level hw/intc: sifive_plic: change interrupt priority register to WARL field hw/intc/sifive_plic.c | 25 ++--- 1 file changed, 22 insertions(+), 3 deletions(-) -- 2.17.1
[PATCH v3 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
PLIC spec [1] requires interrupt source priority registers are WARL
field and the number of supported priority is power-of-2 to simplify SW
discovery.
Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
spec, whose number of supported priority is not power-of-2. Just change
each bit of interrupt priority register to WARL field when the number of
supported priority is power-of-2.
[1]
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
Signed-off-by: Jim Shu
---
hw/intc/sifive_plic.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index f864efa761..c2dfacf028 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
-if (value <= plic->num_priorities) {
+if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
+/*
+ * if "num_priorities + 1" is power-of-2, make each register bit of
+ * interrupt priority WARL (Write-Any-Read-Legal). Just filter
+ * out the access to unsupported priority bits.
+ */
+plic->source_priority[irq] = value % (plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
plic->source_priority[irq] = value;
sifive_plic_update(plic);
}
@@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
uint32_t contextid = (addr & (plic->context_stride - 1));
if (contextid == 0) {
-if (value <= plic->num_priorities) {
+if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
+/*
+ * if "num_priorities + 1" is power-of-2, each register bit of
+ * interrupt priority is WARL (Write-Any-Read-Legal). Just
+ * filter out the access to unsupported priority bits.
+ */
+plic->target_priority[addrid] = value %
+(plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
plic->target_priority[addrid] = value;
sifive_plic_update(plic);
}
--
2.17.1
[PATCH v3 1/2] hw/intc: sifive_plic: fix hard-coded max priority level
The maximum priority level is hard-coded when writing to interrupt
priority register. However, when writing to priority threshold register,
the maximum priority level is from num_priorities Property which is
configured by platform.
Also change interrupt priority register to use num_priorities Property
in maximum priority level.
Signed-off-by: Emmanuel Blot
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
hw/intc/sifive_plic.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index af4ae3630e..f864efa761 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
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;
-sifive_plic_update(plic);
+if (value <= plic->num_priorities) {
+plic->source_priority[irq] = value;
+sifive_plic_update(plic);
+}
} else if (addr_between(addr, plic->pending_base,
plic->num_sources >> 3)) {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.17.1
Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
Hi Clément,
> > > @@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr
> > > addr, uint64_t value,
> > > if (addr_between(addr, plic->priority_base, plic->num_sources << 2))
> > > {
> > > uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> > >
> > > -if (value <= plic->num_priorities) {
> > > +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
> >
> > That's the opposite. If n is a power of 2, n & (n-1) == 0 (eg 8 & 7 ==
> > 0, 9 & 8 == 8).
> > Note that n must be positive too. But I'm not sure it matters here.
> > I'll let you decide.
> >
num_priorities is a uint32_t variable so that n is always positive.
Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
hi Clément,
Thank you very much.
I'll fix it in the next version patch.
Thanks,
Jim Shu
On Fri, Sep 30, 2022 at 8:58 PM Clément Chigot wrote:
>
> Hi Jim,
>
> On Fri, Sep 30, 2022 at 2:32 PM Jim Shu wrote:
> >
> > PLIC spec [1] requires interrupt source priority registers are WARL
> > field and the number of supported priority is power-of-2 to simplify SW
> > discovery.
> >
> > Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
> > spec, whose number of supported priority is not power-of-2. Just change
> > each bit of interrupt priority register to WARL field when the number of
> > supported priority is power-of-2.
> >
> > [1]
> > https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
> >
> > Signed-off-by: Jim Shu
> > ---
> > hw/intc/sifive_plic.c | 21 +++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> > index f864efa761..218ccff8bd 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr
> > addr, uint64_t value,
> > if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
> > uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> >
> > -if (value <= plic->num_priorities) {
> > +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
>
> That's the opposite. If n is a power of 2, n & (n-1) == 0 (eg 8 & 7 ==
> 0, 9 & 8 == 8).
> Note that n must be positive too. But I'm not sure it matters here.
> I'll let you decide.
>
> > +/*
> > + * if "num_priorities + 1" is power-of-2, make each register
> > bit of
> > + * interrupt priority WARL (Write-Any-Read-Legal). Just filter
> > + * out the access to unsupported priority bits.
> > + */
> > +plic->source_priority[irq] = value % (plic->num_priorities +
> > 1);
> > +sifive_plic_update(plic);
> > +} else if (value <= plic->num_priorities) {
> > plic->source_priority[irq] = value;
> > sifive_plic_update(plic);
> > }
> > @@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr
> > addr, uint64_t value,
> > uint32_t contextid = (addr & (plic->context_stride - 1));
> >
> > if (contextid == 0) {
> > -if (value <= plic->num_priorities) {
> > +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
>
> Same.
>
> > +/*
> > + * if "num_priorities + 1" is power-of-2, each register
> > bit of
> > + * interrupt priority is WARL (Write-Any-Read-Legal). Just
> > + * filter out the access to unsupported priority bits.
> > + */
> > +plic->target_priority[addrid] = value %
> > +(plic->num_priorities + 1);
> > +sifive_plic_update(plic);
> > +} else if (value <= plic->num_priorities) {
> > plic->target_priority[addrid] = value;
> > sifive_plic_update(plic);
> > }
> > --
> > 2.17.1
>
> Clément
[PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
PLIC spec [1] requires interrupt source priority registers are WARL
field and the number of supported priority is power-of-2 to simplify SW
discovery.
Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
spec, whose number of supported priority is not power-of-2. Just change
each bit of interrupt priority register to WARL field when the number of
supported priority is power-of-2.
[1]
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
Signed-off-by: Jim Shu
---
hw/intc/sifive_plic.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index f864efa761..218ccff8bd 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
-if (value <= plic->num_priorities) {
+if ((plic->num_priorities + 1) & (plic->num_priorities)) {
+/*
+ * if "num_priorities + 1" is power-of-2, make each register bit of
+ * interrupt priority WARL (Write-Any-Read-Legal). Just filter
+ * out the access to unsupported priority bits.
+ */
+plic->source_priority[irq] = value % (plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
plic->source_priority[irq] = value;
sifive_plic_update(plic);
}
@@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
uint32_t contextid = (addr & (plic->context_stride - 1));
if (contextid == 0) {
-if (value <= plic->num_priorities) {
+if ((plic->num_priorities + 1) & (plic->num_priorities)) {
+/*
+ * if "num_priorities + 1" is power-of-2, each register bit of
+ * interrupt priority is WARL (Write-Any-Read-Legal). Just
+ * filter out the access to unsupported priority bits.
+ */
+plic->target_priority[addrid] = value %
+(plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
plic->target_priority[addrid] = value;
sifive_plic_update(plic);
}
--
2.17.1
[PATCH v2 0/2] Enhance maximum priority support of PLIC
This patchset fixes hard-coded maximum priority of interrupt priority register and also changes this register to WARL field to align the PLIC spec. Changelog: v2: * change interrupt priority register to WARL field. Jim Shu (2): hw/intc: sifive_plic: fix hard-coded max priority level hw/intc: sifive_plic: change interrupt priority register to WARL field hw/intc/sifive_plic.c | 24 +--- 1 file changed, 21 insertions(+), 3 deletions(-) -- 2.17.1
[PATCH v2 1/2] hw/intc: sifive_plic: fix hard-coded max priority level
The maximum priority level is hard-coded when writing to interrupt
priority register. However, when writing to priority threshold register,
the maximum priority level is from num_priorities Property which is
configured by platform.
Also change interrupt priority register to use num_priorities Property
in maximum priority level.
Signed-off-by: Emmanuel Blot
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
hw/intc/sifive_plic.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index af4ae3630e..f864efa761 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
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;
-sifive_plic_update(plic);
+if (value <= plic->num_priorities) {
+plic->source_priority[irq] = value;
+sifive_plic_update(plic);
+}
} else if (addr_between(addr, plic->pending_base,
plic->num_sources >> 3)) {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.17.1
Re: [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
Hi Clément,
Thanks for your opinion. I think it's a good enhancement.
PLIC spec [1] says that interrupt source priority registers should be
WARL fields to allow software to determine "the number and position of
read-write bits" in each priority specification, if any. To simplify
discovery of supported priority values, each priority register must
support any combination of values in the bits that are variable within
the register, i.e., if there are two variable bits in the register,
all four combinations of values in those bits must operate as valid
priority levels.
I think "num_priorities + 1" should be power-of-2 and SW could
discover available bits of interrupt source priority.
I'll do this enhancement in the next version patch.
[1]
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
Thanks,
Jim Shu
On Mon, Sep 26, 2022 at 3:52 PM Clément Chigot wrote:
>
> Hi Jim,
>
> On Sun, Sep 25, 2022 at 3:26 PM Jim Shu wrote:
> >
> > The maximum priority level is hard-coded when writing to interrupt
> > priority register. However, when writing to priority threshold register,
> > the maximum priority level is from num_priorities Property which is
> > configured by platform.
> >
> > Also change interrupt priority register to use num_priorities Property
> > in maximum priority level.
> >
> > Signed-off-by: Emmanuel Blot
> > Signed-off-by: Jim Shu
> > Reviewed-by: Frank Chang
> > ---
> > hw/intc/sifive_plic.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> > index af4ae3630e..f864efa761 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr
> > addr, uint64_t value,
> > 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;
> > -sifive_plic_update(plic);
> > +if (value <= plic->num_priorities) {
> > +plic->source_priority[irq] = value;
> > +sifive_plic_update(plic);
> > +}
>
> If I'm not mistaking the documentation [1], these registers are WARL
> (Write-Any-Read-Legal). However, in your case, any value above
> "num_priorities" will be ignored.
>
> We had an issue related to that and ended up making a local patch.
> However, we are assuming that "num_priorities+1" is a power of 2
> (which is currently the case)
>
> -plic->source_priority[irq] = value & 7;
> +/* Interrupt Priority registers are Write-Any Read-Legal. Cleanup
> + * incoming values before storing them.
> + */
> +plic->source_priority[irq] = value % (plic->num_priorities + 1);
>
> Note that it should also be done for target_priority a bit below.
> -if (value <= plic->num_priorities) {
> -plic->target_priority[addrid] = value;
> -sifive_plic_update(plic);
> -}
> +/* Priority Thresholds registers are Write-Any Read-Legal.
> Cleanup
> + * incoming values before storing them.
> + */
> +plic->target_priority[addrid] = value % (plic->num_priorities +
> 1);
> +sifive_plic_update(plic);
>
> [1] https://static.dev.sifive.com/FE310-G000.pdf
>
> Thanks,
> Clément
Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index.
Hi Tyler,
Thanks for the explanation. I understand the issue here.
I think we should align the priority base in each RISC-V platform to
the same value (no matter 0x0 or 0x4) if they use PLIC in the same
way.
Thanks,
Jim Shu
On Tue, Sep 27, 2022 at 4:04 AM Tyler Ng wrote:
>
> Hi Jim,
>
> Thanks for raising this comment. I think I understand where the confusion
> happens and it's because in the OpenTitan machine (which uses the sifive
> plic), it uses 0x00 as the priority base by default, which was the source of
> the problems. I'll drop this commit in the next version.
>
> -Tyler
>
> On Sun, Sep 25, 2022 at 6:47 AM Jim Shu wrote:
>>
>> Hi Tyler,
>>
>> This fix is incorrect.
>>
>> In PLIC spec, Interrupt Source Priority Memory Map is
>> 0x00: Reserved (interrupt source 0 does not exist)
>> 0x04: Interrupt source 1 priority
>> 0x08: Interrupt source 2 priority
>>
>> Current RISC-V machines (virt, sifive_u) use 0x4 as priority_base, so
>> current formula "irq = ((addr - plic->priority_base) >> 2) + 1" will
>> take offset 0x4 as IRQ source 1, which is correct.
>> Your fix will cause the bug in existing machines.
>>
>> Thanks,
>> Jim Shu
>>
>>
>>
>>
>> On Tue, Sep 6, 2022 at 11:21 PM Tyler Ng wrote:
>> >
>> > Here's the patch SHA that introduced the offset:
>> > 0feb4a7129eb4f120c75849ddc9e50495c50cb63
>> >
>> > -Tyler
>> >
>> > On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones
>> > wrote:
>> >>
>> >> On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote:
>> >> > Fixes a bug in which the index of the interrupt priority is off by 1.
>> >> > For example, using an IRQ number of 3 with a priority of 1 is supposed
>> >> > to set
>> >> > plic->source_priority[2] = 1, but instead it sets
>> >> > plic->source_priority[3] = 1. When an interrupt is claimed to be
>> >> > serviced, it checks the index 2 instead of 3.
>> >> >
>> >> > Signed-off-by: Tyler Ng
>> >>
>> >> Fixes tag?
>> >>
>> >> Thanks,
>> >> drew
>> >>
>> >> > ---
>> >> > hw/intc/sifive_plic.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
>> >> > index af4ae3630e..e75c47300a 100644
>> >> > --- a/hw/intc/sifive_plic.c
>> >> > +++ b/hw/intc/sifive_plic.c
>> >> > @@ -178,7 +178,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) + 0;
>> >> >
>> >> > plic->source_priority[irq] = value & 7;
>> >> > sifive_plic_update(plic);
>> >> > --
>> >> > 2.30.2
>> >> >
Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index.
Hi Tyler,
This fix is incorrect.
In PLIC spec, Interrupt Source Priority Memory Map is
0x00: Reserved (interrupt source 0 does not exist)
0x04: Interrupt source 1 priority
0x08: Interrupt source 2 priority
Current RISC-V machines (virt, sifive_u) use 0x4 as priority_base, so
current formula "irq = ((addr - plic->priority_base) >> 2) + 1" will
take offset 0x4 as IRQ source 1, which is correct.
Your fix will cause the bug in existing machines.
Thanks,
Jim Shu
On Tue, Sep 6, 2022 at 11:21 PM Tyler Ng wrote:
>
> Here's the patch SHA that introduced the offset:
> 0feb4a7129eb4f120c75849ddc9e50495c50cb63
>
> -Tyler
>
> On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones wrote:
>>
>> On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote:
>> > Fixes a bug in which the index of the interrupt priority is off by 1.
>> > For example, using an IRQ number of 3 with a priority of 1 is supposed to
>> > set
>> > plic->source_priority[2] = 1, but instead it sets
>> > plic->source_priority[3] = 1. When an interrupt is claimed to be
>> > serviced, it checks the index 2 instead of 3.
>> >
>> > Signed-off-by: Tyler Ng
>>
>> Fixes tag?
>>
>> Thanks,
>> drew
>>
>> > ---
>> > hw/intc/sifive_plic.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
>> > index af4ae3630e..e75c47300a 100644
>> > --- a/hw/intc/sifive_plic.c
>> > +++ b/hw/intc/sifive_plic.c
>> > @@ -178,7 +178,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) + 0;
>> >
>> > plic->source_priority[irq] = value & 7;
>> > sifive_plic_update(plic);
>> > --
>> > 2.30.2
>> >
[PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
The maximum priority level is hard-coded when writing to interrupt
priority register. However, when writing to priority threshold register,
the maximum priority level is from num_priorities Property which is
configured by platform.
Also change interrupt priority register to use num_priorities Property
in maximum priority level.
Signed-off-by: Emmanuel Blot
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
hw/intc/sifive_plic.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index af4ae3630e..f864efa761 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
uint64_t value,
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;
-sifive_plic_update(plic);
+if (value <= plic->num_priorities) {
+plic->source_priority[irq] = value;
+sifive_plic_update(plic);
+}
} else if (addr_between(addr, plic->pending_base,
plic->num_sources >> 3)) {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.17.1
Re: [PATCH] include/hw/riscv/sifive_e.h: Fix the type of parent_obj of SiFiveEState.
Reviewed-by: Jim Shu
On Fri, Aug 19, 2022 at 3:11 PM Tommy Wu wrote:
>
> Fix the type of parent_obj of SiFiveEState from 'SysBusDevice'
> to 'MachineState'. Because the parent of SiFiveEState is 'MachineState'.
>
> Signed-off-by: Tommy Wu
> ---
> 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 83604da805..24359f9fe5 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -41,7 +41,7 @@ typedef struct SiFiveESoCState {
>
> typedef struct SiFiveEState {
> /*< private >*/
> -SysBusDevice parent_obj;
> +MachineState parent_obj;
>
> /*< public >*/
> SiFiveESoCState soc;
> --
> 2.27.0
>
>
Re: [PATCH] target/riscv: Support SW update of PTE A/D bits and Ssptwad extension
Hi Alistair, Why do we want to support that? We can do either and we are > implementing the much more usual scheme. I don't see a reason to > bother implementing the other one. Is anyone ever going to use it? > Thanks for your response. I got it. Regards, Jim Shu
Re: [PATCH] target/riscv: Support SW update of PTE A/D bits and Ssptwad extension
Hi Anup,
Do you think it is OK if we only use ssptwad as a CPU option name
but not a RISC-V extension? just like MMU and PMP options of RISC-V.
(And we could change it to RISC-V extension in the future
if Ssptwad becomes the formal RISC-V extension)
Both HW/SW update schemes are already defined in RISC-V priv spec,
so I think it's reasonable to implement them in QEMU. The only issue here is
to choose a proper CPU option name to turn on/off HW update of A/D bits.
Regards,
Jim Shu
On Mon, Jul 18, 2022 at 12:02 PM Anup Patel wrote:
> +Atish
>
> On Mon, Jul 18, 2022 at 9:23 AM Jim Shu wrote:
> >
> > RISC-V priv spec v1.12 permits 2 PTE-update schemes of A/D-bit
> > (Access/Dirty bit): HW update or SW update. RISC-V profile defines the
> > extension name 'Ssptwad' for HW update to PTE A/D bits.
> > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
>
> The Ssptwad (even though used by profiles) is not a well defined RISC-V
> ISA extension. Rather, Ssptwad is just a name used in profiles to represent
> an optional feature.
>
> In fact, since it is not a well-defined ISA extension, QEMU cannot include
> Ssptwad in the ISA string as well.
>
> I think for such optionalities which are not well-defined ISA extensions,
> QEMU should treat it differently.
>
> Regards,
> Anup
>
> >
> > Current QEMU RISC-V implements HW update scheme, so this commit
> > introduces SW update scheme to QEMU and uses the 'Ssptwad' extension
> > as the CPU option to select 2 PTE-update schemes. QEMU RISC-V CPU still
> > uses HW update scheme (ext_ssptwad=true) by default to keep the backward
> > compatibility.
> >
> > SW update scheme implemention is based on priv spec v1.12:
> > "When a virtual page is accessed and the A bit is clear, or is written
> > and the D bit is clear, a page-fault exception (corresponding to the
> > original access type) is raised."
> >
> > Signed-off-by: Jim Shu
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/cpu.c| 2 ++
> > target/riscv/cpu.h| 1 +
> > target/riscv/cpu_helper.c | 9 +
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 1bb3973806..1d38c1c1d2 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -857,6 +857,7 @@ static void riscv_cpu_init(Object *obj)
> >
> > cpu->cfg.ext_ifencei = true;
> > cpu->cfg.ext_icsr = true;
> > +cpu->cfg.ext_ssptwad = true;
> > cpu->cfg.mmu = true;
> > cpu->cfg.pmp = true;
> >
> > @@ -900,6 +901,7 @@ static Property riscv_cpu_extensions[] = {
> > DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
> > DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
> > DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
> > +DEFINE_PROP_BOOL("ssptwad", RISCVCPU, cfg.ext_ssptwad, true),
> >
> > DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
> > DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 5c7acc055a..2eee59af98 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -433,6 +433,7 @@ struct RISCVCPUConfig {
> > bool ext_zve32f;
> > bool ext_zve64f;
> > bool ext_zmmul;
> > +bool ext_ssptwad;
> > bool rvv_ta_all_1s;
> >
> > uint32_t mvendorid;
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 59b3680b1b..a8607c0d7b 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -981,6 +981,15 @@ restart:
> >
> > /* Page table updates need to be atomic with MTTCG enabled
> */
> > if (updated_pte != pte) {
> > +if (!cpu->cfg.ext_ssptwad) {
> > +/*
> > + * If A/D bits are managed by SW, HW just raises the
> > + * page fault exception corresponding to the
> original
> > + * access type when A/D bits need to be updated.
> > + */
> > +return TRANSLATE_FAIL;
> > +}
> > +
> > /*
> > * - if accessed or dirty bits need updating, and the
> PTE is
> > * in RAM, then we do so atomically with a compare
> and swap.
> > --
> > 2.17.1
> >
> >
>
[PATCH] target/riscv: Support SW update of PTE A/D bits and Ssptwad extension
RISC-V priv spec v1.12 permits 2 PTE-update schemes of A/D-bit
(Access/Dirty bit): HW update or SW update. RISC-V profile defines the
extension name 'Ssptwad' for HW update to PTE A/D bits.
https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
Current QEMU RISC-V implements HW update scheme, so this commit
introduces SW update scheme to QEMU and uses the 'Ssptwad' extension
as the CPU option to select 2 PTE-update schemes. QEMU RISC-V CPU still
uses HW update scheme (ext_ssptwad=true) by default to keep the backward
compatibility.
SW update scheme implemention is based on priv spec v1.12:
"When a virtual page is accessed and the A bit is clear, or is written
and the D bit is clear, a page-fault exception (corresponding to the
original access type) is raised."
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
target/riscv/cpu.c| 2 ++
target/riscv/cpu.h| 1 +
target/riscv/cpu_helper.c | 9 +
3 files changed, 12 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1bb3973806..1d38c1c1d2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -857,6 +857,7 @@ static void riscv_cpu_init(Object *obj)
cpu->cfg.ext_ifencei = true;
cpu->cfg.ext_icsr = true;
+cpu->cfg.ext_ssptwad = true;
cpu->cfg.mmu = true;
cpu->cfg.pmp = true;
@@ -900,6 +901,7 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
+DEFINE_PROP_BOOL("ssptwad", RISCVCPU, cfg.ext_ssptwad, true),
DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5c7acc055a..2eee59af98 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -433,6 +433,7 @@ struct RISCVCPUConfig {
bool ext_zve32f;
bool ext_zve64f;
bool ext_zmmul;
+bool ext_ssptwad;
bool rvv_ta_all_1s;
uint32_t mvendorid;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 59b3680b1b..a8607c0d7b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -981,6 +981,15 @@ restart:
/* Page table updates need to be atomic with MTTCG enabled */
if (updated_pte != pte) {
+if (!cpu->cfg.ext_ssptwad) {
+/*
+ * If A/D bits are managed by SW, HW just raises the
+ * page fault exception corresponding to the original
+ * access type when A/D bits need to be updated.
+ */
+return TRANSLATE_FAIL;
+}
+
/*
* - if accessed or dirty bits need updating, and the PTE is
* in RAM, then we do so atomically with a compare and swap.
--
2.17.1
Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function
Tested-by: Jim Shu On Fri, Mar 4, 2022 at 7:00 PM Damien Hedde wrote: > > > > On 3/3/22 14:32, Philippe Mathieu-Daudé wrote: > > On 23/2/22 10:12, Damien Hedde wrote: > >> Hi Philippe, > >> > >> I suppose it is ok if I change your mail in the reviewed by ? > > > > No, the email is fine (git tools should take care of using the > > correct email via the .mailmap entry, see commit 90f285fd83). > > > >> Thanks, > >> Damien > >> > > ok. > > Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them. > > -- > Damien > >
Re: [PATCH v4 09/14] none-machine: allow cold plugging sysbus devices
Tested-by: Jim Shu On Thu, Mar 3, 2022 at 10:46 PM Philippe Mathieu-Daudé < philippe.mathieu.da...@gmail.com> wrote: > On 23/2/22 10:07, Damien Hedde wrote: > > Allow plugging any sysbus device on this machine (the sysbus > > devices still need to be 'user-creatable'). > > > > This commit is needed to use the 'none' machine as a base, and > > subsequently to dynamically populate it with sysbus devices using > > qapi commands. > > > > Note that this only concern cold-plug: sysbus devices cann't be hot > > "can not" is easier to understand for non-native / not good level of > English speakers IMHO. > > > plugged because the sysbus bus does not support it. > > > > Signed-off-by: Damien Hedde > > --- > > hw/core/null-machine.c | 4 > > 1 file changed, 4 insertions(+) > > Reviewed-by: Philippe Mathieu-Daudé > >
Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device
Tested-by: Jim Shu
On Wed, Feb 23, 2022 at 5:14 PM Damien Hedde
wrote:
> This device can be used to create a memory wrapped into a
> sysbus device.
> This device has one property 'readonly' which allows
> to choose between a ram or a rom.
>
> The purpose for this device is to be used with qapi command
> device_add.
>
> Signed-off-by: Damien Hedde
> ---
> include/hw/mem/sysbus-memory.h | 28
> hw/mem/sysbus-memory.c | 80 ++
> hw/mem/meson.build | 2 +
> 3 files changed, 110 insertions(+)
> create mode 100644 include/hw/mem/sysbus-memory.h
> create mode 100644 hw/mem/sysbus-memory.c
>
> diff --git a/include/hw/mem/sysbus-memory.h
> b/include/hw/mem/sysbus-memory.h
> new file mode 100644
> index 00..5c596f8b4f
> --- /dev/null
> +++ b/include/hw/mem/sysbus-memory.h
> @@ -0,0 +1,28 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#ifndef HW_SYSBUS_MEMORY_H
> +#define HW_SYSBUS_MEMORY_H
> +
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +#define TYPE_SYSBUS_MEMORY "sysbus-memory"
> +OBJECT_DECLARE_SIMPLE_TYPE(SysBusMemoryState, SYSBUS_MEMORY)
> +
> +struct SysBusMemoryState {
> +/* */
> +SysBusDevice parent_obj;
> +uint64_t size;
> +bool readonly;
> +
> +/* */
> +MemoryRegion mem;
> +};
> +
> +#endif /* HW_SYSBUS_MEMORY_H */
> diff --git a/hw/mem/sysbus-memory.c b/hw/mem/sysbus-memory.c
> new file mode 100644
> index 00..f1ad7ba7ec
> --- /dev/null
> +++ b/hw/mem/sysbus-memory.c
> @@ -0,0 +1,80 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/mem/sysbus-memory.h"
> +#include "hw/qdev-properties.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +
> +static Property sysbus_memory_properties[] = {
> +DEFINE_PROP_UINT64("size", SysBusMemoryState, size, 0),
> +DEFINE_PROP_BOOL("readonly", SysBusMemoryState, readonly, false),
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void sysbus_memory_realize(DeviceState *dev, Error **errp)
> +{
> +SysBusMemoryState *s = SYSBUS_MEMORY(dev);
> +gchar *name;
> +
> +if (!s->size) {
> +error_setg(errp, "'size' must be non-zero.");
> +return;
> +}
> +
> +/*
> + * We impose having an id (which is unique) because we need to
> generate
> + * a unique name for the memory region.
> + * memory_region_init_ram/rom() will abort() (in qemu_ram_set_idstr()
> + * function if 2 system-memory devices are created with the same name
> + * for the memory region).
> + */
> +if (!dev->id) {
> +error_setg(errp, "system-memory device must have an id.");
> +return;
> +}
> +name = g_strdup_printf("%s.region", dev->id);
> +
> +if (s->readonly) {
> +memory_region_init_rom(>mem, OBJECT(dev), name, s->size, errp);
> +} else {
> +memory_region_init_ram(>mem, OBJECT(dev), name, s->size, errp);
> +}
> +
> +g_free(name);
> +if (*errp) {
> +return;
> +}
> +
> +sysbus_init_mmio(SYS_BUS_DEVICE(s), >mem);
> +}
> +
> +static void sysbus_memory_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +dc->user_creatable = true;
> +dc->realize = sysbus_memory_realize;
> +device_class_set_props(dc, sysbus_memory_properties);
> +}
> +
> +static const TypeInfo sysbus_memory_info = {
> +.name = TYPE_SYSBUS_MEMORY,
> +.parent= TYPE_SYS_BUS_DEVICE,
> +.instance_size = sizeof(SysBusMemoryState),
> +.class_init= sysbus_memory_class_init,
> +};
> +
> +static void sysbus_memory_register_types(void)
> +{
> +type_register_static(_memory_info);
> +}
> +
> +type_init(sysbus_memory_register_types)
> diff --git a/hw/mem/meson.build b/hw/mem/meson.build
> index 82f86d117e..04c74e12f2 100644
> --- a/hw/mem/meson.build
> +++ b/hw/mem/meson.build
> @@ -7,3 +7,5 @@ mem_ss.add(when: 'CONFIG_NVDIMM', if_true:
> files('nvdimm.c'))
> softmmu_ss.add_all(when: 'CONFIG_MEM_DEVICE', if_true: mem_ss)
>
> softmmu_ss.add(when: 'CONFIG_SPARSE_MEM', if_true: files('sparse-mem.c'))
> +
> +softmmu_ss.add(files('sysbus-memory.c'))
> --
> 2.35.1
>
>
>
Re: [PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices
Tested-by: Jim Shu On Fri, Mar 4, 2022 at 11:23 PM Philippe Mathieu-Daudé < philippe.mathieu.da...@gmail.com> wrote: > On 23/2/22 10:07, Damien Hedde wrote: > > The devices are: > > + ibex-timer > > + ibex-uart > > + riscv.aclint.swi > > + riscv.aclint.mtimer > > + riscv.hart_array > > + riscv.sifive.e.prci > > + riscv.sifive.plic > > + riscv.sifive.uart > > + sifive_soc.gpio > > + unimplemented-device > > > > These devices are clean regarding error handling in realize. > > > > They are all sysbus devices, so setting user-creatable will only > > enable cold-plugging them on machine having explicitely allowed them > > (only _none_ machine does that). > > > > Note that this commit include the ricv_array which embeds cpus. There > > Typo "includes" I guess. > > Reviewed-by: Philippe Mathieu-Daudé > > > are some deep internal constraints about them: you cannot create more > > cpus than the machine's maxcpus. TCG accelerator's code will for example > > assert if a user try to create too many cpus. > > > > Signed-off-by: Damien Hedde > > --- > > > > I can also split this patch if you think it's better. > > But it is mostly a one-line fix per file. > > > > This patch requires first some cleanups in order to fix error errors > > and some more memory leaks that could happend in legit user-related > > life cycles: a miss-configuration should not be a fatal error anymore. > > > https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.he...@greensocs.com > > --- > > hw/char/ibex_uart.c | 1 + > > hw/char/sifive_uart.c | 1 + > > hw/gpio/sifive_gpio.c | 1 + > > hw/intc/riscv_aclint.c | 2 ++ > > hw/intc/sifive_plic.c | 1 + > > hw/misc/sifive_e_prci.c | 8 > > hw/misc/unimp.c | 1 + > > hw/riscv/riscv_hart.c | 1 + > > hw/timer/ibex_timer.c | 1 + > > 9 files changed, 17 insertions(+) > >
Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property
Tested-by: Jim Shu
On Fri, Mar 4, 2022 at 12:36 AM Damien Hedde
wrote:
>
>
> On 3/3/22 15:41, Philippe Mathieu-Daudé wrote:
> > On 23/2/22 10:07, Damien Hedde wrote:
> >> Add the property to configure a the base address of the ram.
> >> The default value remains zero.
> >>
> >> This commit is needed to use the 'none' machine as a base, and
> >> subsequently to dynamically populate it using qapi commands. Having
> >> a non null 'ram' is really hard to workaround because of the actual
> >> constraints on the generic loader: it prevents loading binaries
> >> bigger than ram_size (with a null ram, we cannot load anything).
> >> For now we need to be able to use the existing ram creation
> >> feature of the none machine with a configurable base address.
> >>
> >> Signed-off-by: Damien Hedde
> >> ---
> >> hw/core/null-machine.c | 34 --
> >> 1 file changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> >> index 7eb258af07..5fd1cc0218 100644
> >> --- a/hw/core/null-machine.c
> >> +++ b/hw/core/null-machine.c
> >> @@ -16,9 +16,11 @@
> >> #include "hw/boards.h"
> >> #include "exec/address-spaces.h"
> >> #include "hw/core/cpu.h"
> >> +#include "qapi/visitor.h"
> >> struct NoneMachineState {
> >> MachineState parent;
> >> +uint64_t ram_addr;
> >> };
> >> #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
> >> @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState,
> >> NONE_MACHINE)
> >> static void machine_none_init(MachineState *mch)
> >> {
> >> +NoneMachineState *nms = NONE_MACHINE(mch);
> >> CPUState *cpu = NULL;
> >> /* Initialize CPU (if user asked for it) */
> >> @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
> >> }
> >> }
> >> -/* RAM at address zero */
> >> +/* RAM at configured address (default: 0) */
> >> if (mch->ram) {
> >> -memory_region_add_subregion(get_system_memory(), 0, mch->ram);
> >> +memory_region_add_subregion(get_system_memory(), nms->ram_addr,
> >> +mch->ram);
> >> +} else if (nms->ram_addr) {
> >> +error_report("'ram-addr' has been specified but the size is
> >> zero");
> >
> > I'm not sure about this error message, IIUC we can get here if no ram
> > backend is provided, not if we have one zero-sized. Otherwise LGTM.
>
> You're most probably right. Keeping the ram_size to 0 is just one way of
> getting here. I can replace the message by a more generic formulation
> "'ram-addr' has been specified but the machine has no ram"
>
>
>
Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command
Tested-by: Jim Shu
On Wed, Feb 23, 2022 at 5:37 PM Damien Hedde
wrote:
> This command allows to map an mmio region of sysbus device onto
> the system memory. Its behavior mimics the sysbus_mmio_map()
> function apart from the automatic unmap (the C function unmaps
> the region if it is already mapped).
> For the qapi function we consider it is an error to try to map
> an already mapped function. If unmapping is required, it is
> probably better to add a sysbus-mmip-unmap command.
>
> This command is still experimental (hence the 'unstable' feature),
> as it is related to the sysbus device creation through qapi commands.
>
> This command is required to be able to dynamically build a machine
> from scratch as there is no qapi-way of doing a memory mapping.
>
> Signed-off-by: Damien Hedde
> ---
> Cc: Alistair Francis
>
> v4:
> + integrate priority parameter
> + use 'unstable' feature flag instead of 'x-' prefix
> + bump version to 7.0
> + dropped Alistair's reviewed-by as a consequence
> ---
> qapi/qdev.json | 31 ++
> hw/core/sysbus.c | 49
> 2 files changed, 80 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2e2de41499..4830e87a90 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -160,3 +160,34 @@
> ##
> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @sysbus-mmio-map:
> +#
> +# Map a sysbus device mmio onto the main system bus.
> +#
> +# @device: the device's QOM path
> +#
> +# @mmio: The mmio number to be mapped (defaults to 0).
> +#
> +# @addr: The base address for the mapping.
> +#
> +# @priority: The priority of the mapping (defaults to 0).
> +#
> +# Features:
> +# @unstable: Command is meant to map sysbus devices
> +#while in preconfig mode.
> +#
> +# Since: 7.0
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'sysbus-mmio-map',
> + 'data': { 'device': 'str',
> +'*mmio': 'uint8',
> +'addr': 'uint64',
> +'*priority': 'int32' },
> + 'features': ['unstable'],
> + 'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d31..df1f1f43a5 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -23,6 +23,7 @@
> #include "hw/sysbus.h"
> #include "monitor/monitor.h"
> #include "exec/address-spaces.h"
> +#include "qapi/qapi-commands-qdev.h"
>
> static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> static char *sysbus_get_fw_dev_path(DeviceState *dev);
> @@ -154,6 +155,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev,
> int n, hwaddr addr,
> }
> }
>
> +void qmp_sysbus_mmio_map(const char *device,
> + bool has_mmio, uint8_t mmio,
> + uint64_t addr,
> + bool has_priority, int32_t priority,
> + Error **errp)
> +{
> +Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE,
> NULL);
> +SysBusDevice *dev;
> +
> +if (phase_get() != PHASE_MACHINE_INITIALIZED) {
> +error_setg(errp, "The command is permitted only when "
> + "the machine is in initialized phase");
> +return;
> +}
> +
> +if (obj == NULL) {
> +error_setg(errp, "Device '%s' not found", device);
> +return;
> +}
> +dev = SYS_BUS_DEVICE(obj);
> +
> +if (!has_mmio) {
> +mmio = 0;
> +}
> +if (!has_priority) {
> +priority = 0;
> +}
> +
> +if (mmio >= dev->num_mmio) {
> +error_setg(errp, "MMIO index '%u' does not exist in '%s'",
> + mmio, device);
> +return;
> +}
> +
> +if (dev->mmio[mmio].addr != (hwaddr)-1) {
> +error_setg(errp, "MMIO index '%u' is already mapped", mmio);
> +return;
> +}
> +
> +if (!memory_region_try_add_subregion(get_system_memory(), addr,
> + dev->mmio[mmio].memory, priority,
> + errp)) {
> +return;
> +}
> +
> +dev->mmio[mmio].addr = addr;
> +}
> +
> void sysbus_mmio_unmap(SysBusDevice *dev, int n)
> {
> assert(n >= 0 && n < dev->num_mmio);
> --
> 2.35.1
>
>
>
Re: [PATCH v4 07/14] none-machine: add the NoneMachineState structure
Tested-by: Jim Shu
On Wed, Feb 23, 2022 at 5:59 PM Damien Hedde
wrote:
> The none machine was using the parent state structure.
> We'll need a custom state to add a field in the following commit.
>
> Signed-off-by: Damien Hedde
> ---
> hw/core/null-machine.c | 24 ++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index f586a4bef5..7eb258af07 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -17,6 +17,13 @@
> #include "exec/address-spaces.h"
> #include "hw/core/cpu.h"
>
> +struct NoneMachineState {
> +MachineState parent;
> +};
> +
> +#define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
> +OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE)
> +
> static void machine_none_init(MachineState *mch)
> {
> CPUState *cpu = NULL;
> @@ -42,8 +49,10 @@ static void machine_none_init(MachineState *mch)
> }
> }
>
> -static void machine_none_machine_init(MachineClass *mc)
> +static void machine_none_class_init(ObjectClass *oc, void *data)
> {
> +MachineClass *mc = MACHINE_CLASS(oc);
> +
> mc->desc = "empty machine";
> mc->init = machine_none_init;
> mc->max_cpus = 1;
> @@ -56,4 +65,15 @@ static void machine_none_machine_init(MachineClass *mc)
> mc->no_sdcard = 1;
> }
>
> -DEFINE_MACHINE("none", machine_none_machine_init)
> +static const TypeInfo none_machine_info = {
> +.name = TYPE_NONE_MACHINE,
> +.parent= TYPE_MACHINE,
> +.instance_size = sizeof(NoneMachineState),
> +.class_init= machine_none_class_init,
> +};
> +
> +static void none_machine_register_types(void)
> +{
> +type_register_static(_machine_info);
> +}
> +type_init(none_machine_register_types);
> --
> 2.35.1
>
>
>
Re: [PATCH v4 05/14] qapi/device_add: handle the rom_order_override when cold-plugging
Tested-by: Jim Shu
On Wed, Feb 23, 2022 at 5:18 PM Damien Hedde
wrote:
> rom_set_order_override() and rom_reset_order_override() were called
> in qemu_create_cli_devices() to set the rom_order_override value
> once and for all when creating the devices added on CLI.
>
> Unfortunately this won't work with qapi commands.
>
> Move the calls inside device_add so that it will be done in every
> case:
> + CLI option: -device
> + QAPI command: device_add
>
> rom_[set|reset]_order_override() are implemented in hw/core/loader.c
> They either do nothing or call fw_cfg_[set|reset]_order_override().
> The later functions are implemented in hw/nvram/fw_cfg.c and only
> change an integer value of a "global" variable.
> In consequence, there are no complex side effects involved and we can
> safely move them from outside the -device option loop to the inner
> function.
>
> Signed-off-by: Damien Hedde
> ---
> softmmu/qdev-monitor.c | 11 +++
> softmmu/vl.c | 2 --
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 47a89aee20..9ec3e0ebff 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -43,6 +43,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/clock.h"
> #include "hw/boards.h"
> +#include "hw/loader.h"
>
> /*
> * Aliases were a bad idea from the start. Let's keep them
> @@ -671,6 +672,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> return NULL;
> }
>
> +if (!is_hotplug) {
> +rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> +}
> +
> /* create device */
> dev = qdev_new(driver);
>
> @@ -712,6 +717,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> if (!qdev_realize(DEVICE(dev), bus, errp)) {
> goto err_del_dev;
> }
> +if (!is_hotplug) {
> +rom_reset_order_override();
> +}
> return dev;
>
> err_del_dev:
> @@ -719,6 +727,9 @@ err_del_dev:
> object_unparent(OBJECT(dev));
> object_unref(OBJECT(dev));
> }
> +if (!is_hotplug) {
> +rom_reset_order_override();
> +}
> return NULL;
> }
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 50337d68b9..b91ae1b8ae 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2680,7 +2680,6 @@ static void qemu_create_cli_devices(void)
> }
>
> /* init generic devices */
> -rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> qemu_opts_foreach(qemu_find_opts("device"),
>device_init_func, NULL, _fatal);
> QTAILQ_FOREACH(opt, _opts, next) {
> @@ -2697,7 +2696,6 @@ static void qemu_create_cli_devices(void)
> object_unref(OBJECT(dev));
> loc_pop(>loc);
> }
> -rom_reset_order_override();
> }
>
> static void qemu_machine_creation_done(void)
> --
> 2.35.1
>
>
>
Re: [PATCH v5 3/6] vl: support machine-initialized target in phase_until()
Tested-by: Jim Shu
On Thu, May 19, 2022 at 11:36 PM Damien Hedde
wrote:
> phase_until() now supports the following transitions:
> + accel-created -> machine-initialized
> + machine-initialized -> machine-ready
>
> As a consequence we can now support the use of qmp_exit_preconfig()
> from phases _accel-created_ and _machine-initialized_.
>
> This commit is a preparation to support cold plugging a device
> using qapi (which will be introduced in a following commit). For this
> we need fine grain control of the phase.
>
> Signed-off-by: Damien Hedde
> Reviewed-by: Philippe Mathieu-Daudé
> ---
>
> v5: update due to refactor of previous commit
> ---
> softmmu/vl.c | 26 +-
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 7f8d15b5b8..ea15e37973 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2698,8 +2698,9 @@ static void qemu_machine_creation_done(void)
>
> void qmp_x_exit_preconfig(Error **errp)
> {
> -if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> -error_setg(errp, "The command is permitted only before machine
> initialization");
> +if (phase_check(PHASE_MACHINE_READY)) {
> +error_setg(errp, "The command is permitted only before"
> + " machine is ready");
> return;
> }
> phase_until(PHASE_MACHINE_READY, errp);
> @@ -2707,9 +2708,6 @@ void qmp_x_exit_preconfig(Error **errp)
>
> static void qemu_phase_ready(Error **errp)
> {
> -qemu_init_board();
> -/* phase is now PHASE_MACHINE_INITIALIZED. */
> -qemu_create_cli_devices();
> cxl_fixed_memory_window_link_targets(errp);
> qemu_machine_creation_done();
> /* Phase is now PHASE_MACHINE_READY. */
> @@ -2749,6 +2747,24 @@ bool phase_until(MachineInitPhase phase, Error
> **errp)
>
> switch (cur_phase) {
> case PHASE_ACCEL_CREATED:
> +qemu_init_board();
> +/* Phase is now PHASE_MACHINE_INITIALIZED. */
> +/*
> + * Handle CLI devices now in order leave this case in a state
> + * where we can cold plug devices with QMP. The following call
> + * handles the CLI options:
> + * + -fw_cfg (has side effects on device cold plug)
> + * + -device
> + */
> +qemu_create_cli_devices();
> +/*
> + * At this point all CLI options are handled apart:
> + * + -S (autostart)
> + * + -incoming
> + */
> +break;
> +
> +case PHASE_MACHINE_INITIALIZED:
> qemu_phase_ready(errp);
> break;
>
> --
> 2.36.1
>
>
>
Re: [PATCH v5 2/6] machine: introduce phase_until() to handle phase transitions
Tested-by: Jim Shu
On Thu, May 19, 2022 at 11:41 PM Damien Hedde
wrote:
> phase_until() is implemented in vl.c and is meant to be used
> to make startup progress up to a specified phase being reached().
> At this point, no behavior change is introduced: phase_until()
> only supports a single double transition corresponding
> to the functionality of qmp_exit_preconfig():
> + accel-created -> machine-initialized -> machine-ready
>
> As a result qmp_exit_preconfig() now uses phase_until().
>
> This commit is a preparation to support cold plugging a device
> using qapi command (which will be introduced in a following commit).
> For this we need fine grain control of the phase.
>
> Signed-off-by: Damien Hedde
> ---
>
> v5:
> + refactor to avoid indentation change
> ---
> include/hw/qdev-core.h | 14 +
> softmmu/vl.c | 46 ++
> 2 files changed, 60 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e29c705b74..5f73d06408 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
> */
> extern void phase_advance(MachineInitPhase phase);
>
> +/**
> + * @phase_until:
> + * @phase: the target phase
> + * @errp: error report
> + *
> + * Make the machine init progress until the target phase is reached.
> + *
> + * Its is a no-op is the target phase is the current or an earlier
> + * phase.
> + *
> + * Returns true in case of success.
> + */
> +extern bool phase_until(MachineInitPhase phase, Error **errp);
> +
> #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 84a31eba76..7f8d15b5b8 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2702,11 +2702,17 @@ void qmp_x_exit_preconfig(Error **errp)
> error_setg(errp, "The command is permitted only before machine
> initialization");
> return;
> }
> +phase_until(PHASE_MACHINE_READY, errp);
> +}
>
> +static void qemu_phase_ready(Error **errp)
> +{
> qemu_init_board();
> +/* phase is now PHASE_MACHINE_INITIALIZED. */
> qemu_create_cli_devices();
> cxl_fixed_memory_window_link_targets(errp);
> qemu_machine_creation_done();
> +/* Phase is now PHASE_MACHINE_READY. */
>
> if (loadvm) {
> load_snapshot(loadvm, NULL, false, NULL, _fatal);
> @@ -2729,6 +2735,46 @@ void qmp_x_exit_preconfig(Error **errp)
> }
> }
>
> +bool phase_until(MachineInitPhase phase, Error **errp)
> +{
> +ERRP_GUARD();
> +if (!phase_check(PHASE_ACCEL_CREATED)) {
> +error_setg(errp, "Phase transition is not supported until
> accelerator"
> + " is created");
> +return false;
> +}
> +
> +while (!phase_check(phase)) {
> +MachineInitPhase cur_phase = phase_get();
> +
> +switch (cur_phase) {
> +case PHASE_ACCEL_CREATED:
> +qemu_phase_ready(errp);
> +break;
> +
> +default:
> +/*
> + * If we end up here, it is because we miss a case above.
> + */
> +error_setg(_abort, "Requested phase transition is not"
> + " implemented");
> +return false;
> +}
> +
> +if (*errp) {
> +return false;
> +}
> +
> +/*
> + * Ensure we made some progress.
> + * With the default case above, it should be enough to prevent
> + * any infinite loop.
> + */
> +assert(cur_phase < phase_get());
> +}
> +return true;
> +}
> +
> void qemu_init(int argc, char **argv, char **envp)
> {
> QemuOpts *opts;
> --
> 2.36.1
>
>
>
Re: [PATCH v5 4/6] qapi/device_add: compute is_hotplug flag
Tested-by: Jim Shu
On Thu, May 19, 2022 at 11:37 PM Damien Hedde
wrote:
> Instead of checking the phase everytime, just store the result
> in a flag. We will use more of it in the following commit.
>
> Signed-off-by: Damien Hedde
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> softmmu/qdev-monitor.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 12fe60c467..d68ef883b5 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -619,6 +619,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> char *id;
> DeviceState *dev = NULL;
> BusState *bus = NULL;
> +bool is_hotplug = phase_check(PHASE_MACHINE_READY);
>
> driver = qdict_get_try_str(opts, "driver");
> if (!driver) {
> @@ -662,7 +663,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> return NULL;
> }
>
> -if (phase_check(PHASE_MACHINE_READY) && bus &&
> !qbus_is_hotpluggable(bus)) {
> +if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) {
> error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
> return NULL;
> }
> @@ -676,7 +677,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> dev = qdev_new(driver);
>
> /* Check whether the hotplug is allowed by the machine */
> -if (phase_check(PHASE_MACHINE_READY)) {
> +if (is_hotplug) {
> if (!qdev_hotplug_allowed(dev, errp)) {
> goto err_del_dev;
> }
> --
> 2.36.1
>
>
>
Re: [PATCH v5 6/6] qapi/device_add: Allow execution in machine initialized phase
Tested-by: Jim Shu
On Thu, May 19, 2022 at 11:37 PM Damien Hedde
wrote:
> From: Mirela Grujic
>
> This commit allows to use the QMP command to add a cold-plugged
> device like we can do with the CLI option -device.
>
> Note: for device_add command in qdev.json adding the 'allow-preconfig'
> option has no effect because the command appears to bypass QAPI (see
> TODO at qapi/qdev.json:61). The option is added there solely to
> document the intent.
> For the same reason, the flags have to be explicitly set in
> monitor_init_qmp_commands() when the device_add command is registered.
>
> Signed-off-by: Mirela Grujic
> Signed-off-by: Damien Hedde
> ---
>
> v4:
> + use phase_until()
> + add missing flag in hmp-commands.hx
> ---
> qapi/qdev.json | 3 ++-
> monitor/misc.c | 2 +-
> softmmu/qdev-monitor.c | 4
> hmp-commands.hx| 1 +
> 4 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 26cd10106b..2e2de41499 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -77,7 +77,8 @@
> { 'command': 'device_add',
>'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
>'gen': false, # so we can get the additional arguments
> - 'features': ['json-cli', 'json-cli-hotplug'] }
> + 'features': ['json-cli', 'json-cli-hotplug'],
> + 'allow-preconfig': true }
>
> ##
> # @device_del:
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 6c5bb82d3b..d3d413d70c 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -233,7 +233,7 @@ static void monitor_init_qmp_commands(void)
> qmp_init_marshal(_commands);
>
> qmp_register_command(_commands, "device_add",
> - qmp_device_add, 0, 0);
> + qmp_device_add, QCO_ALLOW_PRECONFIG, 0);
>
> QTAILQ_INIT(_cap_negotiation_commands);
> qmp_register_command(_cap_negotiation_commands,
> "qmp_capabilities",
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 7cbee2b0d8..c53f62be51 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -855,6 +855,10 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> Error **errp)
> QemuOpts *opts;
> DeviceState *dev;
>
> +if (!phase_until(PHASE_MACHINE_INITIALIZED, errp)) {
> +return;
> +}
> +
> opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
> if (!opts) {
> return;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 03e6a73d1f..0091b8e2dd 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -672,6 +672,7 @@ ERST
> .help = "add device, like -device on the command line",
> .cmd= hmp_device_add,
> .command_completion = device_add_completion,
> +.flags = "p",
> },
>
> SRST
> --
> 2.36.1
>
>
>
Re: [PATCH v5 1/6] machine: add phase_get() and document phase_check()/advance()
Tested-by: Jim Shu
On Thu, May 19, 2022 at 11:41 PM Damien Hedde
wrote:
> phase_get() returns the current phase, we'll use it in next
> commit.
>
> Signed-off-by: Damien Hedde
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> include/hw/qdev-core.h | 19 +++
> hw/core/qdev.c | 5 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..e29c705b74 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -887,7 +887,26 @@ typedef enum MachineInitPhase {
> PHASE_MACHINE_READY,
> } MachineInitPhase;
>
> +/*
> + * phase_get:
> + * Returns the current phase
> + */
> +MachineInitPhase phase_get(void);
> +
> +/**
> + * phase_check:
> + * Test if current phase is at least @phase.
> + *
> + * Returns true if this is the case.
> + */
> extern bool phase_check(MachineInitPhase phase);
> +
> +/**
> + * @phase_advance:
> + * Update the current phase to @phase.
> + *
> + * Must only be used to make a single phase step.
> + */
> extern void phase_advance(MachineInitPhase phase);
>
> #endif
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 84f3019440..632dc0a4be 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -910,6 +910,11 @@ Object *qdev_get_machine(void)
>
> static MachineInitPhase machine_phase;
>
> +MachineInitPhase phase_get(void)
> +{
> +return machine_phase;
> +}
> +
> bool phase_check(MachineInitPhase phase)
> {
> return machine_phase >= phase;
> --
> 2.36.1
>
>
>
Re: [PATCH v4 00/14] Initial support for machine creation via QMP
Hi all, Thanks for the work! I'm from SiFive and we are very interested in this feature. QMP/QAPI configurable QEMU machine is a useful feature in our use case. With this feature, we can both model our versatile FPGA-based platforms more easily and model a new platform without modification of source code. It is helpful for early software development of SoC prototyping. We think this feature is also helpful to the QEMU community. Also, I have tested this patchset (v4) and newer v5 patchset [1] with Damien's firmware [2] and it works correctly. p.s. QMP option "-qmp socket,path=./qmpsocket,server" in v5 patchset instruction may not work? I use the option "-qmp unix:./qmpsocket,server" instead. [1] [PATCH v5 0/6] QAPI support for device cold-plug https://lore.kernel.org/qemu-devel/20220519153402.41540-1-damien.he...@greensocs.com/ [2] Test firmware for patchset v5: https://github.com/GreenSocs/qemu-qmp-machines/tree/master/arm-virt v4: https://github.com/GreenSocs/qemu-qmp-machines/tree/eba16dab8b587e624d65c5c302aeef424bece3a0 On Thu, Mar 3, 2022 at 7:02 PM Damien Hedde wrote: > Ping ! > > It would be good to have some feedback on 1st and 2nd part. > > Thanks, > Damien > > On 2/23/22 10:06, Damien Hedde wrote: > > Hi, > > > > This series adds initial support to build a machine using QMP/QAPI > > commands. With this series, one can start from the 'none' machine, > > create cpus, sysbus devices, memory map them and wire interrupts. > > > > Sorry for the huge cc list on this cover-letter. Apart from people > > who attended the kvm call about this topic, I've cc'ed you only > > according to MAINTAINERS file. > > > > The series is divided in 4 parts which are independent of each other, > > but we need the 4 parts to be able to use this mechanism: > > + Patches 1 to 6 allow to use the qapi command device_add to cold > >plug devices (like CLI -device do) > > + Patches 7 to 10 modify the 'none' machine which serves as base > >machine. > > + Patches 11 to 13 handle memory mapping and memory creation > > + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine > >to build some example. This last patch is based on a cleanup > >series: it probably works without it, but some config errors are > >not handled (see based-on below). > > > > Only patch 11 is reviewed-by. > > > > v4: > > + cold plugging approach changed in order not to conflict with > >startup. I do not add additional command to handle this so that > >we can change everything easily. > > + device_add in cold plug context is also now equivalent to -device > >CLI regarding -fw_cfg. I also added patches to modify the 'none' > >machine. > > + reworked most of the none machine part > > + updated the sybus-mmio-map command patch > > > > Note that there are still lot of limitations (for example if you try > > to create more cpus than the _max_cpus_, tcg will abort()). > > Basically all tasks done by machine init reading some parameters are > > really tricky: for example, loading complex firmware. But we have to > > start by something and all this is not accessible unless the user > > asked for none machine and -preconfig. > > > > I can maintain the code introduced here. I'm not sure what's the > > process. Is there something else to do than propose a patch to > > MAINTAINERS ? > > If there is a global agreement on moving on with these feature, it > > would be great to have a login on qemu wiki so I can document > > limitations and the work being done to solve them. > > > > A simple test can be done with the following scenario which build > > a machine subset of the opentitan. > > > > $ cat commands.qmp > > // RAM 0x1000 > > device_add driver=sysbus-memory id=ram size=0x4000 readonly=false > > sysbus-mmio-map device=ram addr=268435456 > > // CPUS > > device_add driver=riscv.hart_array id=cpus > cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080 > > // ROM 0x8000 > > device_add driver=sysbus-memory id=rom size=0x4000 readonly=true > > sysbus-mmio-map device=rom addr=32768 > > // PLIC 0x4800 > > device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 > num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 > enable-base=0x2000 enable-stride=32 context-base=0x20 context-stride=8 > aperture-size=0x4005000 > > sysbus-mmio-map device=plic addr=1207959552 > > qom-set path=plic property=unnamed-gpio-out[1] > value=cpus/harts[0]/unnamed-gpio-in[11] > > // UART 0x4000 > > device_add driver=ibex-uart id=uart chardev=serial0 > > sysbus-mmio-map device=uart addr=1073741824 > > qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2] > > // FIRMWARE > > device_add driver=loader cpu-num=0 file=/path/to/firmware.elf > > x-exit-preconfig > > > > $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio > -qmp unix:/tmp/qmp-sock,server > > > > In another terminal, you'll need to send the commands with, for example: > > $ grep -v '^//'
[PATCH v2 0/2] Align SiFive PDMA behavior to real hardware
HiFive Unmatched PDMA supports high/low 32-bit access of 64-bit register, but QEMU emulation supports low part access now. Enhance QEMU emulation to support high 32-bit access. Also, permit 4/8-byte valid access in PDMA as we have verified 32/64-bit accesses of PDMA registers are supported. Changelog: v2: * Fix high 32-bit write access of 64-bit RO registers * Fix commit log Jim Shu (2): hw/dma: sifive_pdma: support high 32-bit access of 64-bit register hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers hw/dma/sifive_pdma.c | 181 +-- 1 file changed, 159 insertions(+), 22 deletions(-) -- 2.25.1
[PATCH v2 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
It's obvious that PDMA supports 64-bit access of 64-bit registers, and
in previous commit, we confirm that PDMA supports 32-bit access of
both 32/64-bit registers. Thus, we configure 32/64-bit memory access
of PDMA registers as valid in general.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Reviewed-by: Bin Meng
Tested-by: Bin Meng
---
hw/dma/sifive_pdma.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index f4df16449b..1dd88f3479 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -444,6 +444,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
.impl = {
.min_access_size = 4,
.max_access_size = 8,
+},
+.valid = {
+.min_access_size = 4,
+.max_access_size = 8,
}
};
--
2.25.1
[PATCH v2 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
Real PDMA supports high 32-bit read/write memory access of 64-bit
register.
The following result is PDMA tested in U-Boot on Unmatched board:
1. Real PDMA allows high 32-bit read/write to 64-bit register.
=> mw.l 0x300 0x0 <= Disclaim channel 0
=> mw.l 0x300 0x1 <= Claim channel 0
=> mw.l 0x310 0x8000 <= Write low 32-bit NextDest
(NextDest = 0x28000)
=> mw.l 0x314 0x2 <= Write high 32-bit NextDest
=> md.l 0x310 1<= Dump low 32-bit NextDest
0310: 8000
=> md.l 0x314 1<= Dump high 32-bit NextDest
0314: 0002
=> mw.l 0x318 0x80001000 <= Write low 32-bit NextSrc (NextSrc
= 0x280001000)
=> mw.l 0x31c 0x2 <= Write high 32-bit NextSrc
=> md.l 0x318 1<= Dump low 32-bit NextSrc
0310: 80001000
=> md.l 0x31c 1<= Dump high 32-bit NextSrc
0314: 0002
2. PDMA transfer from 0x280001000 to 0x28000 is OK.
=> mw.q 0x308 0x4 <= NextBytes = 4
=> mw.l 0x304 0x2200 <= wsize = rsize = 2 (2^2 = 4 bytes)
=> mw.l 0x28000 0x87654321 <= Fill test data to dst
=> mw.l 0x280001000 0x12345678 <= Fill test data to src
=> md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory contents
28000: 87654321 !Ce.
280001000: 12345678 xV4.
=> md.l 0x300 8<= Dump PDMA status
0300: 0001 2200 0004 ..."
0310: 8000 0002 80001000 0002
=> mw.l 0x300 0x3 <= Set channel 0 run and claim bits
=> md.l 0x300 8<= Dump PDMA status
0300: 4001 2200 0004 ...@..."
0310: 8000 0002 80001000 0002
=> md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory contents
280000000: 12345678 xV4.
280001000: 12345678 xV4.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
Reviewed-by: Alistair Francis
Reviewed-by: Bin Meng
Tested-by: Bin Meng
---
hw/dma/sifive_pdma.c | 177 +--
1 file changed, 155 insertions(+), 22 deletions(-)
diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index 85fe34f5f3..f4df16449b 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState
*s, int ch)
s->chan[ch].state = DMA_CHAN_STATE_IDLE;
}
-static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
+static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
{
-SiFivePDMAState *s = opaque;
-int ch = SIFIVE_PDMA_CHAN_NO(offset);
uint64_t val = 0;
-if (ch >= SIFIVE_PDMA_CHANS) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
- __func__, ch);
-return 0;
+offset &= 0xfff;
+switch (offset) {
+case DMA_NEXT_BYTES:
+val = s->chan[ch].next_bytes;
+break;
+case DMA_NEXT_DST:
+val = s->chan[ch].next_dst;
+break;
+case DMA_NEXT_SRC:
+val = s->chan[ch].next_src;
+break;
+case DMA_EXEC_BYTES:
+val = s->chan[ch].exec_bytes;
+break;
+case DMA_EXEC_DST:
+val = s->chan[ch].exec_dst;
+break;
+case DMA_EXEC_SRC:
+val = s->chan[ch].exec_src;
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
+ __func__, offset);
+break;
}
+return val;
+}
+
+static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
+{
+uint32_t val = 0;
+
offset &= 0xfff;
switch (offset) {
case DMA_CONTROL:
@@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr
offset, unsigned size)
val = s->chan[ch].next_config;
break;
case DMA_NEXT_BYTES:
-val = s->chan[ch].next_bytes;
+val = extract64(s->chan[ch].next_bytes, 0, 32);
+break;
+case DMA_NEXT_BYTES + 4:
+val = extract64(s->chan[ch].next_bytes, 32, 32);
break;
case DMA_NEXT_DST:
-val = s->chan[ch].next_dst;
+val = extract64(s->chan[ch].next_dst, 0, 32);
+break;
+case DMA_NEXT_DST + 4:
+val = extract64(s->chan[ch].next_dst, 32, 32);
break;
case DMA_NEXT_SRC:
-val = s->chan[ch].next_src;
+v
Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
Hi Bin,
Thanks for the review.
I will fix the commit log and the behavior of writing high 32-bit of RO
registers in v2 patch.
Thanks,
Jim Shu
On Tue, Jan 4, 2022 at 10:55 AM Bin Meng wrote:
> Hi Jim,
>
> On Tue, Dec 28, 2021 at 8:53 AM Jim Shu wrote:
> >
> > Real PDMA support high 32-bit read/write memory access of 64-bit
>
> %s/support/supports
>
> > register.
> >
> > The following result is PDMA tested in U-Boot on Unmatched board:
> >
> > 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
>
> %s/is allowed/allows
>
> > => mw.l 0x300 0x0 <= Disclaim channel 0
> > => mw.l 0x300 0x1 <= Claim channel 0
> > => mw.l 0x310 0x8000 <= Write low 32-bit NextDest
> (NextDest = 0x28000)
> > => mw.l 0x314 0x2 <= Write high 32-bit NextDest
> > => md.l 0x310 1<= Dump low 32-bit NextDest
> > 0310: 8000
> > => md.l 0x314 1<= Dump high 32-bit NextDest
> > 0314: 0002
> > => mw.l 0x318 0x80001000 <= Write low 32-bit NextSrc
> (NextSrc = 0x280001000)
> > => mw.l 0x31c 0x2 <= Write high 32-bit NextSrc
> > => md.l 0x318 1<= Dump low 32-bit NextSrc
> > 0310: 80001000
> > => md.l 0x31c 1<= Dump high 32-bit NextSrc
> > 0314: 0002
> >
> > 2. PDMA transfer from 0x280001000 to 0x28000 is OK.
> > => mw.q 0x308 0x4 <= NextBytes = 4
> > => mw.l 0x304 0x2200 <= wsize = rsize = 2 (2^2 = 4
> bytes)
> > => mw.l 0x28000 0x87654321 <= Fill test data to dst
> > => mw.l 0x280001000 0x12345678 <= Fill test data to src
> > => md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory
> contents
> > 28000: 87654321 !Ce.
> > 280001000: 12345678 xV4.
> > => md.l 0x300 8<= Dump PDMA status
> > 0300: 0001 2200 0004 ..."
> > 0310: 8000 0002 80001000 0002
> > => mw.l 0x300 0x3 <= Set channel 0 run and
> claim bits
> > => md.l 0x300 8<= Dump PDMA status
> > 0300: 4001 2200 0004 ...@..."
> > 0310: 8000 0002 80001000 0002
> > => md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory
> contents
> > 28000: 12345678 xV4.
> > 280001000: 12345678 xV4.
> >
> > Signed-off-by: Jim Shu
> > Reviewed-by: Frank Chang
> > ---
> > hw/dma/sifive_pdma.c | 174 +--
> > 1 file changed, 152 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> > index 85fe34f5f3..b8b198ab4e 100644
> > --- a/hw/dma/sifive_pdma.c
> > +++ b/hw/dma/sifive_pdma.c
> > @@ -177,18 +177,44 @@ static inline void
> sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
> > s->chan[ch].state = DMA_CHAN_STATE_IDLE;
> > }
> >
> > -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned
> size)
> > +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr
> offset)
> > {
> > -SiFivePDMAState *s = opaque;
> > -int ch = SIFIVE_PDMA_CHAN_NO(offset);
> > uint64_t val = 0;
> >
> > -if (ch >= SIFIVE_PDMA_CHANS) {
> > -qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> > - __func__, ch);
> > -return 0;
> > +offset &= 0xfff;
> > +switch (offset) {
> > +case DMA_NEXT_BYTES:
> > +val = s->chan[ch].next_bytes;
> > +break;
> > +case DMA_NEXT_DST:
> > +val = s->chan[ch].next_dst;
> > +break;
> > +case DMA_NEXT_SRC:
> > +val = s->chan[ch].next_src;
> > +break;
> > +case DMA_EXEC_BYTES:
> > +val = s->chan[ch].exec_bytes;
> > +break;
> > +case DMA_EXEC_DST:
> > +val = s->chan[ch].exec_dst;
> > +break;
> > +case DMA_EXEC_SRC:
> > +val = s->chan[ch].exec_src;
[PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
It's obvious that PDMA support 64-bit access of 64-bit registers, and
in previous commit, we confirm that PDMA support 32-bit access of both
32/64-bit registers. Thus, we configure 32/64-bit memory access of
PDMA registers as valid in general.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
hw/dma/sifive_pdma.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index b8b198ab4e..731fcdcf89 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
.impl = {
.min_access_size = 4,
.max_access_size = 8,
+},
+.valid = {
+.min_access_size = 4,
+.max_access_size = 8,
}
};
--
2.25.1
[PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
Real PDMA support high 32-bit read/write memory access of 64-bit
register.
The following result is PDMA tested in U-Boot on Unmatched board:
1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
=> mw.l 0x300 0x0 <= Disclaim channel 0
=> mw.l 0x300 0x1 <= Claim channel 0
=> mw.l 0x310 0x8000 <= Write low 32-bit NextDest
(NextDest = 0x28000)
=> mw.l 0x314 0x2 <= Write high 32-bit NextDest
=> md.l 0x310 1<= Dump low 32-bit NextDest
0310: 8000
=> md.l 0x314 1<= Dump high 32-bit NextDest
0314: 0002
=> mw.l 0x318 0x80001000 <= Write low 32-bit NextSrc (NextSrc
= 0x280001000)
=> mw.l 0x31c 0x2 <= Write high 32-bit NextSrc
=> md.l 0x318 1<= Dump low 32-bit NextSrc
0310: 80001000
=> md.l 0x31c 1<= Dump high 32-bit NextSrc
0314: 0002
2. PDMA transfer from 0x280001000 to 0x28000 is OK.
=> mw.q 0x308 0x4 <= NextBytes = 4
=> mw.l 0x304 0x2200 <= wsize = rsize = 2 (2^2 = 4 bytes)
=> mw.l 0x28000 0x87654321 <= Fill test data to dst
=> mw.l 0x280001000 0x12345678 <= Fill test data to src
=> md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory contents
28000: 87654321 !Ce.
280001000: 12345678 xV4.
=> md.l 0x300 8<= Dump PDMA status
0300: 0001 2200 0004 ..."
0310: 8000 0002 80001000 0002
=> mw.l 0x300 0x3 <= Set channel 0 run and claim bits
=> md.l 0x300 8<= Dump PDMA status
0300: 4001 2200 0004 ...@..."
0310: 8000 0002 80001000 0002
=> md.l 0x28000 1; md.l 0x280001000 1 <= Dump src/dst memory contents
280000000: 12345678 xV4.
280001000: 12345678 xV4.
Signed-off-by: Jim Shu
Reviewed-by: Frank Chang
---
hw/dma/sifive_pdma.c | 174 +--
1 file changed, 152 insertions(+), 22 deletions(-)
diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index 85fe34f5f3..b8b198ab4e 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState
*s, int ch)
s->chan[ch].state = DMA_CHAN_STATE_IDLE;
}
-static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
+static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
{
-SiFivePDMAState *s = opaque;
-int ch = SIFIVE_PDMA_CHAN_NO(offset);
uint64_t val = 0;
-if (ch >= SIFIVE_PDMA_CHANS) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
- __func__, ch);
-return 0;
+offset &= 0xfff;
+switch (offset) {
+case DMA_NEXT_BYTES:
+val = s->chan[ch].next_bytes;
+break;
+case DMA_NEXT_DST:
+val = s->chan[ch].next_dst;
+break;
+case DMA_NEXT_SRC:
+val = s->chan[ch].next_src;
+break;
+case DMA_EXEC_BYTES:
+val = s->chan[ch].exec_bytes;
+break;
+case DMA_EXEC_DST:
+val = s->chan[ch].exec_dst;
+break;
+case DMA_EXEC_SRC:
+val = s->chan[ch].exec_src;
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
+ __func__, offset);
+break;
}
+return val;
+}
+
+static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
+{
+uint32_t val = 0;
+
offset &= 0xfff;
switch (offset) {
case DMA_CONTROL:
@@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr
offset, unsigned size)
val = s->chan[ch].next_config;
break;
case DMA_NEXT_BYTES:
-val = s->chan[ch].next_bytes;
+val = extract64(s->chan[ch].next_bytes, 0, 32);
+break;
+case DMA_NEXT_BYTES + 4:
+val = extract64(s->chan[ch].next_bytes, 32, 32);
break;
case DMA_NEXT_DST:
-val = s->chan[ch].next_dst;
+val = extract64(s->chan[ch].next_dst, 0, 32);
+break;
+case DMA_NEXT_DST + 4:
+val = extract64(s->chan[ch].next_dst, 32, 32);
break;
case DMA_NEXT_SRC:
-val = s->chan[ch].next_src;
+val = extract64(s->chan[ch].next_src, 0, 32);
+break;
+cas
[PATCH 0/2] Align SiFive PDMA behavior to real hardware
HiFive Unmatched PDMA supports high/low 32-bit access of 64-bit register, but QEMU emulation support low part access now. Enhance QEMU emulation to support high 32-bit access. Also, permit 4/8-byte valid access in PDMA as we have verified 32/64-bit accesses of PDMA registers are supported. Jim Shu (2): hw/dma: sifive_pdma: support high 32-bit access of 64-bit register hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers hw/dma/sifive_pdma.c | 178 +-- 1 file changed, 156 insertions(+), 22 deletions(-) -- 2.25.1
[PATCH 1/3] target/riscv: propagate PMP permission to TLB page
Currently, PMP permission checking of TLB page is bypassed if TLB hits
Fix it by propagating PMP permission to TLB page permission.
PMP permission checking also use MMU-style API to change TLB permission
and size.
Signed-off-by: Jim Shu
---
target/riscv/cpu_helper.c | 84 +--
target/riscv/pmp.c| 80 +++--
target/riscv/pmp.h| 4 +-
3 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..f6ac63bf0e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong
newpriv)
env->load_res = -1;
}
+/*
+ * get_physical_address_pmp - check PMP permission for this physical address
+ *
+ * Match the PMP region and check permission for this physical address and it's
+ * TLB page. Returns 0 if the permission checking was successful
+ *
+ * @env: CPURISCVState
+ * @prot: The returned protection attributes
+ * @tlb_size: TLB page size containing addr. It could be modified after PMP
+ *permission checking. NULL if not set TLB page for addr.
+ * @addr: The physical address to be checked permission
+ * @access_type: The type of MMU access
+ * @mode: Indicates current privilege level.
+ */
+static int get_physical_address_pmp(CPURISCVState *env, int *prot,
+target_ulong *tlb_size, hwaddr addr,
+int size, MMUAccessType access_type,
+int mode)
+{
+pmp_priv_t pmp_priv;
+target_ulong tlb_size_pmp = 0;
+
+if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
+*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+return TRANSLATE_SUCCESS;
+}
+
+if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, _priv,
+mode)) {
+*prot = 0;
+return TRANSLATE_PMP_FAIL;
+}
+
+*prot = pmp_priv_to_page_prot(pmp_priv);
+if (tlb_size != NULL) {
+if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), _size_pmp)) {
+*tlb_size = tlb_size_pmp;
+}
+}
+
+return TRANSLATE_SUCCESS;
+}
+
/* get_physical_address - get the physical address for this virtual address
*
* Do a page table walk to obtain the physical address corresponding to a
@@ -442,9 +485,11 @@ restart:
pte_addr = base + idx * ptesize;
}
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-!pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
-1 << MMU_DATA_LOAD, PRV_S)) {
+int pmp_prot;
+int pmp_ret = get_physical_address_pmp(env, _prot, NULL, pte_addr,
+ sizeof(target_ulong),
+ MMU_DATA_LOAD, PRV_S);
+if (pmp_ret != TRANSLATE_SUCCESS) {
return TRANSLATE_PMP_FAIL;
}
@@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
#ifndef CONFIG_USER_ONLY
vaddr im_address;
hwaddr pa = 0;
-int prot, prot2;
+int prot, prot2, prot_pmp;
bool pmp_violation = false;
bool first_stage_error = true;
bool two_stage_lookup = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
-target_ulong tlb_size = 0;
+/* default TLB page size */
+target_ulong tlb_size = TARGET_PAGE_SIZE;
env->guest_phys_fault_addr = 0;
@@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
prot &= prot2;
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
-ret = TRANSLATE_PMP_FAIL;
+if (ret == TRANSLATE_SUCCESS) {
+ret = get_physical_address_pmp(env, _pmp, _size, pa,
+ size, access_type, mode);
+prot &= prot_pmp;
}
if (ret != TRANSLATE_SUCCESS) {
@@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
"%s address=%" VADDR_PRIx " ret %d physical "
TARGET_FMT_plx " prot %d\n",
__func__, address, ret, pa, prot);
-}
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
-ret = TRANSLATE_PMP_FAIL;
+if (ret == TRANSLATE_SUCCESS) {
+ret = get_physical_address_pmp(env, _pmp, _size, pa,
+ size, access_type, mode);
+prot &= prot_pmp;
+}
[PATCH 1/3] target/riscv: propagate PMP permission to TLB page
Currently, PMP permission checking of TLB page is bypassed if TLB hits
Fix it by propagating PMP permission to TLB page permission.
PMP permission checking also use MMU-style API to change TLB permission
and size.
Signed-off-by: Jim Shu
---
target/riscv/cpu_helper.c | 84 +--
target/riscv/pmp.c| 80 +++--
target/riscv/pmp.h| 4 +-
3 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..f6ac63bf0e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong
newpriv)
env->load_res = -1;
}
+/*
+ * get_physical_address_pmp - check PMP permission for this physical address
+ *
+ * Match the PMP region and check permission for this physical address and it's
+ * TLB page. Returns 0 if the permission checking was successful
+ *
+ * @env: CPURISCVState
+ * @prot: The returned protection attributes
+ * @tlb_size: TLB page size containing addr. It could be modified after PMP
+ *permission checking. NULL if not set TLB page for addr.
+ * @addr: The physical address to be checked permission
+ * @access_type: The type of MMU access
+ * @mode: Indicates current privilege level.
+ */
+static int get_physical_address_pmp(CPURISCVState *env, int *prot,
+target_ulong *tlb_size, hwaddr addr,
+int size, MMUAccessType access_type,
+int mode)
+{
+pmp_priv_t pmp_priv;
+target_ulong tlb_size_pmp = 0;
+
+if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
+*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+return TRANSLATE_SUCCESS;
+}
+
+if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, _priv,
+mode)) {
+*prot = 0;
+return TRANSLATE_PMP_FAIL;
+}
+
+*prot = pmp_priv_to_page_prot(pmp_priv);
+if (tlb_size != NULL) {
+if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), _size_pmp)) {
+*tlb_size = tlb_size_pmp;
+}
+}
+
+return TRANSLATE_SUCCESS;
+}
+
/* get_physical_address - get the physical address for this virtual address
*
* Do a page table walk to obtain the physical address corresponding to a
@@ -442,9 +485,11 @@ restart:
pte_addr = base + idx * ptesize;
}
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-!pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
-1 << MMU_DATA_LOAD, PRV_S)) {
+int pmp_prot;
+int pmp_ret = get_physical_address_pmp(env, _prot, NULL, pte_addr,
+ sizeof(target_ulong),
+ MMU_DATA_LOAD, PRV_S);
+if (pmp_ret != TRANSLATE_SUCCESS) {
return TRANSLATE_PMP_FAIL;
}
@@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
#ifndef CONFIG_USER_ONLY
vaddr im_address;
hwaddr pa = 0;
-int prot, prot2;
+int prot, prot2, prot_pmp;
bool pmp_violation = false;
bool first_stage_error = true;
bool two_stage_lookup = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
-target_ulong tlb_size = 0;
+/* default TLB page size */
+target_ulong tlb_size = TARGET_PAGE_SIZE;
env->guest_phys_fault_addr = 0;
@@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
prot &= prot2;
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
-ret = TRANSLATE_PMP_FAIL;
+if (ret == TRANSLATE_SUCCESS) {
+ret = get_physical_address_pmp(env, _pmp, _size, pa,
+ size, access_type, mode);
+prot &= prot_pmp;
}
if (ret != TRANSLATE_SUCCESS) {
@@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
"%s address=%" VADDR_PRIx " ret %d physical "
TARGET_FMT_plx " prot %d\n",
__func__, address, ret, pa, prot);
-}
-if (riscv_feature(env, RISCV_FEATURE_PMP) &&
-(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
-ret = TRANSLATE_PMP_FAIL;
+if (ret == TRANSLATE_SUCCESS) {
+ret = get_physical_address_pmp(env, _pmp, _size, pa,
+ size, access_type, mode);
+prot &= prot_pmp;
+}
[PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits
Sorry for sending this patch set again. The cover letter of my previous mail doesn't add cc list. --- Current implementation of PMP permission checking only has effect when softmmu's TLB miss. PMP checking is bypassed when TLB hits because TLB page permission isn't affected by PMP permission. To fix this issue, this patch set addes the feature to propagate PMP permission to the TLB page and flush TLB pages if PMP permission has been changed. The patch set is tested on Zephyr RTOS userspace testsuite on QEMU riscv32 virt machine. Jim Shu (3): target/riscv: propagate PMP permission to TLB page target/riscv: add log of PMP permission checking target/riscv: flush TLB pages if PMP permission has been changed target/riscv/cpu_helper.c | 96 ++- target/riscv/pmp.c| 84 +- target/riscv/pmp.h| 4 +- 3 files changed, 141 insertions(+), 43 deletions(-) -- 2.30.1
[PATCH 2/3] target/riscv: add log of PMP permission checking
Like MMU translation, add qemu log of PMP permission checking for
debugging.
Signed-off-by: Jim Shu
---
target/riscv/cpu_helper.c | 12
1 file changed, 12 insertions(+)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f6ac63bf0e..c1ecb8a710 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -794,6 +794,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, _pmp, _size, pa,
size, access_type, mode);
+
+qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
@@ -821,6 +827,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, _pmp, _size, pa,
size, access_type, mode);
+
+qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
}
--
2.30.1
[PATCH 2/3] target/riscv: add log of PMP permission checking
Like MMU translation, add qemu log of PMP permission checking for
debugging.
Signed-off-by: Jim Shu
---
target/riscv/cpu_helper.c | 12
1 file changed, 12 insertions(+)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f6ac63bf0e..c1ecb8a710 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -794,6 +794,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, _pmp, _size, pa,
size, access_type, mode);
+
+qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
@@ -821,6 +827,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, _pmp, _size, pa,
size, access_type, mode);
+
+qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
}
--
2.30.1
[PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
If PMP permission of any address has been changed by updating PMP entry, flush all TLB pages to prevent from getting old permission. Signed-off-by: Jim Shu --- target/riscv/pmp.c | 4 1 file changed, 4 insertions(+) diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index ebd874cde3..cff020122a 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -28,6 +28,7 @@ #include "qapi/error.h" #include "cpu.h" #include "trace.h" +#include "exec/exec-all.h" static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index, uint8_t val); @@ -347,6 +348,9 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, cfg_val = (val >> 8 * i) & 0xff; pmp_write_cfg(env, (reg_index * 4) + i, cfg_val); } + +/* If PMP permission of any addr has been changed, flush TLB pages. */ +tlb_flush(env_cpu(env)); } -- 2.30.1
[PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
If PMP permission of any address has been changed by updating PMP entry, flush all TLB pages to prevent from getting old permission. Signed-off-by: Jim Shu --- target/riscv/pmp.c | 4 1 file changed, 4 insertions(+) diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index ebd874cde3..cff020122a 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -28,6 +28,7 @@ #include "qapi/error.h" #include "cpu.h" #include "trace.h" +#include "exec/exec-all.h" static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index, uint8_t val); @@ -347,6 +348,9 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index, cfg_val = (val >> 8 * i) & 0xff; pmp_write_cfg(env, (reg_index * 4) + i, cfg_val); } + +/* If PMP permission of any addr has been changed, flush TLB pages. */ +tlb_flush(env_cpu(env)); } -- 2.30.1
