From: Richard Henderson <richard.henderson@linaro.org>
To: Alistair Francis <alistair.francis@opensource.wdc.com>,
qemu-devel@nongnu.org, qemu-riscv@nongnu.org
Cc: palmer@dabbelt.com, Bin Meng <bin.meng@windriver.com>,
Alistair Francis <alistair.francis@wdc.com>,
bmeng.cn@gmail.com, alistair23@gmail.com
Subject: Re: [PATCH 2/2] target/riscv: Implement the stval/mtval illegal instruction
Date: Fri, 10 Dec 2021 07:11:53 -0800 [thread overview]
Message-ID: <56a5e33d-bf0f-f588-d288-a302b5631fb4@linaro.org> (raw)
In-Reply-To: <20211210062638.824672-3-alistair.francis@opensource.wdc.com>
On 12/9/21 10:26 PM, Alistair Francis wrote:
> @@ -975,7 +975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG);
> target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
> target_ulong deleg = async ? env->mideleg : env->medeleg;
> - bool write_tval = false;
> target_ulong tval = 0;
> target_ulong htval = 0;
> target_ulong mtval2 = 0;
> @@ -1004,9 +1003,11 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> case RISCV_EXCP_INST_PAGE_FAULT:
> case RISCV_EXCP_LOAD_PAGE_FAULT:
> case RISCV_EXCP_STORE_PAGE_FAULT:
> - write_tval = true;
> tval = env->badaddr;
> break;
...
> @@ -1041,17 +1042,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> if (riscv_has_ext(env, RVH)) {
> target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
>
> - if (env->two_stage_lookup && write_tval) {
> - /*
> - * If we are writing a guest virtual address to stval, set
> - * this to 1. If we are trapping to VS we will set this to 0
> - * later.
> - */
> - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> - } else {
> - /* For other HS-mode traps, we set this to 0. */
> - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> - }
> + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
>
> if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
> /* Trap to VS mode */
> @@ -1063,7 +1054,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> cause == IRQ_VS_EXT) {
> cause = cause - 1;
> }
> - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> } else if (riscv_cpu_virt_enabled(env)) {
> /* Trap into HS mode, from virt */
> riscv_cpu_swap_hypervisor_regs(env);
> @@ -1071,6 +1061,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> env->priv);
> env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
> riscv_cpu_virt_enabled(env));
> + if (tval) {
> + /*
> + * If we are writing a guest virtual address to stval, set
> + * this to 1.
> + */
> + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
> + }
The thing that concerns me here is the very legitimate badaddr of NULL. I think you need
to keep some out-of-band flag for that.
In addition, the out-of-band flag should probably be called write_gva, because tval from
ILLEGAL_INST is not a Guest Virtual Address, and so should not set GVA.
You could even push the setting of the bit down so that it's only done once, e.g.
hstatus_gva = true;
tval = env->badaddr;
if (trap to vs) {
...
hstatus_gva = false;
} else if (virt enabled) {
...
} else {
trap into hs
hstatus_gva = false;
}
env->hstatus(set_field(env->hstatus, HSTATUS_GVA, hstatus_gva);
The actual handling of ILLEGAL_INST looks fine.
You may well want to split the GVA handling to a separate patch.
r~
prev parent reply other threads:[~2021-12-10 15:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-10 6:26 [PATCH 0/2] RISC-V: Populate mtval and stval Alistair Francis
2021-12-10 6:26 ` [PATCH 1/2] target/riscv: Set the opcode in DisasContext Alistair Francis
2021-12-10 15:28 ` Richard Henderson
2021-12-10 6:26 ` [PATCH 2/2] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis
2021-12-10 15:11 ` Richard Henderson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56a5e33d-bf0f-f588-d288-a302b5631fb4@linaro.org \
--to=richard.henderson@linaro.org \
--cc=alistair.francis@opensource.wdc.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=bin.meng@windriver.com \
--cc=bmeng.cn@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).