Re: [RFC 10/16] xen/arm: Introduce alternative runtime patching

[Julien Grall <julien.grall@arm.com>]

Hi Konrad,

On 13/05/2016 21:26, Konrad Rzeszutek Wilk wrote:
> On Thu, May 05, 2016 at 05:34:19PM +0100, Julien Grall wrote:
>> Some of the processor erratum will require to modify code sequence.
>> As those modifications may impact the performance, they should only
>> be enabled on affected cores. Furthermore, Xen may also want to take
>> advantage of new hardware features coming up with v8.1 and v8.2.
>>
>> This patch adds an infrastructure to patch Xen during boot time
>> depending on the "features" available on the platform.
>>
>> This code is based on the file arch/arm64/kernel/alternative.c in
>> Linux 4.6-rc3. Any references to arm64 have been dropped to make the
>> code as generic as possible.
>>
>> Furthermore, in Xen the executable sections (.text and .init.text)
>> are always mapped read-only and enforced by SCTLR.WNX. So it is not
>> possible to directly patch Xen.
>
> Is it not possible to temporarily 'unenforce' the SCTLR.WNX?

The problem is not because of SCTLR.WNX but because the entries are 
change from read-write to read-only afterwards.

The ARM ARM (see D4-1732 in ARM DDI 0487A.i) recommends the use of 
break-before-make if either the old or new entry is writable when the 
page table is shared between multiples processor. This is to avoid 
possible TLB conflicts on platform have aggressive prefetcher.

The break-before-make approach requires the following steps:
    - Replacing the old entry with an invalid entry
    - Invalidate the entry by flush the TLBs
    - Write the new entry

However, with the current approach, secondary CPUs runs at the same, so 
even with the break-before-make approach it might be possible to hit a 
data abort if the CPU execute code on an entry that is currently been 
changed.

It would be possible to make the CPUs spinning outside of the 
executable, but I think this make the code more complicate. Note that 
you would have the same issue with XSplice.

You may wonder why Xen is patched when all the CPUs are online. This is 
because in big.LITTLE environment, big and little cores will have 
different errata. The only way to know the list of errata is to probe 
each CPU, and therefore bring them up.

I hope this gives you more insights on why I choose this approach. I can 
update the commit message with it.

>
>>
>> To by-pass this restriction, a temporary writeable mapping is made with
>> vmap. This mapping will be used to write the new instructions.
>>
>> Lastly, runtime patching is currently not necessary for ARM32. So the
>> code is only enabled for ARM64.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/Kconfig              |   5 +
>>   xen/arch/arm/Makefile             |   1 +
>>   xen/arch/arm/alternative.c        | 217 ++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/setup.c              |   8 ++
>>   xen/arch/arm/xen.lds.S            |   7 ++
>>   xen/include/asm-arm/alternative.h | 165 +++++++++++++++++++++++++++++
>>   xen/include/asm-arm/arm64/insn.h  |  16 +++
>>   7 files changed, 419 insertions(+)
>>   create mode 100644 xen/arch/arm/alternative.c
>>   create mode 100644 xen/include/asm-arm/alternative.h
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 6231cd5..9b3e66b 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -14,6 +14,7 @@ config ARM_64
>>   	def_bool y
>>   	depends on 64BIT
>>   	select HAS_GICV3
>> +    select ALTERNATIVE
>
> Hard tabs, not spaces.

Right, I will change in the next version.

>>
>>   config ARM
>>   	def_bool y
>> @@ -46,6 +47,10 @@ config ACPI
>>   config HAS_GICV3
>>   	bool
>>
>> +# Select ALTERNATIVE if the architecture supports runtime patching
>> +config ALTERNATIVE
>> +    bool
>
> Ditto.

Ok.

