[Qemu-devel] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2

[Qemu-devel] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2

[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

[Qemu-riscv] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2

[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

[Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs

[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

[Qemu-riscv] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs

[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

Re: [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs

[Philippe Mathieu-Daudé]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs

[Philippe Mathieu-Daudé]

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

[Qemu-devel] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[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

[Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Jonathan Behrens]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Jonathan Behrens]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Alistair Francis]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Alistair Francis]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Jonathan Behrens]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Jonathan Behrens]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Alistair Francis]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Alistair Francis]

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

[Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[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

[Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Jonathan Behrens]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Jonathan Behrens]

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

Re: [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Philippe Mathieu-Daudé]

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>

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Philippe Mathieu-Daudé]

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>

Re: [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Chih-Min Chao]

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>

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions

[Chih-Min Chao]

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

[Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[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

[Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[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

Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Philippe Mathieu-Daudé]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Philippe Mathieu-Daudé]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Chih-Min Chao]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Chih-Min Chao]

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

Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Alistair Francis]

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

Re: [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Alistair Francis]

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

Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Christophe de Dinechin]

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)

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

[Christophe de Dinechin]

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)

Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled

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

[Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[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

[Qemu-riscv] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[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

Re: [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[Chih-Min Chao]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[Chih-Min Chao]

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

Re: [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[Alistair Francis]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4

[Alistair Francis]

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

[Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[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

[Qemu-riscv] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[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

Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Chih-Min Chao]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Chih-Min Chao]

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

Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Alistair Francis]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Alistair Francis]

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

Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Chih-Min Chao]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Chih-Min Chao]

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

Re: [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Alistair Francis]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names

[Alistair Francis]

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