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

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
snip
> +
> +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;
> +    }

Why is this done in a loop? Can't we just memcpy 
livepatch_insn_len(func) bytes into *new_ptr?

> +}
> +
> +int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> +{
> +    const Elf_Ehdr *hdr = elf->hdr;
> +
> +    if ( hdr->e_machine != EM_AARCH64 ||
> +         hdr->e_ident[EI_CLASS] != ELFCLASS64 )
> +    {
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
> +                elf->name);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +
snip
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> -    return -ENOSYS;
> -}
> +    /* If NOPing only do up to maximum amount we can put in the ->opaque. */
> +    if ( !func->new_addr && func->new_size > sizeof(func->opaque) &&
> +         func->new_size % ARCH_PATCH_INSN_SIZE )
> +        return -EOPNOTSUPP;

Maybe I'm misunderstanding, but shouldn't this be ( !func->new_addr && 
(func->new_size > sizeof(func->opaque) || func->new_size % 
ARCH_PATCH_INSN_SIZE) ) ?

-- 
Ross Lagerwall

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