Re: arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP

[AKASHI Takahiro]

On Tue, Dec 26, 2017 at 02:58:45PM +0800, Dave Young wrote:
> On 12/26/17 at 08:26am, Bhupesh Sharma wrote:
> > On Tue, Dec 26, 2017 at 7:58 AM, AKASHI Takahiro
> >  wrote:
> > > On Tue, Dec 26, 2017 at 09:35:17AM +0800, Dave Young wrote:
> > >> [snip]
> > >> > > > Well, we may be able to change pr_warn() to pr_warn_once() here, 
> > >> > > > but
> > >> > > > I hope that adding "numa=off" to kernel command line should also 
> > >> > > > work.
> > >> > >
> > >> > > Hmm, adding "numa=off" to crashkernel bootargs works, and TBH it was
> > >> > > my initial thought process as well, but I am not sure if this will
> > >> > > cause any regressions on aarch64 systems which use crashdump feature.
> > >> >
> > >> > It should be fine since we use numa=off by default for all other arches
> > >> > ie. x86, ppc64 and s390. Actually disabling numa in kdump kernel can 
> > >> > save
> > >> > mm component memory usage.
> > >> >
> > >>
> > >> Forgot to say I means in RHEL and Fedora we use numa=off for kdump..
> > >
> > > Thank you for the clarification.
> > > (It might be better to make numa off automatically if maxcpus == 0 (and 
> > > 1?).)
> > >
> > 
> > Not sure if we can leave this to the distribution-specific kdump
> > scripts (as the crashkernel boot can be held up for sufficient time
> > and may appear stuck). The distribution scripts may be different (for
> > e.g. ubuntu and RHEL/fedora) across distributions and may have
> > different bootarg options.
> 
> Personally I think distribution should take care of this param as for
> kdump.  But as AKASHI said it could be a issue for 1st kernel with
> nr_cpus=1 booting.  Problem is why we do not see this issue on other
> machines.

The issue won't be kdump-specific. Theoretically, it also takes place
when "mem=" is specified on numa.

Since we can avoid annoying messages by adding "numa=off", I'm reluctant to
suppress most of messages but the first. My suggestion here is to add some
notes in Documentation/kdump/kdump.txt regarding NUMA case.

Thanks,
Takahiro AKASHI


> > 
> > So how about considering a kernel fix only which doesn't require
> > relying on changing the distribution-specific kdump scripts, as we
> > should avoid introducing a regression while trying to fix a regression
> > :)
> > 
> > Just my 2 cents.
> > 
> > Thanks,
> > Bhupesh
> 
> Thanks
> Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP

[AKASHI Takahiro]

On Tue, Dec 26, 2017 at 02:56:36PM +0800, Dave Young wrote:
> On 12/26/17 at 11:28am, AKASHI Takahiro wrote:
> > On Tue, Dec 26, 2017 at 09:35:17AM +0800, Dave Young wrote:
> > > [snip]
> > > > > > Well, we may be able to change pr_warn() to pr_warn_once() here, but
> > > > > > I hope that adding "numa=off" to kernel command line should also 
> > > > > > work.
> > > > > 
> > > > > Hmm, adding "numa=off" to crashkernel bootargs works, and TBH it was
> > > > > my initial thought process as well, but I am not sure if this will
> > > > > cause any regressions on aarch64 systems which use crashdump feature.
> > > > 
> > > > It should be fine since we use numa=off by default for all other arches
> > > > ie. x86, ppc64 and s390. Actually disabling numa in kdump kernel can 
> > > > save
> > > > mm component memory usage. 
> > > > 
> > > 
> > > Forgot to say I means in RHEL and Fedora we use numa=off for kdump..
> > 
> > Thank you for the clarification.
> > (It might be better to make numa off automatically if maxcpus == 0 (and 
> > 1?).)
> 
> Hmm, I did a quick test with qemu/kvm, kdump kernel boot without numa=off
> I'm not sure why I do not see the warning messages on x86
> machines, maybe something arm64 specific?

I didn't see the messages(i.e. "potential offnode page_structs")
on arm64 qemu (with -smp 2 -numa node -numa node).

It seems that qemu doesn't generate acpi slit(inter-node distance table).

Thanks,
-Takahiro AKASHI

> > 
> > -Takahiro AKASHI
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP

[AKASHI Takahiro]

Bhupesh,

