* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-08-26 4:45 [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active Kairui Song
@ 2019-08-26 23:53 ` Kairui Song
2019-08-27 5:46 ` Baoquan He
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Kairui Song @ 2019-08-26 23:53 UTC (permalink / raw)
To: Thomas Lendacky
Cc: Linux Kernel Mailing List, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Baoquan He, Lianbo Jiang, Dave Young,
the arch/x86 maintainers, kexec@lists.infradead.org
On Mon, Aug 26, 2019 at 12:46 PM Kairui Song <kasong@redhat.com> 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@redhat.com>
>
> ---
> Update from V1:
> - Use mem_encrypt_active() instead of "sme_active() || sev_active()"
> - Don't reserve extra memory when ",high" or "@offset" is used, and
> don't print redundant message.
> - Fix coding style problem
>
> arch/x86/kernel/setup.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index bbe35bf879f5..221beb10c55d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -528,7 +528,7 @@ static int __init reserve_crashkernel_low(void)
>
> static void __init reserve_crashkernel(void)
> {
> - unsigned long long crash_size, crash_base, total_mem;
> + unsigned long long crash_size, crash_base, total_mem, mem_enc_req;
> bool high = false;
> int ret;
>
> @@ -550,6 +550,15 @@ static void __init reserve_crashkernel(void)
> return;
> }
>
> + /*
> + * When SME/SEV is active, it will always required an extra SWIOTLB
> + * region.
> + */
> + if (mem_encrypt_active())
> + mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> + else
> + mem_enc_req = 0;
> +
> /* 0 means: find the address automatically */
> if (!crash_base) {
> /*
> @@ -563,11 +572,19 @@ 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 + mem_enc_req,
> + CRASH_ALIGN);
> + /*
> + * For high reservation, an extra low memory for SWIOTLB will
> + * always be reserved later, so no need to reserve extra
> + * memory for memory encryption case here.
> + */
> + if (!crash_base) {
> + mem_enc_req = 0;
> crash_base = memblock_find_in_range(CRASH_ALIGN,
> CRASH_ADDR_HIGH_MAX,
> crash_size, CRASH_ALIGN);
> + }
> if (!crash_base) {
> pr_info("crashkernel reservation failed - No suitable area found.\n");
> return;
> @@ -575,6 +592,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> + mem_enc_req = 0;
> start = memblock_find_in_range(crash_base,
> crash_base + crash_size,
> crash_size, 1 << 20);
> @@ -583,6 +601,13 @@ static void __init reserve_crashkernel(void)
> return;
> }
> }
> +
> + if (mem_enc_req) {
> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> + (unsigned long)(mem_enc_req >> 20));
> + crash_size += mem_enc_req;
> + }
> +
> ret = memblock_reserve(crash_base, crash_size);
> if (ret) {
> pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
> --
> 2.21.0
>
Hi Tom, any comment about V2?
--
Best Regards,
Kairui Song
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-08-26 4:45 [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active Kairui Song
2019-08-26 23:53 ` Kairui Song
@ 2019-08-27 5:46 ` Baoquan He
2019-08-27 13:43 ` Lendacky, Thomas
2019-08-30 16:45 ` Borislav Petkov
3 siblings, 0 replies; 7+ messages in thread
From: Baoquan He @ 2019-08-27 5:46 UTC (permalink / raw)
To: Kairui Song
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Thomas Lendacky, Lianbo Jiang, Dave Young, x86,
kexec@lists.infradead.org
On 08/26/19 at 12:45pm, Kairui Song 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@redhat.com>
The patch looks good to me, ack it.
Acked-by: Baoquan He <bhe@redhat.com>
Thanks
Baoquan
>
> ---
> Update from V1:
> - Use mem_encrypt_active() instead of "sme_active() || sev_active()"
> - Don't reserve extra memory when ",high" or "@offset" is used, and
> don't print redundant message.
> - Fix coding style problem
>
> arch/x86/kernel/setup.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index bbe35bf879f5..221beb10c55d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -528,7 +528,7 @@ static int __init reserve_crashkernel_low(void)
>
> static void __init reserve_crashkernel(void)
> {
> - unsigned long long crash_size, crash_base, total_mem;
> + unsigned long long crash_size, crash_base, total_mem, mem_enc_req;
> bool high = false;
> int ret;
>
> @@ -550,6 +550,15 @@ static void __init reserve_crashkernel(void)
> return;
> }
>
> + /*
> + * When SME/SEV is active, it will always required an extra SWIOTLB
> + * region.
> + */
> + if (mem_encrypt_active())
> + mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> + else
> + mem_enc_req = 0;
> +
> /* 0 means: find the address automatically */
> if (!crash_base) {
> /*
> @@ -563,11 +572,19 @@ 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 + mem_enc_req,
> + CRASH_ALIGN);
> + /*
> + * For high reservation, an extra low memory for SWIOTLB will
> + * always be reserved later, so no need to reserve extra
> + * memory for memory encryption case here.
> + */
> + if (!crash_base) {
> + mem_enc_req = 0;
> crash_base = memblock_find_in_range(CRASH_ALIGN,
> CRASH_ADDR_HIGH_MAX,
> crash_size, CRASH_ALIGN);
> + }
> if (!crash_base) {
> pr_info("crashkernel reservation failed - No suitable area found.\n");
> return;
> @@ -575,6 +592,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> + mem_enc_req = 0;
> start = memblock_find_in_range(crash_base,
> crash_base + crash_size,
> crash_size, 1 << 20);
> @@ -583,6 +601,13 @@ static void __init reserve_crashkernel(void)
> return;
> }
> }
> +
> + if (mem_enc_req) {
> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> + (unsigned long)(mem_enc_req >> 20));
> + crash_size += mem_enc_req;
> + }
> +
> ret = memblock_reserve(crash_base, crash_size);
> if (ret) {
> pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-08-26 4:45 [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active Kairui Song
2019-08-26 23:53 ` Kairui Song
2019-08-27 5:46 ` Baoquan He
@ 2019-08-27 13:43 ` Lendacky, Thomas
2019-08-30 16:45 ` Borislav Petkov
3 siblings, 0 replies; 7+ messages in thread
From: Lendacky, Thomas @ 2019-08-27 13:43 UTC (permalink / raw)
To: Kairui Song, linux-kernel@vger.kernel.org
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Baoquan He,
Lianbo Jiang, Dave Young, x86@kernel.org,
kexec@lists.infradead.org
On 8/25/19 11:45 PM, Kairui Song 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@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> ---
> Update from V1:
> - Use mem_encrypt_active() instead of "sme_active() || sev_active()"
> - Don't reserve extra memory when ",high" or "@offset" is used, and
> don't print redundant message.
> - Fix coding style problem
>
> arch/x86/kernel/setup.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index bbe35bf879f5..221beb10c55d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -528,7 +528,7 @@ static int __init reserve_crashkernel_low(void)
>
> static void __init reserve_crashkernel(void)
> {
> - unsigned long long crash_size, crash_base, total_mem;
> + unsigned long long crash_size, crash_base, total_mem, mem_enc_req;
> bool high = false;
> int ret;
>
> @@ -550,6 +550,15 @@ static void __init reserve_crashkernel(void)
> return;
> }
>
> + /*
> + * When SME/SEV is active, it will always required an extra SWIOTLB
> + * region.
> + */
> + if (mem_encrypt_active())
> + mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> + else
> + mem_enc_req = 0;
> +
> /* 0 means: find the address automatically */
> if (!crash_base) {
> /*
> @@ -563,11 +572,19 @@ 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 + mem_enc_req,
> + CRASH_ALIGN);
> + /*
> + * For high reservation, an extra low memory for SWIOTLB will
> + * always be reserved later, so no need to reserve extra
> + * memory for memory encryption case here.
> + */
> + if (!crash_base) {
> + mem_enc_req = 0;
> crash_base = memblock_find_in_range(CRASH_ALIGN,
> CRASH_ADDR_HIGH_MAX,
> crash_size, CRASH_ALIGN);
> + }
> if (!crash_base) {
> pr_info("crashkernel reservation failed - No suitable area found.\n");
> return;
> @@ -575,6 +592,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> + mem_enc_req = 0;
> start = memblock_find_in_range(crash_base,
> crash_base + crash_size,
> crash_size, 1 << 20);
> @@ -583,6 +601,13 @@ static void __init reserve_crashkernel(void)
> return;
> }
> }
> +
> + if (mem_enc_req) {
> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> + (unsigned long)(mem_enc_req >> 20));
> + crash_size += mem_enc_req;
> + }
> +
> ret = memblock_reserve(crash_base, crash_size);
> if (ret) {
> pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-08-26 4:45 [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active Kairui Song
` (2 preceding siblings ...)
2019-08-27 13:43 ` Lendacky, Thomas
@ 2019-08-30 16:45 ` Borislav Petkov
2019-09-02 7:38 ` Kairui Song
3 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2019-08-30 16:45 UTC (permalink / raw)
To: Kairui Song
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Thomas Lendacky,
Baoquan He, Lianbo Jiang, Dave Young, x86,
kexec@lists.infradead.org
On Mon, Aug 26, 2019 at 12:45:35PM +0800, Kairui Song 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@redhat.com>
>
> ---
> Update from V1:
> - Use mem_encrypt_active() instead of "sme_active() || sev_active()"
> - Don't reserve extra memory when ",high" or "@offset" is used, and
> don't print redundant message.
> - Fix coding style problem
>
> arch/x86/kernel/setup.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index bbe35bf879f5..221beb10c55d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -528,7 +528,7 @@ static int __init reserve_crashkernel_low(void)
>
> static void __init reserve_crashkernel(void)
> {
> - unsigned long long crash_size, crash_base, total_mem;
> + unsigned long long crash_size, crash_base, total_mem, mem_enc_req;
> bool high = false;
> int ret;
>
> @@ -550,6 +550,15 @@ static void __init reserve_crashkernel(void)
> return;
> }
>
> + /*
> + * When SME/SEV is active, it will always required an extra SWIOTLB
> + * region.
> + */
> + if (mem_encrypt_active())
> + mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> + else
> + mem_enc_req = 0;
Hmm, ugly.
You set mem_enc_reg here ...
> +
> /* 0 means: find the address automatically */
> if (!crash_base) {
> /*
> @@ -563,11 +572,19 @@ 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 + mem_enc_req,
> + CRASH_ALIGN);
> + /*
> + * For high reservation, an extra low memory for SWIOTLB will
> + * always be reserved later, so no need to reserve extra
> + * memory for memory encryption case here.
> + */
> + if (!crash_base) {
> + mem_enc_req = 0;
... but you clear it here...
> crash_base = memblock_find_in_range(CRASH_ALIGN,
> CRASH_ADDR_HIGH_MAX,
> crash_size, CRASH_ALIGN);
> + }
> if (!crash_base) {
> pr_info("crashkernel reservation failed - No suitable area found.\n");
> return;
> @@ -575,6 +592,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> + mem_enc_req = 0;
... and here...
> start = memblock_find_in_range(crash_base,
> crash_base + crash_size,
> crash_size, 1 << 20);
> @@ -583,6 +601,13 @@ static void __init reserve_crashkernel(void)
> return;
> }
> }
> +
> + if (mem_enc_req) {
> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> + (unsigned long)(mem_enc_req >> 20));
> + crash_size += mem_enc_req;
> + }
... and then you report only when it is still set.
How about you carve out that if (!crash_base) { ... } else { } piece
into a separate function without any further changes - only code
movement? That is your patch 1.
Your patch 2 is then adding the mem_encrypt_active() check in the if
(!crash_base && !high) case, i.e., only where you need it and issuing
the pr_info from there instead of stretching that logic throughout the
whole function and twisting my brain unnecessarily?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-08-30 16:45 ` Borislav Petkov
@ 2019-09-02 7:38 ` Kairui Song
2019-09-05 16:29 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Kairui Song @ 2019-09-02 7:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Thomas Lendacky,
Baoquan He, Lianbo Jiang, Dave Young, x86,
kexec@lists.infradead.org
On 8/31/19 12:45 AM, Borislav Petkov wrote:
> On Mon, Aug 26, 2019 at 12:45:35PM +0800, Kairui Song 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@redhat.com>
>>
>> ---
>> Update from V1:
>> - Use mem_encrypt_active() instead of "sme_active() || sev_active()"
>> - Don't reserve extra memory when ",high" or "@offset" is used, and
>> don't print redundant message.
>> - Fix coding style problem
>>
>> arch/x86/kernel/setup.c | 31 ++++++++++++++++++++++++++++---
>> 1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index bbe35bf879f5..221beb10c55d 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -528,7 +528,7 @@ static int __init reserve_crashkernel_low(void)
>>
>> static void __init reserve_crashkernel(void)
>> {
>> - unsigned long long crash_size, crash_base, total_mem;
>> + unsigned long long crash_size, crash_base, total_mem, mem_enc_req;
>> bool high = false;
>> int ret;
>>
>> @@ -550,6 +550,15 @@ static void __init reserve_crashkernel(void)
>> return;
>> }
>>
>> + /*
>> + * When SME/SEV is active, it will always required an extra SWIOTLB
>> + * region.
>> + */
>> + if (mem_encrypt_active())
>> + mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
>> + else
>> + mem_enc_req = 0;
>
> Hmm, ugly.
I agree with this, but didn't have a better idea about how toimprove it, so thanks for the suggestions below.
>
> You set mem_enc_reg here ...
>
>> +
>> /* 0 means: find the address automatically */
>> if (!crash_base) {
>> /*
>> @@ -563,11 +572,19 @@ 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 + mem_enc_req,
>> + CRASH_ALIGN);
>> + /*
>> + * For high reservation, an extra low memory for SWIOTLB will
>> + * always be reserved later, so no need to reserve extra
>> + * memory for memory encryption case here.
>> + */
>> + if (!crash_base) {
>> + mem_enc_req = 0;
>
> ... but you clear it here...
>
>> crash_base = memblock_find_in_range(CRASH_ALIGN,
>> CRASH_ADDR_HIGH_MAX,
>> crash_size, CRASH_ALIGN);
>> + }
>> if (!crash_base) {
>> pr_info("crashkernel reservation failed - No suitable area found.\n");
>> return;
>> @@ -575,6 +592,7 @@ static void __init reserve_crashkernel(void)
>> } else {
>> unsigned long long start;
>>
>> + mem_enc_req = 0;
>
> ... and here...
>
>> start = memblock_find_in_range(crash_base,
>> crash_base + crash_size,
>> crash_size, 1 << 20);
>> @@ -583,6 +601,13 @@ static void __init reserve_crashkernel(void)
>> return;
>> }
>> }
>> +
>> + if (mem_enc_req) {
>> + pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
>> + (unsigned long)(mem_enc_req >> 20));
>> + crash_size += mem_enc_req;
>> + }
>
> ... and then you report only when it is still set.
>
> How about you carve out that if (!crash_base) { ... } else { } piece
> into a separate function without any further changes - only code
> movement? That is your patch 1.
>
> Your patch 2 is then adding the mem_encrypt_active() check in the if
> (!crash_base && !high) case, i.e., only where you need it and issuing
> the pr_info from there instead of stretching that logic throughout the
> whole function and twisting my brain unnecessarily?
>
> Thx.
>
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)
+{
+ unsigned long long start, mem_enc_req = 0;
+
+ /*
+ * *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;
+ }
+ 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) {
+ /*
+ * 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;
+ }
+ }
+
+ /* 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;
+}
+
static void __init reserve_crashkernel(void)
{
unsigned long long crash_size, crash_base, total_mem;
@@ -550,39 +613,10 @@ static void __init reserve_crashkernel(void)
return;
}
- /* 0 means: find the address automatically */
- if (!crash_base) {
- /*
- * 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)
- crash_base = memblock_find_in_range(CRASH_ALIGN,
- CRASH_ADDR_LOW_MAX,
- crash_size, CRASH_ALIGN);
- if (!crash_base)
- crash_base = memblock_find_in_range(CRASH_ALIGN,
- CRASH_ADDR_HIGH_MAX,
- crash_size, CRASH_ALIGN);
- if (!crash_base) {
- pr_info("crashkernel reservation failed - No suitable area found.\n");
- return;
- }
- } else {
- unsigned long long start;
+ ret = crashkernel_find_region(&crash_base, &crash_size, high);
+ if (ret)
+ return;
- start = memblock_find_in_range(crash_base,
- crash_base + crash_size,
- crash_size, 1 << 20);
- if (start != crash_base) {
- pr_info("crashkernel reservation failed - memory is in use.\n");
- return;
- }
- }
ret = memblock_reserve(crash_base, crash_size);
if (ret) {
pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
---
If you are OK with this, I will split it into two patch and send V3.
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] x86/kdump: Reserve extra memory when SME or SEV is active
2019-09-02 7:38 ` Kairui Song
@ 2019-09-05 16:29 ` Borislav Petkov
0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2019-09-05 16:29 UTC (permalink / raw)
To: Kairui Song
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Thomas Lendacky,
Baoquan He, Lianbo Jiang, Dave Young, x86,
kexec@lists.infradead.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread