Message-ID: <CACPcB9daWhyvqB_QEvnLuvp5BiZ4cSOoj6KaP8mcSUEiQGFC_w@mail.gmail.com>
Date:   Fri, 23 Aug 2019 10:11:15 +0800
From:   Kairui Song <kasong@...hat.com>
To:     "Lendacky, Thomas" <Thomas.Lendacky@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Baoquan He <bhe@...hat.com>, Lianbo Jiang <lijiang@...hat.com>,
        Dave Young <dyoung@...hat.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "kexec@...ts.infradead.org" <kexec@...ts.infradead.org>
Subject: Re: [PATCH] x86/kdump: Reserve extra memory when SME or SEV is active

On Thu, Aug 22, 2019 at 10:35 PM Lendacky, Thomas
<Thomas.Lendacky@....com> wrote:
>
> On 8/21/19 9:53 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@...hat.com>
> > ---
> >  arch/x86/kernel/setup.c | 26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index bbe35bf879f5..ed91fa9d9f6e 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,17 @@ static void __init reserve_crashkernel(void)
> >               return;
> >       }
> >
> > +     /*
> > +      * When SME/SEV is active, it will always required an extra SWIOTLB
> > +      * region.
> > +      */
> > +     if (sme_active() || sev_active()) {
>
> You can use mem_encrypt_active() here in place of the two checks.

That's a very good suggestion.

>
> > +             mem_enc_req = ALIGN(swiotlb_size_or_default(), SZ_1M);
> > +             pr_info("Memory encryption is active, crashkernel needs %ldMB extra memory\n",
> > +                             (unsigned long)(mem_enc_req >> 20));
>
> There is a point below where you zero out this value, so should this
> be issued later only if mem_enc_req is non-zero?

Yes that's true, but currently if zero out this value when ",high" is
used, then an extra low memory region will be reserved, so this
message will not be very confusing I think? as the required extra
memory is now in the low memory region. And for the "@offset" case
this could be a hint for users. And if the reserve failed due to
enlarged crashkernel size, the user may also be better aware of what
is causing the failure by this message.

>
> Also, looks like one too many tabs.
>
> > +     } else
>
> Since you used braces on the if path, you need braces on the else path.

OK, will fix the code style issues.

>
> Thanks,
> Tom
>
> > +             mem_enc_req = 0;
> > +
> >       /* 0 means: find the address automatically */
> >       if (!crash_base) {
> >               /*
> > @@ -563,11 +574,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;
> > @@ -583,6 +602,7 @@ static void __init reserve_crashkernel(void)
> >                       return;
> >               }
> >       }
> > +     crash_size += mem_enc_req;
> >       ret = memblock_reserve(crash_base, crash_size);
> >       if (ret) {
> >               pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
> >



-- 
Best Regards,
Kairui Song

Powered by blists - more mailing lists