* [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2
@ 2019-08-23 15:21 Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 1/7] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
` (7 more replies)
0 siblings, 8 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
The first three patches are ones that I have pulled out of my original
Hypervisor series at an attempt to reduce the number of patches in the
series.
These three patches all make sense without the Hypervisor series so can
be merged seperatley and will reduce the review burden of the next
version of the patches.
The fource patch is a prep patch for the new v0.4 Hypervisor spec.
The fifth patch is unreleated to Hypervisor that I'm just slipping in
here because it seems easier then sending it by itself.
The final two patches are issues I discovered while adding the v0.4
Hypervisor extension.
v4:
- Drop MIP change patch
- Add a Floating Point fixup patch
v3:
- Change names of all GP registers
- Add two more patches
v2:
- Small corrections based on feedback
- Remove the CSR permission check patch
Alistair Francis (6):
target/riscv: Don't set write permissions on dirty PTEs
riscv: plic: Remove unused interrupt functions
target/riscv: Create function to test if FP is enabled
target/riscv: Update the Hypervisor CSRs to v0.4
target/riscv: Fix mstatus dirty mask
target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
Atish Patra (1):
target/riscv: Use both register name and ABI name
hw/riscv/sifive_plic.c | 12 ------------
include/hw/riscv/sifive_plic.h | 3 ---
target/riscv/cpu.c | 19 ++++++++++--------
target/riscv/cpu.h | 6 +++++-
target/riscv/cpu_bits.h | 35 +++++++++++++++++-----------------
target/riscv/cpu_helper.c | 16 ++++++++++++----
target/riscv/csr.c | 22 +++++++++++----------
7 files changed, 58 insertions(+), 55 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 1/7] target/riscv: Don't set write permissions on dirty PTEs
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 2/7] riscv: plic: Remove unused interrupt functions Alistair Francis
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be because it
ends up with write permission in the second stage translation in cases
where we aren't doing a store.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu_helper.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b6126af..f027be7f16 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -340,10 +340,8 @@ restart:
if ((pte & PTE_X)) {
*prot |= PAGE_EXEC;
}
- /* add write permission on stores or if the page is already dirty,
- so that we TLB miss on later writes to update the dirty bit */
- if ((pte & PTE_W) &&
- (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+ /* add write permission on stores */
+ if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
*prot |= PAGE_WRITE;
}
return TRANSLATE_SUCCESS;
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 2/7] riscv: plic: Remove unused interrupt functions
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 1/7] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled Alistair Francis
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Jonathan Behrens <fintelia@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/sifive_plic.c | 12 ------------
include/hw/riscv/sifive_plic.h | 3 ---
2 files changed, 15 deletions(-)
diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
index 64a1a10380..98e4304b66 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -162,18 +162,6 @@ static void sifive_plic_update(SiFivePLICState *plic)
}
}
-void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq)
-{
- sifive_plic_set_pending(plic, irq, true);
- sifive_plic_update(plic);
-}
-
-void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq)
-{
- sifive_plic_set_pending(plic, irq, false);
- sifive_plic_update(plic);
-}
-
static uint32_t sifive_plic_claim(SiFivePLICState *plic, uint32_t addrid)
{
int i, j;
diff --git a/include/hw/riscv/sifive_plic.h b/include/hw/riscv/sifive_plic.h
index b0edba2884..4421e81249 100644
--- a/include/hw/riscv/sifive_plic.h
+++ b/include/hw/riscv/sifive_plic.h
@@ -69,9 +69,6 @@ typedef struct SiFivePLICState {
uint32_t aperture_size;
} SiFivePLICState;
-void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq);
-void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq);
-
DeviceState *sifive_plic_create(hwaddr addr, char *hart_config,
uint32_t num_sources, uint32_t num_priorities,
uint32_t priority_base, uint32_t pending_base,
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 1/7] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 2/7] riscv: plic: Remove unused interrupt functions Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2020-01-05 16:36 ` Aurelien Jarno
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 4/7] target/riscv: Update the Hypervisor CSRs to v0.4 Alistair Francis
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Let's create a function that tests if floating point support is
enabled. We can then protect all floating point operations based on if
they are enabled.
This patch so far doesn't change anything, it's just preparing for the
Hypervisor support for floating point operations.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu.h | 6 +++++-
target/riscv/cpu_helper.c | 10 ++++++++++
target/riscv/csr.c | 20 +++++++++++---------
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 240b31e2eb..eb7b5b0af3 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -255,6 +255,7 @@ void riscv_cpu_do_interrupt(CPUState *cpu);
int riscv_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
+bool riscv_cpu_fp_enabled(CPURISCVState *env);
int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
@@ -298,7 +299,10 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
#ifdef CONFIG_USER_ONLY
*flags = TB_FLAGS_MSTATUS_FS;
#else
- *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
+ *flags = cpu_mmu_index(env, 0);
+ if (riscv_cpu_fp_enabled(env)) {
+ *flags |= env->mstatus & MSTATUS_FS;
+ }
#endif
}
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f027be7f16..225e407cff 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -71,6 +71,16 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
#if !defined(CONFIG_USER_ONLY)
+/* Return true is floating point support is currently enabled */
+bool riscv_cpu_fp_enabled(CPURISCVState *env)
+{
+ if (env->mstatus & MSTATUS_FS) {
+ return true;
+ }
+
+ return false;
+}
+
int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts)
{
CPURISCVState *env = &cpu->env;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..2789215b5e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -46,7 +46,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
static int fs(CPURISCVState *env, int csrno)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
#endif
@@ -108,7 +108,7 @@ static int pmp(CPURISCVState *env, int csrno)
static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
#endif
@@ -119,7 +119,7 @@ static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
env->mstatus |= MSTATUS_FS;
@@ -131,7 +131,7 @@ static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
#endif
@@ -142,7 +142,7 @@ static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
env->mstatus |= MSTATUS_FS;
@@ -154,7 +154,7 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
#endif
@@ -166,7 +166,7 @@ static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+ if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
return -1;
}
env->mstatus |= MSTATUS_FS;
@@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
{
target_ulong mstatus = env->mstatus;
target_ulong mask = 0;
+ int dirty;
/* flush tlb on mstatus fields that affect VM */
if (env->priv_ver <= PRIV_VERSION_1_09_1) {
@@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
mstatus = (mstatus & ~mask) | (val & mask);
- int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
- ((mstatus & MSTATUS_XS) == MSTATUS_XS);
+ dirty = (riscv_cpu_fp_enabled(env) &&
+ ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
+ ((mstatus & MSTATUS_XS) == MSTATUS_XS);
mstatus = set_field(mstatus, MSTATUS_SD, dirty);
env->mstatus = mstatus;
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 4/7] target/riscv: Update the Hypervisor CSRs to v0.4
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
` (2 preceding siblings ...)
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 5/7] target/riscv: Use both register name and ABI name Alistair Francis
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Update the Hypervisor CSR addresses to match the v0.4 spec.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu_bits.h | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 11f971ad5d..e99834856c 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -173,6 +173,24 @@
#define CSR_SPTBR 0x180
#define CSR_SATP 0x180
+/* Hpervisor CSRs */
+#define CSR_HSTATUS 0x600
+#define CSR_HEDELEG 0x602
+#define CSR_HIDELEG 0x603
+#define CSR_HCOUNTERNEN 0x606
+#define CSR_HGATP 0x680
+
+#if defined(TARGET_RISCV32)
+#define HGATP_MODE SATP32_MODE
+#define HGATP_VMID SATP32_ASID
+#define HGATP_PPN SATP32_PPN
+#endif
+#if defined(TARGET_RISCV64)
+#define HGATP_MODE SATP64_MODE
+#define HGATP_VMID SATP64_ASID
+#define HGATP_PPN SATP64_PPN
+#endif
+
/* Physical Memory Protection */
#define CSR_PMPCFG0 0x3a0
#define CSR_PMPCFG1 0x3a1
@@ -206,23 +224,6 @@
#define CSR_DPC 0x7b1
#define CSR_DSCRATCH 0x7b2
-/* Hpervisor CSRs */
-#define CSR_HSTATUS 0xa00
-#define CSR_HEDELEG 0xa02
-#define CSR_HIDELEG 0xa03
-#define CSR_HGATP 0xa80
-
-#if defined(TARGET_RISCV32)
-#define HGATP_MODE SATP32_MODE
-#define HGATP_ASID SATP32_ASID
-#define HGATP_PPN SATP32_PPN
-#endif
-#if defined(TARGET_RISCV64)
-#define HGATP_MODE SATP64_MODE
-#define HGATP_ASID SATP64_ASID
-#define HGATP_PPN SATP64_PPN
-#endif
-
/* Performance Counters */
#define CSR_MHPMCOUNTER3 0xb03
#define CSR_MHPMCOUNTER4 0xb04
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 5/7] target/riscv: Use both register name and ABI name
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
` (3 preceding siblings ...)
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 4/7] target/riscv: Update the Hypervisor CSRs to v0.4 Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 6/7] target/riscv: Fix mstatus dirty mask Alistair Francis
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
From: Atish Patra <atish.patra@wdc.com>
Use both the generic register name and ABI name for the general purpose
registers and floating point registers.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6d52f97d7c..f13e298a36 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -34,17 +34,20 @@
static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
const char * const riscv_int_regnames[] = {
- "zero", "ra", "sp", "gp", "tp", "t0", "t1", "t2",
- "s0", "s1", "a0", "a1", "a2", "a3", "a4", "a5",
- "a6", "a7", "s2", "s3", "s4", "s5", "s6", "s7",
- "s8", "s9", "s10", "s11", "t3", "t4", "t5", "t6"
+ "x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
+ "x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",
+ "x14/a4", "x15/a5", "x16/a6", "x17/a7", "x18/s2", "x19/s3", "x20/s4",
+ "x21/s5", "x22/s6", "x23/s7", "x24/s8", "x25/s9", "x26/s10", "x27/s11",
+ "x28/t3", "x29/t4", "x30/t5", "x31/t6"
};
const char * const riscv_fpr_regnames[] = {
- "ft0", "ft1", "ft2", "ft3", "ft4", "ft5", "ft6", "ft7",
- "fs0", "fs1", "fa0", "fa1", "fa2", "fa3", "fa4", "fa5",
- "fa6", "fa7", "fs2", "fs3", "fs4", "fs5", "fs6", "fs7",
- "fs8", "fs9", "fs10", "fs11", "ft8", "ft9", "ft10", "ft11"
+ "f0/ft0", "f1/ft1", "f2/ft2", "f3/ft3", "f4/ft4", "f5/ft5",
+ "f6/ft6", "f7/ft7", "f8/fs0", "f9/fs1", "f10/fa0", "f11/fa1",
+ "f12/fa2", "f13/fa3", "f14/fa4", "f15/fa5", "f16/fa6", "f17/fa7",
+ "f18/fs2", "f19/fs3", "f20/fs4", "f21/fs5", "f22/fs6", "f23/fs7",
+ "f24/fs8", "f25/fs9", "f26/fs10", "f27/fs11", "f28/ft8", "f29/ft9",
+ "f30/ft10", "f31/ft11"
};
const char * const riscv_excp_names[] = {
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 6/7] target/riscv: Fix mstatus dirty mask
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
` (4 preceding siblings ...)
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 5/7] target/riscv: Use both register name and ABI name Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point Alistair Francis
2019-09-10 13:16 ` [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Palmer Dabbelt
7 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/csr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 2789215b5e..f767ad24be 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -335,7 +335,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
* RV32: MPV and MTL are not in mstatus. The current plan is to
* add them to mstatush. For now, we just don't support it.
*/
- mask |= MSTATUS_MPP | MSTATUS_MPV;
+ mask |= MSTATUS_MTL | MSTATUS_MPV;
#endif
}
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
` (5 preceding siblings ...)
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 6/7] target/riscv: Fix mstatus dirty mask Alistair Francis
@ 2019-08-23 15:21 ` Alistair Francis
2019-08-23 15:44 ` Peter Maydell
2019-09-10 13:16 ` Palmer Dabbelt
2019-09-10 13:16 ` [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Palmer Dabbelt
7 siblings, 2 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:21 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, bmeng.cn
Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
flags.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/cpu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index eb7b5b0af3..0347be453b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -301,7 +301,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
#else
*flags = cpu_mmu_index(env, 0);
if (riscv_cpu_fp_enabled(env)) {
- *flags |= env->mstatus & MSTATUS_FS;
+ *flags |= TB_FLAGS_MSTATUS_FS;
}
#endif
}
--
2.22.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
2019-08-23 15:44 ` Peter Maydell
@ 2019-08-23 15:43 ` Alistair Francis
0 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2019-08-23 15:43 UTC (permalink / raw)
To: Peter Maydell
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
QEMU Developers, Bin Meng
On Fri, Aug 23, 2019 at 8:44 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 23 Aug 2019 at 16:37, Alistair Francis <alistair.francis@wdc.com> wrote:
> >
> > Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
> > flags.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > target/riscv/cpu.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index eb7b5b0af3..0347be453b 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -301,7 +301,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> > #else
> > *flags = cpu_mmu_index(env, 0);
> > if (riscv_cpu_fp_enabled(env)) {
> > - *flags |= env->mstatus & MSTATUS_FS;
> > + *flags |= TB_FLAGS_MSTATUS_FS;
> > }
> > #endif
>
> The old code was setting the bit in flags only if
> it was also set in env->mstatus; the new code sets
> the bit unconditionally -- deliberate change ?
Yes it is deliberate as the riscv_cpu_fp_enabled() function already
does the check. The function contains the exact same & operation
inside of it.
Alistair
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point Alistair Francis
@ 2019-08-23 15:44 ` Peter Maydell
2019-08-23 15:43 ` Alistair Francis
2019-09-10 13:16 ` Palmer Dabbelt
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2019-08-23 15:44 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
QEMU Developers, Bin Meng
On Fri, 23 Aug 2019 at 16:37, Alistair Francis <alistair.francis@wdc.com> wrote:
>
> Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
> flags.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/cpu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index eb7b5b0af3..0347be453b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -301,7 +301,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> #else
> *flags = cpu_mmu_index(env, 0);
> if (riscv_cpu_fp_enabled(env)) {
> - *flags |= env->mstatus & MSTATUS_FS;
> + *flags |= TB_FLAGS_MSTATUS_FS;
> }
> #endif
The old code was setting the bit in flags only if
it was also set in env->mstatus; the new code sets
the bit unconditionally -- deliberate change ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point Alistair Francis
2019-08-23 15:44 ` Peter Maydell
@ 2019-09-10 13:16 ` Palmer Dabbelt
1 sibling, 0 replies; 17+ messages in thread
From: Palmer Dabbelt @ 2019-09-10 13:16 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Fri, 23 Aug 2019 08:21:25 PDT (-0700), Alistair Francis wrote:
> Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
> flags.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/cpu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index eb7b5b0af3..0347be453b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -301,7 +301,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> #else
> *flags = cpu_mmu_index(env, 0);
> if (riscv_cpu_fp_enabled(env)) {
> - *flags |= env->mstatus & MSTATUS_FS;
> + *flags |= TB_FLAGS_MSTATUS_FS;
I thought this was a functional change, but it's not: fp_enabled() checks
mstatus already.
> }
> #endif
> }
Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
` (6 preceding siblings ...)
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point Alistair Francis
@ 2019-09-10 13:16 ` Palmer Dabbelt
7 siblings, 0 replies; 17+ messages in thread
From: Palmer Dabbelt @ 2019-09-10 13:16 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Fri, 23 Aug 2019 08:21:06 PDT (-0700), Alistair Francis wrote:
>
> The first three patches are ones that I have pulled out of my original
> Hypervisor series at an attempt to reduce the number of patches in the
> series.
>
> These three patches all make sense without the Hypervisor series so can
> be merged seperatley and will reduce the review burden of the next
> version of the patches.
>
> The fource patch is a prep patch for the new v0.4 Hypervisor spec.
>
> The fifth patch is unreleated to Hypervisor that I'm just slipping in
> here because it seems easier then sending it by itself.
>
> The final two patches are issues I discovered while adding the v0.4
> Hypervisor extension.
>
> v4:
> - Drop MIP change patch
> - Add a Floating Point fixup patch
> v3:
> - Change names of all GP registers
> - Add two more patches
> v2:
> - Small corrections based on feedback
> - Remove the CSR permission check patch
>
>
>
> Alistair Francis (6):
> target/riscv: Don't set write permissions on dirty PTEs
> riscv: plic: Remove unused interrupt functions
> target/riscv: Create function to test if FP is enabled
> target/riscv: Update the Hypervisor CSRs to v0.4
> target/riscv: Fix mstatus dirty mask
> target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
>
> Atish Patra (1):
> target/riscv: Use both register name and ABI name
>
> hw/riscv/sifive_plic.c | 12 ------------
> include/hw/riscv/sifive_plic.h | 3 ---
> target/riscv/cpu.c | 19 ++++++++++--------
> target/riscv/cpu.h | 6 +++++-
> target/riscv/cpu_bits.h | 35 +++++++++++++++++-----------------
> target/riscv/cpu_helper.c | 16 ++++++++++++----
> target/riscv/csr.c | 22 +++++++++++----------
> 7 files changed, 58 insertions(+), 55 deletions(-)
Aside from that PTE patch, I've applied
target/riscv: Use both register name and ABI name
target/riscv: Fix mstatus dirty mask
target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
on top of Bin's patch set, as the rest had made it into for-master. Like we
talked about at lunch, I'm not sure the PTE one is actually correct -- it might
just be paranoia, though.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled Alistair Francis
@ 2020-01-05 16:36 ` Aurelien Jarno
2020-01-05 16:59 ` Aurelien Jarno
0 siblings, 1 reply; 17+ messages in thread
From: Aurelien Jarno @ 2020-01-05 16:36 UTC (permalink / raw)
To: Alistair Francis; +Cc: alistair23, palmer, qemu-riscv, qemu-devel, bmeng.cn
Hi,
On 2019-08-23 08:21, Alistair Francis wrote:
> Let's create a function that tests if floating point support is
> enabled. We can then protect all floating point operations based on if
> they are enabled.
>
> This patch so far doesn't change anything, it's just preparing for the
> Hypervisor support for floating point operations.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
> Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> target/riscv/cpu.h | 6 +++++-
> target/riscv/cpu_helper.c | 10 ++++++++++
> target/riscv/csr.c | 20 +++++++++++---------
> 3 files changed, 26 insertions(+), 10 deletions(-)
>
[ snip ]
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..2789215b5e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
[ snip ]
> @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> {
> target_ulong mstatus = env->mstatus;
> target_ulong mask = 0;
> + int dirty;
>
> /* flush tlb on mstatus fields that affect VM */
> if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>
> mstatus = (mstatus & ~mask) | (val & mask);
>
> - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> - ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> + dirty = (riscv_cpu_fp_enabled(env) &&
> + ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> + ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> env->mstatus = mstatus;
This patch, and more precisely the above two hunks broke
qemu-system-riscv64. More precisely, when running a Debian sid system
inside QEMU, sshd hangs during key exchange.
Reverting this commit "fixes" the issue up to the following commit which
breaks things again:
| commit bdce1a5c6d512257f83b6b6831bee2c975643bbd
| Author: Alistair Francis <alistair.francis@wdc.com>
| Date: Fri Aug 23 08:21:25 2019 -0700
|
| target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
|
| Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
| flags.
|
| Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
| Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
| Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
I wonder if the issue is related to the fact that MSTATUS_FS and thus
TB_FLAGS_MSTATUS_FS actually consist in 2 bits and are not a simple
flag.
Overall I am able to get QEMU v4.2 working again by applying the
following patch:
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index e59343e13c..f0ff57e27a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -295,7 +295,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
#else
*flags = cpu_mmu_index(env, 0);
if (riscv_cpu_fp_enabled(env)) {
- *flags |= TB_FLAGS_MSTATUS_FS;
+ *flags |= env->mstatus & MSTATUS_FS;
}
#endif
}
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index da02f9f0b1..1754c6c575 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -307,7 +307,6 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
{
target_ulong mstatus = env->mstatus;
target_ulong mask = 0;
- int dirty;
/* flush tlb on mstatus fields that affect VM */
if (env->priv_ver <= PRIV_VERSION_1_09_1) {
@@ -341,9 +340,8 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
mstatus = (mstatus & ~mask) | (val & mask);
- dirty = (riscv_cpu_fp_enabled(env) &&
- ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
- ((mstatus & MSTATUS_XS) == MSTATUS_XS);
+ int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
+ ((mstatus & MSTATUS_XS) == MSTATUS_XS);
mstatus = set_field(mstatus, MSTATUS_SD, dirty);
env->mstatus = mstatus;
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2020-01-05 16:36 ` Aurelien Jarno
@ 2020-01-05 16:59 ` Aurelien Jarno
2020-01-20 0:31 ` Alistair Francis
0 siblings, 1 reply; 17+ messages in thread
From: Aurelien Jarno @ 2020-01-05 16:59 UTC (permalink / raw)
To: Alistair Francis; +Cc: alistair23, palmer, qemu-riscv, qemu-devel, bmeng.cn
On 2020-01-05 17:36, Aurelien Jarno wrote:
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index e0d4586760..2789215b5e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
>
> [ snip ]
>
> > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > {
> > target_ulong mstatus = env->mstatus;
> > target_ulong mask = 0;
> > + int dirty;
> >
> > /* flush tlb on mstatus fields that affect VM */
> > if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> >
> > mstatus = (mstatus & ~mask) | (val & mask);
> >
> > - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > - ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > + dirty = (riscv_cpu_fp_enabled(env) &&
> > + ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > + ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > env->mstatus = mstatus;
>
> This patch, and more precisely the above two hunks broke
> qemu-system-riscv64. More precisely, when running a Debian sid system
> inside QEMU, sshd hangs during key exchange.
The problem is that at this stage, mstatus != env->status. Prior to that
patch, dirty was computed exclusively on the new mstatus status, after
the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
to "Dirty" (FS == 11), the SD bit is not set.
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2020-01-05 16:59 ` Aurelien Jarno
@ 2020-01-20 0:31 ` Alistair Francis
2020-01-21 20:37 ` Aurelien Jarno
0 siblings, 1 reply; 17+ messages in thread
From: Alistair Francis @ 2020-01-20 0:31 UTC (permalink / raw)
To: Aurelien Jarno
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, Bin Meng
On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index e0d4586760..2789215b5e 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> >
> > [ snip ]
> >
> > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > {
> > > target_ulong mstatus = env->mstatus;
> > > target_ulong mask = 0;
> > > + int dirty;
> > >
> > > /* flush tlb on mstatus fields that affect VM */
> > > if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > >
> > > mstatus = (mstatus & ~mask) | (val & mask);
> > >
> > > - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > - ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > + dirty = (riscv_cpu_fp_enabled(env) &&
> > > + ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > + ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > env->mstatus = mstatus;
> >
> > This patch, and more precisely the above two hunks broke
> > qemu-system-riscv64. More precisely, when running a Debian sid system
> > inside QEMU, sshd hangs during key exchange.
>
> The problem is that at this stage, mstatus != env->status. Prior to that
> patch, dirty was computed exclusively on the new mstatus status, after
> the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> to "Dirty" (FS == 11), the SD bit is not set.
Thanks for reporting this!
Can you try this branch (it should be a PR to mainline QEMU soon) and
let me know if that fixes the issue?
https://github.com/palmer-dabbelt/qemu/commits/for-master
Alistair
>
> --
> Aurelien Jarno GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2020-01-20 0:31 ` Alistair Francis
@ 2020-01-21 20:37 ` Aurelien Jarno
2020-01-21 22:20 ` Alistair Francis
0 siblings, 1 reply; 17+ messages in thread
From: Aurelien Jarno @ 2020-01-21 20:37 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, Bin Meng
Hi,
On 2020-01-20 10:31, Alistair Francis wrote:
> On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
> >
> > On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index e0d4586760..2789215b5e 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > >
> > > [ snip ]
> > >
> > > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > > {
> > > > target_ulong mstatus = env->mstatus;
> > > > target_ulong mask = 0;
> > > > + int dirty;
> > > >
> > > > /* flush tlb on mstatus fields that affect VM */
> > > > if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > >
> > > > mstatus = (mstatus & ~mask) | (val & mask);
> > > >
> > > > - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > > - ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > + dirty = (riscv_cpu_fp_enabled(env) &&
> > > > + ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > > + ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > > env->mstatus = mstatus;
> > >
> > > This patch, and more precisely the above two hunks broke
> > > qemu-system-riscv64. More precisely, when running a Debian sid system
> > > inside QEMU, sshd hangs during key exchange.
> >
> > The problem is that at this stage, mstatus != env->status. Prior to that
> > patch, dirty was computed exclusively on the new mstatus status, after
> > the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> > old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> > to "Dirty" (FS == 11), the SD bit is not set.
>
> Thanks for reporting this!
>
> Can you try this branch (it should be a PR to mainline QEMU soon) and
> let me know if that fixes the issue?
>
> https://github.com/palmer-dabbelt/qemu/commits/for-master
Thanks for the patchset. I confirm this fixes the issue.
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled
2020-01-21 20:37 ` Aurelien Jarno
@ 2020-01-21 22:20 ` Alistair Francis
0 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2020-01-21 22:20 UTC (permalink / raw)
To: Aurelien Jarno
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, Bin Meng
On Wed, Jan 22, 2020 at 6:37 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> Hi,
>
> On 2020-01-20 10:31, Alistair Francis wrote:
> > On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
> > >
> > > On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > index e0d4586760..2789215b5e 100644
> > > > > --- a/target/riscv/csr.c
> > > > > +++ b/target/riscv/csr.c
> > > >
> > > > [ snip ]
> > > >
> > > > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > > > {
> > > > > target_ulong mstatus = env->mstatus;
> > > > > target_ulong mask = 0;
> > > > > + int dirty;
> > > > >
> > > > > /* flush tlb on mstatus fields that affect VM */
> > > > > if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > > >
> > > > > mstatus = (mstatus & ~mask) | (val & mask);
> > > > >
> > > > > - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > > > - ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > > + dirty = (riscv_cpu_fp_enabled(env) &&
> > > > > + ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > > > + ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > > mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > > > env->mstatus = mstatus;
> > > >
> > > > This patch, and more precisely the above two hunks broke
> > > > qemu-system-riscv64. More precisely, when running a Debian sid system
> > > > inside QEMU, sshd hangs during key exchange.
> > >
> > > The problem is that at this stage, mstatus != env->status. Prior to that
> > > patch, dirty was computed exclusively on the new mstatus status, after
> > > the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> > > old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> > > to "Dirty" (FS == 11), the SD bit is not set.
> >
> > Thanks for reporting this!
> >
> > Can you try this branch (it should be a PR to mainline QEMU soon) and
> > let me know if that fixes the issue?
> >
> > https://github.com/palmer-dabbelt/qemu/commits/for-master
>
> Thanks for the patchset. I confirm this fixes the issue.
Great! Sorry we were so slow in replying to you, I was traveling.
Hopefully this is pushed to master soon.
Alistair
>
> Aurelien
>
> --
> Aurelien Jarno GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-01-21 22:22 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 15:21 [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 1/7] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 2/7] riscv: plic: Remove unused interrupt functions Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled Alistair Francis
2020-01-05 16:36 ` Aurelien Jarno
2020-01-05 16:59 ` Aurelien Jarno
2020-01-20 0:31 ` Alistair Francis
2020-01-21 20:37 ` Aurelien Jarno
2020-01-21 22:20 ` Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 4/7] target/riscv: Update the Hypervisor CSRs to v0.4 Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 5/7] target/riscv: Use both register name and ABI name Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 6/7] target/riscv: Fix mstatus dirty mask Alistair Francis
2019-08-23 15:21 ` [Qemu-devel] [PATCH v4 7/7] target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point Alistair Francis
2019-08-23 15:44 ` Peter Maydell
2019-08-23 15:43 ` Alistair Francis
2019-09-10 13:16 ` Palmer Dabbelt
2019-09-10 13:16 ` [Qemu-devel] [PATCH v4 0/7] RISC-V: Hypervisor prep work part 2 Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).