>> +
>>   endmenu
>>
>>   source "common/Kconfig"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 9122f78..2d330aa 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -4,6 +4,7 @@ subdir-y += platforms
>>   subdir-$(CONFIG_ARM_64) += efi
>>   subdir-$(CONFIG_ACPI) += acpi
>>
>> +obj-$(CONFIG_ALTERNATIVE) += alternative.o
>
> This probably needs to be alternative.init.o ?
>>   obj-y += bootfdt.o
>>   obj-y += cpu.o
>>   obj-y += cpufeature.o
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> new file mode 100644
>> index 0000000..0a5d1c8
>> --- /dev/null
>> +++ b/xen/arch/arm/alternative.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + * alternative runtime patching
>> + * inspired by the x86 version
>> + *
>> + * Copyright (C) 2014 ARM Ltd.
>
> Not 2016?

The code has been taken from Linux. I was not sure what to do here. I 
will update with 2014-2016.

>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/config.h>
>> +#include <xen/init.h>
>> +#include <xen/types.h>
>> +#include <xen/kernel.h>
>> +#include <xen/mm.h>
>> +#include <xen/vmap.h>
>> +#include <xen/smp.h>
>> +#include <xen/stop_machine.h>
>> +#include <asm/alternative.h>
>> +#include <asm/atomic.h>
>> +#include <asm/byteorder.h>
>> +#include <asm/cpufeature.h>
>> +#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 {
>> +    const struct alt_instr *begin;
>> +    const struct alt_instr *end;
>> +};
>> +
>> +/*
>> + * Check if the target PC is within an alternative block.
>> + */
>> +static bool_t branch_insn_requires_update(const struct alt_instr *alt,
>
> __init?

No, xSplice will likely contain some alternatives, which Xen will need 
to apply.

I have in mind invasive errata such as the one implemented in patch #13 
and #14. Furthermore, new ARMv8.1 features will requires some to add 
alternatives in spinlock and atomic helpers.

>> +{
>> +    static int patched = 0;
>> +    struct alt_region region = {
>
> const ?

sure.

>> +        .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(&region);
>> +        /* The patching is not expected to fail during boot. */
>> +        BUG_ON(ret != 0);
>> +
>> +        /* Barriers provided by the cache flushing */
>> +        write_atomic(&patched, 1);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void __init apply_alternatives_all(void)
>> +{
>> +    int ret;
>> +
>> +	/* better not try code patching on a live SMP system */
>> +    ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
>
> NR_CPUS? Not 'num_online_cpus' ?

The 3rd argument of stop_machine_run is the CPU where the callback will 
be executed. If NR_CPUS is passed, the callback will be executed on all 
the CPUs.

>
> And I am bit confused. The comment says that 'stop_machine' state machine
> may be patched, but here you use the stop_machine_run which is part of
> stop_machine?!

Yes, hence the callback __apply_alternatives_multi_stop is executed on 
every online CPUs. CPU0 will patch the code whilst the other will spin 
until the code has been patched.

>
>> +
>> +    /* stop_machine_run should never fail at this stage of the boot */
>> +    BUG_ON(ret);
>> +}
>> +
>> +int apply_alternatives(void *start, size_t length)
>
> const void *start?

Could be.

>> +{
>
> const ?

Yes.

>> +    struct alt_region region = {
>> +        .begin = start,
>> +        .end = start + length,
>> +    };
>> +
>> +    return __apply_alternatives(&region);
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 09ff1ea..c4389ef 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -38,6 +38,7 @@
>>   #include <xen/vmap.h>
>>   #include <xen/libfdt/libfdt.h>
>>   #include <xen/acpi.h>
>> +#include <asm/alternative.h>
>>   #include <asm/page.h>
>>   #include <asm/current.h>
>>   #include <asm/setup.h>
>> @@ -834,6 +835,13 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>       do_initcalls();
>>
>> +    /*
>> +     * It needs to be called after do_initcalls to be able to use
>> +     * stop_machine (tasklets initialized via an initcall).
>> +     * XXX: Is it too late?
>
> What else is going to run in initcalls? Why can't this patching be done
> when you only have one CPU?

See my answer above. Xen needs to know the list of errata that affect 
each CPU before patching.

Regards,
-- 
Julien Grall

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