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

[Borislav Petkov]

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