Richard Henderson <richard.hender...@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
