Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Aug 15, 2016 at 02:21:48AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -222,7 +222,7 @@ endmenu
> >  config LIVEPATCH
> >  	bool "Live patching support (TECH PREVIEW)"
> >  	default n
> > -	depends on X86 && HAS_BUILD_ID = "y"
> > +	depends on (X86 || ARM_64) && HAS_BUILD_ID = "y"
> 
> Would this better become a black list?

OK, that would make it easier.
> 
> > @@ -711,9 +711,15 @@ static int prepare_payload(struct payload *payload,
> >                  return -EINVAL;
> >              }
> >          }
> > +#ifndef CONFIG_ARM
> >          apply_alternatives_nocheck(start, end);
> > +#else
> > +        apply_alternatives(start, sec->sec->sh_size);
> > +#endif
> 
> Conditionals like this are ugly - can't this be properly abstracted?

Yes, I can introduce an apply_alternatives_nocheck on ARM that will
hava the same set of arguments on x86.

Or I can make a new function name?
> 
> > --- a/xen/include/xen/elfstructs.h
> > +++ b/xen/include/xen/elfstructs.h
> > @@ -103,6 +103,15 @@ typedef uint64_t	Elf64_Xword;
> >                        (ehdr).e_ident[EI_MAG2] == ELFMAG2 && \
> >                        (ehdr).e_ident[EI_MAG3] == ELFMAG3)
> >  
> > +/* e_flags */
> > +#define EF_ARM_EABI_MASK	0xff000000
> > +#define EF_ARM_EABI_UNKNOWN	0x00000000
> > +#define EF_ARM_EABI_VER1	0x01000000
> > +#define EF_ARM_EABI_VER2	0x02000000
> > +#define EF_ARM_EABI_VER3	0x03000000
> > +#define EF_ARM_EABI_VER4	0x04000000
> > +#define EF_ARM_EABI_VER5	0x05000000
> 
> Aren't these ARM32 definitions, which should be unneeded for
> ARM64 support?

Correct. Let me move that and also the arch/arm/arm32/livepatch:arch_livepatch_verify_elf
code out of this patch.
> 
> > @@ -171,6 +180,7 @@ typedef struct {
> >  #define EM_PPC		20		/* PowerPC */
> >  #define EM_PPC64	21		/* PowerPC 64-bit */
> >  #define EM_ARM		40		/* Advanced RISC Machines ARM */
> > +#define EM_AARCH64	183		/* ARM 64-bit */
> >  #define EM_ALPHA	41		/* DEC ALPHA */
> >  #define EM_SPARCV9	43		/* SPARC version 9 */
> >  #define EM_ALPHA_EXP	0x9026		/* DEC ALPHA */
> 
> I think this tries to be sorted by number.
> 
> > +/*
> > + * S - address of symbol.
> > + * A - addend for relocation (r_addend)
> > + * P - address of the dest being relocated (derieved from r_offset)
> > + * NC -  No check for overflow.
> > + *
> > + * The defines also use _PREL for PC-relative address, and _NC is No Check.
> > + */
> > +#define R_AARCH64_ABS64			257 /* Direct 64 bit. S+A, NC*/
> > +#define R_AARCH64_ABS32			258 /* Direct 32 bit. S+A */
> > +#define R_AARCH64_PREL64		260 /* S+A-P, NC */
> > +#define R_AARCH64_PREL32		261 /* S+A-P */
> > +
> > +#define R_AARCH64_ADR_PREL_LO21		274 /* ADR imm, [20:0]. S+A-P */
> > +#define R_AARCH64_ADR_PREL_PG_HI21	275 /* ADRP imm, [32:12]. Page(S+A) - Page(P).*/
> > +#define R_AARCH64_ADD_ABS_LO12_NC	277 /* ADD imm. [11:0]. S+A, NC */
> > +
> > +#define R_AARCH64_CONDBR19		280 /* Bits 20:2, S+A-P */
> > +#define R_AARCH64_JUMP26		282 /* Bits 27:2, S+A-P */
> > +#define R_AARCH64_CALL26		283 /* Bits 27:2, S+A-P */
> 
> No R_AARCH64_TSTBR14?
> 
> > +#define R_AARCH64_LDST16_ABS_LO12_NC	284 /* LD/ST to bits 11:1, S+A, NC */
> > +#define R_AARCH64_LDST32_ABS_LO12_NC	285 /* LD/ST to bits 11:2, S+A, NC */
> > +#define R_AARCH64_LDST64_ABS_LO12_NC	286 /* LD/ST to bits 11:3, S+A, NC */
> > +#define R_AARCH64_LDST8_ABS_LO12_NC	278 /* LD/ST to bits 11:0, S+A, NC */
> 
> What about R_AARCH64_LDST128_ABS_LO12_NC?

I didnt' see it in the ELF of xen-syms or the livepatch tests cases so I omitted them.
But then I added some that weren't in the ELF files either: LDST16 and LDST8.

I will add the two missing ones and also sort this.

Thanks!
> 
> Jan
> 

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