On Tue, Jan 09, 2018 at 01:30:07AM +0530, Bhupesh Sharma wrote:
> Hello Akashi,
> 
> On Tue, Dec 26, 2017 at 8:26 AM, Bhupesh Sharma  wrote:
> > On Tue, Dec 26, 2017 at 7:58 AM, AKASHI Takahiro
> >  wrote:
> >> On Tue, Dec 26, 2017 at 09:35:17AM +0800, Dave Young wrote:
> >>> [snip]
> >>> > > > Well, we may be able to change pr_warn() to pr_warn_once() here, but
> >>> > > > I hope that adding "numa=off" to kernel command line should also 
> >>> > > > work.
> >>> > >
> >>> > > Hmm, adding "numa=off" to crashkernel bootargs works, and TBH it was
> >>> > > my initial thought process as well, but I am not sure if this will
> >>> > > cause any regressions on aarch64 systems which use crashdump feature.
> >>> >
> >>> > It should be fine since we use numa=off by default for all other arches
> >>> > ie. x86, ppc64 and s390. Actually disabling numa in kdump kernel can 
> >>> > save
> >>> > mm component memory usage.
> >>> >
> >>>
> >>> Forgot to say I means in RHEL and Fedora we use numa=off for kdump..
> >>
> >> Thank you for the clarification.
> >> (It might be better to make numa off automatically if maxcpus == 0 (and 
> >> 1?).)
> >>
> >
> > Not sure if we can leave this to the distribution-specific kdump
> > scripts (as the crashkernel boot can be held up for sufficient time
> > and may appear stuck). The distribution scripts may be different (for
> > e.g. ubuntu and RHEL/fedora) across distributions and may have
> > different bootarg options.
> >
> > So how about considering a kernel fix only which doesn't require
> > relying on changing the distribution-specific kdump scripts, as we
> > should avoid introducing a regression while trying to fix a regression
> > :)
> >
> > Just my 2 cents.
> >
> 
> Sorry for the delay but I was on holidays in the last week.
> 
> Are you planning to send a patch to fix this issue or do you want me
> to send a RFC version instead?

I should have submitted my own patch before my new year holidays,
but I will do so as soon as possible.

Thanks,
-Takahiro AKASHI


> i think this is a blocking issue for aarch64 kdump support on newer
> kernels (v4.14) and we are already hearing about this issue from other
> users as well, so it would be great to get this fixed now that we have
> root-caused the issue and found a possible way around.
> 
> Regards,
> Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] efi: Clarify that reset attack mitigation needs appropriate userspace

[Matthew Garrett]

Some distributions have turned on the reset attack mitigation feature,
which is designed to force the platform to clear the contents of RAM if
the machine is shut down uncleanly. However, in order for the platform
to be able to determine whether the shutdown was clean or not, userspace
has to be configured to clear the MemoryOverwriteRequest flag on
shutdown - otherwise the firmware will end up clearing RAM on every
reboot, which is unnecessarily time consuming. Add some additional
clarity to the kconfig text to reduce the risk of systems being
configured this way.

Signed-off-by: Matthew Garrett 
Cc: Ard Biesheuvel 
Cc: linux-efi@vger.kernel.org
Cc: sta...@vger.kernel.org
---
 drivers/firmware/efi/Kconfig | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2b4c39fdfa91..86210f75d233 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -159,7 +159,10 @@ config RESET_ATTACK_MITIGATION
  using the TCG Platform Reset Attack Mitigation specification. This
  protects against an attacker forcibly rebooting the system while it
  still contains secrets in RAM, booting another OS and extracting the
- secrets.
+ secrets. This should only be enabled when userland is configured to
+ clear the MemoryOverwriteRequest flag on clean shutdown after secrets
+ have been evicted, since otherwise it will trigger even on clean
+ reboots.
 
 endmenu
 
-- 
2.16.0.rc0.223.g4a4ac83678-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP

[Bhupesh Sharma]

Hello Akashi,

On Tue, Dec 26, 2017 at 8:26 AM, Bhupesh Sharma  wrote:
> On Tue, Dec 26, 2017 at 7:58 AM, AKASHI Takahiro
>  wrote:
>> On Tue, Dec 26, 2017 at 09:35:17AM +0800, Dave Young wrote:
>>> [snip]
>>> > > > Well, we may be able to change pr_warn() to pr_warn_once() here, but
>>> > > > I hope that adding "numa=off" to kernel command line should also work.
>>> > >
>>> > > Hmm, adding "numa=off" to crashkernel bootargs works, and TBH it was
>>> > > my initial thought process as well, but I am not sure if this will
>>> > > cause any regressions on aarch64 systems which use crashdump feature.
>>> >
>>> > It should be fine since we use numa=off by default for all other arches
>>> > ie. x86, ppc64 and s390. Actually disabling numa in kdump kernel can save
>>> > mm component memory usage.
>>> >
>>>
>>> Forgot to say I means in RHEL and Fedora we use numa=off for kdump..
>>
>> Thank you for the clarification.
>> (It might be better to make numa off automatically if maxcpus == 0 (and 1?).)
>>
>
> Not sure if we can leave this to the distribution-specific kdump
> scripts (as the crashkernel boot can be held up for sufficient time
> and may appear stuck). The distribution scripts may be different (for
> e.g. ubuntu and RHEL/fedora) across distributions and may have
> different bootarg options.
>
> So how about considering a kernel fix only which doesn't require
> relying on changing the distribution-specific kdump scripts, as we
> should avoid introducing a regression while trying to fix a regression
> :)
>
> Just my 2 cents.
>

Sorry for the delay but I was on holidays in the last week.

