All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2
@ 2019-07-25 18:51 ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:51 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

The first four 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 four 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 fifth patch is a prep patch for the new v0.4 Hypervisor spec.

The final patch is unreleated to Hypervisor that I'm just slipping in
here because it seems easier then sending it by itself.

Alistair Francis (5):
  target/riscv: Don't set write permissions on dirty PTEs
  target/riscv: Remove strict perm checking for CSR R/W
  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

Atish Patra (1):
  target/riscv: Fix Floating Point register names

 hw/riscv/sifive_plic.c         | 12 ------------
 include/hw/riscv/sifive_plic.h |  3 ---
 target/riscv/cpu.c             |  8 ++++----
 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, 50 insertions(+), 52 deletions(-)

-- 
2.22.0



^ permalink raw reply	[flat|nested] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2
@ 2019-07-25 18:51 ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:51 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

The first four 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 four 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 fifth patch is a prep patch for the new v0.4 Hypervisor spec.

The final patch is unreleated to Hypervisor that I'm just slipping in
here because it seems easier then sending it by itself.

Alistair Francis (5):
  target/riscv: Don't set write permissions on dirty PTEs
  target/riscv: Remove strict perm checking for CSR R/W
  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

Atish Patra (1):
  target/riscv: Fix Floating Point register names

 hw/riscv/sifive_plic.c         | 12 ------------
 include/hw/riscv/sifive_plic.h |  3 ---
 target/riscv/cpu.c             |  8 ++++----
 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, 50 insertions(+), 52 deletions(-)

-- 
2.22.0



^ permalink raw reply	[flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be becuase 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>
---
 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] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be becuase 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>
---
 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] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

The privledge check based on the CSR address mask 0x300 doesn't work
when using Hypervisor extensions so remove the check

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

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..af3b762c8b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
 
     /* check privileges and return -1 if check fails */
 #if !defined(CONFIG_USER_ONLY)
-    int csr_priv = get_field(csrno, 0x300);
     int read_only = get_field(csrno, 0xC00) == 3;
-    if ((write_mask && read_only) || (env->priv < csr_priv)) {
+    if (write_mask && read_only) {
         return -1;
     }
 #endif
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

The privledge check based on the CSR address mask 0x300 doesn't work
when using Hypervisor extensions so remove the check

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

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..af3b762c8b 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
 
     /* check privileges and return -1 if check fails */
 #if !defined(CONFIG_USER_ONLY)
-    int csr_priv = get_field(csrno, 0x300);
     int read_only = get_field(csrno, 0xC00) == 3;
-    if ((write_mask && read_only) || (env->priv < csr_priv)) {
+    if (write_mask && read_only) {
         return -1;
     }
 #endif
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -161,18 +161,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 ce8907f6aa..3b8a623919 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] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -161,18 +161,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 ce8907f6aa..3b8a623919 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] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

Let's creaate 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>
---
 target/riscv/cpu.h        |  6 +++++-
 target/riscv/cpu_helper.c | 10 ++++++++++
 target/riscv/csr.c        | 19 ++++++++++---------
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
     env->mstatus = mstatus;
 
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

Let's creaate 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>
---
 target/riscv/cpu.h        |  6 +++++-
 target/riscv/cpu_helper.c | 10 ++++++++++
 target/riscv/csr.c        | 19 ++++++++++---------
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
     env->mstatus = mstatus;
 
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

Update the Hypervisor CSR addresses to match the v0.4 spec.

Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
+
 /* 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] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

Update the Hypervisor CSR addresses to match the v0.4 spec.

Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
+
 /* 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] 52+ messages in thread
* [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
  2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 18:52   ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis

From: Atish Patra <atish.patra@wdc.com>

As per the RISC-V spec, Floating Point registers are named as f0..f31
so lets fix the register names accordingly.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f8d07bd20a..af1e9b7690 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
 };
 
 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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
+  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
+  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
+  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
 };
 
 const char * const riscv_excp_names[] = {
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [Qemu-riscv] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
@ 2019-07-25 18:52   ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-25 18:52 UTC (permalink / raw)
  To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23

From: Atish Patra <atish.patra@wdc.com>

As per the RISC-V spec, Floating Point registers are named as f0..f31
so lets fix the register names accordingly.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f8d07bd20a..af1e9b7690 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
 };
 
 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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
+  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
+  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
+  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
 };
 
 const char * const riscv_excp_names[] = {
-- 
2.22.0



^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-25 21:47     ` Jonathan Behrens
  -1 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-25 21:47 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

Unless I'm missing something, this is the only place that QEMU checks the
privilege level for read and writes to CSRs. The exact computation used
here won't work with the hypervisor extension, but we also can't just get
rid of privilege checking entirely...

Jonathan

On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com>
wrote:

> The privledge check based on the CSR address mask 0x300 doesn't work
> when using Hypervisor extensions so remove the check
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/csr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..af3b762c8b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
>
>      /* check privileges and return -1 if check fails */
>  #if !defined(CONFIG_USER_ONLY)
> -    int csr_priv = get_field(csrno, 0x300);
>      int read_only = get_field(csrno, 0xC00) == 3;
> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> +    if (write_mask && read_only) {
>          return -1;
>      }
>  #endif
> --
> 2.22.0
>
>
>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
@ 2019-07-25 21:47     ` Jonathan Behrens
  0 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-25 21:47 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

Unless I'm missing something, this is the only place that QEMU checks the
privilege level for read and writes to CSRs. The exact computation used
here won't work with the hypervisor extension, but we also can't just get
rid of privilege checking entirely...

Jonathan

On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com>
wrote:

> The privledge check based on the CSR address mask 0x300 doesn't work
> when using Hypervisor extensions so remove the check
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/csr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..af3b762c8b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
>
>      /* check privileges and return -1 if check fails */
>  #if !defined(CONFIG_USER_ONLY)
> -    int csr_priv = get_field(csrno, 0x300);
>      int read_only = get_field(csrno, 0xC00) == 3;
> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> +    if (write_mask && read_only) {
>          return -1;
>      }
>  #endif
> --
> 2.22.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 1769 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-26 15:22     ` Jonathan Behrens
  -1 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-26 15:22 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

