[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 7/9] target/riscv: add support for Zcmt extension
From: |
Richard Henderson |
Subject: |
Re: [PATCH v3 7/9] target/riscv: add support for Zcmt extension |
Date: |
Thu, 17 Nov 2022 12:57:45 -0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 |
On 11/17/22 03:44, weiwei wrote:
Missing a smstateen_check. Not mentioned in the instruction description itself, but it
is within the State Enable section of JVT.
smstateen_check have been added in REQUIRE_ZCMT.
Oh. I see. That's wrong, I think.
Returning false from trans_* means "no match" and continue on to try and match another
pattern. If Zcmt is present in the cpu, but the extension is not enabled by the OS, we
have found the matching insn and should not look for another insn.
You need to separate the check like
REQUIRE_ZCMT(ctx);
if (!smstateen_check(ctx, 0, SMTATEEN0_JVT)) {
gen_exception_illegal(ctx);
return true;
}
I see that the fpcr code that you're modifying in this patch, which is not yet upstream,
is also incorrect in this.
Looking back through your git history,
https://github.com/plctlab/plct-qemu/commit/09668167880c492f88b74d0a921053ed25fc3b5c
is incorrect:
static inline bool smstateen_fcsr_check(DisasContext *ctx, int index)
{
CPUState *cpu = ctx->cs;
CPURISCVState *env = cpu->env_ptr;
uint64_t stateen = env->mstateen[index];
You cannot read from env during translation like this.
Everything that you use for translation must be encoded in tb->flags. Otherwise the state
will not be considered when selecting an existing TranslationBlock to execute, and the
next execution through this instruction will not have the *current* state of env.
You probably get lucky with mstateen, because I could imagine that it gets set once while
the OS is booting and is never changed again. If true, then mstateen chould be treated
like misa and flush all translations on write: see write_misa(). And also add a large
comment to smstateen_check() explaining why the read from env is correct.
But if that "set once" assumption is not true, and mstateen is more like mstatus.fs, where
a given extension is enabled/disabled often for lazy migration of state, then you won't
want to continually flush translations.
r~
- [PATCH v3 5/9] target/riscv: add support for Zcb extension, (continued)
- [PATCH v3 5/9] target/riscv: add support for Zcb extension, Weiwei Li, 2022/11/17
- [PATCH v3 1/9] target/riscv: add cfg properties for Zc* extension, Weiwei Li, 2022/11/17
- [PATCH v3 4/9] target/riscv: add support for Zcd extension, Weiwei Li, 2022/11/17
- [PATCH v3 3/9] target/riscv: add support for Zcf extension, Weiwei Li, 2022/11/17
- [PATCH v3 7/9] target/riscv: add support for Zcmt extension, Weiwei Li, 2022/11/17
[PATCH v3 9/9] disas/riscv.c: add disasm support for Zc*, Weiwei Li, 2022/11/17
[PATCH v3 2/9] target/riscv: add support for Zca extension, Weiwei Li, 2022/11/17
[PATCH v3 8/9] target/riscv: expose properties for Zc* extension, Weiwei Li, 2022/11/17
[PATCH v3 6/9] target/riscv: add support for Zcmp extension, Weiwei Li, 2022/11/17