Are you planning to send a patch to fix this issue or do you want me
to send a RFC version instead?

i think this is a blocking issue for aarch64 kdump support on newer
kernels (v4.14) and we are already hearing about this issue from other
users as well, so it would be great to get this fixed now that we have
root-caused the issue and found a possible way around.

Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.14 13/38] efi/capsule-loader: Reinstate virtual capsule mapping

[Greg Kroah-Hartman]

4.14-stable review patch.  If anyone has any objections, please let me know.

--

From: Ard Biesheuvel 

commit f24c4d478013d82bd1b943df566fff3561d52864 upstream.

Commit:

  82c3768b8d68 ("efi/capsule-loader: Use a cached copy of the capsule header")

... refactored the capsule loading code that maps the capsule header,
to avoid having to map it several times.

However, as it turns out, the vmap() call we ended up removing did not
just map the header, but the entire capsule image, and dropping this
virtual mapping breaks capsules that are processed by the firmware
immediately (i.e., without a reboot).

Unfortunately, that change was part of a larger refactor that allowed
a quirk to be implemented for Quark, which has a non-standard memory
layout for capsules, and we have slightly painted ourselves into a
corner by allowing quirk code to mangle the capsule header and memory
layout.

So we need to fix this without breaking Quark. Fortunately, Quark does
not appear to care about the virtual mapping, and so we can simply
do a partial revert of commit:

  2a457fb31df6 ("efi/capsule-loader: Use page addresses rather than struct page 
pointers")

... and create a vmap() mapping of the entire capsule (including header)
based on the reinstated struct page array, unless running on Quark, in
which case we pass the capsule header copy as before.

Reported-by: Ge Song 
Tested-by: Bryan O'Donoghue 
Tested-by: Ge Song 
Signed-off-by: Ard Biesheuvel 
Cc: Dave Young 
Cc: Linus Torvalds 
Cc: Matt Fleming 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: linux-efi@vger.kernel.org
Fixes: 82c3768b8d68 ("efi/capsule-loader: Use a cached copy of the capsule 
header")
Link: http://lkml.kernel.org/r/20180102172110.17018-3-ard.biesheu...@linaro.org
Signed-off-by: Ingo Molnar 
Signed-off-by: Greg Kroah-Hartman 

---
 arch/x86/platform/efi/quirks.c|   13 +
 drivers/firmware/efi/capsule-loader.c |   45 +++---
 include/linux/efi.h   |4 ++-
 3 files changed, 52 insertions(+), 10 deletions(-)

--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -592,7 +592,18 @@ static int qrk_capsule_setup_info(struct
/*
 * Update the first page pointer to skip over the CSH header.
 */
-   cap_info->pages[0] += csh->headersize;
+   cap_info->phys[0] += csh->headersize;
+
+   /*
+* cap_info->capsule should point at a virtual mapping of the entire
+* capsule, starting at the capsule header. Our image has the Quark
+* security header prepended, so we cannot rely on the default vmap()
+* mapping created by the generic capsule code.
+* Given that the Quark firmware does not appear to care about the
+* virtual mapping, let's just point cap_info->capsule at our copy
+* of the capsule header.
+*/
+   cap_info->capsule = _info->header;
 
return 1;
 }
--- a/drivers/firmware/efi/capsule-loader.c
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -20,10 +20,6 @@
 
 #define NO_FURTHER_WRITE_ACTION -1
 
-#ifndef phys_to_page
-#define phys_to_page(x)pfn_to_page((x) >> PAGE_SHIFT)
-#endif
-
 /**
  * efi_free_all_buff_pages - free all previous allocated buffer pages
  * @cap_info: pointer to current instance of capsule_info structure
@@ -35,7 +31,7 @@
 static void efi_free_all_buff_pages(struct capsule_info *cap_info)
 {
while (cap_info->index > 0)
-   __free_page(phys_to_page(cap_info->pages[--cap_info->index]));
+   __free_page(cap_info->pages[--cap_info->index]);
 
cap_info->index = NO_FURTHER_WRITE_ACTION;
 }
@@ -71,6 +67,14 @@ int __efi_capsule_setup_info(struct caps
 
cap_info->pages = temp_page;
 
+   temp_page = krealloc(cap_info->phys,
+pages_needed * sizeof(phys_addr_t *),
+GFP_KERNEL | __GFP_ZERO);
+   if (!temp_page)
+   return -ENOMEM;
+
+   cap_info->phys = temp_page;
+
return 0;
 }
 
@@ -105,9 +109,24 @@ int __weak efi_capsule_setup_info(struct
  **/
 static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info)
 {
+   bool do_vunmap = false;
int ret;
 
-   ret = efi_capsule_update(_info->header, cap_info->pages);
+   /*
+* cap_info->capsule may have been assigned already by a quirk
+* handler, so only overwrite it if it is NULL
+*/
+   if (!cap_info->capsule) {
+   cap_info->capsule = vmap(cap_info->pages, cap_info->index,
+VM_MAP, PAGE_KERNEL);
+   if (!cap_info->capsule)
+