Re: [Xen-devel] [PATCH v5 11/16] livepatch: tests: Make them compile under ARM64

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

On Thu, Sep 22, 2016 at 02:10:26PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 21/09/16 18:32, Konrad Rzeszutek Wilk wrote:
> > We need to two things:
> > 1) Wrap the platform-specific objcopy parameters in defines
> >    The input and output parameters for $(OBJCOPY) are different
> >    based on the platforms. As such provide them in the
> >    OBJCOPY_MAGIC define and use that.
> > 
> > 2) The alternative is a bit different (exists only under ARM64
> >    and x86), while and there are no exceptions under ARM at all.
> >    We use the LIVEPATCH_FEATURE CPU id feature for ARM similar to
> >    how it is done on x86.
> > 
> > We are not yet attempting to build them under ARM32 so
> > that is still ifdefed out.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > 
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 
> > v1: First submission
> > v2: Corrected description by Julien
> >     Add #ifeq instead of #else for ARM case.
> > v3: Moved 'asm(alter..)' by one space to the left.
> > v4: Rebase on top of "livepatch/tests: Make .livepatch.depends be read-only"
> >     Rewrote the commit description 2) a bit.
> > ---
> >  xen/test/Makefile                         |  2 +-
> >  xen/test/livepatch/Makefile               | 12 ++++++++++--
> >  xen/test/livepatch/xen_hello_world_func.c |  7 +++++++
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/test/Makefile b/xen/test/Makefile
> > index 8c53040..95c1755 100644
> > --- a/xen/test/Makefile
> > +++ b/xen/test/Makefile
> > @@ -1,6 +1,6 @@
> >  .PHONY: tests
> >  tests:
> I am wondering if there is any way to use the
> > -ifeq ($(XEN_TARGET_ARCH),x86_64)
> > +ifneq $(XEN_TARGET_ARCH),arm32)

Sure.
> 
> NIT: I am wondering if you could instead use CONFIG_LIVEPATCH here, so the
> tests would only be built when livepatch is enabled.

The tests are not built by default. But perhaps that can be done
in a future patch? (and have the check in test/livepatch instead).
> 
> >     $(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch livepatch
> >  endif
> > 
> > diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
> > index 48ff843..5db4d9c 100644
> > --- a/xen/test/livepatch/Makefile
> > +++ b/xen/test/livepatch/Makefile
> > @@ -1,5 +1,12 @@
> >  include $(XEN_ROOT)/Config.mk
> > 
> > +ifeq ($(XEN_TARGET_ARCH),x86_64)
> > +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
> > +endif
> > +ifeq ($(XEN_TARGET_ARCH),arm64)
> > +OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
> > +endif
> > +
> >  CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
> >  CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
> > 
> > @@ -54,8 +61,9 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o 
> > note.o
> >  .PHONY: note.o
> >  note.o:
> >     $(OBJCOPY) -O binary --only-section=.note.gnu.build-id 
> > $(BASEDIR)/xen-syms $@.bin
> > -   $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
> > +   $(OBJCOPY) $(OBJCOPY_MAGIC) \
> >                
> > --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents 
> > -S $@.bin $@
> > +              --rename-section=.data=.livepatch.depends -S $@.bin $@
> 
> I am not sure why you added this line. Did you intend to replace the
> previous one?

Oh crud! No - thank you for spotting that!
> 
> >     rm -f $@.bin
> > 
> >  #
> > @@ -65,7 +73,7 @@ note.o:
> >  .PHONY: hello_world_note.o
> >  hello_world_note.o: $(LIVEPATCH)
> >     $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) 
> > $@.bin
> > -   $(OBJCOPY)  -I binary -O elf64-x86-64 -B i386:x86-64 \
> > +   $(OBJCOPY) $(OBJCOPY_MAGIC) \
> >                
> > --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents 
> > -S $@.bin $@
> >     rm -f $@.bin
> > 
> > diff --git a/xen/test/livepatch/xen_hello_world_func.c 
> > b/xen/test/livepatch/xen_hello_world_func.c
> > index 0321f3e..c5c0da1 100644
> > --- a/xen/test/livepatch/xen_hello_world_func.c
> > +++ b/xen/test/livepatch/xen_hello_world_func.c
> > @@ -7,14 +7,17 @@
> > 
> >  #include <asm/alternative.h>
> >  #include <asm/livepatch.h>
> > +#ifdef CONFIG_X86
> >  #include <asm/nops.h>
> >  #include <asm/uaccess.h>
> > 
> >  static unsigned long *non_canonical_addr = (unsigned long 
> > *)0xdead000000000000ULL;
> > +#endif
> > 
> >  /* Our replacement function for xen_extra_version. */
> >  const char *xen_hello_world(void)
> >  {
> > +#ifdef CONFIG_X86
> >      unsigned long tmp;
> >      int rc;
> > 
> > @@ -25,6 +28,10 @@ const char *xen_hello_world(void)
> >       */
> >      rc = __get_user(tmp, non_canonical_addr);
> >      BUG_ON(rc != -EFAULT);
> > +#endif
> > +#ifdef CONFIG_ARM_64
> 
> NIT: I would use:
> 
> #if defined(CONFIG_ARM) && defined(CONFIG_HAS_ALTERNATIVE)
> 
> in order to handle alternative if we decide to add support for ARM32.

/me nods.
> 
> > +    asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
> > +#endif
> > 
> >      return "Hello World";
> >  }
> > 
> 
> Regards,
> 
> -- 
> Julien Grall

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