[PATCH v3 0/2] kexec-tools: arm64: Enable D-cache in purgatory

[Kostiantyn Iarmak]

On 04.04.18 16:28, James Morse wrote:
> Hi Kostiantyn,
>
> On 04/04/18 13:45, Kostiantyn Iarmak wrote:
>> From: Pratyush Anand <panand at redhat.com>
>>> Date: Fri, Jun 2, 2017 at 5:42 PM
>>> Subject: Re: [PATCH v3 0/2] kexec-tools: arm64: Enable D-cache in purgatory
>>> To: James Morse <james.morse at arm.com>
>>> Cc: mark.rutland at arm.com, bhe at redhat.com, kexec at lists.infradead.org,
>>> horms at verge.net.au, dyoung at redhat.com,
>>> linux-arm-kernel at lists.infradead.org
>>>
>>> On Friday 02 June 2017 01:53 PM, James Morse wrote:
>>>> On 23/05/17 06:02, Pratyush Anand wrote:
>>>>> It takes more that 2 minutes to verify SHA in purgatory when vmlinuz image
>>>>> is around 13MB and initramfs is around 30MB. It takes more than 20 second
>>>>> even when we have -O2 optimization enabled. However, if dcache is enabled
>>>>> during purgatory execution then, it takes just a second in SHA
>>>>> verification.
>>>>>
>>>>> Therefore, these patches adds support for dcache enabling facility during
>>>>> purgatory execution.
>>>> I'm still not convinced we need this. Moving the SHA verification to happen
>>>> before the dcache+mmu are disabled would also solve the delay problem,
>>> Humm..I am not sure, if we can do that.
>>> When we leave kernel (and enter into purgatory), icache+dcache+mmu are
>>> already disabled. I think, that would be possible when we will be in a
>>> position to use in-kernel purgatory.
>>>
>>>> and we
>>>> can print an error message or fail the syscall.
>>>>
>>>> For kexec we don't expect memory corruption, what are we testing for?
>>>> I can see the use for kdump, but the kdump-kernel is unmapped so the kernel
>>>> can't accidentally write over it.
>>>>
>>>> (we discussed all this last time, but it fizzled-out. If you and the
>>>>    kexec-tools maintainer think its necessary, fine by me!)
>>> Yes, there had already been discussion and MAINTAINERs have
>>> discouraged none-purgatory implementation.
>>>
>>>> I have some comments on making this code easier to maintain..
>>>>
>>> Thanks.
>>>
>>> I have implemented your review comments and have archived the code in
>>>
>>> https://github.com/pratyushanand/kexec-tools.git : purgatory-enable-dcache
>>>
>>> I will be posting the next version only when someone complains about
>>> ARM64 kdump behavior that it is not as fast as x86.
>> On our ARM64-based platform we have very long main kernel-secondary kernel
>> switch time.
>>
>> This patch set fixes the issue (we are using 4.4 kernel and 2.0.13 kexec-tools
>> version), we can get ~25x speedup, with this patch secondary kernel boots in ~3
>> seconds while on 2.0.13-2.0.16 kexec-tools without this patch switch takes about
>> 75 seconds.
> This is slow because its generating a checksum of the kernel without the benefit
> of the caches. This series generated page tables so that it could enable the MMU
> and caches. But, the purgatory code also needs to be a simple as possible
> because its practically impossible to debug.
>
> The purgatory code does this checksum-ing because its worried the panic() was
> because the kernel cause some memory corruption, and that memory corruption may
> have affected the kdump kernel too.
>
> This can't happen on arm64 as we unmap kdump's crash region, so not even the
> kernel can accidentally write to it. 98d2e1539b84 ("arm64: kdump: protect crash
> dump kernel memory") has all the details.
>
> (we also needed to do this to avoid the risk of mismatched memory attributes if
> kdump boots and some CPUs are still stuck in the old kernel)
>
>
>> When do you plan merge this patch?
> We ended up with the check-summing code because its the default behaviour of
> kexec-tools on other architectures.
>
> One alternative is to rip it out for arm64. Untested:
> --------------------%<--------------------
> diff --git a/purgatory/arch/arm64/Makefile b/purgatory/arch/arm64/Makefile
> index 636abea..f10c148 100644
> --- a/purgatory/arch/arm64/Makefile
> +++ b/purgatory/arch/arm64/Makefile
> @@ -7,7 +7,8 @@ arm64_PURGATORY_EXTRA_CFLAGS = \
>          -Werror-implicit-function-declaration \
>          -Wdeclaration-after-statement \
>          -Werror=implicit-int \
> -       -Werror=strict-prototypes
> +       -Werror=strict-prototypes \
> +       -DNO_SHA_IN_PURGATORY
>
>   arm64_PURGATORY_SRCS += \
>          purgatory/arch/arm64/entry.S \
> diff --git a/purgatory/purgatory.c b/purgatory/purgatory.c
> index 3bbcc09..44e792a 100644
> --- a/purgatory/purgatory.c
> +++ b/purgatory/purgatory.c
> @@ -9,6 +9,8 @@
>   struct sha256_region sha256_regions[SHA256_REGIONS] = {};
>   sha256_digest_t sha256_digest = { };
>
> +#ifndef NO_SHA_IN_PURGATORY
> +
>   int verify_sha256_digest(void)
>   {
>          struct sha256_region *ptr, *end;
> @@ -39,14 +41,18 @@ int verify_sha256_digest(void)
>          return 0;
>   }
>
> +#endif /* NO_SHA_IN_PURGATORY */
> +
>   void purgatory(void)
>   {
>          printf("I'm in purgatory\n");
>          setup_arch();
> +#ifndef NO_SHA_IN_PURGATORY
>          if (verify_sha256_digest()) {
>                  for(;;) {
>                          /* loop forever */
>                  }
>          }
> +#endif /* NO_SHA_IN_PURGATORY */
>          post_verification_setup_arch();
>   }
> --------------------%<--------------------

Thank you, I've tested this patch (no issues).

>
> Thanks,
>
> James

-- 
Best Regards,

   Kostiantyn Iarmak.