Hi Konrad,
On 06/09/2016 20:25, Konrad Rzeszutek Wilk wrote:
On Thu, Sep 01, 2016 at 08:56:39AM +0100, Julien Grall wrote:
With livepatch the alternatives that should be patched are outside of
the Xen hypervisor _start -> _end. The current code is assuming that
only Xen could be patched and therefore will explode when a payload
contains alternatives.
Given that alt_instr contains a relative offset, the function
__apply_alternatives could directly take in parameter the virtual
^- the ^- for
address of the alt_instr set of the re-mapped region. So we can mandate
the callers of __apply_alternatives to ensure the region has read-write
s/ensure the region/provide us with a region that/
access beforehand.
How do we mandate it?
I don't have any idea how to write the check here. I think the only way
would be a comment on __apply_alternatives as you suggested below.
Should the __apply_alternatives have an comment about it?
The only caller that will patch directly the Xen binary is the function
__apply_alternatives_multi_stop. The other caller apply_alternatives
will work on the payload which will still have read-write access at that
time.
Signed-off-by: Julien Grall <julien.gr...@arm.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
This is an alternative of the patch suggested by Konrad [1] to fix
alternatives patching with livepatching.
[1]
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html
You also did
s/_stext/_start/
And s/text_mfn/xen_mfn/
I didn't write a separate patch because the code has been reworked quite
a lot. But I am fine to move this in a separate patch if necessary.
---
xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 8ee5a11..db4bd0f 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
static int __apply_alternatives(const struct alt_region *region)
{
const struct alt_instr *alt;
- const u32 *origptr, *replptr;
- u32 *writeptr, *writemap;
- mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
- unsigned int text_order = get_order_from_bytes(_end - _start);
+ const u32 *replptr;
+ u32 *origptr;
printk(XENLOG_INFO "alternatives: Patching kernel code\n");
- /*
- * The text section is read-only. So re-map Xen to be able to patch
- * the code.
- */
- writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
- VMAP_DEFAULT);
- if ( !writemap )
- {
- printk(XENLOG_ERR "alternatives: Unable to map the text section (size
%u)\n",
- 1 << text_order);
- return -ENOMEM;
- }
-
for ( alt = region->begin; alt < region->end; alt++ )
{
u32 insn;
@@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region
*region)
BUG_ON(alt->alt_len != alt->orig_len);
origptr = ALT_ORIG_PTR(alt);
- writeptr = origptr - (u32 *)_start + writemap;
replptr = ALT_REPL_PTR(alt);
nr_inst = alt->alt_len / sizeof(insn);
@@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region
*region)
for ( i = 0; i < nr_inst; i++ )
{
insn = get_alt_insn(alt, origptr + i, replptr + i);
- *(writeptr + i) = cpu_to_le32(insn);
+ *(origptr + i) = cpu_to_le32(insn);
}
/* Ensure the new instructions reached the memory and nuke */
- clean_and_invalidate_dcache_va_range(writeptr,
- (sizeof (*writeptr) * nr_inst));
+ clean_and_invalidate_dcache_va_range(origptr,
+ (sizeof (*origptr) * nr_inst));
}
/* Nuke the instruction cache */
invalidate_icache();
- vunmap(writemap);
-
return 0;
}
@@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region
*region)
static int __apply_alternatives_multi_stop(void *unused)
{
static int patched = 0;
- const struct alt_region region = {
- .begin = __alt_instructions,
- .end = __alt_instructions_end,
- };
/* We always have a CPU 0 at this point (__init) */
if ( smp_processor_id() )
@@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
else
{
int ret;
+ struct alt_region region;
+ mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
+ unsigned int xen_order = get_order_from_bytes(_end - _start);
+ char *xenmap;
Could this be 'void *'?
BUG_ON(patched);
+
+ /*
+ * The text and inittext section is read-only. So re-map Xen to
+ * be able to patch the code.
+ */
+ xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
+ VMAP_DEFAULT);
+ /* Re-mapping Xen is not expected to fail during boot. */
+ BUG_ON(!xenmap);
+
+ /*
+ * Find the virtual address of the alternative region in the new
+ * mapping.
+ * alt_instr contains relative offset, so the function
+ * __apply_alternatives will patch in the re-mapped version of
+ * Xen.
+ */
+ region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
+ region.end = (void *)((char *)__alt_instructions_end - _start +
xenmap);
Which I think would make this have less casts?
Oh wait. _start is a char *, so we would have to have:
region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
Julien, you are the maintainer, so your call.
Your suggestion seems to be clearer.
+
ret = __apply_alternatives(®ion);
/* The patching is not expected to fail during boot. */
BUG_ON(ret != 0);
+ vunmap(xenmap);
+
/* Barriers provided by the cache flushing */
write_atomic(&patched, 1);
}
--
1.9.1
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
