* [PATCH v2 0/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor
@ 2016-09-07 12:50 Julien Grall
2016-09-07 12:50 ` [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives Julien Grall
2016-09-07 12:50 ` [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
0 siblings, 2 replies; 6+ messages in thread
From: Julien Grall @ 2016-09-07 12:50 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, steve.capper, wei.chen
Hi,
The first patch is a clean-up added in this new version. The second contains the
meat of this series.
Yours sincerely,
Julien Grall (2):
xen/arm: alternative: Clean-up __apply_alternatives
xen/arm: alternative: Make it possible to patch outside of the
hypervisor
xen/arch/arm/alternative.c | 65 ++++++++++++++++++++++++++--------------------
1 file changed, 37 insertions(+), 28 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives
2016-09-07 12:50 [PATCH v2 0/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
@ 2016-09-07 12:50 ` Julien Grall
2016-09-07 14:13 ` Konrad Rzeszutek Wilk
2016-09-07 12:50 ` [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
1 sibling, 1 reply; 6+ messages in thread
From: Julien Grall @ 2016-09-07 12:50 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, steve.capper, wei.chen
This patch contains only renaming and comment update. There are no
functional changes:
- Don't mix _start and _stext, they both point to the same address
but the former makes more sense (we are mapping the Xen binary, not
only the text section).
- s/text_mfn/xen_mfn/ and s/text_order/xen_order/ to make clear that
we map the Xen binary.
- Mention about inittext as alternative may patch this section.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Konrad, I added your signed-off by because I squashed your patch [1]
in it. Let me know if there is any issue for that.
[1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg02890.html
Changes in v2:
- Patch added
---
xen/arch/arm/alternative.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 8ee5a11..0ca97b9 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -99,21 +99,21 @@ 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);
+ mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
+ unsigned int xen_order = get_order_from_bytes(_end - _start);
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.
+ * The text and inittext section are read-only. So re-map Xen to be
+ * able to patch the code.
*/
- writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
+ writemap = __vmap(&xen_mfn, 1 << xen_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);
+ 1 << xen_order);
return -ENOMEM;
}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-07 12:50 [PATCH v2 0/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
2016-09-07 12:50 ` [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives Julien Grall
@ 2016-09-07 12:50 ` Julien Grall
2016-09-07 14:14 ` Konrad Rzeszutek Wilk
1 sibling, 1 reply; 6+ messages in thread
From: Julien Grall @ 2016-09-07 12:50 UTC (permalink / raw)
To: xen-devel; +Cc: Julien Grall, sstabellini, steve.capper, wei.chen
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
address of the alt_instr set of the re-mapped region. So we can mandate
the callers of __apply_alternatives to provide use with a region that has
read-write access.
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.grall@arm.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@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
Changes in v2:
- Fix typoes
- Add a comment to details how __apply_alternative should be
called
- Clean-up the casting for region.begin and region.end
---
xen/arch/arm/alternative.c | 65 ++++++++++++++++++++++++++--------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 0ca97b9..7203bae 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -94,28 +94,18 @@ static u32 get_alt_insn(const struct alt_instr *alt,
return insn;
}
+/*
+ * The region patched should be read-write to allow __apply_alternatives
+ * to replacing the instructions when necessary.
+ */
static int __apply_alternatives(const struct alt_region *region)
{
const struct alt_instr *alt;
- const u32 *origptr, *replptr;
- u32 *writeptr, *writemap;
- mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
- unsigned int xen_order = get_order_from_bytes(_end - _start);
+ const u32 *replptr;
+ u32 *origptr;
- printk(XENLOG_INFO "alternatives: Patching kernel code\n");
-
- /*
- * The text and inittext section are read-only. So re-map Xen to be
- * able to patch the code.
- */
- writemap = __vmap(&xen_mfn, 1 << xen_order, 1, 1, PAGE_HYPERVISOR,
- VMAP_DEFAULT);
- if ( !writemap )
- {
- printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
- 1 << xen_order);
- return -ENOMEM;
- }
+ printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n",
+ region->begin, region->end);
for ( alt = region->begin; alt < region->end; alt++ )
{
@@ -128,7 +118,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 +125,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 +146,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 +157,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);
+ void *xenmap;
BUG_ON(patched);
+
+ /*
+ * The text and inittext section are 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 *)__alt_instructions - (void *)_start + xenmap;
+ region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
+
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives
2016-09-07 12:50 ` [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives Julien Grall
@ 2016-09-07 14:13 ` Konrad Rzeszutek Wilk
2016-09-07 14:26 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-07 14:13 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, steve.capper, wei.chen, xen-devel
On Wed, Sep 07, 2016 at 01:50:43PM +0100, Julien Grall wrote:
> This patch contains only renaming and comment update. There are no
> functional changes:
> - Don't mix _start and _stext, they both point to the same address
> but the former makes more sense (we are mapping the Xen binary, not
> only the text section).
> - s/text_mfn/xen_mfn/ and s/text_order/xen_order/ to make clear that
> we map the Xen binary.
> - Mention about inittext as alternative may patch this section.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> Konrad, I added your signed-off by because I squashed your patch [1]
> in it. Let me know if there is any issue for that.
No issues. Albeit it may be odd for me to review my own patch :-)
See below
>
> [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg02890.html
>
> Changes in v2:
> - Patch added
> ---
> xen/arch/arm/alternative.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 8ee5a11..0ca97b9 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -99,21 +99,21 @@ 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);
> + mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
> + unsigned int xen_order = get_order_from_bytes(_end - _start);
>
> 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.
> + * The text and inittext section are read-only. So re-map Xen to be
> + * able to patch the code.
> */
> - writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> + writemap = __vmap(&xen_mfn, 1 << xen_order, 1, 1, PAGE_HYPERVISOR,
Do you want to make it 1U? (You pointed that out in my patchset so perhaps you
want it here?)
> VMAP_DEFAULT);
> if ( !writemap )
> {
> printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
> - 1 << text_order);
> + 1 << xen_order);
> return -ENOMEM;
> }
>
> --
> 1.9.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor
2016-09-07 12:50 ` [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
@ 2016-09-07 14:14 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-07 14:14 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, steve.capper, wei.chen, xen-devel
On Wed, Sep 07, 2016 at 01:50:44PM +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
> address of the alt_instr set of the re-mapped region. So we can mandate
> the callers of __apply_alternatives to provide use with a region that has
> read-write access.
>
> 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.grall@arm.com>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives
2016-09-07 14:13 ` Konrad Rzeszutek Wilk
@ 2016-09-07 14:26 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2016-09-07 14:26 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: sstabellini, steve.capper, wei.chen, xen-devel
Hi Konrad,
On 07/09/2016 15:13, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 07, 2016 at 01:50:43PM +0100, Julien Grall wrote:
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> index 8ee5a11..0ca97b9 100644
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -99,21 +99,21 @@ 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);
>> + mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
>> + unsigned int xen_order = get_order_from_bytes(_end - _start);
>>
>> 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.
>> + * The text and inittext section are read-only. So re-map Xen to be
>> + * able to patch the code.
>> */
>> - writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
>> + writemap = __vmap(&xen_mfn, 1 << xen_order, 1, 1, PAGE_HYPERVISOR,
>
> Do you want to make it 1U? (You pointed that out in my patchset so perhaps you
> want it here?)
Good point, I used 1U whilst moving the code in patch #2 but forgot here.
I will update the patch and resend it.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-07 14:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 12:50 [PATCH v2 0/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
2016-09-07 12:50 ` [PATCH v2 1/2] xen/arm: alternative: Clean-up __apply_alternatives Julien Grall
2016-09-07 14:13 ` Konrad Rzeszutek Wilk
2016-09-07 14:26 ` Julien Grall
2016-09-07 12:50 ` [PATCH v2 2/2] xen/arm: alternative: Make it possible to patch outside of the hypervisor Julien Grall
2016-09-07 14:14 ` Konrad Rzeszutek Wilk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.