Reviewed-by: Jonathan Behrens <fintelia@gmail.com>

On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
@ 2019-07-26 15:22     ` Jonathan Behrens
  0 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-26 15:22 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Reviewed-by: Jonathan Behrens <fintelia@gmail.com>

On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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
>
>
>

[-- Attachment #2: Type: text/html, Size: 2268 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-26 17:40     ` Chih-Min Chao
  -1 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-26 17:40 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Update the Hypervisor CSR addresses to match the v0.4 spec.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
> +
>

Basd on spec, is HGATP_VMID  preferable ?

chihmin

>  /* 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	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
@ 2019-07-26 17:40     ` Chih-Min Chao
  0 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-26 17:40 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Update the Hypervisor CSR addresses to match the v0.4 spec.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
> +
>

Basd on spec, is HGATP_VMID  preferable ?

chihmin

>  /* 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
>
>
>

[-- Attachment #2: Type: text/html, Size: 3003 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
  2019-07-26 17:40     ` [Qemu-riscv] " Chih-Min Chao
@ 2019-07-26 20:20       ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 20:20 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 10:41 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> Update the Hypervisor CSR addresses to match the v0.4 spec.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
>> +
>
>
> Basd on spec, is HGATP_VMID  preferable ?

Yep, updated.

Alistair

>
> chihmin
>>
>>  /* 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	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
@ 2019-07-26 20:20       ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 20:20 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Fri, Jul 26, 2019 at 10:41 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> Update the Hypervisor CSR addresses to match the v0.4 spec.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.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..97b96c4e19 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_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
>> +
>
>
> Basd on spec, is HGATP_VMID  preferable ?

Yep, updated.

Alistair

>
> chihmin
>>
>>  /* 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	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
  2019-07-25 21:47     ` Jonathan Behrens
@ 2019-07-26 20:24       ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 20:24 UTC (permalink / raw)
  To: Jonathan Behrens
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> Unless I'm missing something, this is the only place that QEMU checks the privilege level for read and writes to CSRs. The exact computation used here won't work with the hypervisor extension, but we also can't just get rid of privilege checking entirely...

The csr_ops struct contains a checker function, so there are still
some checks occurring. I haven't done negative testing on this patch,
but the current check doesn't seem to make any sense so it should be
removed. We can separately discuss adding more checks but this current
way base don CSR address just seems strange.

Alistair

>
> Jonathan
>
> On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> The privledge check based on the CSR address mask 0x300 doesn't work
>> when using Hypervisor extensions so remove the check
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>  target/riscv/csr.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index e0d4586760..af3b762c8b 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>
>>      /* check privileges and return -1 if check fails */
>>  #if !defined(CONFIG_USER_ONLY)
>> -    int csr_priv = get_field(csrno, 0x300);
>>      int read_only = get_field(csrno, 0xC00) == 3;
>> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
>> +    if (write_mask && read_only) {
>>          return -1;
>>      }
>>  #endif
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
@ 2019-07-26 20:24       ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 20:24 UTC (permalink / raw)
  To: Jonathan Behrens
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> Unless I'm missing something, this is the only place that QEMU checks the privilege level for read and writes to CSRs. The exact computation used here won't work with the hypervisor extension, but we also can't just get rid of privilege checking entirely...

The csr_ops struct contains a checker function, so there are still
some checks occurring. I haven't done negative testing on this patch,
but the current check doesn't seem to make any sense so it should be
removed. We can separately discuss adding more checks but this current
way base don CSR address just seems strange.

Alistair

>
> Jonathan
>
> On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> The privledge check based on the CSR address mask 0x300 doesn't work
>> when using Hypervisor extensions so remove the check
>>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>  target/riscv/csr.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index e0d4586760..af3b762c8b 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>>
>>      /* check privileges and return -1 if check fails */
>>  #if !defined(CONFIG_USER_ONLY)
>> -    int csr_priv = get_field(csrno, 0x300);
>>      int read_only = get_field(csrno, 0xC00) == 3;
>> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
>> +    if (write_mask && read_only) {
>>          return -1;
>>      }
>>  #endif
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
  2019-07-26 20:24       ` Alistair Francis
@ 2019-07-26 21:00         ` Jonathan Behrens
  -1 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-26 21:00 UTC (permalink / raw)
  To: Alistair Francis
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

