Re: [Qemu-devel] [Qemu-arm] [PATCH v5 02/17] target/arm: Split out rebuild_hflags_a64

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 9/5/19 11:28 AM, Alex Bennée wrote:
>>> -
>>> -        if (cpu_isar_feature(aa64_bti, cpu)) {
>>> -            /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
>>> -            if (sctlr & (current_el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
>>> -                flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
>>> -            }
>>> +        flags = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);
>>> +        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {
>>>              flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
>>
>> It seems off to only hoist part of the BTI flag check into the helper,
>> was it just missed or is there a reason? If so it could probably do with
>> an additional comment.
>
> The part of the bti stuff that is hoisted is solely based on system registers.
>  The BTYPE field is in PSTATE and is a very different kind of animal -- in
> particular, it is not set by MSR.
>
> But also, comments in cpu.h say which fields are (not) cached in hflags, and
> BTYPE is so documented.
>
> Is your proposed comment really helpful here going forward, or do you just
> think it's weird reviewing this patch, since not all BTI is treated the same
> after the patch?

It was just weird seeing the isar_feature test twice. A mention in the
commit "not all bti related flags will be cached so we have to test the
feature twice" or something like that will suffice.

>
>
> r~


--
Alex Bennée