* [PATCH v1 1/9] livepatch: Bubble up sanity checks on Elf relocs
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 11:56 ` Jan Beulich
2016-08-14 23:07 ` [PATCH v1 2/9] x86/arm: Make 'make debug' work properly Konrad Rzeszutek Wilk
` (8 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Konrad Rzeszutek Wilk
The checks for RELA ELF sanity checks does not need to
be in the platform specific file and can be bubbled up
in the platform agnostic file.
This makes the ARM 32/64 implementation easier as the
duplicate checks don't have to be in the platform specific files.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v1: First submission
---
xen/arch/x86/livepatch.c | 12 ------------
xen/common/livepatch_elf.c | 17 ++++++++++++++++-
2 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index cabd0c1..06c67bc 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -132,18 +132,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
uint64_t val;
uint8_t *dest;
- /* Nothing to do. */
- if ( !rela->sec->sh_size )
- return 0;
-
- if ( rela->sec->sh_entsize < sizeof(Elf_RelA) ||
- rela->sec->sh_size % rela->sec->sh_entsize )
- {
- dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
- elf->name);
- return -EINVAL;
- }
-
for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
{
r = rela->data + i * rela->sec->sh_entsize;
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 789e8fc..ef1a09d 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -365,7 +365,22 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
}
if ( r->sec->sh_type == SHT_RELA )
- rc = arch_livepatch_perform_rela(elf, base, r);
+ {
+ rc = 0;
+
+ if ( !r->sec->sh_size )
+ continue;
+
+ if ( r->sec->sh_entsize < sizeof(Elf_RelA) ||
+ r->sec->sh_size % r->sec->sh_entsize )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
+ elf->name);
+ rc = -EINVAL;
+ }
+ else
+ rc = arch_livepatch_perform_rela(elf, base, r);
+ }
else /* SHT_REL */
rc = arch_livepatch_perform_rel(elf, base, r);
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 1/9] livepatch: Bubble up sanity checks on Elf relocs
2016-08-14 23:07 ` [PATCH v1 1/9] livepatch: Bubble up sanity checks on Elf relocs Konrad Rzeszutek Wilk
@ 2016-08-17 11:56 ` Jan Beulich
0 siblings, 0 replies; 47+ messages in thread
From: Jan Beulich @ 2016-08-17 11:56 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -365,7 +365,22 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
> }
>
> if ( r->sec->sh_type == SHT_RELA )
> - rc = arch_livepatch_perform_rela(elf, base, r);
> + {
> + rc = 0;
> +
> + if ( !r->sec->sh_size )
> + continue;
> +
> + if ( r->sec->sh_entsize < sizeof(Elf_RelA) ||
> + r->sec->sh_size % r->sec->sh_entsize )
> + {
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n",
> + elf->name);
> + rc = -EINVAL;
> + }
> + else
> + rc = arch_livepatch_perform_rela(elf, base, r);
> + }
> else /* SHT_REL */
> rc = arch_livepatch_perform_rel(elf, base, r);
Shouldn't this be mirrored to the SHT_REL case then (with the
appropriate minor adjustments)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 2/9] x86/arm: Make 'make debug' work properly.
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
2016-08-14 23:07 ` [PATCH v1 1/9] livepatch: Bubble up sanity checks on Elf relocs Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 12:00 ` Jan Beulich
2016-08-14 23:07 ` [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files Konrad Rzeszutek Wilk
` (7 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Jan Beulich,
Konrad Rzeszutek Wilk
On x86 it works great but on ARM 32,64 not so much.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
v1: First submission
---
xen/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/Makefile b/xen/Makefile
index 4427771..a4c72be 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -101,7 +101,7 @@ _uninstall:
.PHONY: _debug
_debug:
- objdump -D -S $(TARGET)-syms > $(TARGET).s
+ $(OBJDUMP) -D -S $(TARGET)-syms > $(TARGET).s
.PHONY: _clean
_clean: delete-unfresh-files
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 2/9] x86/arm: Make 'make debug' work properly.
2016-08-14 23:07 ` [PATCH v1 2/9] x86/arm: Make 'make debug' work properly Konrad Rzeszutek Wilk
@ 2016-08-17 12:00 ` Jan Beulich
2016-08-22 17:05 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 47+ messages in thread
From: Jan Beulich @ 2016-08-17 12:00 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> On x86 it works great but on ARM 32,64 not so much.
Considering the nature of the change ...
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -101,7 +101,7 @@ _uninstall:
>
> .PHONY: _debug
> _debug:
> - objdump -D -S $(TARGET)-syms > $(TARGET).s
> + $(OBJDUMP) -D -S $(TARGET)-syms > $(TARGET).s
... I'd have expected breakage to be the other way around (works
in native build but breaks in cross ones). Can you explain in which
way a plain objdump fails here in the native case?
Irrespective of that the patch of course if fine, i.e.
Acked-by: Jan Beulich <jbeulich@suse.com>
just that's it like it to be explained better.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 2/9] x86/arm: Make 'make debug' work properly.
2016-08-17 12:00 ` Jan Beulich
@ 2016-08-22 17:05 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-22 17:05 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, ross.lagerwall, Andrew Cooper, julien.grall, xen-devel
On Wed, Aug 17, 2016 at 06:00:22AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> > On x86 it works great but on ARM 32,64 not so much.
>
> Considering the nature of the change ...
>
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -101,7 +101,7 @@ _uninstall:
> >
> > .PHONY: _debug
> > _debug:
> > - objdump -D -S $(TARGET)-syms > $(TARGET).s
> > + $(OBJDUMP) -D -S $(TARGET)-syms > $(TARGET).s
>
> ... I'd have expected breakage to be the other way around (works
> in native build but breaks in cross ones). Can you explain in which
Yes!
> way a plain objdump fails here in the native case?
I meant cross-builds. Doing compiling on ARM 32 proper platform
takes quite a while. And got so used that I am doing it now all the time.
>
> Irrespective of that the patch of course if fine, i.e.
> Acked-by: Jan Beulich <jbeulich@suse.com>
> just that's it like it to be explained better.
Thank you. Changing the commit to say;
When doing cross-compilation we should use proper $(OBJDUMP).
Otherwise decompiling say ARM32 code using x86 objdump won't help much.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files.
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
2016-08-14 23:07 ` [PATCH v1 1/9] livepatch: Bubble up sanity checks on Elf relocs Konrad Rzeszutek Wilk
2016-08-14 23:07 ` [PATCH v1 2/9] x86/arm: Make 'make debug' work properly Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 12:15 ` Jan Beulich
2016-08-17 16:26 ` Julien Grall
2016-08-14 23:07 ` [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings Konrad Rzeszutek Wilk
` (6 subsequent siblings)
9 siblings, 2 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Jan Beulich,
Konrad Rzeszutek Wilk
That way common code can use the same macro to access
the most common attributes without much #ifdef.
Take advantage of it right away in the livepatch code.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v1: First submission
---
xen/arch/arm/alternative.c | 4 ----
xen/common/livepatch.c | 4 ++--
xen/include/asm-arm/alternative.h | 4 ++++
xen/include/asm-x86/alternative.h | 4 ++++
4 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 8ee5a11..bf4101c 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -32,10 +32,6 @@
#include <asm/insn.h>
#include <asm/page.h>
-#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f)
-#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
-#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
-
extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
struct alt_region {
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 3a22de2..9c45270 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -700,8 +700,8 @@ static int prepare_payload(struct payload *payload,
for ( a = start; a < end; a++ )
{
- const void *instr = &a->instr_offset + a->instr_offset;
- const void *replacement = &a->repl_offset + a->repl_offset;
+ const void *instr = ALT_ORIG_PTR(a);
+ const void *replacement = ALT_REPL_PTR(a);
if ( (instr < region->start && instr >= region->end) ||
(replacement < region->start && replacement >= region->end) )
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 4287bac..58d5fa7 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -21,6 +21,10 @@ struct alt_instr {
u8 alt_len; /* size of new instruction(s), <= orig_len */
};
+#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f)
+#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
+#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
+
void __init apply_alternatives_all(void);
int apply_alternatives(void *start, size_t length);
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index acaeded..b72c401 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,10 @@ struct alt_instr {
u8 replacementlen; /* length of new instruction, <= instrlen */
};
+#define __ALT_PTR(a,f) (u8 *)((void *)&(a)->f + (a)->f)
+#define ALT_ORIG_PTR(a) __ALT_PTR(a, instr_offset)
+#define ALT_REPL_PTR(a) __ALT_PTR(a, repl_offset)
+
extern void add_nops(void *insns, unsigned int len);
/* Similar to apply_alternatives except it can be run with IRQs enabled. */
extern void apply_alternatives_nocheck(struct alt_instr *start,
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files.
2016-08-14 23:07 ` [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files Konrad Rzeszutek Wilk
@ 2016-08-17 12:15 ` Jan Beulich
2016-08-17 16:26 ` Julien Grall
1 sibling, 0 replies; 47+ messages in thread
From: Jan Beulich @ 2016-08-17 12:15 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> That way common code can use the same macro to access
> the most common attributes without much #ifdef.
>
> Take advantage of it right away in the livepatch code.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
x86 part
Acked-by: Jan Beulich <jbeulich@suse.com>
with one adjustment:
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -23,6 +23,10 @@ struct alt_instr {
> u8 replacementlen; /* length of new instruction, <= instrlen */
> };
>
> +#define __ALT_PTR(a,f) (u8 *)((void *)&(a)->f + (a)->f)
This needs another set of parentheses around the entire expression
(also in the ARM variant).
And I'd like to also note that from an x86 perspective the "Move" in the
title isn't really correct.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files.
2016-08-14 23:07 ` [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files Konrad Rzeszutek Wilk
2016-08-17 12:15 ` Jan Beulich
@ 2016-08-17 16:26 ` Julien Grall
1 sibling, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 16:26 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, sstabellini, Jan Beulich
Hi Konrad,
On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
> That way common code can use the same macro to access
> the most common attributes without much #ifdef.
>
> Take advantage of it right away in the livepatch code.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> v1: First submission
> ---
> xen/arch/arm/alternative.c | 4 ----
> xen/common/livepatch.c | 4 ++--
> xen/include/asm-arm/alternative.h | 4 ++++
> xen/include/asm-x86/alternative.h | 4 ++++
> 4 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 8ee5a11..bf4101c 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -32,10 +32,6 @@
> #include <asm/insn.h>
> #include <asm/page.h>
>
> -#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f)
> -#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
> -#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
> -
> extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
>
> struct alt_region {
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 3a22de2..9c45270 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -700,8 +700,8 @@ static int prepare_payload(struct payload *payload,
>
> for ( a = start; a < end; a++ )
> {
> - const void *instr = &a->instr_offset + a->instr_offset;
> - const void *replacement = &a->repl_offset + a->repl_offset;
> + const void *instr = ALT_ORIG_PTR(a);
> + const void *replacement = ALT_REPL_PTR(a);
>
> if ( (instr < region->start && instr >= region->end) ||
> (replacement < region->start && replacement >= region->end) )
> diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> index 4287bac..58d5fa7 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -21,6 +21,10 @@ struct alt_instr {
> u8 alt_len; /* size of new instruction(s), <= orig_len */
> };
>
> +#define __ALT_PTR(a,f) (u32 *)((void *)&(a)->f + (a)->f)
> +#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
> +#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
> +
alternative.h is a verbatim copy of the Linux one. So can you add a
comment such as /* Xen: helpers used by the common code */?
Also this should be indented with hard tab as the file is following the
Linux coding style.
> void __init apply_alternatives_all(void);
> int apply_alternatives(void *start, size_t length);
>
> diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
> index acaeded..b72c401 100644
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -23,6 +23,10 @@ struct alt_instr {
> u8 replacementlen; /* length of new instruction, <= instrlen */
> };
>
> +#define __ALT_PTR(a,f) (u8 *)((void *)&(a)->f + (a)->f)
> +#define ALT_ORIG_PTR(a) __ALT_PTR(a, instr_offset)
> +#define ALT_REPL_PTR(a) __ALT_PTR(a, repl_offset)
> +
> extern void add_nops(void *insns, unsigned int len);
> /* Similar to apply_alternatives except it can be run with IRQs enabled. */
> extern void apply_alternatives_nocheck(struct alt_instr *start,
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (2 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 3/9] x86/arm64: Move the ALT_[ORIG|REPL]_PTR macros to header files Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 16:54 ` Julien Grall
2016-08-14 23:07 ` [PATCH v1 5/9] arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions Konrad Rzeszutek Wilk
` (5 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Konrad Rzeszutek Wilk
Which is only used by Livepatch code. The purpose behind
this call is to modify the page table entries flags.
Specifically the .ro and .nx flags. The current mechanism
puts cache attributes in the flags and the .ro and .nx are
locked down and assumed to be .ro=0, nx=1.
Livepatch needs .nx=0 and also .ro to be set to 1.
We introduce a new 'flags' where various bits determine
whether .ro and .nx bits are set or cleared. We can't use
an enum as the function prototype would diverge from x86.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
--
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
[Added as he wrote the x86 modify_xen_mappings version]
RFC: First submission.
v1: Add #defines to make it simpler to understand the bit-shifting.
---
xen/arch/arm/mm.c | 21 ++++++++++++++++++---
xen/include/asm-arm/page.h | 11 +++++++++++
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 4e256c2..520c0e0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -836,6 +836,7 @@ static int create_xen_table(lpae_t *entry)
enum xenmap_operation {
INSERT,
REMOVE,
+ MODIFY,
RESERVE
};
@@ -881,14 +882,22 @@ static int create_xen_entries(enum xenmap_operation op,
pte.pt.table = 1;
write_pte(&third[third_table_offset(addr)], pte);
break;
+ case MODIFY:
case REMOVE:
if ( !third[third_table_offset(addr)].pt.valid )
{
- printk("create_xen_entries: trying to remove a non-existing mapping addr=%lx\n",
- addr);
+ printk("create_xen_entries: trying to %s a non-existing mapping addr=%lx\n",
+ op == REMOVE ? "remove" : "modify", addr);
return -EINVAL;
}
- pte.bits = 0;
+ if ( op == REMOVE )
+ pte.bits = 0;
+ else
+ {
+ pte = third[third_table_offset(addr)];
+ pte.pt.ro = PTE_RO_MASK(ai);
+ pte.pt.xn = PTE_NX_MASK(ai);
+ }
write_pte(&third[third_table_offset(addr)], pte);
break;
default:
@@ -922,6 +931,12 @@ void destroy_xen_mappings(unsigned long v, unsigned long e)
create_xen_entries(REMOVE, v, 0, (e - v) >> PAGE_SHIFT, 0);
}
+void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
+{
+ ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
+ create_xen_entries(MODIFY, s, 0, (e - s) >> PAGE_SHIFT, flags);
+}
+
enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
{
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 05d9f82..2f66740 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -66,6 +66,17 @@
#define PAGE_HYPERVISOR_WC (DEV_WC)
/*
+ * Defines for changing the PTE .ro and .nx bits. This is only to be
+ * used with modify_xen_mappings.
+ */
+#define _PTE_NX_BIT 0U
+#define _PTE_RO_BIT 1U
+#define PTE_NX (1U << _PTE_NX_BIT)
+#define PTE_RO (1U << _PTE_RO_BIT)
+#define PTE_NX_MASK(x) (((x) >> _PTE_NX_BIT) & 0x1U)
+#define PTE_RO_MASK(x) (((x) >> _PTE_RO_BIT) & 0x1U)
+
+/*
* Stage 2 Memory Type.
*
* These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings
2016-08-14 23:07 ` [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings Konrad Rzeszutek Wilk
@ 2016-08-17 16:54 ` Julien Grall
0 siblings, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 16:54 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, sstabellini
Hi Konrad,
On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
> Which is only used by Livepatch code. The purpose behind
> this call is to modify the page table entries flags.
>
> Specifically the .ro and .nx flags. The current mechanism
> puts cache attributes in the flags and the .ro and .nx are
> locked down and assumed to be .ro=0, nx=1.
>
> Livepatch needs .nx=0 and also .ro to be set to 1.
>
> We introduce a new 'flags' where various bits determine
> whether .ro and .nx bits are set or cleared. We can't use
> an enum as the function prototype would diverge from x86.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> --
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> [Added as he wrote the x86 modify_xen_mappings version]
>
> RFC: First submission.
> v1: Add #defines to make it simpler to understand the bit-shifting.
> ---
> xen/arch/arm/mm.c | 21 ++++++++++++++++++---
> xen/include/asm-arm/page.h | 11 +++++++++++
> 2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 4e256c2..520c0e0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -836,6 +836,7 @@ static int create_xen_table(lpae_t *entry)
> enum xenmap_operation {
> INSERT,
> REMOVE,
> + MODIFY,
> RESERVE
> };
>
> @@ -881,14 +882,22 @@ static int create_xen_entries(enum xenmap_operation op,
> pte.pt.table = 1;
> write_pte(&third[third_table_offset(addr)], pte);
> break;
> + case MODIFY:
> case REMOVE:
> if ( !third[third_table_offset(addr)].pt.valid )
> {
> - printk("create_xen_entries: trying to remove a non-existing mapping addr=%lx\n",
> - addr);
> + printk("create_xen_entries: trying to %s a non-existing mapping addr=%lx\n",
> + op == REMOVE ? "remove" : "modify", addr);
> return -EINVAL;
> }
> - pte.bits = 0;
> + if ( op == REMOVE )
> + pte.bits = 0;
> + else
> + {
> + pte = third[third_table_offset(addr)];
> + pte.pt.ro = PTE_RO_MASK(ai);
> + pte.pt.xn = PTE_NX_MASK(ai);
I would like to see some sanity check for read-only and execute-never
bit. For instance, ro = 0 and xn = 0 would lead to a crash later on
because Xen is enabling SCTLR_EL2.WXN.
I think the other combinations should be fine.
> + }
> write_pte(&third[third_table_offset(addr)], pte);
> break;
> default:
> @@ -922,6 +931,12 @@ void destroy_xen_mappings(unsigned long v, unsigned long e)
> create_xen_entries(REMOVE, v, 0, (e - v) >> PAGE_SHIFT, 0);
> }
>
> +void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
create_xen_entries may fail, don't you want to report the error to the
caller?
> +{
> + ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
> + create_xen_entries(MODIFY, s, 0, (e - s) >> PAGE_SHIFT, flags);
> +}
> +
> enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
> {
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 05d9f82..2f66740 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -66,6 +66,17 @@
> #define PAGE_HYPERVISOR_WC (DEV_WC)
>
> /*
> + * Defines for changing the PTE .ro and .nx bits. This is only to be
> + * used with modify_xen_mappings.
> + */
> +#define _PTE_NX_BIT 0U
> +#define _PTE_RO_BIT 1U
> +#define PTE_NX (1U << _PTE_NX_BIT)
> +#define PTE_RO (1U << _PTE_RO_BIT)
> +#define PTE_NX_MASK(x) (((x) >> _PTE_NX_BIT) & 0x1U)
> +#define PTE_RO_MASK(x) (((x) >> _PTE_RO_BIT) & 0x1U)
> +
> +/*
> * Stage 2 Memory Type.
> *
> * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 5/9] arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (3 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-14 23:07 ` [PATCH v1 6/9] livepatch: Initial ARM64 support Konrad Rzeszutek Wilk
` (4 subsequent siblings)
9 siblings, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: julien.grall, sstabellini, Konrad Rzeszutek Wilk
This is copied from Linux 4.7, and the initial commit
that put this in is 5c5bf25d4f7a950382f94fc120a5818197b48fe9
"arm64: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions"
This lays the groundwork for Livepatch to generate the
trampoline to jump to the new replacement function.
Also allows us to NOP the callsites.
This lays the groundwork for Livepatch to generate the
trampoline to jump to the new replacement function.
And also to NOP insns.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
--
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
RFC: First submission
v1: The full copy of insn_gen_branch instead of just the code to make branch
---
xen/arch/arm/arm64/insn.c | 61 ++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/arm64/insn.h | 23 +++++++++++++++
2 files changed, 84 insertions(+)
diff --git a/xen/arch/arm/arm64/insn.c b/xen/arch/arm/arm64/insn.c
index 12b4d96..c5f7e93 100644
--- a/xen/arch/arm/arm64/insn.c
+++ b/xen/arch/arm/arm64/insn.c
@@ -157,6 +157,67 @@ u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
return insn;
}
+static inline long branch_imm_common(unsigned long pc, unsigned long addr,
+ long range)
+{
+ long offset;
+
+ if ((pc & 0x3) || (addr & 0x3)) {
+ pr_err("%s: A64 instructions must be word aligned\n", __func__);
+ return range;
+ }
+
+ offset = ((long)addr - (long)pc);
+
+ if (offset < -range || offset >= range) {
+ pr_err("%s: offset out of range\n", __func__);
+ return range;
+ }
+
+ return offset;
+}
+
+u32 __kprobes aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
+ enum aarch64_insn_branch_type type)
+{
+ u32 insn;
+ long offset;
+
+ /*
+ * B/BL support [-128M, 128M) offset
+ * ARM64 virtual address arrangement guarantees all kernel and module
+ * texts are within +/-128M.
+ */
+ offset = branch_imm_common(pc, addr, SZ_128M);
+ if (offset >= SZ_128M)
+ return AARCH64_BREAK_FAULT;
+
+ switch (type) {
+ case AARCH64_INSN_BRANCH_LINK:
+ insn = aarch64_insn_get_bl_value();
+ break;
+ case AARCH64_INSN_BRANCH_NOLINK:
+ insn = aarch64_insn_get_b_value();
+ break;
+ default:
+ pr_err("%s: unknown branch encoding %d\n", __func__, type);
+ return AARCH64_BREAK_FAULT;
+ }
+
+ return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
+ offset >> 2);
+}
+
+u32 __kprobes aarch64_insn_gen_hint(enum aarch64_insn_hint_op op)
+{
+ return aarch64_insn_get_hint_value() | op;
+}
+
+u32 __kprobes aarch64_insn_gen_nop(void)
+{
+ return aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
+}
+
/*
* Decode the imm field of a branch, and return the byte offset as a
* signed value (so it can be used when computing a new branch
diff --git a/xen/include/asm-arm/arm64/insn.h b/xen/include/asm-arm/arm64/insn.h
index 6ce37be..c8362e5 100644
--- a/xen/include/asm-arm/arm64/insn.h
+++ b/xen/include/asm-arm/arm64/insn.h
@@ -23,6 +23,15 @@
#include <xen/types.h>
#include <xen/stdbool.h>
+enum aarch64_insn_hint_op {
+ AARCH64_INSN_HINT_NOP = 0x0 << 5,
+ AARCH64_INSN_HINT_YIELD = 0x1 << 5,
+ AARCH64_INSN_HINT_WFE = 0x2 << 5,
+ AARCH64_INSN_HINT_WFI = 0x3 << 5,
+ AARCH64_INSN_HINT_SEV = 0x4 << 5,
+ AARCH64_INSN_HINT_SEVL = 0x5 << 5,
+};
+
enum aarch64_insn_imm_type {
AARCH64_INSN_IMM_ADR,
AARCH64_INSN_IMM_26,
@@ -38,6 +47,14 @@ enum aarch64_insn_imm_type {
AARCH64_INSN_IMM_MAX
};
+enum aarch64_insn_branch_type {
+ AARCH64_INSN_BRANCH_NOLINK,
+ AARCH64_INSN_BRANCH_LINK,
+ AARCH64_INSN_BRANCH_RETURN,
+ AARCH64_INSN_BRANCH_COMP_ZERO,
+ AARCH64_INSN_BRANCH_COMP_NONZERO,
+};
+
#define __AARCH64_INSN_FUNCS(abbr, mask, val) \
static always_inline bool_t aarch64_insn_is_##abbr(u32 code) \
{ return (code & (mask)) == (val); } \
@@ -51,6 +68,7 @@ __AARCH64_INSN_FUNCS(cbnz, 0x7F000000, 0x35000000)
__AARCH64_INSN_FUNCS(tbz, 0x7F000000, 0x36000000)
__AARCH64_INSN_FUNCS(tbnz, 0x7F000000, 0x37000000)
__AARCH64_INSN_FUNCS(bcond, 0xFF000010, 0x54000000)
+__AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
bool aarch64_insn_is_branch_imm(u32 insn);
@@ -61,6 +79,11 @@ u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
s32 aarch64_get_branch_offset(u32 insn);
u32 aarch64_set_branch_offset(u32 insn, s32 offset);
+u32 aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
+ enum aarch64_insn_branch_type type);
+u32 aarch64_insn_gen_hint(enum aarch64_insn_hint_op op);
+u32 aarch64_insn_gen_nop(void);
+
/* Wrapper for common code */
static inline bool insn_is_branch_imm(u32 insn)
{
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (4 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 5/9] arm64/insn: introduce aarch64_insn_gen_{nop|branch_imm}() helper functions Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-15 8:21 ` Jan Beulich
` (2 more replies)
2016-08-14 23:07 ` [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p] Konrad Rzeszutek Wilk
` (3 subsequent siblings)
9 siblings, 3 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Konrad Rzeszutek Wilk
As compared to x86 the va of the hypervisor .text
is locked down - we cannot modify the running pagetables
to have the .ro flag unset. We borrow the same idea that
alternative patching has - which is to vmap the entire
.text region and use the alternative virtual address
for patching.
Since we are doing vmap we may fail, hence the
arch_livepatch_quiesce is changed to return an error value
which will be bubbled in payload->rc and provided to the
user (along with messages in the ring buffer).
And the livepatch virtual address space (where the
new functions are) needs to be close to the hypervisor virtual address
so that the trampoline can reach it. As such we re-use
the BOOT_RELOC_VIRT_START which is not used after bootup
(alternatively we can also use the space after the _end to
FIXMAP_ADDR(0), but that may be too small).
The ELF relocation engine at the start was coded from
the "ELF for the ARM 64-bit Architecture (AArch64)"
(http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf)
but after a while of trying to write the correct bit shifting
and masking from scratch I ended up borrowing from Linux, the
'reloc_insn_imm' (Linux v4.7 arch/arm64/kernel/module.c function.
See 257cb251925f854da435cbf79b140984413871ac "arm64: Loadable modules")
In the livepatch payload loading code we tweak the #ifdef to
only exclude ARM_32. The exceptions are not part of ARM 32/64 hence
they are still behind the #ifdef.
We also expand the MAINTAINERS file to include the arm64 and arm32
platform specific livepatch file.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
RFC: Wholy cow! It works!
v1: Finished the various TODOs and reviews outlined by Julien in RFC.
---
MAINTAINERS | 1 +
xen/arch/arm/Makefile | 13 ++-
xen/arch/arm/arm32/Makefile | 2 +-
xen/arch/arm/arm32/livepatch.c | 55 +++++++++
xen/arch/arm/arm64/Makefile | 1 +
xen/arch/arm/arm64/livepatch.c | 247 +++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/domain.c | 6 +
xen/arch/arm/livepatch.c | 99 +++++++++++++----
xen/arch/arm/livepatch.h | 28 +++++
xen/arch/x86/livepatch.c | 4 +-
xen/common/Kconfig | 2 +-
xen/common/livepatch.c | 25 ++++-
xen/include/asm-arm/config.h | 8 +-
xen/include/asm-arm/current.h | 7 ++
xen/include/xen/elfstructs.h | 40 ++++++-
xen/include/xen/livepatch.h | 2 +-
16 files changed, 503 insertions(+), 37 deletions(-)
create mode 100644 xen/arch/arm/arm32/livepatch.c
create mode 100644 xen/arch/arm/arm64/livepatch.c
create mode 100644 xen/arch/arm/livepatch.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 97720a8..ae0b6bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -269,6 +269,7 @@ S: Supported
F: docs/misc/livepatch.markdown
F: tools/misc/xen-livepatch.c
F: xen/arch/*/livepatch*
+F: xen/arch/*/*/livepatch*
F: xen/common/livepatch*
F: xen/include/xen/livepatch*
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 0a96713..9f75c5c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -58,6 +58,15 @@ ALL_OBJS := $(TARGET_SUBARCH)/head.o $(ALL_OBJS)
DEPS += $(TARGET_SUBARCH)/.head.o.d
+ifdef CONFIG_LIVEPATCH
+all_symbols = --all-symbols
+ifdef CONFIG_FAST_SYMBOL_LOOKUP
+all_symbols = --all-symbols --sort-by-name
+endif
+else
+all_symbols =
+endif
+
$(TARGET): $(TARGET)-syms $(TARGET).axf
$(OBJCOPY) -O binary -S $< $@
ifeq ($(CONFIG_ARM_64),y)
@@ -93,12 +102,12 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
$(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
- | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
+ | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S
$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
$(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
- | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S
+ | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S
$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
$(@D)/.$(@F).1.o -o $@
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index b20db64..5966de0 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,8 +4,8 @@ obj-$(EARLY_PRINTK) += debug.o
obj-y += domctl.o
obj-y += domain.o
obj-y += entry.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
obj-y += proc-v7.o proc-caxx.o
obj-y += smpboot.o
obj-y += traps.o
obj-y += vfp.o
-
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
new file mode 100644
index 0000000..89ee2f5
--- /dev/null
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/livepatch_elf.h>
+#include <xen/livepatch.h>
+
+void arch_livepatch_apply_jmp(struct livepatch_func *func)
+{
+}
+
+void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+{
+}
+
+int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
+{
+ const Elf_Ehdr *hdr = elf->hdr;
+
+ if ( hdr->e_machine != EM_ARM ||
+ hdr->e_ident[EI_CLASS] != ELFCLASS32 )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
+ elf->name);
+ return -EOPNOTSUPP;
+ }
+
+ if ( (hdr->e_flags & EF_ARM_EABI_MASK) != EF_ARM_EABI_VER5 )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF EABI(%x)!\n",
+ elf->name, hdr->e_flags);
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+ const struct livepatch_elf_sec *base,
+ const struct livepatch_elf_sec *rela)
+{
+ return -ENOSYS;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index c1fa43f..149b6b3 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -6,6 +6,7 @@ obj-y += domctl.o
obj-y += domain.o
obj-y += entry.o
obj-y += insn.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
obj-y += smpboot.o
obj-y += traps.o
obj-y += vfp.o
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 0000000..e348942
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -0,0 +1,247 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include "../livepatch.h"
+#include <xen/bitops.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/livepatch_elf.h>
+#include <xen/livepatch.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+
+#include <asm/bitops.h>
+#include <asm/byteorder.h>
+#include <asm/insn.h>
+
+void arch_livepatch_apply_jmp(struct livepatch_func *func)
+{
+ uint32_t insn;
+ uint32_t *old_ptr;
+ uint32_t *new_ptr;
+
+ BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
+ BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
+
+ ASSERT(vmap_of_xen_text);
+
+ /* Save old one. */
+ old_ptr = func->old_addr;
+ memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+
+ if ( func->new_size )
+ 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();
+
+ new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+
+ /* PATCH! */
+ *(new_ptr) = cpu_to_le32(insn);
+
+ clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
+}
+
+void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+{
+ uint32_t *new_ptr;
+ uint32_t insn;
+
+ memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
+
+ new_ptr = (uint32_t *)func->old_addr - (u32 *)_start + vmap_of_xen_text;
+
+ /* PATCH! */
+ *(new_ptr) = cpu_to_le32(insn);
+
+ clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*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;
+}
+
+static int reloc_insn_imm(void *dest, u64 val, int lsb, int len,
+ enum aarch64_insn_imm_type imm_type)
+{
+ u64 imm, imm_mask;
+ s64 sval = val;
+ u32 insn = *(u32 *)dest;
+
+ /* Calculate the relocation value. */
+ sval >>= lsb;
+
+ /* Extract the value bits and shift them to bit 0. */
+ imm_mask = (BIT(lsb + len) - 1) >> lsb;
+ imm = sval & imm_mask;
+
+ /* Update the instruction's immediate field. */
+ insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
+ *(u32 *)dest = insn;
+
+ /*
+ * Extract the upper value bits (including the sign bit) and
+ * shift them to bit 0.
+ */
+ sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1);
+
+ /*
+ * Overflow has occurred if the upper bits are not all equal to
+ * the sign bit of the value.
+ */
+ if ((u64)(sval + 1) >= 2)
+ return -EOVERFLOW;
+ return 0;
+}
+
+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+ const struct livepatch_elf_sec *base,
+ const struct livepatch_elf_sec *rela)
+{
+ const Elf_RelA *r;
+ unsigned int symndx, i;
+ uint64_t val;
+ void *dest;
+
+ for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+ {
+ int err = 0;
+
+ r = rela->data + i * rela->sec->sh_entsize;
+
+ symndx = ELF64_R_SYM(r->r_info);
+
+ if ( symndx > elf->nsym )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
+ elf->name, symndx);
+ return -EINVAL;
+ }
+
+ dest = base->load_addr + r->r_offset; /* P */
+ val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
+
+ /* ARM64 operations at minimum are always 32-bit. */
+ if ( r->r_offset >= base->sec->sh_size ||
+ (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
+ goto bad_offset;
+
+ switch ( ELF64_R_TYPE(r->r_info) ) {
+ /* Data */
+ case R_AARCH64_ABS64:
+ if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+ goto bad_offset;
+ *(int64_t *)dest = val;
+ break;
+
+ case R_AARCH64_ABS32:
+ *(int32_t *)dest = val;
+ if ( (int64_t)val != *(int32_t *)dest )
+ err = -EOVERFLOW;
+ break;
+
+ case R_AARCH64_PREL64:
+ if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+ goto bad_offset;
+
+ val -= (uint64_t)dest;
+ *(int64_t *)dest = val;
+ break;
+
+ case R_AARCH64_PREL32:
+ val -= (uint64_t)dest;
+ *(int32_t *)dest = val;
+ if ( (int64_t)val != *(int32_t *)dest )
+ err = -EOVERFLOW;
+ break;
+
+ /* Instructions. */
+ case R_AARCH64_ADR_PREL_LO21:
+ val -= (uint64_t)dest;
+ err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR);
+ break;
+
+ case R_AARCH64_ADR_PREL_PG_HI21:
+ val = (val & ~0xfff) - ((u64)dest & ~0xfff);
+ err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR);
+ break;
+
+ case R_AARCH64_LDST8_ABS_LO12_NC:
+ case R_AARCH64_ADD_ABS_LO12_NC:
+ err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12);
+ if ( err == -EOVERFLOW )
+ err = 0;
+ break;
+
+ case R_AARCH64_LDST16_ABS_LO12_NC:
+ err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12);
+ if ( err == -EOVERFLOW )
+ err = 0;
+ break;
+
+ case R_AARCH64_LDST32_ABS_LO12_NC:
+ err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12);
+ if ( err == -EOVERFLOW )
+ err = 0;
+ break;
+
+ case R_AARCH64_LDST64_ABS_LO12_NC:
+ err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12);
+ if ( err == -EOVERFLOW )
+ err = 0;
+ break;
+
+ case R_AARCH64_CONDBR19:
+ err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19);
+ break;
+
+ case R_AARCH64_JUMP26:
+ case R_AARCH64_CALL26:
+ val -= (uint64_t)dest;
+ err = reloc_insn_imm(dest, val, 2, 26, AARCH64_INSN_IMM_26);
+ break;
+
+ default:
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation %lu\n",
+ elf->name, ELF64_R_TYPE(r->r_info));
+ return -EOPNOTSUPP;
+ }
+
+ if ( err )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
+ elf->name, i, rela->name, base->name);
+ return err;
+ }
+ }
+ return 0;
+
+ bad_offset:
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation offset is past %s section!\n",
+ elf->name, base->name);
+ return -EINVAL;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 20bb2ba..607ee59 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -13,6 +13,7 @@
#include <xen/hypercall.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/livepatch.h>
#include <xen/sched.h>
#include <xen/softirq.h>
#include <xen/wait.h>
@@ -55,6 +56,11 @@ void idle_loop(void)
do_tasklet();
do_softirq();
+ /*
+ * We MUST be last (or before dsb, wfi). Otherwise after we get the
+ * softirq we would execute dsb,wfi (and sleep) and not patch.
+ */
+ check_for_livepatch_work();
}
}
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 3093554..19f6033 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -1,56 +1,93 @@
/*
* Copyright (C) 2016 Citrix Systems R&D Ltd.
*/
+
+#include "livepatch.h"
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/livepatch_elf.h>
#include <xen/livepatch.h>
+#include <xen/vmap.h>
+
+#include <asm/mm.h>
-/* On ARM32,64 instructions are always 4 bytes long. */
-#define PATCH_INSN_SIZE 4
+void *vmap_of_xen_text;
int arch_verify_insn_length(unsigned long len)
{
return len != PATCH_INSN_SIZE;
}
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
{
+ mfn_t text_mfn;
+ unsigned int text_order;
+
+ if ( vmap_of_xen_text )
+ return -EINVAL;
+
+ text_mfn = _mfn(virt_to_mfn(_stext));
+ text_order = get_order_from_bytes(_end - _start);
+
+ /*
+ * The text section is read-only. So re-map Xen to be able to patch
+ * the code.
+ */
+ vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
+ VMAP_DEFAULT);
+
+ if ( !vmap_of_xen_text )
+ {
+ printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
+ text_order);
+ return -ENOMEM;
+ }
+ return 0;
}
void arch_livepatch_revive(void)
{
+ /*
+ * Nuke the instruction cache. It has been cleaned before in
+ * arch_livepatch_apply_jmp.
+ */
+ invalidate_icache();
+
+ if ( vmap_of_xen_text )
+ vunmap(vmap_of_xen_text);
+
+ vmap_of_xen_text = NULL;
+
+ /*
+ * Need to flush the branch predicator for ARMv7 as it may be
+ * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
+ */
+ flush_xen_text_tlb_local();
}
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
- return -ENOSYS;
-}
-
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
-{
-}
+ if ( func->old_size < PATCH_INSN_SIZE )
+ return -EINVAL;
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
-{
+ return 0;
}
void arch_livepatch_post_action(void)
{
+ /* arch_livepatch_revive has nuked the instruction cache. */
}
void arch_livepatch_mask(void)
{
+ /* Mask System Error (SError) */
+ local_abort_disable();
}
void arch_livepatch_unmask(void)
{
-}
-
-int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
-{
- return -ENOSYS;
+ local_abort_enable();
}
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
@@ -60,20 +97,34 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
return -ENOSYS;
}
-int arch_livepatch_perform_rela(struct livepatch_elf *elf,
- const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
-{
- return -ENOSYS;
-}
-
int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
{
- return -ENOSYS;
+ unsigned long start = (unsigned long)va;
+ unsigned int flags;
+
+ ASSERT(va);
+ ASSERT(pages);
+
+ if ( type == LIVEPATCH_VA_RX )
+ flags = PTE_RO; /* R set, NX clear */
+ else if ( type == LIVEPATCH_VA_RW )
+ flags = PTE_NX; /* R clear, NX set */
+ else
+ flags = PTE_NX | PTE_RO; /* R set, NX set */
+
+ modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
+
+ return 0;
}
void __init arch_livepatch_init(void)
{
+ void *start, *end;
+
+ start = (void *)LIVEPATCH_VMAP;
+ end = start + MB(2);
+
+ vm_init_type(VMAP_XEN, start, end);
}
/*
diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
new file mode 100644
index 0000000..8c8d625
--- /dev/null
+++ b/xen/arch/arm/livepatch.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_ARM_LIVEPATCH_H__
+#define __XEN_ARM_LIVEPATCH_H__
+
+/* On ARM32,64 instructions are always 4 bytes long. */
+#define PATCH_INSN_SIZE 4
+
+/*
+ * The va of the hypervisor .text region. We need this as the
+ * normal va are write protected.
+ */
+extern void *vmap_of_xen_text;
+
+#endif /* __XEN_ARM_LIVEPATCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 06c67bc..e3f3f37 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -15,10 +15,12 @@
#define PATCH_INSN_SIZE 5
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
{
/* Disable WP to allow changes to read-only pages. */
write_cr0(read_cr0() & ~X86_CR0_WP);
+
+ return 0;
}
void arch_livepatch_revive(void)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 51afa24..2fc76b6 100644
--- 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"
---help---
Allows a running Xen hypervisor to be dynamically patched using
binary patches without rebooting. This is primarily used to binarily
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 9c45270..af9443d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
sizeof(*region->frame[i].bugs);
}
-#ifndef CONFIG_ARM
+#ifndef CONFIG_ARM_32
sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
if ( sec )
{
@@ -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
}
+#endif
+#ifndef CONFIG_ARM
sec = livepatch_elf_sec_by_name(elf, ".ex_table");
if ( sec )
{
@@ -1083,12 +1089,17 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
static int apply_payload(struct payload *data)
{
unsigned int i;
+ int rc;
printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
data->name, data->nfuncs);
- arch_livepatch_quiesce();
-
+ rc = arch_livepatch_quiesce();
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+ return rc;
+ }
/*
* Since we are running with IRQs disabled and the hooks may call common
* code - which expects the spinlocks to run with IRQs enabled - we temporarly
@@ -1119,10 +1130,16 @@ static int apply_payload(struct payload *data)
static int revert_payload(struct payload *data)
{
unsigned int i;
+ int rc;
printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
- arch_livepatch_quiesce();
+ rc = arch_livepatch_quiesce();
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+ return rc;
+ }
for ( i = 0; i < data->nfuncs; i++ )
arch_livepatch_revert_jmp(&data->funcs[i]);
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index a96f845..8d876f6 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -80,9 +80,10 @@
* 4M - 6M Fixmap: special-purpose 4K mapping slots
* 6M - 8M Early boot mapping of FDT
* 8M - 10M Early relocation address (used when relocating Xen)
+ * and later for livepatch vmap (if compiled in)
*
* ARM32 layout:
- * 0 - 8M <COMMON>
+ * 0 - 10M <COMMON>
*
* 32M - 128M Frametable: 24 bytes per page for 16GB of RAM
* 256M - 1G VMAP: ioremap and early_ioremap use this virtual address
@@ -93,7 +94,7 @@
*
* ARM64 layout:
* 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
- * 0 - 8M <COMMON>
+ * 0 - 10M <COMMON>
*
* 1G - 2G VMAP: ioremap and early_ioremap
*
@@ -113,6 +114,9 @@
#define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
#define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
#define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00800000)
+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_VMAP _AT(vaddr_t,0x00800000)
+#endif
#define HYPERVISOR_VIRT_START XEN_VIRT_START
diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index 65c0cdf..f4fcfd6 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void)
#define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
+#ifdef CONFIG_LIVEPATCH
+#define switch_stack_and_jump(stack, fn) \
+ asm volatile ("mov sp,%0;" \
+ "bl check_for_livepatch_work;" \
+ "b " STR(fn) : : "r" (stack) : "memory" )
+#else
#define switch_stack_and_jump(stack, fn) \
asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
+#endif
#define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 5f2082e..a68874b 100644
--- 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
+
/* ELF Header */
typedef struct elfhdr {
unsigned char e_ident[EI_NIDENT]; /* ELF Identification */
@@ -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 */
@@ -353,12 +363,40 @@ typedef struct {
#define ELF64_R_TYPE(info) ((info) & 0xFFFFFFFF)
#define ELF64_R_INFO(s,t) (((s) << 32) + (u_int32_t)(t))
-/* x86-64 relocation types. We list only the ones Live Patch implements. */
+/*
+ * Relocation types for x86_64 and ARM 64. We list only the ones Live Patch
+ * implements.
+ */
#define R_X86_64_NONE 0 /* No reloc */
#define R_X86_64_64 1 /* Direct 64 bit */
#define R_X86_64_PC32 2 /* PC relative 32 bit signed */
#define R_X86_64_PLT32 4 /* 32 bit PLT address */
+/*
+ * 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 */
+#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 */
+
/* Program Header */
typedef struct {
Elf32_Word p_type; /* segment type */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2e64686..6f30c0d 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func);
* These functions are called around the critical region patching live code,
* for an architecture to take make appropratie global state adjustments.
*/
-void arch_livepatch_quiesce(void);
+int arch_livepatch_quiesce(void);
void arch_livepatch_revive(void);
void arch_livepatch_apply_jmp(struct livepatch_func *func);
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-14 23:07 ` [PATCH v1 6/9] livepatch: Initial ARM64 support Konrad Rzeszutek Wilk
@ 2016-08-15 8:21 ` Jan Beulich
2016-08-15 14:09 ` Konrad Rzeszutek Wilk
2016-08-15 15:17 ` Julien Grall
2016-08-17 18:22 ` Julien Grall
2 siblings, 1 reply; 47+ messages in thread
From: Jan Beulich @ 2016-08-15 8:21 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> 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?
> @@ -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?
> --- 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?
> @@ -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?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 8:21 ` Jan Beulich
@ 2016-08-15 14:09 ` Konrad Rzeszutek Wilk
2016-08-15 14:23 ` Jan Beulich
0 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-15 14:09 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, ross.lagerwall, Andrew Cooper, julien.grall, xen-devel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 14:09 ` Konrad Rzeszutek Wilk
@ 2016-08-15 14:23 ` Jan Beulich
2016-08-15 14:57 ` Julien Grall
0 siblings, 1 reply; 47+ messages in thread
From: Jan Beulich @ 2016-08-15 14:23 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: sstabellini, ross.lagerwall, Andrew Cooper, julien.grall, xen-devel
>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
> 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:
>> > @@ -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?
Either way is fine with me, with a slight preference to the former
one.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 14:23 ` Jan Beulich
@ 2016-08-15 14:57 ` Julien Grall
2016-08-15 15:17 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 47+ messages in thread
From: Julien Grall @ 2016-08-15 14:57 UTC (permalink / raw)
To: Jan Beulich, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, sstabellini, xen-devel, ross.lagerwall
Hi Jan and Konrad,
On 15/08/2016 16:23, Jan Beulich wrote:
>>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
>> 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:
>>>> @@ -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?
>
> Either way is fine with me, with a slight preference to the former
> one.
I am fine with the prototype of the function apply_alternatives_nocheck
but I don't think the name is relevant for ARM.
Is there any reason we don't want to call directly apply_alternatives in
x86?
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 14:57 ` Julien Grall
@ 2016-08-15 15:17 ` Konrad Rzeszutek Wilk
2016-08-15 15:25 ` Julien Grall
0 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-15 15:17 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, ross.lagerwall, Andrew Cooper, Jan Beulich, xen-devel
On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
> Hi Jan and Konrad,
>
> On 15/08/2016 16:23, Jan Beulich wrote:
> > > > > On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
> > > 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:
> > > > > @@ -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?
> >
> > Either way is fine with me, with a slight preference to the former
> > one.
>
> I am fine with the prototype of the function apply_alternatives_nocheck but
> I don't think the name is relevant for ARM.
>
> Is there any reason we don't want to call directly apply_alternatives in
> x86?
It assumes (and has an ASSERT) that it is called with interrupts disabled.
And we don't need to do that (as during livepatch loading we can modify the
livepatch payload without worrying about interrupts).
P.S.
loading != applying.
I could do a patch where we rename 'apply_alternatives' -> 'apply_alternatives_boot'
and 'apply_alternatives_nocheck' to 'apply_alternatives'.
Also the x86 version instead of having:
apply_alternatives(struct alt_instr *start, struct alt_instr *end)
would end up with:
apply_alternatives(void *start, size_t length)
to comply with ARM version.
>
> Regards,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 15:17 ` Konrad Rzeszutek Wilk
@ 2016-08-15 15:25 ` Julien Grall
2016-08-15 15:27 ` Andrew Cooper
0 siblings, 1 reply; 47+ messages in thread
From: Julien Grall @ 2016-08-15 15:25 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: sstabellini, ross.lagerwall, Andrew Cooper, Jan Beulich, xen-devel
On 15/08/2016 17:17, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
>> Hi Jan and Konrad,
>>
>> On 15/08/2016 16:23, Jan Beulich wrote:
>>>>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
>>>> 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:
>>>>>> @@ -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?
>>>
>>> Either way is fine with me, with a slight preference to the former
>>> one.
>>
>> I am fine with the prototype of the function apply_alternatives_nocheck but
>> I don't think the name is relevant for ARM.
>>
>> Is there any reason we don't want to call directly apply_alternatives in
>> x86?
>
> It assumes (and has an ASSERT) that it is called with interrupts disabled.
> And we don't need to do that (as during livepatch loading we can modify the
> livepatch payload without worrying about interrupts).
Oh, it makes more sense now.
>
> P.S.
> loading != applying.
>
> I could do a patch where we rename 'apply_alternatives' -> 'apply_alternatives_boot'
> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
>
> Also the x86 version instead of having:
>
> apply_alternatives(struct alt_instr *start, struct alt_instr *end)
>
> would end up with:
>
> apply_alternatives(void *start, size_t length)
I would be fine with the prototype
apply_alternatives(struct alt_instr *start, struct alt_instr *end)
for ARM. It is up to you.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 15:25 ` Julien Grall
@ 2016-08-15 15:27 ` Andrew Cooper
2016-08-17 2:45 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 47+ messages in thread
From: Andrew Cooper @ 2016-08-15 15:27 UTC (permalink / raw)
To: Julien Grall, Konrad Rzeszutek Wilk
Cc: ross.lagerwall, sstabellini, xen-devel, Jan Beulich
On 15/08/16 16:25, Julien Grall wrote:
>
>
> On 15/08/2016 17:17, Konrad Rzeszutek Wilk wrote:
>> On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
>>> Hi Jan and Konrad,
>>>
>>> On 15/08/2016 16:23, Jan Beulich wrote:
>>>>>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
>>>>> 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:
>>>>>>> @@ -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?
>>>>
>>>> Either way is fine with me, with a slight preference to the former
>>>> one.
>>>
>>> I am fine with the prototype of the function
>>> apply_alternatives_nocheck but
>>> I don't think the name is relevant for ARM.
>>>
>>> Is there any reason we don't want to call directly
>>> apply_alternatives in
>>> x86?
>>
>> It assumes (and has an ASSERT) that it is called with interrupts
>> disabled.
>> And we don't need to do that (as during livepatch loading we can
>> modify the
>> livepatch payload without worrying about interrupts).
>
> Oh, it makes more sense now.
>
>>
>> P.S.
>> loading != applying.
>>
>> I could do a patch where we rename 'apply_alternatives' ->
>> 'apply_alternatives_boot'
>> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
The only reason apply_alternatives() is named thusly is to match Linux.
I am not fussed if it changes.
~Andrew
>>
>> Also the x86 version instead of having:
>>
>> apply_alternatives(struct alt_instr *start, struct alt_instr *end)
>>
>> would end up with:
>>
>> apply_alternatives(void *start, size_t length)
>
> I would be fine with the prototype
> apply_alternatives(struct alt_instr *start, struct alt_instr *end)
>
> for ARM. It is up to you.
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 15:27 ` Andrew Cooper
@ 2016-08-17 2:45 ` Konrad Rzeszutek Wilk
2016-08-17 9:02 ` Andrew Cooper
0 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-17 2:45 UTC (permalink / raw)
To: Andrew Cooper
Cc: sstabellini, ross.lagerwall, Julien Grall, Jan Beulich, xen-devel
On Mon, Aug 15, 2016 at 04:27:12PM +0100, Andrew Cooper wrote:
> On 15/08/16 16:25, Julien Grall wrote:
> >
> >
> > On 15/08/2016 17:17, Konrad Rzeszutek Wilk wrote:
> >> On Mon, Aug 15, 2016 at 04:57:26PM +0200, Julien Grall wrote:
> >>> Hi Jan and Konrad,
> >>>
> >>> On 15/08/2016 16:23, Jan Beulich wrote:
> >>>>>>> On 15.08.16 at 16:09, <konrad.wilk@oracle.com> wrote:
> >>>>> 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:
> >>>>>>> @@ -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?
> >>>>
> >>>> Either way is fine with me, with a slight preference to the former
> >>>> one.
> >>>
> >>> I am fine with the prototype of the function
> >>> apply_alternatives_nocheck but
> >>> I don't think the name is relevant for ARM.
> >>>
> >>> Is there any reason we don't want to call directly
> >>> apply_alternatives in
> >>> x86?
> >>
> >> It assumes (and has an ASSERT) that it is called with interrupts
> >> disabled.
> >> And we don't need to do that (as during livepatch loading we can
> >> modify the
> >> livepatch payload without worrying about interrupts).
> >
> > Oh, it makes more sense now.
> >
> >>
> >> P.S.
> >> loading != applying.
> >>
> >> I could do a patch where we rename 'apply_alternatives' ->
> >> 'apply_alternatives_boot'
> >> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
>
> The only reason apply_alternatives() is named thusly is to match Linux.
> I am not fussed if it changes.
Would this be OK with folks?
There is a bit of disreprancy - ARM has 'const struct alt_instr *'
where the 'const' gets dropped later on. That can't be done x86
as 'apply_alternatives' at the gecko modifies the structure.
Thoughts? Make it the same on ARM and x86?
From 2c26d4d214926cd23b73f98c1fdaecd98b010da6 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 16 Aug 2016 22:20:54 -0400
Subject: [PATCH] alternatives: x86 rename and change parameters on ARM
On x86 we rename 'apply_alternatives' -> 'apply_alternatives_boot'
and 'apply_alternatives_nocheck' to 'apply_alternatives'.
On ARM we change the parameters for 'apply_alternatives'
to be of 'const struct alt_instr *' instead of void pointer and
size length.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
v3.1: First submission.
---
xen/arch/arm/alternative.c | 4 ++--
xen/arch/x86/alternative.c | 8 ++++----
xen/common/livepatch.c | 2 +-
xen/include/asm-arm/alternative.h | 2 +-
xen/include/asm-x86/alternative.h | 5 ++---
5 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index bf4101c..aba06db 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -200,11 +200,11 @@ void __init apply_alternatives_all(void)
BUG_ON(ret);
}
-int apply_alternatives(void *start, size_t length)
+int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
{
const struct alt_region region = {
.begin = start,
- .end = start + length,
+ .end = end,
};
return __apply_alternatives(®ion);
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index fd8528e..7addc2c 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -144,7 +144,7 @@ static void *init_or_livepatch text_poke(void *addr, const void *opcode, size_t
* APs have less capabilities than the boot processor are not handled.
* Tough. Make sure you disable such features by hand.
*/
-void init_or_livepatch apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)
+void init_or_livepatch apply_alternatives(struct alt_instr *start, struct alt_instr *end)
{
struct alt_instr *a;
u8 *instr, *replacement;
@@ -187,7 +187,7 @@ void init_or_livepatch apply_alternatives_nocheck(struct alt_instr *start, struc
* This routine is called with local interrupt disabled and used during
* bootup.
*/
-void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+void __init apply_alternatives_boot(struct alt_instr *start, struct alt_instr *end)
{
unsigned long cr0 = read_cr0();
@@ -196,7 +196,7 @@ void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
/* Disable WP to allow application of alternatives to read-only pages. */
write_cr0(cr0 & ~X86_CR0_WP);
- apply_alternatives_nocheck(start, end);
+ apply_alternatives(start, end);
/* Reinstate WP. */
write_cr0(cr0);
@@ -225,7 +225,7 @@ void __init alternative_instructions(void)
* expect a machine check to cause undue problems during to code
* patching.
*/
- apply_alternatives(__alt_instructions, __alt_instructions_end);
+ apply_alternatives_boot(__alt_instructions, __alt_instructions_end);
set_nmi_callback(saved_nmi_callback);
}
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 238492a..4458751 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -698,7 +698,7 @@ static int prepare_payload(struct payload *payload,
return -EINVAL;
}
}
- apply_alternatives_nocheck(start, end);
+ apply_alternatives(start, end);
}
sec = livepatch_elf_sec_by_name(elf, ".ex_table");
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 58d5fa7..b6fa827 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -26,7 +26,7 @@ struct alt_instr {
#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
void __init apply_alternatives_all(void);
-int apply_alternatives(void *start, size_t length);
+int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
#define ALTINSTR_ENTRY(feature) \
" .word 661b - .\n" /* label */ \
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index b72c401..47fa1be 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -28,10 +28,9 @@ struct alt_instr {
#define ALT_REPL_PTR(a) __ALT_PTR(a, repl_offset)
extern void add_nops(void *insns, unsigned int len);
-/* Similar to apply_alternatives except it can be run with IRQs enabled. */
-extern void apply_alternatives_nocheck(struct alt_instr *start,
- struct alt_instr *end);
+/* Similar to apply_alternatives_boot except it can be run with IRQs enabled. */
extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+extern void apply_alternatives_boot(struct alt_instr *start, struct alt_instr *end);
extern void alternative_instructions(void);
#define OLDINSTR(oldinstr) "661:\n\t" oldinstr "\n662:\n"
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 2:45 ` Konrad Rzeszutek Wilk
@ 2016-08-17 9:02 ` Andrew Cooper
0 siblings, 0 replies; 47+ messages in thread
From: Andrew Cooper @ 2016-08-17 9:02 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: sstabellini, ross.lagerwall, Julien Grall, Jan Beulich, xen-devel
>> The only reason apply_alternatives() is named thusly is to match Linux.
>> I am not fussed if it changes.
> Would this be OK with folks?
>
> There is a bit of disreprancy - ARM has 'const struct alt_instr *'
> where the 'const' gets dropped later on. That can't be done x86
> as 'apply_alternatives' at the gecko modifies the structure.
Where? I can't see anything which actually modifies the alt table, and
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index be40b13..3dd5de8 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -146,7 +146,7 @@ static void *init_or_livepatch text_poke(void *addr,
const void *opcode, size_t
*/
void init_or_livepatch apply_alternatives_nocheck(struct alt_instr
*start, struct alt_instr *end)
{
- struct alt_instr *a;
+ const struct alt_instr *a;
u8 *instr, *replacement;
u8 insnbuf[MAX_PATCH_LEN];
compiles just fine.
>
> Thoughts? Make it the same on ARM and x86?
>
> From 2c26d4d214926cd23b73f98c1fdaecd98b010da6 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Tue, 16 Aug 2016 22:20:54 -0400
> Subject: [PATCH] alternatives: x86 rename and change parameters on ARM
>
> On x86 we rename 'apply_alternatives' -> 'apply_alternatives_boot'
> and 'apply_alternatives_nocheck' to 'apply_alternatives'.
Looking at it, the current x86 apply_alternatives() can be folded into
its sole caller alternative_instructions(), removing the need for the
new apply_alternatives_boot().
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-14 23:07 ` [PATCH v1 6/9] livepatch: Initial ARM64 support Konrad Rzeszutek Wilk
2016-08-15 8:21 ` Jan Beulich
@ 2016-08-15 15:17 ` Julien Grall
2016-08-17 1:50 ` Konrad Rzeszutek Wilk
2016-08-17 17:12 ` Julien Grall
2016-08-17 18:22 ` Julien Grall
2 siblings, 2 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-15 15:17 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, sstabellini
Hi Konrad,
On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote:
[...]
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index b20db64..5966de0 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -4,8 +4,8 @@ obj-$(EARLY_PRINTK) += debug.o
> obj-y += domctl.o
> obj-y += domain.o
> obj-y += entry.o
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> obj-y += proc-v7.o proc-caxx.o
> obj-y += smpboot.o
> obj-y += traps.o
> obj-y += vfp.o
> -
Spurious change?
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> new file mode 100644
> index 0000000..89ee2f5
> --- /dev/null
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch_elf.h>
> +#include <xen/livepatch.h>
> +
> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> +{
> +}
> +
> +void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> +{
> +}
> +
> +int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> +{
> + const Elf_Ehdr *hdr = elf->hdr;
> +
> + if ( hdr->e_machine != EM_ARM ||
> + hdr->e_ident[EI_CLASS] != ELFCLASS32 )
> + {
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
> + elf->name);
> + return -EOPNOTSUPP;
> + }
> +
> + if ( (hdr->e_flags & EF_ARM_EABI_MASK) != EF_ARM_EABI_VER5 )
> + {
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF EABI(%x)!\n",
> + elf->name, hdr->e_flags);
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
I would prefer if you introduce ARM32 implementation in one go. I.e can
you only return -EOPNOTSUPP here for now?
[...]
> +int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> + const struct livepatch_elf_sec *base,
> + const struct livepatch_elf_sec *rela)
> +{
> + const Elf_RelA *r;
> + unsigned int symndx, i;
> + uint64_t val;
> + void *dest;
> +
> + for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> + {
> + int err = 0;
> +
> + r = rela->data + i * rela->sec->sh_entsize;
> +
> + symndx = ELF64_R_SYM(r->r_info);
> +
> + if ( symndx > elf->nsym )
> + {
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
> + elf->name, symndx);
> + return -EINVAL;
> + }
> +
> + dest = base->load_addr + r->r_offset; /* P */
> + val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
> +
> + /* ARM64 operations at minimum are always 32-bit. */
> + if ( r->r_offset >= base->sec->sh_size ||
> + (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
> + goto bad_offset;
> +
> + switch ( ELF64_R_TYPE(r->r_info) ) {
> + /* Data */
> + case R_AARCH64_ABS64:
> + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
> + goto bad_offset;
As you borrow the code from Linux, could we keep the abstraction with
reloc_data and defer the overflow check? It would avoid to have the same
if in multiple place in this code.
> + *(int64_t *)dest = val;
> + break;
> +
> + case R_AARCH64_ABS32:
> + *(int32_t *)dest = val;
> + if ( (int64_t)val != *(int32_t *)dest )
> + err = -EOVERFLOW;
> + break;
> +
> + case R_AARCH64_PREL64:
> + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
> + goto bad_offset;
> +
> + val -= (uint64_t)dest;
> + *(int64_t *)dest = val;
> + break;
> +
> + case R_AARCH64_PREL32:
> + val -= (uint64_t)dest;
> + *(int32_t *)dest = val;
> + if ( (int64_t)val != *(int32_t *)dest )
> + err = -EOVERFLOW;
> + break;
> +
> + /* Instructions. */
> + case R_AARCH64_ADR_PREL_LO21:
> + val -= (uint64_t)dest;
> + err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR);
> + break;
> +
It seems the implementation of R_AARCH64_ADR_PREL_PG_HI21_NC is missing.
> + case R_AARCH64_ADR_PREL_PG_HI21:
> + val = (val & ~0xfff) - ((u64)dest & ~0xfff);
> + err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR);
> + break;
Hmmm, I think we want to avoid the payload having
R_AARCH64_ADR_PREL_PG_HI21* relocations due to the erratum #843419 on
some Cortex-A53.
I haven't yet looked at how you build the payload, but this may also
affects the process to do it. I will give a look on it.
> +
> + case R_AARCH64_LDST8_ABS_LO12_NC:
> + case R_AARCH64_ADD_ABS_LO12_NC:
> + err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12);
> + if ( err == -EOVERFLOW )
> + err = 0;
> + break;
> +
> + case R_AARCH64_LDST16_ABS_LO12_NC:
> + err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12);
> + if ( err == -EOVERFLOW )
> + err = 0;
> + break;
> +
> + case R_AARCH64_LDST32_ABS_LO12_NC:
> + err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12);
> + if ( err == -EOVERFLOW )
> + err = 0;
> + break;
> +
> + case R_AARCH64_LDST64_ABS_LO12_NC:
> + err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12);
> + if ( err == -EOVERFLOW )
> + err = 0;
> + break;
> +
> + case R_AARCH64_CONDBR19:
> + err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19);
> + break;
> +
> + case R_AARCH64_JUMP26:
> + case R_AARCH64_CALL26:
> + val -= (uint64_t)dest;
> + err = reloc_insn_imm(dest, val, 2, 26, AARCH64_INSN_IMM_26);
> + break;
> +
> + default:
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation %lu\n",
> + elf->name, ELF64_R_TYPE(r->r_info));
> + return -EOPNOTSUPP;
> + }
> +
> + if ( err )
> + {
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
> + elf->name, i, rela->name, base->name);
> + return err;
> + }
> + }
> + return 0;
> +
> + bad_offset:
> + dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation offset is past %s section!\n",
> + elf->name, base->name);
> + return -EINVAL;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 15:17 ` Julien Grall
@ 2016-08-17 1:50 ` Konrad Rzeszutek Wilk
2016-08-17 19:44 ` Julien Grall
2016-08-17 17:12 ` Julien Grall
1 sibling, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-17 1:50 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ross.lagerwall, Andrew Cooper, sstabellini
> > +int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> > + const struct livepatch_elf_sec *base,
> > + const struct livepatch_elf_sec *rela)
> > +{
.. snip..
> > + switch ( ELF64_R_TYPE(r->r_info) ) {
> > + /* Data */
> > + case R_AARCH64_ABS64:
> > + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
> > + goto bad_offset;
>
> As you borrow the code from Linux, could we keep the abstraction with
> reloc_data and defer the overflow check? It would avoid to have the same if
> in multiple place in this code.
The above 'if' conditional is a check to make sure that we don't
go past the section (sh_size). In other words it is a boundary check to
make sure the Elf file is not messed up.
I can still copy the reloc_data so we lessen the:
> > + if ( (int64_t)val != *(int32_t *)dest )
> > + err = -EOVERFLOW;
And such.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 1:50 ` Konrad Rzeszutek Wilk
@ 2016-08-17 19:44 ` Julien Grall
0 siblings, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 19:44 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel, ross.lagerwall, Andrew Cooper, sstabellini
On 17/08/2016 02:50, Konrad Rzeszutek Wilk wrote:
>>> +int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>>> + const struct livepatch_elf_sec *base,
>>> + const struct livepatch_elf_sec *rela)
>>> +{
> .. snip..
>>> + switch ( ELF64_R_TYPE(r->r_info) ) {
>>> + /* Data */
>>> + case R_AARCH64_ABS64:
>>> + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
>>> + goto bad_offset;
>>
>> As you borrow the code from Linux, could we keep the abstraction with
>> reloc_data and defer the overflow check? It would avoid to have the same if
>> in multiple place in this code.
>
> The above 'if' conditional is a check to make sure that we don't
> go past the section (sh_size). In other words it is a boundary check to
> make sure the Elf file is not messed up.
Oh, Linux does not do those check. Sorry I though it was done. So I am
fine with that.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-15 15:17 ` Julien Grall
2016-08-17 1:50 ` Konrad Rzeszutek Wilk
@ 2016-08-17 17:12 ` Julien Grall
1 sibling, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 17:12 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, sstabellini
Hi Konrad,
On 15/08/16 16:17, Julien Grall wrote:
> On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote:
>> + case R_AARCH64_ADR_PREL_PG_HI21:
>> + val = (val & ~0xfff) - ((u64)dest & ~0xfff);
>> + err = reloc_insn_imm(dest, val, 12, 21,
>> AARCH64_INSN_IMM_ADR);
>> + break;
>
> Hmmm, I think we want to avoid the payload having
> R_AARCH64_ADR_PREL_PG_HI21* relocations due to the erratum #843419 on
> some Cortex-A53.
>
> I haven't yet looked at how you build the payload, but this may also
> affects the process to do it. I will give a look on it.
Actually, I am not sure I will have time to look at it during the next
few weeks. Could you add a TODO for now?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-14 23:07 ` [PATCH v1 6/9] livepatch: Initial ARM64 support Konrad Rzeszutek Wilk
2016-08-15 8:21 ` Jan Beulich
2016-08-15 15:17 ` Julien Grall
@ 2016-08-17 18:22 ` Julien Grall
2016-08-17 18:57 ` Konrad Rzeszutek Wilk
2016-08-24 2:14 ` Konrad Rzeszutek Wilk
2 siblings, 2 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 18:22 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, sstabellini
Hi Konrad,
On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
[...]
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> new file mode 100644
> index 0000000..e348942
> --- /dev/null
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -0,0 +1,247 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#include "../livepatch.h"
> +#include <xen/bitops.h>
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch_elf.h>
> +#include <xen/livepatch.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/byteorder.h>
> +#include <asm/insn.h>
> +
> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> +{
> + uint32_t insn;
> + uint32_t *old_ptr;
> + uint32_t *new_ptr;
> +
> + BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> + BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> +
> + ASSERT(vmap_of_xen_text);
> +
> + /* Save old one. */
> + old_ptr = func->old_addr;
> + memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> +
> + if ( func->new_size )
> + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> + (unsigned long)func->new_addr,
> + AARCH64_INSN_BRANCH_NOLINK);
The function aarch64_insn_gen_branch_imm will fail to create the branch
if the difference between old_addr and new_addr is greater than 128MB.
In this case, the function will return AARCH64_BREAK_FAULT which will
crash Xen when the instruction is executed.
I cannot find any sanity check in the hypervisor code. So are we
trusting the payload?
> + else
> + insn = aarch64_insn_gen_nop();
> +
> + new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +
> + /* PATCH! */
> + *(new_ptr) = cpu_to_le32(insn);
> +
> + clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
> +}
> +
> +void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> +{
> + uint32_t *new_ptr;
> + uint32_t insn;
> +
> + memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
> +
> + new_ptr = (uint32_t *)func->old_addr - (u32 *)_start + vmap_of_xen_text;
> +
> + /* PATCH! */
> + *(new_ptr) = cpu_to_le32(insn);
> +
> + clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*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;
> +}
> +
[...]
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 20bb2ba..607ee59 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -13,6 +13,7 @@
> #include <xen/hypercall.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> +#include <xen/livepatch.h>
> #include <xen/sched.h>
> #include <xen/softirq.h>
> #include <xen/wait.h>
> @@ -55,6 +56,11 @@ void idle_loop(void)
>
> do_tasklet();
> do_softirq();
> + /*
> + * We MUST be last (or before dsb, wfi). Otherwise after we get the
> + * softirq we would execute dsb,wfi (and sleep) and not patch.
> + */
> + check_for_livepatch_work();
> }
> }
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 3093554..19f6033 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -1,56 +1,93 @@
> /*
> * Copyright (C) 2016 Citrix Systems R&D Ltd.
> */
> +
Spurious change?
> +#include "livepatch.h"
> #include <xen/errno.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/livepatch_elf.h>
> #include <xen/livepatch.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/mm.h>
>
> -/* On ARM32,64 instructions are always 4 bytes long. */
> -#define PATCH_INSN_SIZE 4
Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly
move it in patch [1]?
> +void *vmap_of_xen_text;
>
> int arch_verify_insn_length(unsigned long len)
> {
> return len != PATCH_INSN_SIZE;
> }
>
> -void arch_livepatch_quiesce(void)
> +int arch_livepatch_quiesce(void)
It would have been nice to move the prototype change out of this patch
to keep it "straight forward".
> {
> + mfn_t text_mfn;
> + unsigned int text_order;
> +
> + if ( vmap_of_xen_text )
> + return -EINVAL;
> +
> + text_mfn = _mfn(virt_to_mfn(_stext));
> + text_order = get_order_from_bytes(_end - _start);
It is a bit odd that you use _stext before and the _start. But I won't
complain as I did the same in alternatives.c :/.
However, I think it should be enough to remap _stext -> _etext. I had to
map all the Xen binary for the alternatives because we may patch the
init text.
I forgot to mention it in the code, so I will send a patch to update it.
> +
> + /*
> + * The text section is read-only. So re-map Xen to be able to patch
> + * the code.
> + */
> + vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> + VMAP_DEFAULT);
> +
> + if ( !vmap_of_xen_text )
> + {
> + printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
> + text_order);
> + return -ENOMEM;
> + }
> + return 0;
> }
>
> void arch_livepatch_revive(void)
> {
> + /*
> + * Nuke the instruction cache. It has been cleaned before in
I guess you want to replace "It" by "Data cache" otherwise it does not
make much sense.
> + * arch_livepatch_apply_jmp.
What about the payload? It may contain instructions, so we need to
ensure that all the data reached the memory.
> + */
> + invalidate_icache();
> +
> + if ( vmap_of_xen_text )
> + vunmap(vmap_of_xen_text);
> +
> + vmap_of_xen_text = NULL;
> +
> + /*
> + * Need to flush the branch predicator for ARMv7 as it may be
s/predicator/predictor/
> + * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
> + */
> + flush_xen_text_tlb_local();
I am a bit confused. In your comment you mention the branch but flush
the TLBs. The two are not related.
However, I would prefer the branch predictor to be flushed directly in
invalidate_icache by calling BPIALLIS. This is because flushing the
cache means that you likely want to flush the branch predictor too.
> }
>
> int arch_livepatch_verify_func(const struct livepatch_func *func)
> {
> - return -ENOSYS;
> -}
> -
> -void arch_livepatch_apply_jmp(struct livepatch_func *func)
> -{
> -}
> + if ( func->old_size < PATCH_INSN_SIZE )
> + return -EINVAL;
>
> -void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> -{
> + return 0;
> }
>
> void arch_livepatch_post_action(void)
> {
> + /* arch_livepatch_revive has nuked the instruction cache. */
> }
>
> void arch_livepatch_mask(void)
> {
> + /* Mask System Error (SError) */
> + local_abort_disable();
> }
>
> void arch_livepatch_unmask(void)
> {
> -}
> -
> -int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> -{
> - return -ENOSYS;
> + local_abort_enable();
> }
>
> int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> @@ -60,20 +97,34 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> return -ENOSYS;
> }
>
> -int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> - const struct livepatch_elf_sec *base,
> - const struct livepatch_elf_sec *rela)
> -{
> - return -ENOSYS;
> -}
> -
> int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
> {
> - return -ENOSYS;
> + unsigned long start = (unsigned long)va;
> + unsigned int flags;
> +
> + ASSERT(va);
> + ASSERT(pages);
> +
> + if ( type == LIVEPATCH_VA_RX )
> + flags = PTE_RO; /* R set, NX clear */
> + else if ( type == LIVEPATCH_VA_RW )
> + flags = PTE_NX; /* R clear, NX set */
> + else
> + flags = PTE_NX | PTE_RO; /* R set, NX set */
va_type is an enum. So I would prefer to see a switch. So we can catch
more easily any addition of a new member.
> +
> + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
> +
> + return 0;
> }
>
> void __init arch_livepatch_init(void)
> {
> + void *start, *end;
> +
> + start = (void *)LIVEPATCH_VMAP;
> + end = start + MB(2);
Could you define the in config.h? So in the future we can rework more
easily the memory layout.
> +
> + vm_init_type(VMAP_XEN, start, end);
> }
>
> /*
> diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
> new file mode 100644
> index 0000000..8c8d625
> --- /dev/null
> +++ b/xen/arch/arm/livepatch.h
I am not sure why this header is living in arch/arm/ and not
include/asm-arm/
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + */
> +
> +#ifndef __XEN_ARM_LIVEPATCH_H__
> +#define __XEN_ARM_LIVEPATCH_H__
> +
> +/* On ARM32,64 instructions are always 4 bytes long. */
> +#define PATCH_INSN_SIZE 4
> +
> +/*
> + * The va of the hypervisor .text region. We need this as the
> + * normal va are write protected.
> + */
> +extern void *vmap_of_xen_text;
> +
> +#endif /* __XEN_ARM_LIVEPATCH_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 06c67bc..e3f3f37 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -15,10 +15,12 @@
>
> #define PATCH_INSN_SIZE 5
>
> -void arch_livepatch_quiesce(void)
> +int arch_livepatch_quiesce(void)
> {
> /* Disable WP to allow changes to read-only pages. */
> write_cr0(read_cr0() & ~X86_CR0_WP);
> +
> + return 0;
> }
>
> void arch_livepatch_revive(void)
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 51afa24..2fc76b6 100644
> --- 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"
> ---help---
> Allows a running Xen hypervisor to be dynamically patched using
> binary patches without rebooting. This is primarily used to binarily
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 9c45270..af9443d 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
> sizeof(*region->frame[i].bugs);
> }
>
> -#ifndef CONFIG_ARM
> +#ifndef CONFIG_ARM_32
I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.
> sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
> if ( sec )
> {
> @@ -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
> }
> +#endif
>
> +#ifndef CONFIG_ARM
> sec = livepatch_elf_sec_by_name(elf, ".ex_table");
> if ( sec )
> {
Any reason to not move .ex_table in an x86 specific file? Or maybe we
should define a new config option HAVE_ARCH_EX_TABLE to avoid
architecture specific check in the common code.
> @@ -1083,12 +1089,17 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
> static int apply_payload(struct payload *data)
> {
> unsigned int i;
> + int rc;
>
> printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
> data->name, data->nfuncs);
>
> - arch_livepatch_quiesce();
> -
> + rc = arch_livepatch_quiesce();
> + if ( rc )
> + {
> + printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> + return rc;
> + }
> /*
> * Since we are running with IRQs disabled and the hooks may call common
> * code - which expects the spinlocks to run with IRQs enabled - we temporarly
> @@ -1119,10 +1130,16 @@ static int apply_payload(struct payload *data)
> static int revert_payload(struct payload *data)
> {
> unsigned int i;
> + int rc;
>
> printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
>
> - arch_livepatch_quiesce();
> + rc = arch_livepatch_quiesce();
> + if ( rc )
> + {
> + printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> + return rc;
> + }
>
> for ( i = 0; i < data->nfuncs; i++ )
> arch_livepatch_revert_jmp(&data->funcs[i]);
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index a96f845..8d876f6 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -80,9 +80,10 @@
> * 4M - 6M Fixmap: special-purpose 4K mapping slots
> * 6M - 8M Early boot mapping of FDT
> * 8M - 10M Early relocation address (used when relocating Xen)
> + * and later for livepatch vmap (if compiled in)
> *
> * ARM32 layout:
> - * 0 - 8M <COMMON>
> + * 0 - 10M <COMMON>
May I ask to have this change and ...
> *
> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM
> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address
> @@ -93,7 +94,7 @@
> *
> * ARM64 layout:
> * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> - * 0 - 8M <COMMON>
> + * 0 - 10M <COMMON>
this change in a separate patch to keep this patch livepatch only?
> *
> * 1G - 2G VMAP: ioremap and early_ioremap
> *
> @@ -113,6 +114,9 @@
> #define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> #define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
> #define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00800000)
> +#ifdef CONFIG_LIVEPATCH
> +#define LIVEPATCH_VMAP _AT(vaddr_t,0x00800000)
> +#endif
>
> #define HYPERVISOR_VIRT_START XEN_VIRT_START
>
> diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
> index 65c0cdf..f4fcfd6 100644
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void)
>
> #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
>
> +#ifdef CONFIG_LIVEPATCH
> +#define switch_stack_and_jump(stack, fn) \
> + asm volatile ("mov sp,%0;" \
> + "bl check_for_livepatch_work;" \
> + "b " STR(fn) : : "r" (stack) : "memory" )
> +#else
> #define switch_stack_and_jump(stack, fn) \
> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> +#endif
I have commented on the previous version about switch_stack_and_jump a
while after you send this series. So I will spare you new comments here :).
> #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
>
[...]
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 2e64686..6f30c0d 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func);
> * These functions are called around the critical region patching live code,
> * for an architecture to take make appropratie global state adjustments.
> */
> -void arch_livepatch_quiesce(void);
> +int arch_livepatch_quiesce(void);
> void arch_livepatch_revive(void);
>
> void arch_livepatch_apply_jmp(struct livepatch_func *func);
>
[1]
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 18:22 ` Julien Grall
@ 2016-08-17 18:57 ` Konrad Rzeszutek Wilk
2016-08-17 19:50 ` Julien Grall
2016-08-22 19:22 ` Konrad Rzeszutek Wilk
2016-08-24 2:14 ` Konrad Rzeszutek Wilk
1 sibling, 2 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-17 18:57 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ross.lagerwall, Andrew Cooper, sstabellini
> > +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > +{
> > + uint32_t insn;
> > + uint32_t *old_ptr;
> > + uint32_t *new_ptr;
> > +
> > + BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > + BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> > +
> > + ASSERT(vmap_of_xen_text);
> > +
> > + /* Save old one. */
> > + old_ptr = func->old_addr;
> > + memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> > +
> > + if ( func->new_size )
> > + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> > + (unsigned long)func->new_addr,
> > + AARCH64_INSN_BRANCH_NOLINK);
>
> The function aarch64_insn_gen_branch_imm will fail to create the branch if
> the difference between old_addr and new_addr is greater than 128MB.
>
> In this case, the function will return AARCH64_BREAK_FAULT which will crash
> Xen when the instruction is executed.
>
> I cannot find any sanity check in the hypervisor code. So are we trusting
> the payload?
Ugh. This is a thorny one. I concur we need to check this - and better of
do it when we load the payload to make sure the distance is correct.
And that should also be on x86, so will spin of a seperate patch.
And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT
.. snip..
> > -/* On ARM32,64 instructions are always 4 bytes long. */
> > -#define PATCH_INSN_SIZE 4
>
> Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly
> move it in patch [1]?
Sure.
>
> > +void *vmap_of_xen_text;
> >
> > int arch_verify_insn_length(unsigned long len)
> > {
> > return len != PATCH_INSN_SIZE;
> > }
> >
> > -void arch_livepatch_quiesce(void)
> > +int arch_livepatch_quiesce(void)
>
> It would have been nice to move the prototype change out of this patch to
> keep it "straight forward".
OK.
>
> > {
> > + mfn_t text_mfn;
> > + unsigned int text_order;
> > +
> > + if ( vmap_of_xen_text )
> > + return -EINVAL;
> > +
> > + text_mfn = _mfn(virt_to_mfn(_stext));
> > + text_order = get_order_from_bytes(_end - _start);
>
> It is a bit odd that you use _stext before and the _start. But I won't
> complain as I did the same in alternatives.c :/.
Heheh.
>
> However, I think it should be enough to remap _stext -> _etext. I had to map
> all the Xen binary for the alternatives because we may patch the init text.
I agree.
>
> I forgot to mention it in the code, so I will send a patch to update it.
OK.
>
> > +
> > + /*
> > + * The text section is read-only. So re-map Xen to be able to patch
> > + * the code.
> > + */
> > + vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> > + VMAP_DEFAULT);
> > +
> > + if ( !vmap_of_xen_text )
> > + {
> > + printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
> > + text_order);
> > + return -ENOMEM;
> > + }
> > + return 0;
> > }
> >
> > void arch_livepatch_revive(void)
> > {
> > + /*
> > + * Nuke the instruction cache. It has been cleaned before in
>
> I guess you want to replace "It" by "Data cache" otherwise it does not make
> much sense.
>
> > + * arch_livepatch_apply_jmp.
>
> What about the payload? It may contain instructions, so we need to ensure
> that all the data reached the memory.
>
> > + */
> > + invalidate_icache();
> > +
> > + if ( vmap_of_xen_text )
> > + vunmap(vmap_of_xen_text);
> > +
> > + vmap_of_xen_text = NULL;
> > +
> > + /*
> > + * Need to flush the branch predicator for ARMv7 as it may be
>
> s/predicator/predictor/
>
> > + * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
> > + */
> > + flush_xen_text_tlb_local();
>
> I am a bit confused. In your comment you mention the branch but flush the
> TLBs. The two are not related.
>
> However, I would prefer the branch predictor to be flushed directly in
> invalidate_icache by calling BPIALLIS. This is because flushing the cache
> means that you likely want to flush the branch predictor too.
OK.
>
> > }
> >
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > - return -ENOSYS;
> > -}
> > -
> > -void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > -{
> > -}
> > + if ( func->old_size < PATCH_INSN_SIZE )
> > + return -EINVAL;
> >
> > -void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> > -{
> > + return 0;
> > }
> >
> > void arch_livepatch_post_action(void)
> > {
> > + /* arch_livepatch_revive has nuked the instruction cache. */
> > }
> >
> > void arch_livepatch_mask(void)
> > {
> > + /* Mask System Error (SError) */
> > + local_abort_disable();
> > }
> >
> > void arch_livepatch_unmask(void)
> > {
> > -}
> > -
> > -int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> > -{
> > - return -ENOSYS;
> > + local_abort_enable();
> > }
> >
> > int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> > @@ -60,20 +97,34 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> > return -ENOSYS;
> > }
> >
> > -int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> > - const struct livepatch_elf_sec *base,
> > - const struct livepatch_elf_sec *rela)
> > -{
> > - return -ENOSYS;
> > -}
> > -
> > int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
> > {
> > - return -ENOSYS;
> > + unsigned long start = (unsigned long)va;
> > + unsigned int flags;
> > +
> > + ASSERT(va);
> > + ASSERT(pages);
> > +
> > + if ( type == LIVEPATCH_VA_RX )
> > + flags = PTE_RO; /* R set, NX clear */
> > + else if ( type == LIVEPATCH_VA_RW )
> > + flags = PTE_NX; /* R clear, NX set */
> > + else
> > + flags = PTE_NX | PTE_RO; /* R set, NX set */
>
> va_type is an enum. So I would prefer to see a switch. So we can catch more
> easily any addition of a new member.
>
> > +
> > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
> > +
> > + return 0;
> > }
> >
> > void __init arch_livepatch_init(void)
> > {
> > + void *start, *end;
> > +
> > + start = (void *)LIVEPATCH_VMAP;
> > + end = start + MB(2);
>
> Could you define the in config.h? So in the future we can rework more easily
> the memory layout.
LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ?
>
> > +
> > + vm_init_type(VMAP_XEN, start, end);
> > }
> >
> > /*
> > diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
> > new file mode 100644
> > index 0000000..8c8d625
> > --- /dev/null
> > +++ b/xen/arch/arm/livepatch.h
>
> I am not sure why this header is living in arch/arm/ and not
> include/asm-arm/
>
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> > + *
> > + */
> > +
> > +#ifndef __XEN_ARM_LIVEPATCH_H__
> > +#define __XEN_ARM_LIVEPATCH_H__
> > +
> > +/* On ARM32,64 instructions are always 4 bytes long. */
> > +#define PATCH_INSN_SIZE 4
> > +
> > +/*
> > + * The va of the hypervisor .text region. We need this as the
> > + * normal va are write protected.
> > + */
> > +extern void *vmap_of_xen_text;
> > +
> > +#endif /* __XEN_ARM_LIVEPATCH_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> > index 06c67bc..e3f3f37 100644
> > --- a/xen/arch/x86/livepatch.c
> > +++ b/xen/arch/x86/livepatch.c
> > @@ -15,10 +15,12 @@
> >
> > #define PATCH_INSN_SIZE 5
> >
> > -void arch_livepatch_quiesce(void)
> > +int arch_livepatch_quiesce(void)
> > {
> > /* Disable WP to allow changes to read-only pages. */
> > write_cr0(read_cr0() & ~X86_CR0_WP);
> > +
> > + return 0;
> > }
> >
> > void arch_livepatch_revive(void)
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index 51afa24..2fc76b6 100644
> > --- 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"
> > ---help---
> > Allows a running Xen hypervisor to be dynamically patched using
> > binary patches without rebooting. This is primarily used to binarily
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 9c45270..af9443d 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
> > sizeof(*region->frame[i].bugs);
> > }
> >
> > -#ifndef CONFIG_ARM
> > +#ifndef CONFIG_ARM_32
>
> I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.
That is not exposed on x86 thought.
I can expose HAVE_ALTERNATIVE on x86 and use that instead.
>
> > sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
> > if ( sec )
> > {
> > @@ -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
> > }
> > +#endif
> >
> > +#ifndef CONFIG_ARM
> > sec = livepatch_elf_sec_by_name(elf, ".ex_table");
> > if ( sec )
> > {
>
> Any reason to not move .ex_table in an x86 specific file? Or maybe we should
Would need to create an arch specific hook for this.
> define a new config option HAVE_ARCH_EX_TABLE to avoid architecture specific
> check in the common code.
That could do it too.
>
> > @@ -1083,12 +1089,17 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
> > static int apply_payload(struct payload *data)
> > {
> > unsigned int i;
> > + int rc;
> >
> > printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
> > data->name, data->nfuncs);
> >
> > - arch_livepatch_quiesce();
> > -
> > + rc = arch_livepatch_quiesce();
> > + if ( rc )
> > + {
> > + printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> > + return rc;
> > + }
> > /*
> > * Since we are running with IRQs disabled and the hooks may call common
> > * code - which expects the spinlocks to run with IRQs enabled - we temporarly
> > @@ -1119,10 +1130,16 @@ static int apply_payload(struct payload *data)
> > static int revert_payload(struct payload *data)
> > {
> > unsigned int i;
> > + int rc;
> >
> > printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
> >
> > - arch_livepatch_quiesce();
> > + rc = arch_livepatch_quiesce();
> > + if ( rc )
> > + {
> > + printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> > + return rc;
> > + }
> >
> > for ( i = 0; i < data->nfuncs; i++ )
> > arch_livepatch_revert_jmp(&data->funcs[i]);
> > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > index a96f845..8d876f6 100644
> > --- a/xen/include/asm-arm/config.h
> > +++ b/xen/include/asm-arm/config.h
> > @@ -80,9 +80,10 @@
> > * 4M - 6M Fixmap: special-purpose 4K mapping slots
> > * 6M - 8M Early boot mapping of FDT
> > * 8M - 10M Early relocation address (used when relocating Xen)
> > + * and later for livepatch vmap (if compiled in)
> > *
> > * ARM32 layout:
> > - * 0 - 8M <COMMON>
> > + * 0 - 10M <COMMON>
>
> May I ask to have this change and ...
>
> > *
> > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM
> > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address
> > @@ -93,7 +94,7 @@
> > *
> > * ARM64 layout:
> > * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> > - * 0 - 8M <COMMON>
> > + * 0 - 10M <COMMON>
>
> this change in a separate patch to keep this patch livepatch only?
Of course.
>
> > *
> > * 1G - 2G VMAP: ioremap and early_ioremap
> > *
> > @@ -113,6 +114,9 @@
> > #define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
> > #define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
> > #define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00800000)
> > +#ifdef CONFIG_LIVEPATCH
> > +#define LIVEPATCH_VMAP _AT(vaddr_t,0x00800000)
> > +#endif
> >
> > #define HYPERVISOR_VIRT_START XEN_VIRT_START
> >
> > diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
> > index 65c0cdf..f4fcfd6 100644
> > --- a/xen/include/asm-arm/current.h
> > +++ b/xen/include/asm-arm/current.h
> > @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void)
> >
> > #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
> >
> > +#ifdef CONFIG_LIVEPATCH
> > +#define switch_stack_and_jump(stack, fn) \
> > + asm volatile ("mov sp,%0;" \
> > + "bl check_for_livepatch_work;" \
> > + "b " STR(fn) : : "r" (stack) : "memory" )
> > +#else
> > #define switch_stack_and_jump(stack, fn) \
> > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> > +#endif
>
> I have commented on the previous version about switch_stack_and_jump a while
> after you send this series. So I will spare you new comments here :).
:-)
>
> > #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
> >
>
> [...]
>
> > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> > index 2e64686..6f30c0d 100644
> > --- a/xen/include/xen/livepatch.h
> > +++ b/xen/include/xen/livepatch.h
> > @@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func);
> > * These functions are called around the critical region patching live code,
> > * for an architecture to take make appropratie global state adjustments.
> > */
> > -void arch_livepatch_quiesce(void);
> > +int arch_livepatch_quiesce(void);
> > void arch_livepatch_revive(void);
> >
> > void arch_livepatch_apply_jmp(struct livepatch_func *func);
> >
>
> [1]
> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 18:57 ` Konrad Rzeszutek Wilk
@ 2016-08-17 19:50 ` Julien Grall
2016-08-22 19:22 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 19:50 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel, ross.lagerwall, Andrew Cooper, sstabellini
Hi Konrad,
On 17/08/2016 19:57, Konrad Rzeszutek Wilk wrote:
>>> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
>>> +{
>>> + uint32_t insn;
>>> + uint32_t *old_ptr;
>>> + uint32_t *new_ptr;
>>> +
>>> + BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>>> + BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
>>> +
>>> + ASSERT(vmap_of_xen_text);
>>> +
>>> + /* Save old one. */
>>> + old_ptr = func->old_addr;
>>> + memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>>> +
>>> + if ( func->new_size )
>>> + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
>>> + (unsigned long)func->new_addr,
>>> + AARCH64_INSN_BRANCH_NOLINK);
>>
>> The function aarch64_insn_gen_branch_imm will fail to create the branch if
>> the difference between old_addr and new_addr is greater than 128MB.
>>
>> In this case, the function will return AARCH64_BREAK_FAULT which will crash
>> Xen when the instruction is executed.
>>
>> I cannot find any sanity check in the hypervisor code. So are we trusting
>> the payload?
>
> Ugh. This is a thorny one. I concur we need to check this - and better of
> do it when we load the payload to make sure the distance is correct.
>
> And that should also be on x86, so will spin of a seperate patch.
>
> And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT
It sounds good to me.
[...]
>>> +
>>> + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
>>> +
>>> + return 0;
>>> }
>>>
>>> void __init arch_livepatch_init(void)
>>> {
>>> + void *start, *end;
>>> +
>>> + start = (void *)LIVEPATCH_VMAP;
>>> + end = start + MB(2);
>>
>> Could you define the in config.h? So in the future we can rework more easily
>> the memory layout.
>
> LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ?
Something like that.
>
>>
>>> +
>>> + vm_init_type(VMAP_XEN, start, end);
>>> }
>>>
>>> /*
>>> diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
>>> new file mode 100644
>>> index 0000000..8c8d625
>>> --- /dev/null
>>> +++ b/xen/arch/arm/livepatch.h
>>
>> I am not sure why this header is living in arch/arm/ and not
>> include/asm-arm/
>>
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
>>> + *
>>> + */
>>> +
>>> +#ifndef __XEN_ARM_LIVEPATCH_H__
>>> +#define __XEN_ARM_LIVEPATCH_H__
>>> +
>>> +/* On ARM32,64 instructions are always 4 bytes long. */
>>> +#define PATCH_INSN_SIZE 4
>>> +
>>> +/*
>>> + * The va of the hypervisor .text region. We need this as the
>>> + * normal va are write protected.
>>> + */
>>> +extern void *vmap_of_xen_text;
>>> +
>>> +#endif /* __XEN_ARM_LIVEPATCH_H__ */
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
>>> index 06c67bc..e3f3f37 100644
>>> --- a/xen/arch/x86/livepatch.c
>>> +++ b/xen/arch/x86/livepatch.c
>>> @@ -15,10 +15,12 @@
>>>
>>> #define PATCH_INSN_SIZE 5
>>>
>>> -void arch_livepatch_quiesce(void)
>>> +int arch_livepatch_quiesce(void)
>>> {
>>> /* Disable WP to allow changes to read-only pages. */
>>> write_cr0(read_cr0() & ~X86_CR0_WP);
>>> +
>>> + return 0;
>>> }
>>>
>>> void arch_livepatch_revive(void)
>>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>>> index 51afa24..2fc76b6 100644
>>> --- 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"
>>> ---help---
>>> Allows a running Xen hypervisor to be dynamically patched using
>>> binary patches without rebooting. This is primarily used to binarily
>>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>>> index 9c45270..af9443d 100644
>>> --- a/xen/common/livepatch.c
>>> +++ b/xen/common/livepatch.c
>>> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
>>> sizeof(*region->frame[i].bugs);
>>> }
>>>
>>> -#ifndef CONFIG_ARM
>>> +#ifndef CONFIG_ARM_32
>>
>> I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.
>
> That is not exposed on x86 thought.
>
> I can expose HAVE_ALTERNATIVE on x86 and use that instead.
True. I would like to avoid using CONFIG_ARM_* in the common code as it
is a call to forget to remove the #ifdef when ARM32 will gain
alternative patching.
You are the maintainer of this code, so it is your call :).
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 18:57 ` Konrad Rzeszutek Wilk
2016-08-17 19:50 ` Julien Grall
@ 2016-08-22 19:22 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-22 19:22 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ross.lagerwall, Andrew Cooper, sstabellini
> > > -/* On ARM32,64 instructions are always 4 bytes long. */
> > > -#define PATCH_INSN_SIZE 4
> >
> > Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly
> > move it in patch [1]?
>
> Sure.
The patch [1] changed where it does not have <len> anymore. And because
of that (and some other changes) this patch is the one that introduces
the #define. So I think having it in include/asm-arm/livepatch.h would
work. When I send out the patchset that hopefully should be more clarified.
..
> > [1]
> > https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html
> >
> > --
> > Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-17 18:22 ` Julien Grall
2016-08-17 18:57 ` Konrad Rzeszutek Wilk
@ 2016-08-24 2:14 ` Konrad Rzeszutek Wilk
2016-08-25 13:54 ` Julien Grall
1 sibling, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-24 2:14 UTC (permalink / raw)
To: Julien Grall; +Cc: Xen-devel, Ross Lagerwall, Andrew Cooper, Stefano Stabellini
>> + flush_xen_text_tlb_local();
>
>
> I am a bit confused. In your comment you mention the branch but flush the
> TLBs. The two are not related.
That code also flushes the I-cache (which sounds redundant with
invalidate_icache).
And it also does 'isb' ("Ensure synchronization with previous changes
to text ") which
invalidate_icache() does not.
I am wondering if should just move 'isb' in invalidate_icache() and
drop altogether the call to flush_xen_text_tlb_local()?
It works fine in simulator
>
> However, I would prefer the branch predictor to be flushed directly in
> invalidate_icache by calling BPIALLIS. This is because flushing the cache
> means that you likely want to flush the branch predictor too.
I had no trouble adding invalidate_icache to asm-arm/arm32/page.h and
having it do :
asm volatile (
CMD_CP32(ICIALLUIS) /* Flush I-cache. */
CMD_CP32(BPIALLIS) /* Flush branch predictor. */
: : : "memory");
But obviously that does not help ARM64.
And that is is where I hit an snag. "bp iallis" instruction does not
compile? (I also tried "bp iall", "bpiallis"). The error is:
{standard input}: Assembler messages:
{standard input}:454: Error: unknown mnemonic `bpiallis' -- `bpiallis'
Ideas?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.
2016-08-24 2:14 ` Konrad Rzeszutek Wilk
@ 2016-08-25 13:54 ` Julien Grall
0 siblings, 0 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-25 13:54 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Xen-devel, Ross Lagerwall, Andrew Cooper, Stefano Stabellini
Hi Konrad,
On 23/08/2016 22:14, Konrad Rzeszutek Wilk wrote:
>>> + flush_xen_text_tlb_local();
>>
>>
>> I am a bit confused. In your comment you mention the branch but flush the
>> TLBs. The two are not related.
>
> That code also flushes the I-cache (which sounds redundant with
> invalidate_icache).
> And it also does 'isb' ("Ensure synchronization with previous changes
> to text ") which
> invalidate_icache() does not.
>
> I am wondering if should just move 'isb' in invalidate_icache() and
> drop altogether the call to flush_xen_text_tlb_local()?
flush_xen_text_tlb_local will flush the TLB for the local processor, I
am not sure why we would want to do that. It would be better to have a
invalidate_icache helpers on ARM32.
>
> It works fine in simulator
>
>>
>> However, I would prefer the branch predictor to be flushed directly in
>> invalidate_icache by calling BPIALLIS. This is because flushing the cache
>> means that you likely want to flush the branch predictor too.
>
> I had no trouble adding invalidate_icache to asm-arm/arm32/page.h and
> having it do :
>
> asm volatile (
> CMD_CP32(ICIALLUIS) /* Flush I-cache. */
> CMD_CP32(BPIALLIS) /* Flush branch predictor. */
> : : : "memory");
>
> But obviously that does not help ARM64.
>
> And that is is where I hit an snag. "bp iallis" instruction does not
> compile? (I also tried "bp iall", "bpiallis"). The error is:
>
> {standard input}: Assembler messages:
> {standard input}:454: Error: unknown mnemonic `bpiallis' -- `bpiallis'
The instruction bpiallis does not exist on ARM64 because the branch
predictor does not require explicit flush.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (5 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 6/9] livepatch: Initial ARM64 support Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 12:21 ` Jan Beulich
2016-08-14 23:07 ` [PATCH v1 8/9] livepatch: Move test-cases to common Konrad Rzeszutek Wilk
` (2 subsequent siblings)
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Jan Beulich,
Konrad Rzeszutek Wilk
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@oracle.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v1: First submission
---
xen/arch/arm/livepatch.c | 29 +++++++++++++++++++++++++++++
xen/arch/x86/livepatch.c | 7 +++++++
xen/common/livepatch.c | 2 +-
xen/include/xen/livepatch.h | 2 ++
4 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 19f6033..833b06b 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -90,6 +90,35 @@ void arch_livepatch_unmask(void)
local_abort_enable();
}
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+ const struct livepatch_elf_sym *sym)
+{
+ /*
+ * - Mapping symbols - denote the "start of a sequence of bytes of the
+ * appropiate type" to mark certain features - such as start of region
+ * containing A64 ($x), ARM ($a), or Thumb instructions ($t); or data ($d)
+ *
+ * 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'
+#else
+ p == 'x'
+#endif
+ )
+ return 0;
+ }
+ return 1;
+}
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
const struct livepatch_elf_sec *rela)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index e3f3f37..f3c6f46 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -116,6 +116,13 @@ int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
return 0;
}
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+ const struct livepatch_elf_sym *sym)
+{
+ /* No special checks on x86. */
+ return 1;
+}
+
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
const struct livepatch_elf_sec *rela)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index af9443d..94bfb07 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -780,7 +780,7 @@ static bool_t is_payload_symbol(const struct livepatch_elf *elf,
!strncmp(sym->name, ".L", 2) )
return 0;
- return 1;
+ return arch_is_payload_symbol(elf, sym);
}
static int build_symbol_table(struct payload *payload,
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 6f30c0d..d94155f 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -46,6 +46,8 @@ bool_t is_patch(const void *addr);
/* Arch hooks. */
int arch_verify_insn_length(unsigned long len);
int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+ const struct livepatch_elf_sym *sym);
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
const struct livepatch_elf_sec *rela);
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]
2016-08-14 23:07 ` [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p] Konrad Rzeszutek Wilk
@ 2016-08-17 12:21 ` Jan Beulich
2016-08-22 17:14 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 47+ messages in thread
From: Jan Beulich @ 2016-08-17 12:21 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
According to the code you mean $t instead of $p in the subject.
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -90,6 +90,35 @@ void arch_livepatch_unmask(void)
> local_abort_enable();
> }
>
> +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> + const struct livepatch_elf_sym *sym)
> +{
> + /*
> + * - Mapping symbols - denote the "start of a sequence of bytes of the
> + * appropiate type" to mark certain features - such as start of region
> + * containing A64 ($x), ARM ($a), or Thumb instructions ($t); or data ($d)
> + *
> + * 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' ||
May I suggest not nesting two if()-s like this?
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -780,7 +780,7 @@ static bool_t is_payload_symbol(const struct livepatch_elf *elf,
> !strncmp(sym->name, ".L", 2) )
> return 0;
>
> - return 1;
> + return arch_is_payload_symbol(elf, sym);
> }
Taking into consideration what's still visible as context here - are .L
prefixed symbols really architecture independent? If not, checking
for them should probably be moved.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]
2016-08-17 12:21 ` Jan Beulich
@ 2016-08-22 17:14 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-22 17:14 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, ross.lagerwall, Andrew Cooper, julien.grall, xen-devel
On Wed, Aug 17, 2016 at 06:21:03AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
>
> According to the code you mean $t instead of $p in the subject.
Yes! Thanks for noticing that.
>
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -90,6 +90,35 @@ void arch_livepatch_unmask(void)
> > local_abort_enable();
> > }
> >
> > +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> > + const struct livepatch_elf_sym *sym)
> > +{
> > + /*
> > + * - Mapping symbols - denote the "start of a sequence of bytes of the
> > + * appropiate type" to mark certain features - such as start of region
> > + * containing A64 ($x), ARM ($a), or Thumb instructions ($t); or data ($d)
> > + *
> > + * 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' ||
>
> May I suggest not nesting two if()-s like this?
>
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -780,7 +780,7 @@ static bool_t is_payload_symbol(const struct livepatch_elf *elf,
> > !strncmp(sym->name, ".L", 2) )
> > return 0;
> >
> > - return 1;
> > + return arch_is_payload_symbol(elf, sym);
> > }
>
> Taking into consideration what's still visible as context here - are .L
> prefixed symbols really architecture independent? If not, checking
They are architecture independent and even the ARM ELF document mentions
it:
"Note: Multiple conventions exist for the names of compiler temporary symbols
(for example, ARMCC uses Lxxx.yyy, while GNU uses .Lxxx)."
(pg 23)
> for them should probably be moved.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 8/9] livepatch: Move test-cases to common
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (6 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p] Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 12:24 ` Jan Beulich
2016-08-14 23:07 ` [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64 Konrad Rzeszutek Wilk
2016-08-15 14:52 ` [PATCH v1] Livepatch ARM 64 implementation Julien Grall
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: Andrew Cooper, julien.grall, sstabellini, Jan Beulich,
Konrad Rzeszutek Wilk
So they can be shared with ARM64 (but not yet, so they
are only built on x86).
No functional change.
We also need to tweak the MAINTAINERS and .gitignore file
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v1: First submission
---
.gitignore | 8 +--
MAINTAINERS | 1 +
xen/Makefile | 2 +-
xen/arch/arm/Makefile | 3 --
xen/arch/x86/Makefile | 4 --
xen/arch/x86/test/Makefile | 85 ------------------------------
xen/arch/x86/test/xen_bye_world.c | 34 ------------
xen/arch/x86/test/xen_bye_world_func.c | 22 --------
xen/arch/x86/test/xen_hello_world.c | 68 ------------------------
xen/arch/x86/test/xen_hello_world_func.c | 39 --------------
xen/arch/x86/test/xen_replace_world.c | 33 ------------
xen/arch/x86/test/xen_replace_world_func.c | 22 --------
xen/common/Makefile | 6 +++
xen/common/test/Makefile | 85 ++++++++++++++++++++++++++++++
xen/common/test/xen_bye_world.c | 34 ++++++++++++
xen/common/test/xen_bye_world_func.c | 22 ++++++++
xen/common/test/xen_hello_world.c | 68 ++++++++++++++++++++++++
xen/common/test/xen_hello_world_func.c | 39 ++++++++++++++
xen/common/test/xen_replace_world.c | 33 ++++++++++++
xen/common/test/xen_replace_world_func.c | 22 ++++++++
20 files changed, 315 insertions(+), 315 deletions(-)
delete mode 100644 xen/arch/x86/test/Makefile
delete mode 100644 xen/arch/x86/test/xen_bye_world.c
delete mode 100644 xen/arch/x86/test/xen_bye_world_func.c
delete mode 100644 xen/arch/x86/test/xen_hello_world.c
delete mode 100644 xen/arch/x86/test/xen_hello_world_func.c
delete mode 100644 xen/arch/x86/test/xen_replace_world.c
delete mode 100644 xen/arch/x86/test/xen_replace_world_func.c
create mode 100644 xen/common/test/Makefile
create mode 100644 xen/common/test/xen_bye_world.c
create mode 100644 xen/common/test/xen_bye_world_func.c
create mode 100644 xen/common/test/xen_hello_world.c
create mode 100644 xen/common/test/xen_hello_world_func.c
create mode 100644 xen/common/test/xen_replace_world.c
create mode 100644 xen/common/test/xen_replace_world_func.c
diff --git a/.gitignore b/.gitignore
index 44cc7bf..bef1dee 100644
--- a/.gitignore
+++ b/.gitignore
@@ -255,14 +255,14 @@ xen/arch/x86/efi.lds
xen/arch/x86/efi/check.efi
xen/arch/x86/efi/disabled
xen/arch/x86/efi/mkreloc
-xen/arch/x86/test/config.h
-xen/arch/x86/test/xen_hello_world.livepatch
-xen/arch/x86/test/xen_bye_world.livepatch
-xen/arch/x86/test/xen_replace_world.livepatch
xen/arch/*/efi/boot.c
xen/arch/*/efi/compat.c
xen/arch/*/efi/efi.h
xen/arch/*/efi/runtime.c
+xen/common/test/config.h
+xen/common/test/xen_bye_world.livepatch
+xen/common/test/xen_hello_world.livepatch
+xen/common/test/xen_replace_world.livepatch
xen/include/headers.chk
xen/include/headers++.chk
xen/include/asm
diff --git a/MAINTAINERS b/MAINTAINERS
index ae0b6bc..e5763f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -271,6 +271,7 @@ F: tools/misc/xen-livepatch.c
F: xen/arch/*/livepatch*
F: xen/arch/*/*/livepatch*
F: xen/common/livepatch*
+F: xen/common/test/*
F: xen/include/xen/livepatch*
MACHINE CHECK (MCA) & RAS
diff --git a/xen/Makefile b/xen/Makefile
index a4c72be..dc8980e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -79,7 +79,7 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
.PHONY: _tests
_tests:
- $(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) tests
+ $(MAKE) -f $(BASEDIR)/Rules.mk -C common tests
.PHONY: _uninstall
_uninstall: D=$(DESTDIR)
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9f75c5c..9dc0797 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -73,9 +73,6 @@ ifeq ($(CONFIG_ARM_64),y)
ln -sf $(notdir $@) ../../$(notdir $@).efi
endif
-.PHONY: tests
-tests:
-
$(TARGET).axf: $(TARGET)-syms
# XXX: VE model loads by VMA so instead of
# making a proper ELF we link with LMA == VMA and adjust crudely
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d87ecd8..6f74346 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -92,10 +92,6 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \
`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
-.PHONY: tests
-tests:
- $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
-
ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
ifeq ($(lto),y)
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
deleted file mode 100644
index 23dff1d..0000000
--- a/xen/arch/x86/test/Makefile
+++ /dev/null
@@ -1,85 +0,0 @@
-include $(XEN_ROOT)/Config.mk
-
-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}')
-
-.PHONY: default
-
-LIVEPATCH := xen_hello_world.livepatch
-LIVEPATCH_BYE := xen_bye_world.livepatch
-LIVEPATCH_REPLACE := xen_replace_world.livepatch
-
-default: livepatch
-
-install: livepatch
- $(INSTALL_DATA) $(LIVEPATCH) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
- $(INSTALL_DATA) $(LIVEPATCH_BYE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
- $(INSTALL_DATA) $(LIVEPATCH_REPLACE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
-uninstall:
- rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
- rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
- rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
-
-.PHONY: clean
-clean::
- rm -f *.o .*.o.d *.livepatch config.h
-
-#
-# To compute these values we need the binary files: xen-syms
-# and xen_hello_world_func.o to be already compiled.
-#
-.PHONY: config.h
-config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version)
-config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world)
-config.h: xen_hello_world_func.o
- (set -e; \
- echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \
- echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
-
-xen_hello_world.o: config.h
-
-.PHONY: $(LIVEPATCH)
-$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
- $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
-
-#
-# This target is only accessible if CONFIG_LIVEPATCH is defined, which
-# depends on $(build_id_linker) being available. Hence we do not
-# need any checks.
-#
-# N.B. The reason we don't use arch/x86/note.o is that it may
-# not be built (it is for EFI builds), and that we do not have
-# the note.o.bin to muck with (as it gets deleted)
-#
-.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 \
- --rename-section=.data=.livepatch.depends -S $@.bin $@
- rm -f $@.bin
-
-#
-# Extract the build-id of the xen_hello_world.livepatch
-# (which xen_bye_world will depend on).
-#
-.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 \
- --rename-section=.data=.livepatch.depends -S $@.bin $@
- rm -f $@.bin
-
-xen_bye_world.o: config.h
-
-.PHONY: $(LIVEPATCH_BYE)
-$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o
- $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
-
-xen_replace_world.o: config.h
-
-.PHONY: $(LIVEPATCH_REPLACE)
-$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o
- $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
-
-.PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE)
diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/arch/x86/test/xen_bye_world.c
deleted file mode 100644
index b75e0b1..0000000
--- a/xen/arch/x86/test/xen_bye_world.c
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include "config.h"
-#include <xen/lib.h>
-#include <xen/types.h>
-#include <xen/version.h>
-#include <xen/livepatch.h>
-
-#include <public/sysctl.h>
-
-static char bye_world_patch_this_fnc[] = "xen_extra_version";
-extern const char *xen_bye_world(void);
-
-struct livepatch_func __section(".livepatch.funcs") livepatch_xen_bye_world = {
- .version = LIVEPATCH_PAYLOAD_VERSION,
- .name = bye_world_patch_this_fnc,
- .new_addr = xen_bye_world,
- .old_addr = xen_extra_version,
- .new_size = NEW_CODE_SZ,
- .old_size = OLD_CODE_SZ,
-};
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/test/xen_bye_world_func.c b/xen/arch/x86/test/xen_bye_world_func.c
deleted file mode 100644
index 32ef341..0000000
--- a/xen/arch/x86/test/xen_bye_world_func.c
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include <xen/types.h>
-
-/* Our replacement function for xen_hello_world. */
-const char *xen_bye_world(void)
-{
- return "Bye World!";
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
deleted file mode 100644
index 66c5b8e..0000000
--- a/xen/arch/x86/test/xen_hello_world.c
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include "config.h"
-#include <xen/lib.h>
-#include <xen/types.h>
-#include <xen/version.h>
-#include <xen/livepatch.h>
-#include <xen/livepatch_payload.h>
-
-#include <public/sysctl.h>
-
-static char hello_world_patch_this_fnc[] = "xen_extra_version";
-extern const char *xen_hello_world(void);
-static unsigned int cnt;
-
-static void apply_hook(void)
-{
- printk(KERN_DEBUG "Hook executing.\n");
-}
-
-static void revert_hook(void)
-{
- printk(KERN_DEBUG "Hook unloaded.\n");
-}
-
-static void hi_func(void)
-{
- printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
-};
-
-/* If we are sorted we _MUST_ be the last .livepatch.hook section. */
-static void z_check_fnc(void)
-{
- printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
- BUG_ON(cnt == 0 || cnt > 2);
-}
-
-LIVEPATCH_LOAD_HOOK(apply_hook);
-LIVEPATCH_UNLOAD_HOOK(revert_hook);
-
-/* Imbalance here. Two load and three unload. */
-
-LIVEPATCH_LOAD_HOOK(hi_func);
-LIVEPATCH_UNLOAD_HOOK(hi_func);
-
-LIVEPATCH_UNLOAD_HOOK(z_check_fnc);
-
-struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
- .version = LIVEPATCH_PAYLOAD_VERSION,
- .name = hello_world_patch_this_fnc,
- .new_addr = xen_hello_world,
- .old_addr = xen_extra_version,
- .new_size = NEW_CODE_SZ,
- .old_size = OLD_CODE_SZ,
-};
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/arch/x86/test/xen_hello_world_func.c
deleted file mode 100644
index 03d6b84..0000000
--- a/xen/arch/x86/test/xen_hello_world_func.c
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include <xen/types.h>
-
-#include <asm/alternative.h>
-#include <asm/nops.h>
-#include <asm/uaccess.h>
-
-static unsigned long *non_canonical_addr = (unsigned long *)0xdead000000000000ULL;
-
-/* Our replacement function for xen_extra_version. */
-const char *xen_hello_world(void)
-{
- unsigned long tmp;
- int rc;
-
- alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);
- /*
- * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
- * exceptions will be caught and processed properly.
- */
- rc = __get_user(tmp, non_canonical_addr);
- BUG_ON(rc != -EFAULT);
-
- return "Hello World";
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/test/xen_replace_world.c b/xen/arch/x86/test/xen_replace_world.c
deleted file mode 100644
index a2a221a..0000000
--- a/xen/arch/x86/test/xen_replace_world.c
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include "config.h"
-#include <xen/lib.h>
-#include <xen/types.h>
-#include <xen/livepatch.h>
-
-#include <public/sysctl.h>
-
-static char xen_replace_world_name[] = "xen_extra_version";
-extern const char *xen_replace_world(void);
-
-struct livepatch_func __section(".livepatch.funcs") livepatch_xen_replace_world = {
- .version = LIVEPATCH_PAYLOAD_VERSION,
- .name = xen_replace_world_name,
- .old_addr = 0, /* Forces the hypervisor to lookup .name */
- .new_addr = xen_replace_world,
- .new_size = NEW_CODE_SZ,
- .old_size = OLD_CODE_SZ,
-};
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/test/xen_replace_world_func.c b/xen/arch/x86/test/xen_replace_world_func.c
deleted file mode 100644
index afb5cda..0000000
--- a/xen/arch/x86/test/xen_replace_world_func.c
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
- *
- */
-
-#include <xen/types.h>
-
-/* Our replacement function for xen_hello_world. */
-const char *xen_replace_world(void)
-{
- return "Hello Again World!";
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/common/Makefile b/xen/common/Makefile
index c2e6846..22806b6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -79,3 +79,9 @@ subdir-$(coverage) += gcov
subdir-y += libelf
subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+
+.PHONY: tests
+tests:
+ifeq ($(XEN_TARGET_ARCH),x86_64)
+ $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
+endif
diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
new file mode 100644
index 0000000..23dff1d
--- /dev/null
+++ b/xen/common/test/Makefile
@@ -0,0 +1,85 @@
+include $(XEN_ROOT)/Config.mk
+
+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}')
+
+.PHONY: default
+
+LIVEPATCH := xen_hello_world.livepatch
+LIVEPATCH_BYE := xen_bye_world.livepatch
+LIVEPATCH_REPLACE := xen_replace_world.livepatch
+
+default: livepatch
+
+install: livepatch
+ $(INSTALL_DATA) $(LIVEPATCH) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
+ $(INSTALL_DATA) $(LIVEPATCH_BYE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
+ $(INSTALL_DATA) $(LIVEPATCH_REPLACE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
+uninstall:
+ rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH)
+ rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE)
+ rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE)
+
+.PHONY: clean
+clean::
+ rm -f *.o .*.o.d *.livepatch config.h
+
+#
+# To compute these values we need the binary files: xen-syms
+# and xen_hello_world_func.o to be already compiled.
+#
+.PHONY: config.h
+config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version)
+config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world)
+config.h: xen_hello_world_func.o
+ (set -e; \
+ echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \
+ echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
+
+xen_hello_world.o: config.h
+
+.PHONY: $(LIVEPATCH)
+$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
+
+#
+# This target is only accessible if CONFIG_LIVEPATCH is defined, which
+# depends on $(build_id_linker) being available. Hence we do not
+# need any checks.
+#
+# N.B. The reason we don't use arch/x86/note.o is that it may
+# not be built (it is for EFI builds), and that we do not have
+# the note.o.bin to muck with (as it gets deleted)
+#
+.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 \
+ --rename-section=.data=.livepatch.depends -S $@.bin $@
+ rm -f $@.bin
+
+#
+# Extract the build-id of the xen_hello_world.livepatch
+# (which xen_bye_world will depend on).
+#
+.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 \
+ --rename-section=.data=.livepatch.depends -S $@.bin $@
+ rm -f $@.bin
+
+xen_bye_world.o: config.h
+
+.PHONY: $(LIVEPATCH_BYE)
+$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
+
+xen_replace_world.o: config.h
+
+.PHONY: $(LIVEPATCH_REPLACE)
+$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
+
+.PHONY: livepatch
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE)
diff --git a/xen/common/test/xen_bye_world.c b/xen/common/test/xen_bye_world.c
new file mode 100644
index 0000000..b75e0b1
--- /dev/null
+++ b/xen/common/test/xen_bye_world.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+
+#include <public/sysctl.h>
+
+static char bye_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_bye_world(void);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_bye_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = bye_world_patch_this_fnc,
+ .new_addr = xen_bye_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/test/xen_bye_world_func.c b/xen/common/test/xen_bye_world_func.c
new file mode 100644
index 0000000..32ef341
--- /dev/null
+++ b/xen/common/test/xen_bye_world_func.c
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include <xen/types.h>
+
+/* Our replacement function for xen_hello_world. */
+const char *xen_bye_world(void)
+{
+ return "Bye World!";
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/test/xen_hello_world.c b/xen/common/test/xen_hello_world.c
new file mode 100644
index 0000000..66c5b8e
--- /dev/null
+++ b/xen/common/test/xen_hello_world.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+static char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+static unsigned int cnt;
+
+static void apply_hook(void)
+{
+ printk(KERN_DEBUG "Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+ printk(KERN_DEBUG "Hook unloaded.\n");
+}
+
+static void hi_func(void)
+{
+ printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
+};
+
+/* If we are sorted we _MUST_ be the last .livepatch.hook section. */
+static void z_check_fnc(void)
+{
+ printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
+ BUG_ON(cnt == 0 || cnt > 2);
+}
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+/* Imbalance here. Two load and three unload. */
+
+LIVEPATCH_LOAD_HOOK(hi_func);
+LIVEPATCH_UNLOAD_HOOK(hi_func);
+
+LIVEPATCH_UNLOAD_HOOK(z_check_fnc);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/test/xen_hello_world_func.c b/xen/common/test/xen_hello_world_func.c
new file mode 100644
index 0000000..03d6b84
--- /dev/null
+++ b/xen/common/test/xen_hello_world_func.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include <xen/types.h>
+
+#include <asm/alternative.h>
+#include <asm/nops.h>
+#include <asm/uaccess.h>
+
+static unsigned long *non_canonical_addr = (unsigned long *)0xdead000000000000ULL;
+
+/* Our replacement function for xen_extra_version. */
+const char *xen_hello_world(void)
+{
+ unsigned long tmp;
+ int rc;
+
+ alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);
+ /*
+ * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
+ * exceptions will be caught and processed properly.
+ */
+ rc = __get_user(tmp, non_canonical_addr);
+ BUG_ON(rc != -EFAULT);
+
+ return "Hello World";
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/test/xen_replace_world.c b/xen/common/test/xen_replace_world.c
new file mode 100644
index 0000000..a2a221a
--- /dev/null
+++ b/xen/common/test/xen_replace_world.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/livepatch.h>
+
+#include <public/sysctl.h>
+
+static char xen_replace_world_name[] = "xen_extra_version";
+extern const char *xen_replace_world(void);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_replace_world = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = xen_replace_world_name,
+ .old_addr = 0, /* Forces the hypervisor to lookup .name */
+ .new_addr = xen_replace_world,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/test/xen_replace_world_func.c b/xen/common/test/xen_replace_world_func.c
new file mode 100644
index 0000000..afb5cda
--- /dev/null
+++ b/xen/common/test/xen_replace_world_func.c
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include <xen/types.h>
+
+/* Our replacement function for xen_hello_world. */
+const char *xen_replace_world(void)
+{
+ return "Hello Again World!";
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 8/9] livepatch: Move test-cases to common
2016-08-14 23:07 ` [PATCH v1 8/9] livepatch: Move test-cases to common Konrad Rzeszutek Wilk
@ 2016-08-17 12:24 ` Jan Beulich
0 siblings, 0 replies; 47+ messages in thread
From: Jan Beulich @ 2016-08-17 12:24 UTC (permalink / raw)
To: konrad, Konrad Rzeszutek Wilk
Cc: Andrew Cooper, julien.grall, sstabellini, xen-devel, ross.lagerwall
>>> On 15.08.16 at 01:07, <konrad.wilk@oracle.com> wrote:
> xen/common/test/Makefile | 85 ++++++++++++++++++++++++++++++
> xen/common/test/xen_bye_world.c | 34 ++++++++++++
> xen/common/test/xen_bye_world_func.c | 22 ++++++++
> xen/common/test/xen_hello_world.c | 68 ++++++++++++++++++++++++
> xen/common/test/xen_hello_world_func.c | 39 ++++++++++++++
> xen/common/test/xen_replace_world.c | 33 ++++++++++++
> xen/common/test/xen_replace_world_func.c | 22 ++++++++
May I suggest xen/test/livepatch/ for all of these?
> 20 files changed, 315 insertions(+), 315 deletions(-)
Did you perhaps forget to use git's move detection to reduce
the size of this diff?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (7 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 8/9] livepatch: Move test-cases to common Konrad Rzeszutek Wilk
@ 2016-08-14 23:07 ` Konrad Rzeszutek Wilk
2016-08-17 18:29 ` Julien Grall
2016-08-15 14:52 ` [PATCH v1] Livepatch ARM 64 implementation Julien Grall
9 siblings, 1 reply; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-14 23:07 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall
Cc: sstabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, julien.grall,
Jan Beulich
We need to two things:
1) Wrap the platform-specific objcopy paramters in defines
The input and output parmeters 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 and there are no
exceptions under ARM.
We are not yet attempting to build them under ARM32 so
that is still ifdefed out.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
v1: First submission
---
xen/common/Makefile | 2 +-
xen/common/test/Makefile | 10 ++++++++--
xen/common/test/xen_hello_world_func.c | 7 ++++++-
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 22806b6..fe83653 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
.PHONY: tests
tests:
-ifeq ($(XEN_TARGET_ARCH),x86_64)
+ifneq ($(XEN_TARGET_ARCH),arm32)
$(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
endif
diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
index 23dff1d..3eed6dd 100644
--- a/xen/common/test/Makefile
+++ b/xen/common/test/Makefile
@@ -1,5 +1,11 @@
include $(XEN_ROOT)/Config.mk
+ifeq ($(XEN_TARGET_ARCH),x86_64)
+OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
+else
+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,7 +60,7 @@ $(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 -S $@.bin $@
rm -f $@.bin
@@ -65,7 +71,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 -S $@.bin $@
rm -f $@.bin
diff --git a/xen/common/test/xen_hello_world_func.c b/xen/common/test/xen_hello_world_func.c
index 03d6b84..0e1a722 100644
--- a/xen/common/test/xen_hello_world_func.c
+++ b/xen/common/test/xen_hello_world_func.c
@@ -6,14 +6,17 @@
#include <xen/types.h>
#include <asm/alternative.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;
@@ -24,7 +27,9 @@ const char *xen_hello_world(void)
*/
rc = __get_user(tmp, non_canonical_addr);
BUG_ON(rc != -EFAULT);
-
+#else
+ asm(ALTERNATIVE("nop", "nop", 1));
+#endif
return "Hello World";
}
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-14 23:07 ` [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64 Konrad Rzeszutek Wilk
@ 2016-08-17 18:29 ` Julien Grall
2016-08-17 19:00 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 47+ messages in thread
From: Julien Grall @ 2016-08-17 18:29 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall
Cc: sstabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Jan Beulich
Hi Konrad,
On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
> We need to two things:
> 1) Wrap the platform-specific objcopy paramters in defines
s/paramters/parameters/
> The input and output parmeters for $(OBJCOPY) are different
Ditto.
> based on the platforms. As such provide them in the
> OBJCOPY_MAGIC define and use that.
>
> 2) The alternative is a bit different and there are no
> exceptions under ARM.
>
> We are not yet attempting to build them under ARM32 so
> that is still ifdefed out.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
>
> v1: First submission
> ---
> xen/common/Makefile | 2 +-
> xen/common/test/Makefile | 10 ++++++++--
> xen/common/test/xen_hello_world_func.c | 7 ++++++-
> 3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 22806b6..fe83653 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>
> .PHONY: tests
> tests:
> -ifeq ($(XEN_TARGET_ARCH),x86_64)
> +ifneq ($(XEN_TARGET_ARCH),arm32)
> $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
> endif
> diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
> index 23dff1d..3eed6dd 100644
> --- a/xen/common/test/Makefile
> +++ b/xen/common/test/Makefile
> @@ -1,5 +1,11 @@
> include $(XEN_ROOT)/Config.mk
>
> +ifeq ($(XEN_TARGET_ARCH),x86_64)
> +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
> +else
Is there any reason to fallback on arm64 flags by default? Would not it
be better to have an else if here?
> +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,7 +60,7 @@ $(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 -S $@.bin $@
> rm -f $@.bin
>
> @@ -65,7 +71,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 -S $@.bin $@
> rm -f $@.bin
>
> diff --git a/xen/common/test/xen_hello_world_func.c b/xen/common/test/xen_hello_world_func.c
> index 03d6b84..0e1a722 100644
> --- a/xen/common/test/xen_hello_world_func.c
> +++ b/xen/common/test/xen_hello_world_func.c
> @@ -6,14 +6,17 @@
> #include <xen/types.h>
>
> #include <asm/alternative.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;
>
> @@ -24,7 +27,9 @@ const char *xen_hello_world(void)
> */
> rc = __get_user(tmp, non_canonical_addr);
> BUG_ON(rc != -EFAULT);
> -
> +#else
> + asm(ALTERNATIVE("nop", "nop", 1));
Why the hardcoded 1 here? I am wondering if we should introduce a new
capability "LIVEPATCH_TEST" which is enabled by default. So we can test
that the the alternative is working on all the platform. What do you think?
> +#endif
> return "Hello World";
> }
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 18:29 ` Julien Grall
@ 2016-08-17 19:00 ` Konrad Rzeszutek Wilk
2016-08-17 19:57 ` Julien Grall
2016-08-22 19:59 ` Konrad Rzeszutek Wilk
0 siblings, 2 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-17 19:00 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, ross.lagerwall, Jan Beulich, xen-devel
On Wed, Aug 17, 2016 at 07:29:12PM +0100, Julien Grall wrote:
> Hi Konrad,
>
> On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
> > We need to two things:
> > 1) Wrap the platform-specific objcopy paramters in defines
>
> s/paramters/parameters/
>
> > The input and output parmeters for $(OBJCOPY) are different
>
> Ditto.
>
> > based on the platforms. As such provide them in the
> > OBJCOPY_MAGIC define and use that.
> >
> > 2) The alternative is a bit different and there are no
> > exceptions under ARM.
> >
> > We are not yet attempting to build them under ARM32 so
> > that is still ifdefed out.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> >
> > v1: First submission
> > ---
> > xen/common/Makefile | 2 +-
> > xen/common/test/Makefile | 10 ++++++++--
> > xen/common/test/xen_hello_world_func.c | 7 ++++++-
> > 3 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index 22806b6..fe83653 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
> >
> > .PHONY: tests
> > tests:
> > -ifeq ($(XEN_TARGET_ARCH),x86_64)
> > +ifneq ($(XEN_TARGET_ARCH),arm32)
> > $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
> > endif
> > diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
> > index 23dff1d..3eed6dd 100644
> > --- a/xen/common/test/Makefile
> > +++ b/xen/common/test/Makefile
> > @@ -1,5 +1,11 @@
> > include $(XEN_ROOT)/Config.mk
> >
> > +ifeq ($(XEN_TARGET_ARCH),x86_64)
> > +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
> > +else
>
> Is there any reason to fallback on arm64 flags by default? Would not it be
> better to have an else if here?
>
> > +OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
I presume you are referring to this comment. I am not sure how you would
identify whether the elf64-littleaarch64 is not part of the OBJCOPY?
Oh I guess you can: objcopy --info
Or are you saying just use 'arm64' instead of '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,7 +60,7 @@ $(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 -S $@.bin $@
> > rm -f $@.bin
> >
> > @@ -65,7 +71,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 -S $@.bin $@
> > rm -f $@.bin
> >
> > diff --git a/xen/common/test/xen_hello_world_func.c b/xen/common/test/xen_hello_world_func.c
> > index 03d6b84..0e1a722 100644
> > --- a/xen/common/test/xen_hello_world_func.c
> > +++ b/xen/common/test/xen_hello_world_func.c
> > @@ -6,14 +6,17 @@
> > #include <xen/types.h>
> >
> > #include <asm/alternative.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;
> >
> > @@ -24,7 +27,9 @@ const char *xen_hello_world(void)
> > */
> > rc = __get_user(tmp, non_canonical_addr);
> > BUG_ON(rc != -EFAULT);
> > -
> > +#else
> > + asm(ALTERNATIVE("nop", "nop", 1));
>
> Why the hardcoded 1 here? I am wondering if we should introduce a new
> capability "LIVEPATCH_TEST" which is enabled by default. So we can test that
> the the alternative is working on all the platform. What do you think?
Sure, but I am not sure what number you would like? Perhaps 42 :-) ?
>
> > +#endif
> > return "Hello World";
> > }
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 19:00 ` Konrad Rzeszutek Wilk
@ 2016-08-17 19:57 ` Julien Grall
2016-08-17 20:08 ` Andrew Cooper
2016-08-22 17:41 ` Konrad Rzeszutek Wilk
2016-08-22 19:59 ` Konrad Rzeszutek Wilk
1 sibling, 2 replies; 47+ messages in thread
From: Julien Grall @ 2016-08-17 19:57 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: sstabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, ross.lagerwall, Jan Beulich, xen-devel
Hi Konrad,
On 17/08/2016 20:00, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 17, 2016 at 07:29:12PM +0100, Julien Grall wrote:
>> On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
[...]
>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>> index 22806b6..fe83653 100644
>>> --- a/xen/common/Makefile
>>> +++ b/xen/common/Makefile
>>> @@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>>
>>> .PHONY: tests
>>> tests:
>>> -ifeq ($(XEN_TARGET_ARCH),x86_64)
>>> +ifneq ($(XEN_TARGET_ARCH),arm32)
>>> $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
>>> endif
>>> diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
>>> index 23dff1d..3eed6dd 100644
>>> --- a/xen/common/test/Makefile
>>> +++ b/xen/common/test/Makefile
>>> @@ -1,5 +1,11 @@
>>> include $(XEN_ROOT)/Config.mk
>>>
>>> +ifeq ($(XEN_TARGET_ARCH),x86_64)
>>> +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
>>> +else
>>
>> Is there any reason to fallback on arm64 flags by default? Would not it be
>> better to have an else if here?
>>
>>> +OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
>
> I presume you are referring to this comment. I am not sure how you would
> identify whether the elf64-littleaarch64 is not part of the OBJCOPY?
>
> Oh I guess you can: objcopy --info
>
> Or are you saying just use 'arm64' instead of 'aarch64' ?
I was suggesting to do
ifeq ($(XEN_TARGET_ARCH),x86_64))
OBJCOPY_MAGIC := ...
endif
ifeq ($(XEN_TARGET_ARCH),arm64))
OBJCOPY_MAGIC := ...
endif
>
>>> +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,7 +60,7 @@ $(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 -S $@.bin $@
>>> rm -f $@.bin
>>>
>>> @@ -65,7 +71,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 -S $@.bin $@
>>> rm -f $@.bin
>>>
>>> diff --git a/xen/common/test/xen_hello_world_func.c b/xen/common/test/xen_hello_world_func.c
>>> index 03d6b84..0e1a722 100644
>>> --- a/xen/common/test/xen_hello_world_func.c
>>> +++ b/xen/common/test/xen_hello_world_func.c
>>> @@ -6,14 +6,17 @@
>>> #include <xen/types.h>
>>>
>>> #include <asm/alternative.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;
>>>
>>> @@ -24,7 +27,9 @@ const char *xen_hello_world(void)
>>> */
>>> rc = __get_user(tmp, non_canonical_addr);
>>> BUG_ON(rc != -EFAULT);
>>> -
>>> +#else
>>> + asm(ALTERNATIVE("nop", "nop", 1));
>>
>> Why the hardcoded 1 here? I am wondering if we should introduce a new
>> capability "LIVEPATCH_TEST" which is enabled by default. So we can test that
>> the the alternative is working on all the platform. What do you think?
>
> Sure, but I am not sure what number you would like? Perhaps 42 :-) ?
Unfortunately the number is an index in the bitmap, so we have to
allocate them contiguously.
Also, this made me realize that the current implementation of
apply_alternatives cannot work if we are patching outside the xen binary. :/
I need to have a think to see how we can handle any region.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 19:57 ` Julien Grall
@ 2016-08-17 20:08 ` Andrew Cooper
2016-08-18 10:38 ` Jan Beulich
2016-08-22 17:41 ` Konrad Rzeszutek Wilk
1 sibling, 1 reply; 47+ messages in thread
From: Andrew Cooper @ 2016-08-17 20:08 UTC (permalink / raw)
To: Julien Grall, Konrad Rzeszutek Wilk
Cc: sstabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson,
ross.lagerwall, Jan Beulich, xen-devel
On 17/08/2016 20:57, Julien Grall wrote:
> Hi Konrad,
>
> On 17/08/2016 20:00, Konrad Rzeszutek Wilk wrote:
>> On Wed, Aug 17, 2016 at 07:29:12PM +0100, Julien Grall wrote:
>>> On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
>
> [...]
>
>>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>>> index 22806b6..fe83653 100644
>>>> --- a/xen/common/Makefile
>>>> +++ b/xen/common/Makefile
>>>> @@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>>>
>>>> .PHONY: tests
>>>> tests:
>>>> -ifeq ($(XEN_TARGET_ARCH),x86_64)
>>>> +ifneq ($(XEN_TARGET_ARCH),arm32)
>>>> $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
>>>> endif
>>>> diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
>>>> index 23dff1d..3eed6dd 100644
>>>> --- a/xen/common/test/Makefile
>>>> +++ b/xen/common/test/Makefile
>>>> @@ -1,5 +1,11 @@
>>>> include $(XEN_ROOT)/Config.mk
>>>>
>>>> +ifeq ($(XEN_TARGET_ARCH),x86_64)
>>>> +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
>>>> +else
>>>
>>> Is there any reason to fallback on arm64 flags by default? Would not
>>> it be
>>> better to have an else if here?
>>>
>>>> +OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
>>
>> I presume you are referring to this comment. I am not sure how you would
>> identify whether the elf64-littleaarch64 is not part of the OBJCOPY?
>>
>> Oh I guess you can: objcopy --info
>>
>> Or are you saying just use 'arm64' instead of 'aarch64' ?
>
> I was suggesting to do
>
> ifeq ($(XEN_TARGET_ARCH),x86_64))
> OBJCOPY_MAGIC := ...
> endif
> ifeq ($(XEN_TARGET_ARCH),arm64))
> OBJCOPY_MAGIC := ...
> endif
You can chain "else ifeq" together like this in a makefile to avoid most
of those endif's
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 20:08 ` Andrew Cooper
@ 2016-08-18 10:38 ` Jan Beulich
0 siblings, 0 replies; 47+ messages in thread
From: Jan Beulich @ 2016-08-18 10:38 UTC (permalink / raw)
To: Julien Grall, Andrew Cooper, Konrad Rzeszutek Wilk
Cc: sstabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson,
ross.lagerwall, xen-devel
>>> On 17.08.16 at 22:08, <andrew.cooper3@citrix.com> wrote:
> On 17/08/2016 20:57, Julien Grall wrote:
>> Hi Konrad,
>>
>> On 17/08/2016 20:00, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Aug 17, 2016 at 07:29:12PM +0100, Julien Grall wrote:
>>>> On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
>>
>> [...]
>>
>>>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>>>> index 22806b6..fe83653 100644
>>>>> --- a/xen/common/Makefile
>>>>> +++ b/xen/common/Makefile
>>>>> @@ -82,6 +82,6 @@ subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>>>>
>>>>> .PHONY: tests
>>>>> tests:
>>>>> -ifeq ($(XEN_TARGET_ARCH),x86_64)
>>>>> +ifneq ($(XEN_TARGET_ARCH),arm32)
>>>>> $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch
>>>>> endif
>>>>> diff --git a/xen/common/test/Makefile b/xen/common/test/Makefile
>>>>> index 23dff1d..3eed6dd 100644
>>>>> --- a/xen/common/test/Makefile
>>>>> +++ b/xen/common/test/Makefile
>>>>> @@ -1,5 +1,11 @@
>>>>> include $(XEN_ROOT)/Config.mk
>>>>>
>>>>> +ifeq ($(XEN_TARGET_ARCH),x86_64)
>>>>> +OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
>>>>> +else
>>>>
>>>> Is there any reason to fallback on arm64 flags by default? Would not
>>>> it be
>>>> better to have an else if here?
>>>>
>>>>> +OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
>>>
>>> I presume you are referring to this comment. I am not sure how you would
>>> identify whether the elf64-littleaarch64 is not part of the OBJCOPY?
>>>
>>> Oh I guess you can: objcopy --info
>>>
>>> Or are you saying just use 'arm64' instead of 'aarch64' ?
>>
>> I was suggesting to do
>>
>> ifeq ($(XEN_TARGET_ARCH),x86_64))
>> OBJCOPY_MAGIC := ...
>> endif
>> ifeq ($(XEN_TARGET_ARCH),arm64))
>> OBJCOPY_MAGIC := ...
>> endif
>
> You can chain "else ifeq" together like this in a makefile to avoid most
> of those endif's
Except that iirc that doesn't work with make 3.80, which is what I
think we document as a minimal requirement.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 19:57 ` Julien Grall
2016-08-17 20:08 ` Andrew Cooper
@ 2016-08-22 17:41 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-22 17:41 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, ross.lagerwall, Jan Beulich, xen-devel
> > > > +#else
> > > > + asm(ALTERNATIVE("nop", "nop", 1));
> > >
> > > Why the hardcoded 1 here? I am wondering if we should introduce a new
> > > capability "LIVEPATCH_TEST" which is enabled by default. So we can test that
> > > the the alternative is working on all the platform. What do you think?
> >
> > Sure, but I am not sure what number you would like? Perhaps 42 :-) ?
>
> Unfortunately the number is an index in the bitmap, so we have to allocate
> them contiguously.
OK.
>
> Also, this made me realize that the current implementation of
> apply_alternatives cannot work if we are patching outside the xen binary. :/
Oh yeah. The vmap part :-)
>
> I need to have a think to see how we can handle any region.
We can just double-check that the region->begin address >= __alt_instructions
and region->End <= __alt_instructions_end and then use vmap.
Otherwise, just writemap = replptr I think?
Let me prep a patch and test it.
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64
2016-08-17 19:00 ` Konrad Rzeszutek Wilk
2016-08-17 19:57 ` Julien Grall
@ 2016-08-22 19:59 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-22 19:59 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, ross.lagerwall, Jan Beulich, xen-devel
> > > @@ -24,7 +27,9 @@ const char *xen_hello_world(void)
> > > */
> > > rc = __get_user(tmp, non_canonical_addr);
> > > BUG_ON(rc != -EFAULT);
> > > -
> > > +#else
> > > + asm(ALTERNATIVE("nop", "nop", 1));
> >
> > Why the hardcoded 1 here? I am wondering if we should introduce a new
> > capability "LIVEPATCH_TEST" which is enabled by default. So we can test that
> > the the alternative is working on all the platform. What do you think?
>
> Sure, but I am not sure what number you would like? Perhaps 42 :-) ?
I am coming back to this and I think it may be better to just piggyback
on the ones that exist.
On x86 it is simple - just pick the most common one. On ARM - I have no clue?
Also having an 'LIVEPATCH_TEST" cpu configuration bit means it can be exposed
via the framework to domctl users. And that seems rather odd.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v1] Livepatch ARM 64 implementation
2016-08-14 23:07 [PATCH v1] Livepatch ARM 64 implementation Konrad Rzeszutek Wilk
` (8 preceding siblings ...)
2016-08-14 23:07 ` [PATCH v1 9/9] livepatch: tests: Make them compile under ARM64 Konrad Rzeszutek Wilk
@ 2016-08-15 14:52 ` Julien Grall
2016-08-15 15:49 ` Konrad Rzeszutek Wilk
9 siblings, 1 reply; 47+ messages in thread
From: Julien Grall @ 2016-08-15 14:52 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, xen-devel, konrad, ross.lagerwall; +Cc: sstabellini
On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote:
> Hey!
Hi Konrad,
> This is the first (non RFC) posting of the enablement of Livepatch under ARM64.
>
> The patches are based on: [PATCH v3] Livepatch fixes and features for v4.8.
> (https://lists.xen.org/archives/html/xen-devel/2016-08/msg01825.html)
>
> And the git tree is:
> git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v3
>
> I've only tested this under Foundation Platform with only one CPU working
> (the other CPUs wouldn't boot up for some reason) and without a proper working
> disk image (can't recall why, but it did have an initramfs) - I ended
> up building the hypervisor with the livepatch built in and loading it during
> dom0 execution (via timers), see
> (http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commit;h=39517b2b807025d0d63d4f042ada5eb3de32ff45)
> [That patch is not part of this patchset of course]
I am able to use both SMP and the rootfs on the foundation model. What
is hte command line you are using? How about the device tree?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v1] Livepatch ARM 64 implementation
2016-08-15 14:52 ` [PATCH v1] Livepatch ARM 64 implementation Julien Grall
@ 2016-08-15 15:49 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 47+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-15 15:49 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, ross.lagerwall, sstabellini
On Mon, Aug 15, 2016 at 04:52:58PM +0200, Julien Grall wrote:
>
>
> On 15/08/2016 01:07, Konrad Rzeszutek Wilk wrote:
> > Hey!
>
> Hi Konrad,
>
> > This is the first (non RFC) posting of the enablement of Livepatch under ARM64.
> >
> > The patches are based on: [PATCH v3] Livepatch fixes and features for v4.8.
> > (https://lists.xen.org/archives/html/xen-devel/2016-08/msg01825.html)
> >
> > And the git tree is:
> > git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v3
> >
> > I've only tested this under Foundation Platform with only one CPU working
> > (the other CPUs wouldn't boot up for some reason) and without a proper working
> > disk image (can't recall why, but it did have an initramfs) - I ended
> > up building the hypervisor with the livepatch built in and loading it during
> > dom0 execution (via timers), see
> > (http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commit;h=39517b2b807025d0d63d4f042ada5eb3de32ff45)
> > [That patch is not part of this patchset of course]
>
> I am able to use both SMP and the rootfs on the foundation model. What is
> hte command line you are using? How about the device tree?
~/ARM/Foundation_Platformpkg/models/Linux64_GCC-4.7/Foundation_Platform --image ~/ARM/boot-wrapper-aarch64.git/xen-system.axf
And the boot-wrapper-aarch64.git does:
aarch64 <konrad@localhost:~/ARM/boot-wrapper-aarch64.git> git log
--oneline | head -1
b564cbf Fix build when USE_INITRD not set
aarch64 <konrad@localhost:~/ARM/boot-wrapper-aarch64.git> make xen-system.axf
aarch64-linux-gnu-gcc -DCNTFRQ=0x01800000 -DUART_BASE=0x1c090000 -DLED=0x0008 -DSYSREGS_BASE=0x1c010000 -DGIC_DIST_BASE=0x2c001000 -DGIC_CPU_BASE=0x2c002000 -c -o boot.xen.o boot.S -DXEN
aarch64-linux-gnu-gcc -DPHYS_OFFSET=0x80000000 -DMBOX_OFFSET=0xfff8 -DBOOT=boot.xen.o -DXEN_OFFSET=0xA00000 -DKERNEL_OFFSET=0x80000 -DFDT_OFFSET=0x08000000 -DFS_OFFSET=0x10000000 -DXEN=Xen -DKERNEL=Image -DFILESYSTEM=filesystem.cpio.gz -E -P -C -o model.xen.lds model.lds.S
And it looks I tried at some point to play with the LEDs (not very well)
diff --git a/Makefile b/Makefile
index 241091f..dd50f09 100644
--- a/Makefile
+++ b/Makefile
@@ -13,7 +13,7 @@ SYSREGS_BASE := 0x1c010000
GIC_DIST_BASE := 0x2c001000
GIC_CPU_BASE := 0x2c002000
CNTFRQ := 0x01800000 # 24Mhz
-
+LED := 0x0008
#INITRD_FLAGS := -DUSE_INITRD
CPPFLAGS += $(INITRD_FLAGS)
@@ -78,13 +78,13 @@ $(XIMAGE): boot.xen.o model.xen.lds fdt.dtb $(XEN) $(KERNEL) $(FILESYSTEM)
$(LD) -o $@ --script=model.xen.lds
boot.o: $(BOOTLOADER) Makefile
- $(CC) $(CPPFLAGS) -DCNTFRQ=$(CNTFRQ) -DUART_BASE=$(UART_BASE) -DSYSREGS_BASE=$(SYSREGS_BASE) -DGIC_DIST_BASE=$(GIC_DIST_BASE) -DGIC_CPU_BASE=$(GIC_CPU_BASE) -c -o $@ $(BOOTLOADER)
+ $(CC) $(CPPFLAGS) -DCNTFRQ=$(CNTFRQ) -DUART_BASE=$(UART_BASE) -DLED=$(LED) -DSYSREGS_BASE=$(SYSREGS_BASE) -DGIC_DIST_BASE=$(GIC_DIST_BASE) -DGIC_CPU_BASE=$(GIC_CPU_BASE) -c -o $@ $(BOOTLOADER)
model.lds: $(LD_SCRIPT) Makefile
$(CC) $(CPPFLAGS) -DPHYS_OFFSET=$(PHYS_OFFSET) -DMBOX_OFFSET=$(MBOX_OFFSET) -DBOOT=boot.o -DKERNEL_OFFSET=$(KERNEL_OFFSET) -DFDT_OFFSET=$(FDT_OFFSET) -DFS_OFFSET=$(FS_OFFSET) -DKERNEL=$(KERNEL) -DFILESYSTEM=$(FILESYSTEM) -E -P -C -o $@ $<
boot.xen.o: $(BOOTLOADER) Makefile
- $(CC) $(CPPFLAGS) -DCNTFRQ=$(CNTFRQ) -DUART_BASE=$(UART_BASE) -DSYSREGS_BASE=$(SYSREGS_BASE) -DGIC_DIST_BASE=$(GIC_DIST_BASE) -DGIC_CPU_BASE=$(GIC_CPU_BASE) -c -o $@ $(BOOTLOADER) -DXEN
+ $(CC) $(CPPFLAGS) -DCNTFRQ=$(CNTFRQ) -DUART_BASE=$(UART_BASE) -DLED=$(LED) -DSYSREGS_BASE=$(SYSREGS_BASE) -DGIC_DIST_BASE=$(GIC_DIST_BASE) -DGIC_CPU_BASE=$(GIC_CPU_BASE) -c -o $@ $(BOOTLOADER) -DXEN
model.xen.lds: $(LD_SCRIPT) Makefile
$(CC) $(CPPFLAGS) -DPHYS_OFFSET=$(PHYS_OFFSET) -DMBOX_OFFSET=$(MBOX_OFFSET) -DBOOT=boot.xen.o -DXEN_OFFSET=$(XEN_OFFSET) -DKERNEL_OFFSET=$(KERNEL_OFFSET) -DFDT_OFFSET=$(FDT_OFFSET) -DFS_OFFSET=$(FS_OFFSET) -DXEN=$(XEN) -DKERNEL=$(KERNEL) -DFILESYSTEM=$(FILESYSTEM) -E -P -C -o $@ $<
diff --git a/boot.S b/boot.S
index 3e2cecd..60b642c 100644
--- a/boot.S
+++ b/boot.S
@@ -107,6 +107,13 @@ start_ns:
str wzr, [x4, #0xa0] // V2M_SYS_CFGDATA
str w5, [x4, #0xa4] // V2M_SYS_CFGCTRL
+ ldr x4, =LED
+ mov w5, #0xFF
+ str w5, [x4]
+
+ mov x4, #0x00A4
+ mov w5, #0xC0800000
+ str w5, [x4]
/*
* Primary CPU
*/
Anyhow the more important is the DTS:
total 248784
drwxrwxr-x. 3 konrad konrad 4096 Aug 15 11:46 .
drwxrwxr-x. 11 konrad konrad 4096 Aug 15 11:40 ..
-rw-rw-r--. 1 konrad konrad 90957497 Apr 15 15:50 64
-rw-rw-r--. 1 konrad konrad 90571528 Apr 15 15:49 a
-rw-rw-r--. 1 konrad konrad 2388 Apr 15 22:54 boot.S
-rw-rw-r--. 1 konrad konrad 1544 Aug 15 11:46 boot.xen.o
lrwxrwxrwx. 1 konrad konrad 41 Apr 15 14:17 fdt.dtb -> ../arm-dts/fast_models/rtsm_ve-aemv8a.dtb
drwxrwxr-x. 8 konrad konrad 4096 Apr 15 14:17 .git
-rw-rw-r--. 1 konrad konrad 44 Apr 15 14:17 .gitignore
lrwxrwxrwx. 1 konrad konrad 10 Apr 15 14:24 Image -> ../vmlinuz
-rw-rw-r--. 1 konrad konrad 1508 Apr 15 14:17 LICENSE.txt
-rw-rw-r--. 1 konrad konrad 3701 Apr 15 22:53 Makefile
-rw-rw-r--. 1 konrad konrad 935 Apr 15 14:17 model.lds.S
-rw-rw-r--. 1 konrad konrad 2457 Aug 15 11:46 model.xen.lds
-rw-rw-r--. 1 konrad konrad 667 Apr 15 14:17 README
lrwxrwxrwx. 1 konrad konrad 41 Apr 15 15:26 rtsm_ve-aemv8a.dts -> ../arm-dts/fast_models/rtsm_ve-aemv8a.dts
-rw-rw-r--. 1 konrad konrad 6007 Apr 15 15:28 rtsm_ve-motherboard.dtsi
lrwxrwxrwx. 1 konrad konrad 14 Apr 22 15:19 Xen -> ../xen/xen/xen
-rwxrwxr-x. 1 konrad konrad 853552 Apr 15 15:40 xen64
-rwxrwxr-x. 1 konrad konrad 853552 Apr 15 23:12 Xen.ok
lrwxrwxrwx. 1 konrad konrad 24 Apr 15 21:55 Xen.old -> /home/konrad/xen/xen/xen
-rwxrwxr-x. 1 konrad konrad 6761469 Aug 15 11:46 xen-system.axf
-rw-rw-r--. 1 konrad konrad 64854103 Apr 15 15:46 z
aarch64 <konrad@localhost:~/ARM/arm-dts> git log --oneline | head -1
cbfbe3d versatile_express: Update to 3.19 state
With this change:
diff --git a/fast_models/rtsm_ve-aemv8a.dts b/fast_models/rtsm_ve-aemv8a.dts
index 50b544d..6c6240c 100644
--- a/fast_models/rtsm_ve-aemv8a.dts
+++ b/fast_models/rtsm_ve-aemv8a.dts
@@ -18,7 +18,17 @@
#address-cells = <2>;
#size-cells = <2>;
- chosen { };
+ chosen {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ xen,xen-bootargs = "dtuart=serial0 loglvl=all guesd_loglvl=all dom0_mem=512M,max:512";
+ module@1 {
+ compatible = "xen,linux-zimage", "xen,multiboot-module";
+ reg = <0x80080000 0x800000>;
+ bootargs = "";
+ };
+ };
aliases {
serial0 = &v2m_serial0;
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 47+ messages in thread