The remaining checks are not sufficient. If you look at the bottom of
csr.c, you'll see that for most of the M-mode CSRs the predicate is set to
"any" which unconditionally allows access regardless of privilege mode. The
S-mode CSR predicates similarly only check that supervisor mode exists, but
not that the processor is current running in that mode. I do agree with you
that using the register address to determine the required privilege mode is
a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR
address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.

I also tested by booting RVirt <https://github.com/mit-pdos/RVirt> with a
Linux guest and found that this patch caused it to instantly crash because
guest CSR accesses weren't intercepted.

Jonathan

On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com>
> wrote:
> >
> > Unless I'm missing something, this is the only place that QEMU checks
> the privilege level for read and writes to CSRs. The exact computation used
> here won't work with the hypervisor extension, but we also can't just get
> rid of privilege checking entirely...
>
> The csr_ops struct contains a checker function, so there are still
> some checks occurring. I haven't done negative testing on this patch,
> but the current check doesn't seem to make any sense so it should be
> removed. We can separately discuss adding more checks but this current
> way base don CSR address just seems strange.
>
> Alistair
>
> >
> > Jonathan
> >
> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <
> alistair.francis@wdc.com> wrote:
> >>
> >> The privledge check based on the CSR address mask 0x300 doesn't work
> >> when using Hypervisor extensions so remove the check
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >>  target/riscv/csr.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index e0d4586760..af3b762c8b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> >>
> >>      /* check privileges and return -1 if check fails */
> >>  #if !defined(CONFIG_USER_ONLY)
> >> -    int csr_priv = get_field(csrno, 0x300);
> >>      int read_only = get_field(csrno, 0xC00) == 3;
> >> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> >> +    if (write_mask && read_only) {
> >>          return -1;
> >>      }
> >>  #endif
> >> --
> >> 2.22.0
> >>
> >>
>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
@ 2019-07-26 21:00         ` Jonathan Behrens
  0 siblings, 0 replies; 52+ messages in thread
From: Jonathan Behrens @ 2019-07-26 21:00 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

The remaining checks are not sufficient. If you look at the bottom of
csr.c, you'll see that for most of the M-mode CSRs the predicate is set to
"any" which unconditionally allows access regardless of privilege mode. The
S-mode CSR predicates similarly only check that supervisor mode exists, but
not that the processor is current running in that mode. I do agree with you
that using the register address to determine the required privilege mode is
a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR
address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.

I also tested by booting RVirt <https://github.com/mit-pdos/RVirt> with a
Linux guest and found that this patch caused it to instantly crash because
guest CSR accesses weren't intercepted.

Jonathan

On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com>
> wrote:
> >
> > Unless I'm missing something, this is the only place that QEMU checks
> the privilege level for read and writes to CSRs. The exact computation used
> here won't work with the hypervisor extension, but we also can't just get
> rid of privilege checking entirely...
>
> The csr_ops struct contains a checker function, so there are still
> some checks occurring. I haven't done negative testing on this patch,
> but the current check doesn't seem to make any sense so it should be
> removed. We can separately discuss adding more checks but this current
> way base don CSR address just seems strange.
>
> Alistair
>
> >
> > Jonathan
> >
> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <
> alistair.francis@wdc.com> wrote:
> >>
> >> The privledge check based on the CSR address mask 0x300 doesn't work
> >> when using Hypervisor extensions so remove the check
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >>  target/riscv/csr.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index e0d4586760..af3b762c8b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> >>
> >>      /* check privileges and return -1 if check fails */
> >>  #if !defined(CONFIG_USER_ONLY)
> >> -    int csr_priv = get_field(csrno, 0x300);
> >>      int read_only = get_field(csrno, 0xC00) == 3;
> >> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> >> +    if (write_mask && read_only) {
> >>          return -1;
> >>      }
> >>  #endif
> >> --
> >> 2.22.0
> >>
> >>
>

[-- Attachment #2: Type: text/html, Size: 3608 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
  2019-07-26 21:00         ` Jonathan Behrens
@ 2019-07-26 22:28           ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 22:28 UTC (permalink / raw)
  To: Jonathan Behrens
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 2:00 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> The remaining checks are not sufficient. If you look at the bottom of csr.c, you'll see that for most of the M-mode CSRs the predicate is set to "any" which unconditionally allows access regardless of privilege mode. The S-mode CSR predicates similarly only check that supervisor mode exists, but not that the processor is current running in that mode. I do agree with you that using the register address to determine the required privilege mode is a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.

Ah, I didn't realise that was guaranteed. I will drop this patch from
this series and update it in my Hypervisor series.

Alistair

