[v4,04/16] livepatch: Initial ARM64 support.

Commit Message

Konrad Rzeszutek Wilk Sept. 19, 2016, 2:33 p.m. UTC

> 
> >  void arch_livepatch_revive(void)
> >  {
> > +    /*
> > +     * Nuke the instruction cache. Data cache has been cleaned before in
> > +     * arch_livepatch_apply_jmp.
> 
> I think you forgot to clean text region from the payload. Without that, you
> may receive a crash if you have a separate cache for data and instruction.

Help me out here please.

Why would we need to call clean_and_invalidate_dcache_va_range on the
payload .text area (the func->new_addr and func->new_size)?

We don't modify that .text area and after this function is done
(arch_livepatch_revive) it would be very first time that code would be called.

Hence there would not be any cache remains at all? 

Or did you mean the old_addr (the one we just patched?)

If we are reverting it then we just clear at func->old_addr for one
instruction? We could drop the dcache for the func->new_addr (so new
.text code), to explicitly tell the CPU to not waste cache space for
those instructions? Is that what you meant?

Anyhow did this:

And added the invalidation of dcache at old_addr (so the function we
patched).

Comments

Julien Grall Sept. 20, 2016, 9:40 a.m. UTC | #1

Hi Konrad,

On 19/09/2016 16:33, Konrad Rzeszutek Wilk wrote:
>>
>>>  void arch_livepatch_revive(void)
>>>  {
>>> +    /*
>>> +     * Nuke the instruction cache. Data cache has been cleaned before in
>>> +     * arch_livepatch_apply_jmp.
>>
>> I think you forgot to clean text region from the payload. Without that, you
>> may receive a crash if you have a separate cache for data and instruction.
>
> Help me out here please.
>
> Why would we need to call clean_and_invalidate_dcache_va_range on the
> payload .text area (the func->new_addr and func->new_size)?
>
> We don't modify that .text area and after this function is done
> (arch_livepatch_revive) it would be very first time that code would be called.
>
> Hence there would not be any cache remains at all?

Because when you copy the payload to the memory, it will go through the 
data cache (the region is cacheable). So until the data cache is 
cleaned, the data may not reach the memory.

In the case the data and instruction cache are separated, you may read 
invalid instruction from the memory because the data cache have not yet 
synced with the memory.

>
> Or did you mean the old_addr (the one we just patched?)

We don't care about the previous function. It will never changed except 
for the instruction patched.

>
> If we are reverting it then we just clear at func->old_addr for one
> instruction? We could drop the dcache for the func->new_addr (so new
> .text code), to explicitly tell the CPU to not waste cache space for
> those instructions? Is that what you meant?

The data cache should not contain any cache line of the previous 
function because it only contains instructions. However the instructions 
cache will contain some of them, but they will be removed by the 
invalidate cache instruction in arch_live_revive.

Regards,

Patch

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 49eb69b..07f0ce7 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -49,7 +49,10 @@  void arch_livepatch_apply_jmp(struct livepatch_func *func)
     for ( i = 0; i < len; i++ )
         *(new_ptr + i) = insn;
 
+    /* There should not be _any_ aliasing using vmap's, but just in case. */
     clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) * len);
+    /* And definitly clear the old code. */
+    clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr) * len);
 }
 
 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
@@ -68,6 +71,9 @@  void arch_livepatch_revert_jmp(const struct livepatch_func *func)
         *(new_ptr + i) = insn;
     }
 
+    /* There should not be _any_ aliasing using vmap's, but just in case. */
+    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) * len);
+    /* And definitly clear the old code. */
     clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr) * len);
 }