[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v5 02/17] target/arm: Split out rebui
From: |
Alex Bennée |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v5 02/17] target/arm: Split out rebuild_hflags_a64 |
Date: |
Fri, 06 Sep 2019 16:52:23 +0100 |
User-agent: |
mu4e 1.3.4; emacs 27.0.50 |
Richard Henderson <address@hidden> 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