>
> I also tested by booting RVirt with a Linux guest and found that this patch caused it to instantly crash because guest CSR accesses weren't intercepted.
>
> Jonathan
>
> On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>> >
>> > Unless I'm missing something, this is the only place that QEMU checks the privilege level for read and writes to CSRs. The exact computation used here won't work with the hypervisor extension, but we also can't just get rid of privilege checking entirely...
>>
>> The csr_ops struct contains a checker function, so there are still
>> some checks occurring. I haven't done negative testing on this patch,
>> but the current check doesn't seem to make any sense so it should be
>> removed. We can separately discuss adding more checks but this current
>> way base don CSR address just seems strange.
>>
>> Alistair
>>
>> >
>> > Jonathan
>> >
>> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com> wrote:
>> >>
>> >> The privledge check based on the CSR address mask 0x300 doesn't work
>> >> when using Hypervisor extensions so remove the check
>> >>
>> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> >> ---
>> >>  target/riscv/csr.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> >> index e0d4586760..af3b762c8b 100644
>> >> --- a/target/riscv/csr.c
>> >> +++ b/target/riscv/csr.c
>> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>> >>
>> >>      /* check privileges and return -1 if check fails */
>> >>  #if !defined(CONFIG_USER_ONLY)
>> >> -    int csr_priv = get_field(csrno, 0x300);
>> >>      int read_only = get_field(csrno, 0xC00) == 3;
>> >> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
>> >> +    if (write_mask && read_only) {
>> >>          return -1;
>> >>      }
>> >>  #endif
>> >> --
>> >> 2.22.0
>> >>
>> >>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
@ 2019-07-26 22:28           ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-26 22:28 UTC (permalink / raw)
  To: Jonathan Behrens
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Fri, Jul 26, 2019 at 2:00 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>
> The remaining checks are not sufficient. If you look at the bottom of csr.c, you'll see that for most of the M-mode CSRs the predicate is set to "any" which unconditionally allows access regardless of privilege mode. The S-mode CSR predicates similarly only check that supervisor mode exists, but not that the processor is current running in that mode. I do agree with you that using the register address to determine the required privilege mode is a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.

Ah, I didn't realise that was guaranteed. I will drop this patch from
this series and update it in my Hypervisor series.

Alistair

>
> I also tested by booting RVirt with a Linux guest and found that this patch caused it to instantly crash because guest CSR accesses weren't intercepted.
>
> Jonathan
>
> On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <fintelia@gmail.com> wrote:
>> >
>> > Unless I'm missing something, this is the only place that QEMU checks the privilege level for read and writes to CSRs. The exact computation used here won't work with the hypervisor extension, but we also can't just get rid of privilege checking entirely...
>>
>> The csr_ops struct contains a checker function, so there are still
>> some checks occurring. I haven't done negative testing on this patch,
>> but the current check doesn't seem to make any sense so it should be
>> removed. We can separately discuss adding more checks but this current
>> way base don CSR address just seems strange.
>>
>> Alistair
>>
>> >
>> > Jonathan
>> >
>> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <alistair.francis@wdc.com> wrote:
>> >>
>> >> The privledge check based on the CSR address mask 0x300 doesn't work
>> >> when using Hypervisor extensions so remove the check
>> >>
>> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> >> ---
>> >>  target/riscv/csr.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> >> index e0d4586760..af3b762c8b 100644
>> >> --- a/target/riscv/csr.c
>> >> +++ b/target/riscv/csr.c
>> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>> >>
>> >>      /* check privileges and return -1 if check fails */
>> >>  #if !defined(CONFIG_USER_ONLY)
>> >> -    int csr_priv = get_field(csrno, 0x300);
>> >>      int read_only = get_field(csrno, 0xC00) == 3;
>> >> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
>> >> +    if (write_mask && read_only) {
>> >>          return -1;
>> >>      }
>> >>  #endif
>> >> --
>> >> 2.22.0
>> >>
>> >>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 14:28     ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:28 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
@ 2019-07-29 14:28     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:28 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 14:31     ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:31 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Let's creaate a function that tests if floating point support is

"create"

> 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>

> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;
>  
> 


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-29 14:31     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:31 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Let's creaate a function that tests if floating point support is

"create"

> 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>

> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;
>  
> 


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 14:33     ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:33 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be becuase it

"because"

> 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>
> ---
>  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;
> 


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs
@ 2019-07-29 14:33     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 52+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-29 14:33 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer

On 7/25/19 8:52 PM, Alistair Francis wrote:
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be becuase it

"because"

> 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>
> ---
>  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;
> 


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 15:19     ` Chih-Min Chao
  -1 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 15:19 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> From: Atish Patra <atish.patra@wdc.com>
>
> As per the RISC-V spec, Floating Point registers are named as f0..f31
> so lets fix the register names accordingly.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f8d07bd20a..af1e9b7690 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>  };
>
>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>  };
>

Could you indicate the section of the spec ?
By chapter 20 of user spec, the patch changes the floating register name to
architecture name but leave the integer register use the ABI name.

chihmin

>  const char * const riscv_excp_names[] = {
> --
> 2.22.0
>
>
>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
@ 2019-07-29 15:19     ` Chih-Min Chao
  0 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 15:19 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> From: Atish Patra <atish.patra@wdc.com>
