[PATCH] arm64/alternatives: use subsections for replacement sequences

[Alexandru Elisei]

Hi Ard,

Thank you so much for your quick reply!

On 7/9/20 1:39 PM, Ard Biesheuvel wrote:
> On Thu, 9 Jul 2020 at 15:31, Ard Biesheuvel <ardb at kernel.org> wrote:
>> On Thu, 9 Jul 2020 at 14:11, Alexandru Elisei <alexandru.elisei at arm.com> wrote:
>>> Hi,
>>>
>> Hi Alex,
>>
>> Apologies for the breakage.
>>
>>
>>> I should post the entire boot log:
>> ...
>>> [    0.002204] pc : work_pending+0x32c/0x348
>> This suggests that you ended executing directly from the alternatives
>> replacement section that gets appended to the end of work_pending (and
>> therefore gets mistaken for being part of it)
>>
>> It appears that the following code in alternatives.c
>>
>> static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
>> {
>>     unsigned long replptr;
>>
>>     if (kernel_text_address(pc))
>>        return true;
>>
>> returns true inadvertently for the branch in this piece of code in entry.S
>>
>> alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>>     ldr x20, [sp, #S_PMR_SAVE]
>>     msr_s SYS_ICC_PMR_EL1, x20
>>     mrs_s x21, SYS_ICC_CTLR_EL1
>>     tbz x21, #6, .L__skip_pmr_sync\@ // Check for ICC_CTLR_EL1.PMHE
>>     dsb sy // Ensure priority change is seen by redistributor
>> .L__skip_pmr_sync\@:
>>
>>
>> due to the fact that kernel_text_address() has no way of
>> distinguishing branches inside the subsection from branches that
>> require updating. So the alternatives patching code dutifully updates
>> the tbz opcode and points it to its original target in the subsection.
>>
>> This is going to be rather tricky to fix, unless we special case
>> tbz/cbz branches and other branches with limited range that would
>> never have worked before anyway.
>>
>> For now, better to just revert it and revisit it later.
>>
> ... unless we decide to fix up all branches pointing outside the
> replacement sequence, which is not an entirely unreasonable thing to
> do:
>
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index d1757ef1b1e7..7c205f9202a3 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -45,18 +45,11 @@
>  {
>         unsigned long replptr;
>
> -       if (kernel_text_address(pc))
> -               return true;
> -
>         replptr = (unsigned long)ALT_REPL_PTR(alt);
>         if (pc >= replptr && pc <= (replptr + alt->alt_len))
>                 return false;
>
> -       /*
> -        * Branching into *another* alternate sequence is doomed, and
> -        * we're not even trying to fix it up.
> -        */
> -       BUG();
> +       return true;
>  }

Both fixes work for me. I've been running some tests on my rockpro64 with your
patch reverted, so that definitely fixes the issue. With the above diff applied, I
was able to boot and run some PMU tests using NMIs.

Thanks,
Alex