lkml.org 
[lkml]   [2019]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
On Mon, Sep 02, 2019 at 03:38:22PM +0800, Kairui Song wrote:
> Will it be good if the final code looks like this?
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 48115cf11e0f..754b25d6e785 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -526,6 +526,69 @@ static int __init reserve_crashkernel_low(void)
> return 0;
> }
> +static int __init crashkernel_find_region(
> + unsigned long long *base,
> + unsigned long long *size,
> + bool high)

Those should be aligned at the opening brace.

> +{
> + unsigned long long start, mem_enc_req = 0;

Declare that mem_enc_req in the if (!high) branch below, where you need it only.

> +
> + /*
> + * *base == 0 means: find the address automatically, else just
> + * verify the region is useable
> + */
> + if (*base) {
> + start = memblock_find_in_range(*base, *base + *size,
> + *size, 1 << 20);
> + if (start != *base) {
> + pr_info("crashkernel reservation failed - memory is in use.\n");
> + return -EBUSY;

I don't like functions which change external variables passed as
pointers but then in the error case, change those unnecessarily. Write
into *base and *size only in the success case pls and use local vars for
the intermediate results.

Also, those retvals are not visible to userspace - just return negative for
error and 0 for success.

> + }
> + return 0;
> + }
> +
> + /*
> + * Set CRASH_ADDR_LOW_MAX upper bound for crash memory,
> + * crashkernel=x,high reserves memory over 4G, also allocates
> + * 256M extra low memory for DMA buffers and swiotlb.
> + * But the extra memory is not required for all machines.
> + * So try low memory first and fall back to high memory
> + * unless "crashkernel=size[KMG],high" is specified.
> + */
> + if (!high) {

if (high)
goto high_reserve;

< now save an indentation level >

> + /*
> + * 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 + mem_enc_req,
> + CRASH_ALIGN);
> + if (*base) {
> + if (mem_enc_req) {
> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> + (unsigned long)(mem_enc_req >> 20));
> + *size += mem_enc_req;
> + }
> + return 0;
> + }
> + }
> +

high_reserve:

> + /* Try high reserve */
> + *base = memblock_find_in_range(CRASH_ALIGN,
> + CRASH_ADDR_HIGH_MAX,
> + *size, CRASH_ALIGN);
> + if (!*base) {
> + pr_info("crashkernel reservation failed - No suitable area found.\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}

...

> If you are OK with this, I will split it into two patch and send V3.

With that, yes, this looks a bit better.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

\
 
 \ /
  Last update: 2019-09-05 18:30    [W:0.066 / U:4.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site