>
> As per the RISC-V spec, Floating Point registers are named as f0..f31
> so lets fix the register names accordingly.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f8d07bd20a..af1e9b7690 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>  };
>
>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>  };
>

Could you indicate the section of the spec ?
By chapter 20 of user spec, the patch changes the floating register name to
architecture name but leave the integer register use the ABI name.

chihmin

>  const char * const riscv_excp_names[] = {
> --
> 2.22.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 2958 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 16:56     ` Chih-Min Chao
  -1 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 16:56 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Let's creaate 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>
> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>

  FS are 2bits
  original:
        only 3 is true
  new
       1, 2, 3 all make dirty true

  Since only 3 means dirty, keeps this part unchanged should be reasonable.

chihmin

     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;
>
> --
> 2.22.0
>
>
>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-29 16:56     ` Chih-Min Chao
  0 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 16:56 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 6063 bytes --]

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Let's creaate 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>
> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>

  FS are 2bits
  original:
        only 3 is true
  new
       1, 2, 3 all make dirty true

  Since only 3 means dirty, keeps this part unchanged should be reasonable.

chihmin

     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;
>
> --
> 2.22.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 7657 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-29 17:32     ` Chih-Min Chao
  -1 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 17:32 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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
>
>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
@ 2019-07-29 17:32     ` Chih-Min Chao
  0 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-29 17:32 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Alistair Francis, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com>
wrote:

> Signed-off-by: Alistair Francis <alistair.francis@wdc.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 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,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 ce8907f6aa..3b8a623919 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
>
>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>

[-- Attachment #2: Type: text/html, Size: 2286 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
@ 2019-07-30  8:49     ` Christophe de Dinechin
  -1 siblings, 0 replies; 52+ messages in thread
From: Christophe de Dinechin @ 2019-07-30  8:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: alistair23, alistair.francis, palmer, qemu-riscv


Alistair Francis writes:

> Let's creaate a function that tests if floating point support is

Typo: create

> 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>
> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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;

Will there be more conditions that lead to the "true" case?
If not, please consider making it a one-liner for readability, e.g.

   return env->mstatus & MSTATUS_FS;

(just a personal preference, feel free to ignore)

> +}
> +
>  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 af3b762c8b..7b73b73cf7 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)) {

This was existing behavior, but I'm curious why all these
tests are disabled when env->debugger is set. This was introduced
in 753e3fe20, but I see no rationale in the commit message.
I find it odd, maybe even suspicious, that activating the debugger
would change the behavior :-)

>          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,8 @@ 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_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-30  8:49     ` Christophe de Dinechin
  0 siblings, 0 replies; 52+ messages in thread
From: Christophe de Dinechin @ 2019-07-30  8:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-riscv, alistair23, palmer, alistair.francis


Alistair Francis writes:

> Let's creaate a function that tests if floating point support is

Typo: create

> 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>
> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 19 ++++++++++---------
>  3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2dc9b17678 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;

Will there be more conditions that lead to the "true" case?
If not, please consider making it a one-liner for readability, e.g.

   return env->mstatus & MSTATUS_FS;

(just a personal preference, feel free to ignore)

> +}
> +
>  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 af3b762c8b..7b73b73cf7 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)) {

This was existing behavior, but I'm curious why all these
tests are disabled when env->debugger is set. This was introduced
in 753e3fe20, but I see no rationale in the commit message.
I find it odd, maybe even suspicious, that activating the debugger
would change the behavior :-)

>          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,8 @@ 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_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-29 16:56     ` Chih-Min Chao
@ 2019-07-30 18:32       ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:32 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Mon, Jul 29, 2019 at 9:56 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> Let's creaate 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>
>> ---
>>  target/riscv/cpu.h        |  6 +++++-
>>  target/riscv/cpu_helper.c | 10 ++++++++++
>>  target/riscv/csr.c        | 19 ++++++++++---------
>>  3 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>
>
>   FS are 2bits
>   original:
>         only 3 is true
>   new
>        1, 2, 3 all make dirty true
>
>   Since only 3 means dirty, keeps this part unchanged should be reasonable.

Good point, I have updated this.

Alistair

>
> chihmin
>
>>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>>      env->mstatus = mstatus;
>>
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-30 18:32       ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:32 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Mon, Jul 29, 2019 at 9:56 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> Let's creaate 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>
>> ---
>>  target/riscv/cpu.h        |  6 +++++-
>>  target/riscv/cpu_helper.c | 10 ++++++++++
>>  target/riscv/csr.c        | 19 ++++++++++---------
>>  3 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 0adb307f32..2dc9b17678 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 af3b762c8b..7b73b73cf7 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,8 @@ 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_XS) == MSTATUS_XS);
>
>
>   FS are 2bits
>   original:
>         only 3 is true
>   new
>        1, 2, 3 all make dirty true
>
>   Since only 3 means dirty, keeps this part unchanged should be reasonable.

Good point, I have updated this.

Alistair

>
> chihmin
>
>>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>>      env->mstatus = mstatus;
>>
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
  2019-07-30  8:49     ` [Qemu-riscv] " Christophe de Dinechin
