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~