From: Julien Grall <julien.grall@arm.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: andre.przywara@arm.com, sstabellini@kernel.org,
steve.capper@arm.com, wei.chen@arm.com, xen-devel@lists.xen.org
Subject: Re: [PATCH v3 09/16] xen/arm: Introduce alternative runtime patching
Date: Wed, 8 Jun 2016 10:39:27 +0100 [thread overview]
Message-ID: <33377210-feec-dc6c-b81e-ea8ffb45c39b@arm.com> (raw)
In-Reply-To: <20160607172457.GA14200@char.us.oracle.com>
Hi Konrad,
On 07/06/2016 18:24, Konrad Rzeszutek Wilk wrote:
>> +
>> +/*
>> + * We might be patching the stop_machine state machine, so implement a
>> + * really simple polling protocol here.
>> + */
>> +static int __apply_alternatives_multi_stop(void *unused)
>> +{
>> + static int patched = 0;
>> + 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() )
>> + {
>> + while ( !read_atomic(&patched) )
>> + cpu_relax();
>> + isb();
>> + }
>> + else
>> + {
>> + int ret;
>> +
>> + BUG_ON(patched);
>> + ret = __apply_alternatives(®ion);
>> + /* The patching is not expected to fail during boot. */
>> + BUG_ON(ret != 0);
>> +
>> + /* Barriers provided by the cache flushing */
>
> Could you add the stop at the end here (and above in the 'We always have..)'?
Will do.
>
>> + write_atomic(&patched, 1);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * This function should only be called during boot and before CPU0 jump
>
> Would it make sense to then have an ASSERT or BUG on:
>
> SYS_STATE_early_boot ?
No need, this function is part of __init. Anyone calling this function
after boot will crash the hypervisor :).
>
>> + * into the idle_loop.
>> + */
>> +void __init apply_alternatives_all(void)
>
> s/apply_alternatives_all/apply_alternatives_init/ ?
I think _all makes more sense because we apply all the alternatives to
the hypervisor.
>
>> +{
>> + int ret;
>> +
>> + /* better not try code patching on a live SMP system */
>
> Something is off with this comment (wrong column). And while
> at it you could also add the '.' at the end.
It contains an hard tab. I will fix it in the next version.
>
>> + ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
>> +
>> + /* stop_machine_run should never fail at this stage of the boot */
>
> Missing stpo.
ok.
>> + BUG_ON(ret);
>> +}
>> +
> ..snip..
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 1f010bd..495f9d8 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -129,6 +129,9 @@ SECTIONS
>> _sinittext = .;
>> *(.init.text)
>> _einittext = .;
>> +#ifdef CONFIG_ALTERNATIVE
>> + *(.altinstr_replacement)
>> +#endif
>
> This is outside the _einittext? x86 looks to have .altinstr_replacement
> inside the _einittext.
Yes, I looked at the x86 code when I did the implement and I did not
find any good reason to keep .altinstr_replace inside the inittext.
altinstr_replacement contains replacement instructions. Anything inside
the inittext region will be mark executable, which is not what we want here.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-08 9:39 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-07 16:06 [PATCH v3 00/16] xen/arm: Introduce alternative runtime patching for ARM64 Julien Grall
2016-06-07 16:06 ` [PATCH v3 01/16] xen/arm: Makefile: Sort the entries alphabetically Julien Grall
2016-06-07 16:06 ` [PATCH v3 02/16] xen/arm: Include the header asm-arm/system.h in asm-arm/page.h Julien Grall
2016-06-07 16:06 ` [PATCH v3 03/16] xen/arm: Add macros to handle the MIDR Julien Grall
2016-06-07 16:06 ` [PATCH v3 04/16] xen/arm: Add cpu_hwcap bitmap Julien Grall
2016-06-07 16:06 ` [PATCH v3 05/16] xen/arm64: Add an helper to invalidate all instruction caches Julien Grall
2016-06-07 16:06 ` [PATCH v3 06/16] xen/arm: arm64: Move the define BRK_BUG_FRAME into a separate header Julien Grall
2016-06-07 16:06 ` [PATCH v3 07/16] xen/arm: arm64: Reserve a brk immediate to fault on purpose Julien Grall
2016-06-07 16:06 ` [PATCH v3 08/16] xen/arm: arm64: Add helpers to decode and encode branch instructions Julien Grall
2016-06-07 16:06 ` [PATCH v3 09/16] xen/arm: Introduce alternative runtime patching Julien Grall
2016-06-07 17:24 ` Konrad Rzeszutek Wilk
2016-06-08 9:39 ` Julien Grall [this message]
2016-06-08 18:17 ` Konrad Rzeszutek Wilk
2016-06-08 18:22 ` Julien Grall
2016-06-08 18:35 ` Konrad Rzeszutek Wilk
2016-06-09 13:33 ` Julien Grall
2016-06-07 16:06 ` [PATCH v3 10/16] xen/arm: cpufeature: Provide an helper to check if a capability is supported Julien Grall
2016-06-22 9:59 ` Stefano Stabellini
2016-06-22 10:09 ` Julien Grall
2016-06-22 10:14 ` Stefano Stabellini
2016-06-07 16:06 ` [PATCH v3 11/16] xen/arm: Detect silicon revision and set cap bits accordingly Julien Grall
2016-06-22 10:00 ` Stefano Stabellini
2016-06-07 16:06 ` [PATCH v3 12/16] xen/arm: Document the errata implemented in Xen Julien Grall
2016-06-22 10:03 ` Stefano Stabellini
2016-06-07 16:06 ` [PATCH v3 13/16] xen/arm: arm64: Add Cortex-A53 cache errata workaround Julien Grall
2016-06-22 10:04 ` Stefano Stabellini
2016-06-07 16:06 ` [PATCH v3 14/16] xen/arm: arm64: Add cortex-A57 erratum 832075 workaround Julien Grall
2016-06-22 10:10 ` Stefano Stabellini
2016-06-07 16:06 ` [PATCH v3 15/16] xen/arm: traps: Don't inject a fault if the translation VA -> IPA fails Julien Grall
2016-06-07 16:06 ` [PATCH v3 16/16] xen/arm: arm64: Document Cortex-A57 erratum 834220 Julien Grall
2016-06-14 12:15 ` Julien Grall
2016-06-22 10:08 ` Stefano Stabellini
2016-06-22 10:18 ` Julien Grall
2016-06-22 10:37 ` Stefano Stabellini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33377210-feec-dc6c-b81e-ea8ffb45c39b@arm.com \
--to=julien.grall@arm.com \
--cc=andre.przywara@arm.com \
--cc=konrad.wilk@oracle.com \
--cc=sstabellini@kernel.org \
--cc=steve.capper@arm.com \
--cc=wei.chen@arm.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).