@ 2019-07-30 18:33       ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:33 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Tue, Jul 30, 2019 at 1:49 AM Christophe de Dinechin
<dinechin@redhat.com> wrote:
>
>
> Alistair Francis writes:
>
> > Let's creaate a function that tests if floating point support is
>
> Typo: create

Fixed

>
> > 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>
> > ---
> >  target/riscv/cpu.h        |  6 +++++-
> >  target/riscv/cpu_helper.c | 10 ++++++++++
> >  target/riscv/csr.c        | 19 ++++++++++---------
> >  3 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0adb307f32..2dc9b17678 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;
>
> Will there be more conditions that lead to the "true" case?
> If not, please consider making it a one-liner for readability, e.g.
>
>    return env->mstatus & MSTATUS_FS;
>
> (just a personal preference, feel free to ignore)

There are going to be more conditionals when adding the Hypervisor extension.

>
> > +}
> > +
> >  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 af3b762c8b..7b73b73cf7 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)) {
>
> This was existing behavior, but I'm curious why all these
> tests are disabled when env->debugger is set. This was introduced
> in 753e3fe20, but I see no rationale in the commit message.
> I find it odd, maybe even suspicious, that activating the debugger
> would change the behavior :-)

This is to allow GDB to access the registers.

>
> >          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,8 @@ 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_XS) == MSTATUS_XS);
> >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> >      env->mstatus = mstatus;
>
> Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

Thanks!

Alistair

>
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
@ 2019-07-30 18:33       ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:33 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
	Palmer Dabbelt, Alistair Francis

On Tue, Jul 30, 2019 at 1:49 AM Christophe de Dinechin
<dinechin@redhat.com> wrote:
>
>
> Alistair Francis writes:
>
> > Let's creaate a function that tests if floating point support is
>
> Typo: create

Fixed

>
> > 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>
> > ---
> >  target/riscv/cpu.h        |  6 +++++-
> >  target/riscv/cpu_helper.c | 10 ++++++++++
> >  target/riscv/csr.c        | 19 ++++++++++---------
> >  3 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0adb307f32..2dc9b17678 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;
>
> Will there be more conditions that lead to the "true" case?
> If not, please consider making it a one-liner for readability, e.g.
>
>    return env->mstatus & MSTATUS_FS;
>
> (just a personal preference, feel free to ignore)

There are going to be more conditionals when adding the Hypervisor extension.

>
> > +}
> > +
> >  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 af3b762c8b..7b73b73cf7 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)) {
>
> This was existing behavior, but I'm curious why all these
> tests are disabled when env->debugger is set. This was introduced
> in 753e3fe20, but I see no rationale in the commit message.
> I find it odd, maybe even suspicious, that activating the debugger
> would change the behavior :-)

This is to allow GDB to access the registers.

>
> >          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,8 @@ 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_XS) == MSTATUS_XS);
> >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> >      env->mstatus = mstatus;
>
> Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

Thanks!

Alistair

>
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
  2019-07-29 15:19     ` [Qemu-riscv] " Chih-Min Chao
@ 2019-07-30 18:37       ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:37 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
> On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> From: Atish Patra <atish.patra@wdc.com>
>>
>> As per the RISC-V spec, Floating Point registers are named as f0..f31
>> so lets fix the register names accordingly.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>  target/riscv/cpu.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index f8d07bd20a..af1e9b7690 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>>  };
>>
>>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
>> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
>> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
>> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>>  };
>
>
> Could you indicate the section of the spec ?

Chapter 11: "“F” Standard Extension for Single-Precision
Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
f32.

> By chapter 20 of user spec, the patch changes the floating register name to architecture name but leave the integer register use the ABI name.

You mean the Packed-SIMD extension?

Alistair

>
> chihmin
>>
>>  const char * const riscv_excp_names[] = {
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
@ 2019-07-30 18:37       ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-07-30 18:37 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
> On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>>
>> From: Atish Patra <atish.patra@wdc.com>
>>
>> As per the RISC-V spec, Floating Point registers are named as f0..f31
>> so lets fix the register names accordingly.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>  target/riscv/cpu.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index f8d07bd20a..af1e9b7690 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>>  };
>>
>>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
>> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
>> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
>> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>>  };
>
>
> Could you indicate the section of the spec ?

Chapter 11: "“F” Standard Extension for Single-Precision
Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
f32.

> By chapter 20 of user spec, the patch changes the floating register name to architecture name but leave the integer register use the ABI name.

You mean the Packed-SIMD extension?

Alistair

>
> chihmin
>>
>>  const char * const riscv_excp_names[] = {
>> --
>> 2.22.0
>>
>>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
  2019-07-30 18:37       ` [Qemu-riscv] " Alistair Francis
