qemu-riscv
[Top][All Lists]
Advanced

[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: weiwei
Subject: Re: [PATCH v3 7/9] target/riscv: add support for Zcmt extension
Date: Fri, 18 Nov 2022 09:46:20 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2


On 2022/11/18 04:57, Richard Henderson wrote:
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:


Yeah. This patchset is not yet upstream but have been added to riscv-to-apply.next.  I also suggested  similar way

 in this patchset at the beginning. However, to some extent, JVT and FCSR in statenen CSR are  used to enable/disable

Zfinx and Zcmt extensions.  When they are disabled, It seems reasonable to look for another insn, just like the

processor doesn't support them at all.

From the other aspect, is it possible that we support many overlapping extensions(such as Zcmt and Zcd or CD) in one

processor  and only one work once (just disable anothers if we need another to work)?


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.

Yeah. I didn't realize this question. I think we can use a specific helper to do this check, since  tb_flags may not be  big enough to catch all the information of xStateen csr.


r~




reply via email to

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