[Top][All Lists]

[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


    if (!smstateen_check(ctx, 0, SMTATEEN0_JVT)) {
        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,


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]