Re: [Xen-devel] [PATCH v2 14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Those symbols are used to help final linkers to replace insn.
> The ARM ELF specification mandates that they are present
> to denote the start of certain CPU features. There are two
> variants of it - short and long format.
>
> Either way - we can ignore these symbols.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> v1: First submission
> v2: Update the order of symbols, fix title
>     Add {} in after the first if - per Jan's recommendation.
> ---
>  xen/arch/arm/livepatch.c    | 32 ++++++++++++++++++++++++++++++++
>  xen/arch/x86/livepatch.c    |  7 +++++++
>  xen/common/livepatch.c      |  2 +-
>  xen/include/xen/livepatch.h |  2 ++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index f49e347..c290602 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -82,6 +82,38 @@ void arch_livepatch_unmask(void)
>      local_abort_enable();
>  }
>
> +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> +                           const struct livepatch_elf_sym *sym)
I think this function should return bool (or bool_t) as the return will 
be used by is_payload_symbol as bool.
> +{
> +    /*
> +     * - Mapping symbols - denote the "start of a sequence of bytes of the
> +     *   appropiate type" to mark certain features - such as start of region
s/appropiate/appropriate/

> +     *   containing data ($d); ARM ($a), A64 ($x), or Thumb instructions ($t).
> +     *
> +     * The format is either short: '$x' or long: '$x.<any>'. We do not
> +     * need this and more importantly - each payload will contain this
> +     * resulting in symbol collisions.
> +     */
> +    if ( *sym->name == '$' && sym->name[1] != '\0' )
> +    {
> +        char p = sym->name[1];
> +        size_t len = strlen(sym->name);
> +
> +        if ( (len >= 3 && ( sym->name[2] == '.' )) || (len == 2) )
> +        {
> +            if ( p == 'd' ||
> +#ifdef CONFIG_ARM_32
> +                 p == 'a' || p == 't'
Note that Xen is not using Thumb instructions which have variable 
length, so we shouldn't expect to see $t. symbols.
> +#else
> +                 p == 'x'
> +#endif
> +               )
> +                return 0;
Please use false here.

> +        }
> +    }
> +    return 1;
Please use true here.

> +}
> +
Regards,

--
Julien Grall

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