Re: [Xen-devel] [PATCH v4 04/16] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> 
> >  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:

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);
 }

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel