* [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-05-22 15:59 ` Jose Martins
0 siblings, 0 replies; 16+ messages in thread
From: Jose Martins @ 2021-05-22 15:59 UTC (permalink / raw)
To: qemu-devel
Cc: Jose Martins, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Bastian Koppelmann, open list:RISC-V TCG CPUs
The specification mandates for certain bits to be hardwired in the
hypervisor delegation registers. This was not being enforced.
Signed-off-by: Jose Martins <josemartins90@gmail.com>
---
target/riscv/csr.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d2585395bf..9b74a00cc9 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
VS_MODE_INTERRUPTS;
+static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
VS_MODE_INTERRUPTS;
static const target_ulong delegable_excps =
@@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
(1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
(1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
(1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
+static const target_ulong vs_delegable_excps = delegable_excps &
+ ~((1ULL << (RISCV_EXCP_S_ECALL)) |
+ (1ULL << (RISCV_EXCP_VS_ECALL)) |
+ (1ULL << (RISCV_EXCP_M_ECALL)) |
+ (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
+ (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
+ (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
+ (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
@@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
{
- env->hedeleg = val;
+ env->hedeleg = val & vs_delegable_excps;
return 0;
}
@@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
{
- env->hideleg = val;
+ env->hideleg = val & vs_delegable_ints;
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-05-22 15:59 ` Jose Martins
@ 2021-05-25 7:22 ` LIU Zhiwei
-1 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-25 7:22 UTC (permalink / raw)
To: Jose Martins, qemu-devel
Cc: Bastian Koppelmann, Alistair Francis, open list:RISC-V TCG CPUs,
Palmer Dabbelt, Sagar Karandikar
[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]
On 5/22/21 11:59 PM, Jose Martins wrote:
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
We can use it directly if only one macro VS_MODE_INTERRUPTS.
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from
the specification.
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
I think it's OK here.
However, as hedeleg is WARL, you had better reserve the other fields
like medeleg:
env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
I really don't understand why medeleg codes this way. Is there anyone
can give a better explanation?
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
The similar comments for hedeleg.
Zhiwei
[-- Attachment #2: Type: text/html, Size: 4068 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-05-25 7:22 ` LIU Zhiwei
0 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-25 7:22 UTC (permalink / raw)
To: Jose Martins, qemu-devel
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
Alistair Francis, Palmer Dabbelt
[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]
On 5/22/21 11:59 PM, Jose Martins wrote:
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
We can use it directly if only one macro VS_MODE_INTERRUPTS.
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from
the specification.
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
I think it's OK here.
However, as hedeleg is WARL, you had better reserve the other fields
like medeleg:
env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
I really don't understand why medeleg codes this way. Is there anyone
can give a better explanation?
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
The similar comments for hedeleg.
Zhiwei
[-- Attachment #2: Type: text/html, Size: 4068 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-05-25 7:22 ` LIU Zhiwei
@ 2021-05-25 17:50 ` Jose Martins
-1 siblings, 0 replies; 16+ messages in thread
From: Jose Martins @ 2021-05-25 17:50 UTC (permalink / raw)
To: LIU Zhiwei
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
> We can use it directly if only one macro VS_MODE_INTERRUPTS.
I wrote it like this to be more coherent with what was already there
which also makes it more readable. Furthermore, the compiler will just
probably optimize the variable away, right?
> I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from the specification.
You are right. I had doubts about this also. The table that defines it
in the spec is missing this bit. I raised an issue on the spec repo
(https://github.com/riscv/riscv-isa-manual/issues/649). But in my
opinion, it wouldn't really make sense to allow this exception to be
delegated. What do you think? Is there any use case for this to be
allowed? Maybe we'll need a clarification from the spec to reach a
final decision.
> However, as hedeleg is WARL, you had better reserve the other fields like medeleg:
>
> env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
Isn't the patch's implementation of hedeleg/hideleg providing a WARL
behavior already? I don't think we need this preservation behavior
since in the case of hideleg/hedeleg there can only be 0-wired bits. I
believe this won't change. For hedeleg the spec states that "Each bit
of hedeleg shall be either writable or hardwired to zero". For
hideleg: "Among bits 15:0 of hideleg, only bits 10, 6, and 2
(corresponding to the standard VS-level interrupts) shall be writable,
and the others shall be hardwired to zero."
> I really don't understand why medeleg codes this way. Is there anyone can give a better explanation?
I don't know if I fully understood your question, but I don't get why
you would need to preserve non-delegable bits in medeleg in this way,
even to keep WARL behavior. Again, the specification only allows
medeleg bits to be hardwired to zero: "An implementation shall not
hardwire any bits of medeleg to one, i.e., any synchronous trap that
can be delegated must support not being delegated.", so a bitwise-and
should suffice.
José
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-05-25 17:50 ` Jose Martins
0 siblings, 0 replies; 16+ messages in thread
From: Jose Martins @ 2021-05-25 17:50 UTC (permalink / raw)
To: LIU Zhiwei
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V TCG CPUs,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt
> We can use it directly if only one macro VS_MODE_INTERRUPTS.
I wrote it like this to be more coherent with what was already there
which also makes it more readable. Furthermore, the compiler will just
probably optimize the variable away, right?
> I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from the specification.
You are right. I had doubts about this also. The table that defines it
in the spec is missing this bit. I raised an issue on the spec repo
(https://github.com/riscv/riscv-isa-manual/issues/649). But in my
opinion, it wouldn't really make sense to allow this exception to be
delegated. What do you think? Is there any use case for this to be
allowed? Maybe we'll need a clarification from the spec to reach a
final decision.
> However, as hedeleg is WARL, you had better reserve the other fields like medeleg:
>
> env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
Isn't the patch's implementation of hedeleg/hideleg providing a WARL
behavior already? I don't think we need this preservation behavior
since in the case of hideleg/hedeleg there can only be 0-wired bits. I
believe this won't change. For hedeleg the spec states that "Each bit
of hedeleg shall be either writable or hardwired to zero". For
hideleg: "Among bits 15:0 of hideleg, only bits 10, 6, and 2
(corresponding to the standard VS-level interrupts) shall be writable,
and the others shall be hardwired to zero."
> I really don't understand why medeleg codes this way. Is there anyone can give a better explanation?
I don't know if I fully understood your question, but I don't get why
you would need to preserve non-delegable bits in medeleg in this way,
even to keep WARL behavior. Again, the specification only allows
medeleg bits to be hardwired to zero: "An implementation shall not
hardwire any bits of medeleg to one, i.e., any synchronous trap that
can be delegated must support not being delegated.", so a bitwise-and
should suffice.
José
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-05-25 17:50 ` Jose Martins
@ 2021-05-28 1:56 ` LIU Zhiwei
-1 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-28 1:56 UTC (permalink / raw)
To: Jose Martins
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On 5/26/21 1:50 AM, Jose Martins wrote:
>> We can use it directly if only one macro VS_MODE_INTERRUPTS.
> I wrote it like this to be more coherent with what was already there
> which also makes it more readable. Furthermore, the compiler will just
> probably optimize the variable away, right?
Hi Jose,
Sorry I missed this mail.
Sounds good. Just keep it.
>
>> I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from the specification.
> You are right. I had doubts about this also. The table that defines it
> in the spec is missing this bit. I raised an issue on the spec repo
> (https://github.com/riscv/riscv-isa-manual/issues/649). But in my
> opinion, it wouldn't really make sense to allow this exception to be
> delegated.
Agree.
> What do you think? Is there any use case for this to be
> allowed? Maybe we'll need a clarification from the spec to reach a
> final decision.
That's will be great.
>
>> However, as hedeleg is WARL, you had better reserve the other fields like medeleg:
>>
>> env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
> Isn't the patch's implementation of hedeleg/hideleg providing a WARL
> behavior already? I don't think we need this preservation behavior
> since in the case of hideleg/hedeleg there can only be 0-wired bits. I
> believe this won't change. For hedeleg the spec states that "Each bit
> of hedeleg shall be either writable or hardwired to zero". For
> hideleg: "Among bits 15:0 of hideleg, only bits 10, 6, and 2
> (corresponding to the standard VS-level interrupts) shall be writable,
> and the others shall be hardwired to zero."
Agree.
>
>> I really don't understand why medeleg codes this way. Is there anyone can give a better explanation?
>>
>> I don't know if I fully understood your question, but I don't get why
>> you would need to preserve non-delegable bits in medeleg in this way,
>> even to keep WARL behavior.
> Again, the specification only allows
> medeleg bits to be hardwired to zero: "An implementation shall not
> hardwire any bits of medeleg to one, i.e., any synchronous trap that
> can be delegated must support not being delegated.", so a bitwise-and
> should suffice.
That's the current way to implement medeleg in QEMU. I just copy the code.
In my opinion, WARL means:
1) For writable fields with WARL, any illegal written value will be
discarded.
2) For reserved fields with WARL, any written value will be discarded
and it should always keep hardwired value.
I agree with your opinion. We can just use bitwise-and for medeleg.
Best Regards,
Zhiwei
> José
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-05-28 1:56 ` LIU Zhiwei
0 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-28 1:56 UTC (permalink / raw)
To: Jose Martins
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V TCG CPUs,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt
On 5/26/21 1:50 AM, Jose Martins wrote:
>> We can use it directly if only one macro VS_MODE_INTERRUPTS.
> I wrote it like this to be more coherent with what was already there
> which also makes it more readable. Furthermore, the compiler will just
> probably optimize the variable away, right?
Hi Jose,
Sorry I missed this mail.
Sounds good. Just keep it.
>
>> I didn't find that the RISCV_EXCP_VS_ECALL should be read-only 0 from the specification.
> You are right. I had doubts about this also. The table that defines it
> in the spec is missing this bit. I raised an issue on the spec repo
> (https://github.com/riscv/riscv-isa-manual/issues/649). But in my
> opinion, it wouldn't really make sense to allow this exception to be
> delegated.
Agree.
> What do you think? Is there any use case for this to be
> allowed? Maybe we'll need a clarification from the spec to reach a
> final decision.
That's will be great.
>
>> However, as hedeleg is WARL, you had better reserve the other fields like medeleg:
>>
>> env->medeleg = (env->medeleg & ~delegable_excps) | (val & delegable_excps);
> Isn't the patch's implementation of hedeleg/hideleg providing a WARL
> behavior already? I don't think we need this preservation behavior
> since in the case of hideleg/hedeleg there can only be 0-wired bits. I
> believe this won't change. For hedeleg the spec states that "Each bit
> of hedeleg shall be either writable or hardwired to zero". For
> hideleg: "Among bits 15:0 of hideleg, only bits 10, 6, and 2
> (corresponding to the standard VS-level interrupts) shall be writable,
> and the others shall be hardwired to zero."
Agree.
>
>> I really don't understand why medeleg codes this way. Is there anyone can give a better explanation?
>>
>> I don't know if I fully understood your question, but I don't get why
>> you would need to preserve non-delegable bits in medeleg in this way,
>> even to keep WARL behavior.
> Again, the specification only allows
> medeleg bits to be hardwired to zero: "An implementation shall not
> hardwire any bits of medeleg to one, i.e., any synchronous trap that
> can be delegated must support not being delegated.", so a bitwise-and
> should suffice.
That's the current way to implement medeleg in QEMU. I just copy the code.
In my opinion, WARL means:
1) For writable fields with WARL, any illegal written value will be
discarded.
2) For reserved fields with WARL, any written value will be discarded
and it should always keep hardwired value.
I agree with your opinion. We can just use bitwise-and for medeleg.
Best Regards,
Zhiwei
> José
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-05-22 15:59 ` Jose Martins
@ 2021-05-28 2:00 ` LIU Zhiwei
-1 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-28 2:00 UTC (permalink / raw)
To: Jose Martins, qemu-devel
Cc: Bastian Koppelmann, Alistair Francis, open list:RISC-V TCG CPUs,
Palmer Dabbelt, Sagar Karandikar
Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
Zhiwei
On 5/22/21 11:59 PM, Jose Martins wrote:
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-05-28 2:00 ` LIU Zhiwei
0 siblings, 0 replies; 16+ messages in thread
From: LIU Zhiwei @ 2021-05-28 2:00 UTC (permalink / raw)
To: Jose Martins, qemu-devel
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
Alistair Francis, Palmer Dabbelt
Reviewed-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
Zhiwei
On 5/22/21 11:59 PM, Jose Martins wrote:
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-05-22 15:59 ` Jose Martins
@ 2021-06-10 23:12 ` Alistair Francis
-1 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2021-06-10 23:12 UTC (permalink / raw)
To: Jose Martins
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On Sun, May 23, 2021 at 1:59 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
These two are both read only 0, shouldn't they not be included in this list?
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
Because we then allow a write to occur here.
Alistair
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-06-10 23:12 ` Alistair Francis
0 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2021-06-10 23:12 UTC (permalink / raw)
To: Jose Martins
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V TCG CPUs,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt
On Sun, May 23, 2021 at 1:59 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> The specification mandates for certain bits to be hardwired in the
> hypervisor delegation registers. This was not being enforced.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> target/riscv/csr.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d2585395bf..9b74a00cc9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -394,6 +394,7 @@ static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
>
> static const target_ulong delegable_ints = S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> +static const target_ulong vs_delegable_ints = VS_MODE_INTERRUPTS;
> static const target_ulong all_ints = M_MODE_INTERRUPTS | S_MODE_INTERRUPTS |
> VS_MODE_INTERRUPTS;
> static const target_ulong delegable_excps =
> @@ -416,6 +417,14 @@ static const target_ulong delegable_excps =
> (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT));
> +static const target_ulong vs_delegable_excps = delegable_excps &
> + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> + (1ULL << (RISCV_EXCP_M_ECALL)) |
These two are both read only 0, shouldn't they not be included in this list?
> + (1ULL << (RISCV_EXCP_INST_GUEST_PAGE_FAULT)) |
> + (1ULL << (RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT)) |
> + (1ULL << (RISCV_EXCP_VIRT_INSTRUCTION_FAULT)) |
> + (1ULL << (RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT)));
> static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
> SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> @@ -963,7 +972,7 @@ static int read_hedeleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hedeleg = val;
> + env->hedeleg = val & vs_delegable_excps;
Because we then allow a write to occur here.
Alistair
> return 0;
> }
>
> @@ -975,7 +984,7 @@ static int read_hideleg(CPURISCVState *env, int csrno, target_ulong *val)
>
> static int write_hideleg(CPURISCVState *env, int csrno, target_ulong val)
> {
> - env->hideleg = val;
> + env->hideleg = val & vs_delegable_ints;
> return 0;
> }
>
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-06-10 23:12 ` Alistair Francis
@ 2021-06-24 13:48 ` Jose Martins
-1 siblings, 0 replies; 16+ messages in thread
From: Jose Martins @ 2021-06-24 13:48 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
> > +static const target_ulong vs_delegable_excps = delegable_excps &
> > + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
>
> > + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> > + (1ULL << (RISCV_EXCP_M_ECALL)) |
>
> These two are both read only 0, shouldn't they not be included in this list?
>
> > static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> > {
> > - env->hedeleg = val;
> > + env->hedeleg = val & vs_delegable_excps;
>
> Because we then allow a write to occur here.
Note that the list is being bitwise negated, so both of these are
actually not writable (ie read-only 0). There is still the question
regarding the VS_ECALL (exception 10) bit raised by Zhiwei, since
table 5.2 in the spec does not explicitly classify it. However, I
believe it is safe to assume that exception 10 is non-delegable.
José
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-06-24 13:48 ` Jose Martins
0 siblings, 0 replies; 16+ messages in thread
From: Jose Martins @ 2021-06-24 13:48 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V TCG CPUs,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt
> > +static const target_ulong vs_delegable_excps = delegable_excps &
> > + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
>
> > + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> > + (1ULL << (RISCV_EXCP_M_ECALL)) |
>
> These two are both read only 0, shouldn't they not be included in this list?
>
> > static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> > {
> > - env->hedeleg = val;
> > + env->hedeleg = val & vs_delegable_excps;
>
> Because we then allow a write to occur here.
Note that the list is being bitwise negated, so both of these are
actually not writable (ie read-only 0). There is still the question
regarding the VS_ECALL (exception 10) bit raised by Zhiwei, since
table 5.2 in the spec does not explicitly classify it. However, I
believe it is safe to assume that exception 10 is non-delegable.
José
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
2021-06-24 13:48 ` Jose Martins
@ 2021-07-08 4:45 ` Alistair Francis
-1 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2021-07-08 4:45 UTC (permalink / raw)
To: Jose Martins
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On Thu, Jun 24, 2021 at 11:48 PM Jose Martins <josemartins90@gmail.com> wrote:
>
> > > +static const target_ulong vs_delegable_excps = delegable_excps &
> > > + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> >
> > > + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> > > + (1ULL << (RISCV_EXCP_M_ECALL)) |
> >
> > These two are both read only 0, shouldn't they not be included in this list?
> >
> > > static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> > > {
> > > - env->hedeleg = val;
> > > + env->hedeleg = val & vs_delegable_excps;
> >
> > Because we then allow a write to occur here.
>
> Note that the list is being bitwise negated, so both of these are
> actually not writable (ie read-only 0). There is still the question
> regarding the VS_ECALL (exception 10) bit raised by Zhiwei, since
> table 5.2 in the spec does not explicitly classify it. However, I
> believe it is safe to assume that exception 10 is non-delegable.
Ah, I see.
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Applied to riscv-to-apply.next
I improved the indentation and rebased this on the latest master.
Alistair
>
> José
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target/riscv: hardwire bits in hideleg and hedeleg
@ 2021-07-08 4:45 ` Alistair Francis
0 siblings, 0 replies; 16+ messages in thread
From: Alistair Francis @ 2021-07-08 4:45 UTC (permalink / raw)
To: Jose Martins
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V TCG CPUs,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt
On Thu, Jun 24, 2021 at 11:48 PM Jose Martins <josemartins90@gmail.com> wrote:
>
> > > +static const target_ulong vs_delegable_excps = delegable_excps &
> > > + ~((1ULL << (RISCV_EXCP_S_ECALL)) |
> >
> > > + (1ULL << (RISCV_EXCP_VS_ECALL)) |
> > > + (1ULL << (RISCV_EXCP_M_ECALL)) |
> >
> > These two are both read only 0, shouldn't they not be included in this list?
> >
> > > static int write_hedeleg(CPURISCVState *env, int csrno, target_ulong val)
> > > {
> > > - env->hedeleg = val;
> > > + env->hedeleg = val & vs_delegable_excps;
> >
> > Because we then allow a write to occur here.
>
> Note that the list is being bitwise negated, so both of these are
> actually not writable (ie read-only 0). There is still the question
> regarding the VS_ECALL (exception 10) bit raised by Zhiwei, since
> table 5.2 in the spec does not explicitly classify it. However, I
> believe it is safe to assume that exception 10 is non-delegable.
Ah, I see.
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Applied to riscv-to-apply.next
I improved the indentation and rebased this on the latest master.
Alistair
>
> José
^ permalink raw reply [flat|nested] 16+ messages in thread