Hi folks,
I've sent new v2 patches where, I hope, I managed to address all the
comments and suggestions that were provided.
Thanks!
ср, 14 окт. 2020 г. в 23:10, Alexey Baturo <baturo.ale...@gmail.com>:
> > I suggest adding a stub version of this function to patch 5, and then
> swap patch 4 and patch 5.
> Thanks, will do.
>
> >This bakes in values from ENV without adding any way to verify that those
> values are still current.
> If I correctly get your idea, you're talking about the situation, when
> DisasContext was initialized with some values, which at some point got
> changed, so this could lead to incorrect address masking. I tried to handle
> this situation by dropping the translation cache in case different values
> are written in any of the PM CSRs, which I assumed would lead to refilling
> the DIsasContext structure.
> This is obviously not the best way to do it, since it may lead to
> performance degradation in some cases, so let me process your suggestion
> and try to implement it.
>
> Thanks!
>
> ср, 14 окт. 2020 г. в 22:19, Richard Henderson <
> richard.hender...@linaro.org>:
>
>> On 10/14/20 10:01 AM, Alexey Baturo wrote:
>> > +static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
>> > +{
>> > + gen_pm_adjust_address(s, addr, addr);
>> > + return addr;
>> > +}
>>
>> This function is unused in this patch, which means the series as a whole
>> is
>> non-bisectable.
>>
>> Rather than merge the two, I suggest adding a stub version of this
>> function to
>> patch 5, and then swap patch 4 and patch 5. So you will add uses of
>> apply_pointer_masking without actually implementing it yet. Which should
>> be fine.
>>
>> > @@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context
>> > } else {
>> > ctx->virt_enabled = false;
>> > }
>> > + if (riscv_has_ext(env, RVJ)) {
>> > + switch (env->priv) {
>> > + case PRV_U:
>> > + ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
>> > + ctx->pm_mask = env->upmmask;
>> > + ctx->pm_base = env->upmbase;
>> > + break;
>> > + case PRV_S:
>> > + ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
>> > + ctx->pm_mask = env->spmmask;
>> > + ctx->pm_base = env->spmbase;
>> > + break;
>> > + case PRV_M:
>> > + ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
>> > + ctx->pm_mask = env->mpmmask;
>>
>> You can't read env like this.
>>
>> This bakes in values from ENV without adding any way to verify that those
>> values are still current.
>>
>> The one thing that you must bake into the generated code is the state of
>> PM_ENABLE. Anything else would penalize normal risc-v emulation.
>>
>> You do that in cpu_get_tb_cpu_state(). Allocate one bit to hold
>> the current state of the flag. E.g.
>>
>> FIELD(TB_FLAGS, PM_ENABLED, 9, 1)
>>
>> then fill it in from the correct mmte bit for priv (which itself is
>> encoded by
>> cpu_mmu_index()).
>>
>> Except for special cases, the mask and base variables cannot be placed
>> into
>> TB_FLAGS. For now, I think it's best to ignore the special cases and
>> implement
>> them all as tcg globals. Which means that we do *not* bake in a
>> particular
>> value, but instead read the value from env at runtime.
>>
>> So, in riscv_translate_init, you create new globals for each of the mask
>> and
>> base. In riscv_tr_init_disas_context you examine priv (via mmu_index) and
>> assign one pair of the globals to DisasContext, so that you don't have to
>> keep
>> looking them up.
>>
>> Then you have
>>
>> static void gen_pm_adjust_address(DisasContext *s,
>> TCGv_i64 dst,
>> TCGv_i64 src)
>> {
>> if (s->pm_enabled == 0) {
>> /* Load unmodified address */
>> tcg_gen_mov_i64(dst, src);
>> } else {
>> tcg_gen_andc_i64(dst, src, s->pm_mask);
>> tcg_gen_or_i64(dst, dst, s->pm_base);
>> }
>> }
>>
>>
>> r~
>>
>