@ 2019-07-31  8:10         ` Chih-Min Chao
  -1 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-31  8:10 UTC (permalink / raw)
  To: Alistair Francis
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Wed, Jul 31, 2019 at 2:41 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com>
> wrote:
> >
> >
> > On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <
> alistair.francis@wdc.com> wrote:
> >>
> >> From: Atish Patra <atish.patra@wdc.com>
> >>
> >> As per the RISC-V spec, Floating Point registers are named as f0..f31
> >> so lets fix the register names accordingly.
> >>
> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >>  target/riscv/cpu.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index f8d07bd20a..af1e9b7690 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
> >>  };
> >>
> >>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
> >> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
> >> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
> >> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
> >>  };
> >
> >
> > Could you indicate the section of the spec ?
>
> Chapter 11: "“F” Standard Extension for Single-Precision
> Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
> f32.
>
> > By chapter 20 of user spec, the patch changes the floating register name
> to architecture name but leave the integer register use the ABI name.
>
> You mean the Packed-SIMD extension?
>
> Alistair
>

I means  "Chapter 20RISC-V Assembly Programmer’s Handbook".
There is an table, "Table 20.1: Assembler mnemonics for RISC-V integer and
floating-point registers.",  describes
the architecture name and ABI name for integer and floating-point register.

By the way,  I reference the riscv-spec-2.2

chihmin



> >
> > chihmin
> >>
> >>  const char * const riscv_excp_names[] = {
> >> --
> >> 2.22.0
> >>
> >>
>

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
@ 2019-07-31  8:10         ` Chih-Min Chao
  0 siblings, 0 replies; 52+ messages in thread
From: Chih-Min Chao @ 2019-07-31  8:10 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On Wed, Jul 31, 2019 at 2:41 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com>
> wrote:
> >
> >
> > On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <
> alistair.francis@wdc.com> wrote:
> >>
> >> From: Atish Patra <atish.patra@wdc.com>
> >>
> >> As per the RISC-V spec, Floating Point registers are named as f0..f31
> >> so lets fix the register names accordingly.
> >>
> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >>  target/riscv/cpu.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index f8d07bd20a..af1e9b7690 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
> >>  };
> >>
> >>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
> >> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
> >> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
> >> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
> >>  };
> >
> >
> > Could you indicate the section of the spec ?
>
> Chapter 11: "“F” Standard Extension for Single-Precision
> Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
> f32.
>
> > By chapter 20 of user spec, the patch changes the floating register name
> to architecture name but leave the integer register use the ABI name.
>
> You mean the Packed-SIMD extension?
>
> Alistair
>

I means  "Chapter 20RISC-V Assembly Programmer’s Handbook".
There is an table, "Table 20.1: Assembler mnemonics for RISC-V integer and
floating-point registers.",  describes
the architecture name and ABI name for integer and floating-point register.

By the way,  I reference the riscv-spec-2.2

chihmin



> >
> > chihmin
> >>
> >>  const char * const riscv_excp_names[] = {
> >> --
> >> 2.22.0
> >>
> >>
>

[-- Attachment #2: Type: text/html, Size: 4296 bytes --]

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
  2019-07-31  8:10         ` [Qemu-riscv] " Chih-Min Chao
@ 2019-08-05 17:49           ` Alistair Francis
  -1 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-08-05 17:49 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
	qemu-devel@nongnu.org Developers

On Wed, Jul 31, 2019 at 1:10 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Wed, Jul 31, 2019 at 2:41 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>> >
>> >
>> > On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>> >>
>> >> From: Atish Patra <atish.patra@wdc.com>
>> >>
>> >> As per the RISC-V spec, Floating Point registers are named as f0..f31
>> >> so lets fix the register names accordingly.
>> >>
>> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> >> ---
>> >>  target/riscv/cpu.c | 8 ++++----
>> >>  1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> >> index f8d07bd20a..af1e9b7690 100644
>> >> --- a/target/riscv/cpu.c
>> >> +++ b/target/riscv/cpu.c
>> >> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>> >>  };
>> >>
>> >>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
>> >> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
>> >> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
>> >> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>> >>  };
>> >
>> >
>> > Could you indicate the section of the spec ?
>>
>> Chapter 11: "“F” Standard Extension for Single-Precision
>> Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
>> f32.
>>
>> > By chapter 20 of user spec, the patch changes the floating register name to architecture name but leave the integer register use the ABI name.
>>
>> You mean the Packed-SIMD extension?
>>
>> Alistair
>
>
> I means  "Chapter 20RISC-V Assembly Programmer’s Handbook".
> There is an table, "Table 20.1: Assembler mnemonics for RISC-V integer and floating-point registers.",  describes
> the architecture name and ABI name for integer and floating-point register.

Ah ok. In general I think it makes sense to base the names on the spec
and not other sources.

Alistair

