Re: [PATCH v3 2/2] x86/kdump: Reserve extra memory when SME or SEV is active

[Dave Young]

On 10/12/19 at 05:24pm, Kairui Song wrote:
> On 9/27/19 1:42 PM, Dave Young wrote:
> > On 09/25/19 at 06:36pm, Kairui Song wrote:
> > > On Wed, Sep 11, 2019 at 1:56 PM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> > > > * Kairui Song <kasong@xxxxxxxxxx> wrote:
> > > > 
> > > > > Since commit c7753208a94c ("x86, swiotlb: Add memory encryption support"),
> > > > > SWIOTLB will be enabled even if there is less than 4G of memory when SME
> > > > > is active, to support DMA of devices that not support address with the
> > > > > encrypt bit.
> > > > > 
> > > > > And commit aba2d9a6385a ("iommu/amd: Do not disable SWIOTLB if SME is
> > > > > active") make the kernel keep SWIOTLB enabled even if there is an IOMMU.
> > > > > 
> > > > > Then commit d7b417fa08d1 ("x86/mm: Add DMA support for SEV memory
> > > > > encryption") will always force SWIOTLB to be enabled when SEV is active
> > > > > in all cases.
> > > > > 
> > > > > Now, when either SME or SEV is active, SWIOTLB will be force enabled,
> > > > > and this is also true for kdump kernel. As a result kdump kernel will
> > > > > run out of already scarce pre-reserved memory easily.
> > > > > 
> > > > > So when SME/SEV is active, reserve extra memory for SWIOTLB to ensure
> > > > > kdump kernel have enough memory, except when "crashkernel=size[KMG],high"
> > > > > is specified or any offset is used. As for the high reservation case, an
> > > > > extra low memory region will always be reserved and that is enough for
> > > > > SWIOTLB. Else if the offset format is used, user should be fully aware
> > > > > of any possible kdump kernel memory requirement and have to organize the
> > > > > memory usage carefully.
> > > > > 
> > > > > Signed-off-by: Kairui Song <kasong@xxxxxxxxxx>
> > > > > ---
> > > > >   arch/x86/kernel/setup.c | 20 +++++++++++++++++---
> > > > >   1 file changed, 17 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > > > index 71f20bb18cb0..ee6a2f1e2226 100644
> > > > > --- a/arch/x86/kernel/setup.c
> > > > > +++ b/arch/x86/kernel/setup.c
> > > > > @@ -530,7 +530,7 @@ static int __init crashkernel_find_region(unsigned long long *crash_base,
> > > > >                                          unsigned long long *crash_size,
> > > > >                                          bool high)
> > > > >   {
> > > > > -     unsigned long long base, size;
> > > > > +     unsigned long long base, size, mem_enc_req = 0;
> > > > > 
> > > > >        base = *crash_base;
> > > > >        size = *crash_size;
> > > > > @@ -561,11 +561,25 @@ static int __init crashkernel_find_region(unsigned long long *crash_base,
> > > > >        if (high)
> > > > >                goto high_reserve;
> > > > > 
> > > > > +     /*
> > > > > +      * When SME/SEV is active and not using high reserve,
> > > > > +      * it will always required an extra SWIOTLB region.
> > > > > +      */
> > > > > +     if (mem_encrypt_active())
> > > > > +             mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> > > > > +
> > > > >        base = memblock_find_in_range(CRASH_ALIGN,
> > > > > -                                   CRASH_ADDR_LOW_MAX, size,
> > > > > +                                   CRASH_ADDR_LOW_MAX,
> > > > > +                                   size + mem_enc_req,
> > > > >                                      CRASH_ALIGN);
> > > > 
> > > 
> > > Hi Ingo,
> > > 
> > > I re-read my previous reply, it's long and tedious, let me try to make
> > > a more effective reply:
> > > 
> > > > What sizes are we talking about here?
> > > 
> > > The size here is how much memory will be reserved for kdump kernel, to
> > > ensure kdump kernel and userspace can run without OOM.
> > > 
> > > > 
> > > > - What is the possible size range of swiotlb_size_or_default()
> > > 
> > > swiotlb_size_or_default() returns the swiotlb size, it's specified by
> > > user using swiotlb=<size>, or default size (64MB)
> > > 
> > > > 
> > > > - What is the size of CRASH_ADDR_LOW_MAX (the old limit)?
> > > 
> > > It's 4G.
> > > 
> > > > 
> > > > - Why do we replace one fixed limit with another fixed limit instead of
> > > >    accurately sizing the area, with each required feature adding its own
> > > >    requirement to the reservation size?
> > > 
> > > It's quite hard to "accurately sizing the area".
> > > 
> > > No way to tell the exact amount of memory kdump needs, we can only estimate.
> > > Kdump kernel use different cmdline, drivers and components will have
> > > special handling for kdump, and userspace is totally different.
> > 
> > Agreed about your above, but specific this the problem in this patch
> > There should be other ways.
> > 
> > First thought about doing generic handling in swiotlb part, and do
> > something like kdump_memory_reserve(size) Ingo suggested,  but according
> > to you swiotlb init is late, so it can not increase the size, OTOH if
> > reserve another region for kdump in swiotlb will cause other issues.
> > 
> > So let's think about other improvement, for example to see if you can
> > call kdump_memory_reserve(size) in AMD SME init path, for example in
> > mem_encrypt_init(), is it before crashkernel reservation?
> > 
> > If doable it will be at least cleaner than the code in this patch.
> > 
> > Thanks
> > Dave
> > 
> 
> How about something simple as following code? The logic and new function is as simple as
> possible, just always reserve extra low memory when SME/SEV is active, ignore the high/low
> reservation case. It will waste some memory with SME and high reservation though.
> 
> Was hesitating a lot about this series, one thing I'm thinking is that what is the point
> of "crashkernel=" argument, if the crashkernel value could be adjusted according, the value
> specified will seems more meanless or confusing...
> 
> And currently there isn't anything like crashkernel=auto or anything similiar to let kernel
> calculate the value automatically, maybe the admin should be aware of the value or be informed
> about the suitable crashkernel value after all?

Hmm, it is reasonable that a user defined value should be just as is
without any change by kernel.  So it is a good reason to introduce
a crashkernel=auto so that kernel can tune the crashkernel size
accordingly on top of some base value which can be configurable by
kernel configs (arch dependent).

> 
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index ed8ec011a9fd..7263a237f689 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -44,6 +44,7 @@ void early_platform_quirks(void);
>  extern unsigned long saved_video_mode;
> +extern void kdump_need_extra_low_memory(unsigned long size);
>  extern void reserve_standard_io_resources(void);
>  extern void i386_reserve_resources(void);
>  extern unsigned long __startup_64(unsigned long physaddr, struct boot_params *bp);
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 77ea96b794bd..e5888fb8e4bc 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -473,6 +473,13 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>  # define CRASH_ADDR_HIGH_MAX	SZ_64T
>  #endif
> +static __initdata unsigned long long crash_low_extra;
> +
> +void __init kdump_need_extra_low_memory(unsigned long size)
> +{
> +	crash_low_extra += size;
> +}
> +
>  static int __init reserve_crashkernel_low(void)
>  {
>  #ifdef CONFIG_X86_64
> @@ -501,6 +508,7 @@ static int __init reserve_crashkernel_low(void)
>  			return 0;
>  	}
> +	low_size += crash_low_extra;
>  	low_base = memblock_find_in_range(0, 1ULL << 32, low_size, CRASH_ALIGN);
>  	if (!low_base) {
>  		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
> @@ -563,8 +571,17 @@ static void __init reserve_crashkernel(void)
>  		if (!high)
>  			crash_base = memblock_find_in_range(CRASH_ALIGN,
>  						CRASH_ADDR_LOW_MAX,
> -						crash_size, CRASH_ALIGN);
> -		if (!crash_base)
> +						crash_size + crash_low_extra,
> +						CRASH_ALIGN);
> +		/*
> +		 * If reserving the crashkernel memory in low region, then also
> +		 * include the extra low memory requirement declared by other
> +		 * components. If falled back to high reservation the dedicated
> +		 * low crash memory will take care of that.
> +		 */
> +		if (crash_base)
> +			crash_size += crash_low_extra;
> +		else
>  			crash_base = memblock_find_in_range(CRASH_ALIGN,
>  						CRASH_ADDR_HIGH_MAX,
>  						crash_size, CRASH_ALIGN);
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 9268c12458c8..b4556d2dcb8e 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -415,6 +415,8 @@ void __init mem_encrypt_init(void)
>  	if (sev_active())
>  		static_branch_enable(&sev_enable_key);
> +	kdump_need_extra_low_memory(swiotlb_size_or_default());
> +
>  	pr_info("AMD %s active\n",
>  		sev_active() ? "Secure Encrypted Virtualization (SEV)"
>  			     : "Secure Memory Encryption (SME)");

BTW, your above patch chunks are not malformed, can not be applied cleanly.

Thanks
Dave