From: Alistair Francis <alistair23@gmail.com>
To: Alexei Filippov <alexei.filippov@syntacore.com>
Cc: palmer@dabbelt.com, alistair.francis@wdc.com,
bin.meng@windriver.com, liwei1518@gmail.com,
dbarboza@ventanamicro.com, zhiwei_liu@linux.alibaba.com,
qemu-riscv@nongnu.org, qemu-devel@nongnu.org,
Joseph Chan <jchan@ventanamicro.com>
Subject: Re: [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception()
Date: Tue, 14 May 2024 15:48:57 +1000 [thread overview]
Message-ID: <CAKmqyKNYFyVEOsRyOQCjZBMO_-Wut+k2Bn_5ep_uZdFi-5O-jQ@mail.gmail.com> (raw)
In-Reply-To: <20240413105929.7030-1-alexei.filippov@syntacore.com>
On Sat, Apr 13, 2024 at 9:00 PM Alexei Filippov
<alexei.filippov@syntacore.com> wrote:
>
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
> raise_mmu_exception(), as is today, is prioritizing guest page faults by
> checking first if virt_enabled && !first_stage, and then considering the
> regular inst/load/store faults.
>
> There's no mention in the spec about guest page fault being a higher
> priority that PMP faults. In fact, privileged spec section 3.7.1 says:
>
> "Attempting to fetch an instruction from a PMP region that does not have
> execute permissions raises an instruction access-fault exception.
> Attempting to execute a load or load-reserved instruction which accesses
> a physical address within a PMP region without read permissions raises a
> load access-fault exception. Attempting to execute a store,
> store-conditional, or AMO instruction which accesses a physical address
> within a PMP region without write permissions raises a store
> access-fault exception."
>
> So, in fact, we're doing it wrong - PMP faults should always be thrown,
> regardless of also being a first or second stage fault.
>
> The way riscv_cpu_tlb_fill() and get_physical_address() work is
> adequate: a TRANSLATE_PMP_FAIL error is immediately reported and
> reflected in the 'pmp_violation' flag. What we need is to change
> raise_mmu_exception() to prioritize it.
>
> Reported-by: Joseph Chan <jchan@ventanamicro.com>
> Fixes: 82d53adfbb ("target/riscv/cpu_helper.c: Invalid exception on MMU translation stage")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_helper.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index bc70ab5abc..196166f8dd 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1203,28 +1203,30 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
>
> switch (access_type) {
> case MMU_INST_FETCH:
> - if (env->virt_enabled && !first_stage) {
> + if (pmp_violation) {
> + cs->exception_index = RISCV_EXCP_INST_ACCESS_FAULT;
> + } else if (env->virt_enabled && !first_stage) {
> cs->exception_index = RISCV_EXCP_INST_GUEST_PAGE_FAULT;
> } else {
> - cs->exception_index = pmp_violation ?
> - RISCV_EXCP_INST_ACCESS_FAULT : RISCV_EXCP_INST_PAGE_FAULT;
> + cs->exception_index = RISCV_EXCP_INST_PAGE_FAULT;
> }
> break;
> case MMU_DATA_LOAD:
> - if (two_stage && !first_stage) {
> + if (pmp_violation) {
> + cs->exception_index = RISCV_EXCP_LOAD_ACCESS_FAULT;
> + } else if (two_stage && !first_stage) {
> cs->exception_index = RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT;
> } else {
> - cs->exception_index = pmp_violation ?
> - RISCV_EXCP_LOAD_ACCESS_FAULT : RISCV_EXCP_LOAD_PAGE_FAULT;
> + cs->exception_index = RISCV_EXCP_LOAD_PAGE_FAULT;
> }
> break;
> case MMU_DATA_STORE:
> - if (two_stage && !first_stage) {
> + if (pmp_violation) {
> + cs->exception_index = RISCV_EXCP_STORE_AMO_ACCESS_FAULT;
> + } else if (two_stage && !first_stage) {
> cs->exception_index = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT;
> } else {
> - cs->exception_index = pmp_violation ?
> - RISCV_EXCP_STORE_AMO_ACCESS_FAULT :
> - RISCV_EXCP_STORE_PAGE_FAULT;
> + cs->exception_index = RISCV_EXCP_STORE_PAGE_FAULT;
> }
> break;
> default:
> --
> 2.34.1
>
>
next prev parent reply other threads:[~2024-05-14 5:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-13 10:59 [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Alexei Filippov
2024-04-13 10:59 ` [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults Alexei Filippov
2024-04-29 9:43 ` Daniel Henrique Barboza
2024-05-03 10:30 ` [PATCH v2 " Alexei Filippov
2024-05-14 5:59 ` Alistair Francis
2024-05-14 6:16 ` Alistair Francis
2024-04-15 18:50 ` [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Joseph Chan
2024-04-16 3:14 ` Joseph Chan
2024-05-14 5:48 ` Alistair Francis [this message]
2024-05-27 12:11 ` [PATCH v2 " Alexei Filippov
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=CAKmqyKNYFyVEOsRyOQCjZBMO_-Wut+k2Bn_5ep_uZdFi-5O-jQ@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=alexei.filippov@syntacore.com \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=dbarboza@ventanamicro.com \
--cc=jchan@ventanamicro.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).