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

[Richard Henderson]

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?


r~