Re: [Xen-devel] [PATCH v5 05/16] livepatch: Initial ARM64 support.

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 21/09/16 18:32, Konrad Rzeszutek Wilk wrote:

[...]

> diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
> index f2ae52a..ff2cfb8 100644
> --- a/docs/misc/livepatch.markdown
> +++ b/docs/misc/livepatch.markdown
> @@ -1108,7 +1108,7 @@ and no .data or .bss sections.
>  The hypervisor should verify that the in-place patching would fit within
>  the code or data.
>
> -### Trampoline (e9 opcode)
> +### Trampoline (e9 opcode), x86
>
>  The e9 opcode used for jmpq uses a 32-bit signed displacement. That means
>  we are limited to up to 2GB of virtual address to place the new code
> @@ -1143,3 +1143,15 @@ that in the hypervisor is advised.
>  The tool for generating payloads currently does perform a compile-time
>  check to ensure that the function to be replaced is large enough.
>
> +#### Trampoline (ea opcode), ARM
Please drop "(ea opcode)" as it is only valid for ARM32.

> +
> +The unconditional branch instruction (for the encoding see the
> +DDI 0406C.c and DDI 0487A.j Architecture Reference Manual's).
> +with proper offset is used for an unconditional branch to the new code.
> +This means that that `old_size` **MUST** be at least four bytes if patching
> +in trampoline.
> +
> +The new code is placed in the 8M - 10M virtual address space while the
> +Xen code is in 2M - 4M. That gives us enough space.
> +
> +The hypervisor also checks the displacement during loading of the payload.
[...]

> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> new file mode 100644
> index 0000000..7cb1812
> --- /dev/null
> +++ b/xen/arch/arm/arm64/livepatch.c
[...]

> +void arch_livepatch_apply(struct livepatch_func *func)
> +{
> +    uint32_t insn;
> +    uint32_t *new_ptr;
> +    unsigned int i, len;
> +
> +    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
> +    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
> +
> +    ASSERT(vmap_of_xen_text);
> +
> +    len = livepatch_insn_len(func);
> +    if ( !len )
> +        return;
> +
> +    /* Save old ones. */
> +    memcpy(func->opaque, func->old_addr, len);
> +
> +    if ( func->new_addr )
> +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> +                                           (unsigned long)func->new_addr,
> +                                           AARCH64_INSN_BRANCH_NOLINK);
> +    else
> +        insn = aarch64_insn_gen_nop();
> +
> +    ASSERT(insn != AARCH64_BREAK_FAULT);
> +
> +    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    len = len / sizeof(uint32_t);
> +
> +    /* PATCH! */
> +    for ( i = 0; i < len; i++ )
> +        *(new_ptr + i) = insn;
> +
> +    /*
> +    * When we upload the payload, it will go through the data cache
> +    * (the region is cacheable). Until the data cache is cleaned, the data
> +    * may not reach the memory. And in the case the data and instruction cache
> +    * are separated, we may read invalid instruction from the memory because
> +    * the data cache have not yet synced with the memory. Hence sync it.
> +    */
> +    if ( func->new_addr )
> +        clean_and_invalidate_dcache_va_range(func->new_addr, func->new_size);
Why did you drop the clean_and_invalidate_dcache_va_range to the 
instruction just patched?
I guess it is because my mail in the previous thread was confusing, 
sorry for that.
"> 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.
"

I meant that it is not necessary to flush the whole region of the old 
code (Xen already flushed it at boot time), but only the region you 
patched. The interesting bit in the ARM ARM is B2.3.4 DDI 0487A.j.
So the code should look like:

if ( func->new_addr )
   clean_and_invalidate_dcache_va_range(func->new_addr, func->new_size);
clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) * len);


> +}
> +
> +void arch_livepatch_revert(const struct livepatch_func *func)
> +{
> +    uint32_t *new_ptr;
> +    unsigned int i, len;
> +
> +    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +    len = livepatch_insn_len(func) / sizeof(uint32_t);
> +    for ( i = 0; i < len; i++ )
> +    {
> +        uint32_t insn;
> +
> +        memcpy(&insn, func->opaque + (i * sizeof(uint32_t)), 
> ARCH_PATCH_INSN_SIZE);
> +        /* PATCH! */
> +        *(new_ptr + i) = insn;
> +    }
Similarly:

clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) * len);



> +}
> +
[...]

> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 7f067a0..9284766 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
[...]

>  void arch_livepatch_revive(void)
>  {
> +    /*
> +     * Nuke the instruction cache. Data cache has been cleaned before in
> +     * arch_livepatch_apply.
NIT: Would it be worth to mention arch_livepatch_revert here?

> +     */
> +    invalidate_icache();
> +
> +    if ( vmap_of_xen_text )
> +        vunmap(vmap_of_xen_text);
> +
> +    vmap_of_xen_text = NULL;
>  }
Regards,

--
Julien Grall

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