Hi Steve,
On 19/12/17 13:04, Steve Capper wrote:
> Hi Marc,
>
> On Mon, Dec 18, 2017 at 05:39:12PM +0000, Marc Zyngier wrote:
>> We've so far relied on a patching infrastructure that only gave us
>> a single alternative, without any way to finely control what gets
>> patched. For a single feature, this is an all or nothing thing.
>>
>> It would be interesting to have a more fine grained way of patching
>> the kernel though, where we could dynamically tune the code that gets
>> injected.
>>
>> In order to achive this, let's introduce a new form of alternative
>> that is associated with a callback. This callback gets the instruction
>> sequence number and the old instruction as a parameter, and returns
>> the new instruction. This callback is always called, as the patching
>> decision is now done at runtime (not patching is equivalent to returning
>> the same instruction).
>>
>> Patching with a callback is declared with the new ALTERNATIVE_CB
>> and alternative_cb directives:
>>
>> asm volatile(ALTERNATIVE_CB("mov %0, #0\n", callback)
>> : "r" (v));
>> or
>> alternative_cb callback
>> mov x0, #0
>> alternative_cb_end
>>
>> where callback is the C function computing the alternative.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/include/asm/alternative.h | 36 ++++++++++++++++++++++++++----
>> arch/arm64/include/asm/alternative_types.h | 3 +++
>> arch/arm64/kernel/alternative.c | 21 +++++++++++++----
>> 3 files changed, 52 insertions(+), 8 deletions(-)
>>
>
> [...]
>
>> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
>> index 6dd0a3a3e5c9..cd299af96c95 100644
>> --- a/arch/arm64/kernel/alternative.c
>> +++ b/arch/arm64/kernel/alternative.c
>> @@ -110,25 +110,38 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>> struct alt_instr *alt;
>> struct alt_region *region = alt_region;
>> __le32 *origptr, *replptr, *updptr;
>> + alternative_cb_t alt_cb;
>>
>> for (alt = region->begin; alt < region->end; alt++) {
>> u32 insn;
>> int i, nr_inst;
>>
>> - if (!cpus_have_cap(alt->cpufeature))
>> + /* Use ARM64_NCAPS as an unconditional patch */
>> + if (alt->cpufeature < ARM64_NCAPS &&
>> + !cpus_have_cap(alt->cpufeature))
>> continue;
>>
>> - BUG_ON(alt->alt_len != alt->orig_len);
>> + if (alt->cpufeature == ARM64_NCAPS)
>> + BUG_ON(alt->alt_len != 0);
>> + else
>> + BUG_ON(alt->alt_len != alt->orig_len);
>>
>> pr_info_once("patching kernel code\n");
>>
>> origptr = ALT_ORIG_PTR(alt);
>> replptr = ALT_REPL_PTR(alt);
>> + alt_cb = ALT_REPL_PTR(alt);
>> updptr = use_linear_alias ? lm_alias(origptr) : origptr;
>> - nr_inst = alt->alt_len / sizeof(insn);
>> + nr_inst = alt->orig_len / sizeof(insn);
>>
>> for (i = 0; i < nr_inst; i++) {
>> - insn = get_alt_insn(alt, origptr + i, replptr + i);
>> + if (alt->cpufeature == ARM64_NCAPS) {
>> + insn = le32_to_cpu(updptr[i]);
>> + insn = alt_cb(alt, i, insn);
>> + } else {
>> + insn = get_alt_insn(alt, origptr + i,
>> + replptr + i);
>> + }
>> updptr[i] = cpu_to_le32(insn);
>> }
>
> Is it possible to call the callback only once per entry (rather than
> once per instruction)? That would allow one to retain some more
> execution state in the callback, which may be handy if things get more
> elaborate.
Yeah, it was something that Catalin suggested too. I guess the only
thing that really annoys me about that is that we'd let the callback do
the write to the kernel text, which I find a bit... meh.
But overall I agree that it would be more useful, and make the loop a
bit less ugly.
I'll work something out for the next round!
Thanks,
M.
--
Jazz is not dead. It just smells funny...