>
> By the way,  I reference the riscv-spec-2.2
>
> chihmin
>
>
>>
>> >
>> > chihmin
>> >>
>> >>  const char * const riscv_excp_names[] = {
>> >> --
>> >> 2.22.0
>> >>
>> >>


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names
@ 2019-08-05 17:49           ` Alistair Francis
  0 siblings, 0 replies; 52+ messages in thread
From: Alistair Francis @ 2019-08-05 17:49 UTC (permalink / raw)
  To: Chih-Min Chao
  Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
	open list:RISC-V, Palmer Dabbelt

On Wed, Jul 31, 2019 at 1:10 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>
>
>
> On Wed, Jul 31, 2019 at 2:41 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Mon, Jul 29, 2019 at 8:19 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote:
>> >
>> >
>> > On Fri, Jul 26, 2019 at 2:56 AM Alistair Francis <alistair.francis@wdc.com> wrote:
>> >>
>> >> From: Atish Patra <atish.patra@wdc.com>
>> >>
>> >> As per the RISC-V spec, Floating Point registers are named as f0..f31
>> >> so lets fix the register names accordingly.
>> >>
>> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> >> ---
>> >>  target/riscv/cpu.c | 8 ++++----
>> >>  1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> >> index f8d07bd20a..af1e9b7690 100644
>> >> --- a/target/riscv/cpu.c
>> >> +++ b/target/riscv/cpu.c
>> >> @@ -40,10 +40,10 @@ const char * const riscv_int_regnames[] = {
>> >>  };
>> >>
>> >>  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", "f1", "f2",  "f3",  "f4", "f5", "f6", "f7",
>> >> +  "f8", "f9", "f10",  "f11",  "f12", "f13", "f14", "f15",
>> >> +  "f16", "f17", "f18",  "f19",  "f20", "f21", "f22", "f23",
>> >> +  "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31"
>> >>  };
>> >
>> >
>> > Could you indicate the section of the spec ?
>>
>> Chapter 11: "“F” Standard Extension for Single-Precision
>> Floating-Point, Version 2.2", section 11.1, Figure 11.1 shows f0 -
>> f32.
>>
>> > By chapter 20 of user spec, the patch changes the floating register name to architecture name but leave the integer register use the ABI name.
>>
>> You mean the Packed-SIMD extension?
>>
>> Alistair
>
>
> I means  "Chapter 20RISC-V Assembly Programmer’s Handbook".
> There is an table, "Table 20.1: Assembler mnemonics for RISC-V integer and floating-point registers.",  describes
> the architecture name and ABI name for integer and floating-point register.

Ah ok. In general I think it makes sense to base the names on the spec
and not other sources.

Alistair

>
> By the way,  I reference the riscv-spec-2.2
>
> chihmin
>
>
>>
>> >
>> > chihmin
>> >>
>> >>  const char * const riscv_excp_names[] = {
>> >> --
>> >> 2.22.0
>> >>
>> >>


^ permalink raw reply	[flat|nested] 52+ messages in thread
end of thread, other threads:[~2019-08-05 17:53 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25 18:51 [Qemu-devel] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2 Alistair Francis
2019-07-25 18:51 ` [Qemu-riscv] " Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-29 14:33   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-07-29 14:33     ` [Qemu-riscv] " Philippe Mathieu-Daudé
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-25 21:47   ` [Qemu-devel] " Jonathan Behrens
2019-07-25 21:47     ` Jonathan Behrens
2019-07-26 20:24     ` [Qemu-devel] " Alistair Francis
2019-07-26 20:24       ` Alistair Francis
2019-07-26 21:00       ` [Qemu-devel] " Jonathan Behrens
2019-07-26 21:00         ` Jonathan Behrens
2019-07-26 22:28         ` [Qemu-devel] " Alistair Francis
2019-07-26 22:28           ` Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-26 15:22   ` [Qemu-devel] " Jonathan Behrens
2019-07-26 15:22     ` Jonathan Behrens
2019-07-29 14:28   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-07-29 14:28     ` [Qemu-riscv] " Philippe Mathieu-Daudé
2019-07-29 17:32   ` Chih-Min Chao
2019-07-29 17:32     ` [Qemu-riscv] " Chih-Min Chao
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-29 14:31   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-07-29 14:31     ` [Qemu-riscv] " Philippe Mathieu-Daudé
2019-07-29 16:56   ` [Qemu-devel] [Qemu-riscv] " Chih-Min Chao
2019-07-29 16:56     ` Chih-Min Chao
2019-07-30 18:32     ` [Qemu-devel] " Alistair Francis
2019-07-30 18:32       ` Alistair Francis
2019-07-30  8:49   ` [Qemu-devel] " Christophe de Dinechin
2019-07-30  8:49     ` [Qemu-riscv] " Christophe de Dinechin
2019-07-30 18:33     ` Alistair Francis
2019-07-30 18:33       ` [Qemu-riscv] " Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4 Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-26 17:40   ` [Qemu-devel] " Chih-Min Chao
2019-07-26 17:40     ` [Qemu-riscv] " Chih-Min Chao
2019-07-26 20:20     ` Alistair Francis
2019-07-26 20:20       ` [Qemu-riscv] " Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names Alistair Francis
2019-07-25 18:52   ` [Qemu-riscv] " Alistair Francis
2019-07-29 15:19   ` [Qemu-devel] " Chih-Min Chao
2019-07-29 15:19     ` [Qemu-riscv] " Chih-Min Chao
2019-07-30 18:37     ` Alistair Francis
2019-07-30 18:37       ` [Qemu-riscv] " Alistair Francis
2019-07-31  8:10       ` Chih-Min Chao
2019-07-31  8:10         ` [Qemu-riscv] " Chih-Min Chao
2019-08-05 17:49         ` Alistair Francis
2019-08-05 17:49           ` [Qemu-riscv] " Alistair Francis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.