Re: [RFC PATCH 00/16] Signature verification of hibernate snapshot
Hi Jiri, On Fri, Jul 24, 2015 at 07:08:18PM +0200, Jiri Kosina wrote: > On Thu, 16 Jul 2015, Lee, Chun-Yi wrote: > > > This patchset is the implementation of signature verification of hibernate > > snapshot image. The origin idea is from Jiri Kosina: Let EFI bootloader > > generate key-pair in UEFI secure boot environment, then forward it to kernel > > for sign/verify hibernate image. > > I've finally managed to fully go through the series and test it on my > secure boot testing setup. > > Reviewed-by: Jiri Kosina > Tested-by: Jiri Kosina > > for the whole series. Thanks, > > -- > Jiri Kosina > SUSE Labs Thanks for your review and test. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ACPI / init: Switch over platform to the ACPI mode later
On Wed, Jun 10, 2015 at 01:48:29AM +0200, Rafael J. Wysocki wrote: > On Wednesday, June 03, 2015 11:13:57 AM Toshi Kani wrote: > > On Sat, 2015-05-30 at 14:21 +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki > > > > > > Commit 73f7d1ca3263 "ACPI / init: Run acpi_early_init() before > > > timekeeping_init()" moved the ACPI subsystem initialization, > > > including the ACPI mode enabling, to an earlier point in the > > > initialization sequence, to allow the timekeeping subsystem > > > use ACPI early. Unfortunately, that resulted in boot regressions > > > on some systems and the early ACPI initialization was moved toward > > > its original position in the kernel initialization code by commit > > > c4e1acbb35e4 "ACPI / init: Invoke early ACPI initialization later". > > > > > > However, that turns out to be insufficient, as boot is still broken > > > on the Tyan S8812 mainboard. > > > > > > To fix that issue, split the ACPI early initialization code into > > > two pieces so the majority of it still located in acpi_early_init() > > > and the part switching over the platform into the ACPI mode goes into > > > a new function, acpi_subsystem_init(), executed at the original early > > > ACPI initialization spot. > > > > > > That fixes the Tyan S8812 boot problem, but still allows ACPI > > > tables to be loaded earlier which is useful to the EFI code in > > > efi_enter_virtual_mode(). > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=97141 > > > Reported-and-tested-by: Marius Tolzmann > > > Signed-off-by: Rafael J. Wysocki > > > > Can you add comments to acpi_early_init() and acpi_subsystem_init() to > > clarify what ACPI features are enabled at each phase, and what > > dependency they have in the boot sequence? (The same goes to > > early_acpi_boot_init() and acpi_boot_init().) > > OK, update follows. > > --- > From: Rafael J. Wysocki > Subject: ACPI / init: Switch over platform to the ACPI mode later > > Commit 73f7d1ca3263 "ACPI / init: Run acpi_early_init() before > timekeeping_init()" moved the ACPI subsystem initialization, > including the ACPI mode enabling, to an earlier point in the > initialization sequence, to allow the timekeeping subsystem > use ACPI early. Unfortunately, that resulted in boot regressions > on some systems and the early ACPI initialization was moved toward > its original position in the kernel initialization code by commit > c4e1acbb35e4 "ACPI / init: Invoke early ACPI initialization later". > > However, that turns out to be insufficient, as boot is still broken > on the Tyan S8812 mainboard. > > To fix that issue, split the ACPI early initialization code into > two pieces so the majority of it still located in acpi_early_init() > and the part switching over the platform into the ACPI mode goes into > a new function, acpi_subsystem_init(), executed at the original early > ACPI initialization spot. > > That fixes the Tyan S8812 boot problem, but still allows ACPI > tables to be loaded earlier which is useful to the EFI code in > efi_enter_virtual_mode(). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=97141 > Fixes: 73f7d1ca3263 "ACPI / init: Run acpi_early_init() before > timekeeping_init()" > Reported-and-tested-by: Marius Tolzmann > Signed-off-by: Rafael J. Wysocki > Acked-by: Toshi Kani > --- > drivers/acpi/bus.c | 56 > +-- > include/linux/acpi.h |2 + > init/main.c |1 > 3 files changed, 44 insertions(+), 15 deletions(-) About bko#97141, I am still curious why the platform didn't set SCI_EN in that early stage. Thanks for Rafael's patch. Reviewed-by: Lee, Chun-Yi Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Unreliable hibernation on Lenovo x230 (regression)
On Fri, Apr 03, 2015 at 11:43:30PM +0200, Rafael J. Wysocki wrote: > On Friday, April 03, 2015 05:58:25 PM rhn wrote: > > On Thu, 2 Apr 2015 17:28:05 +0200 > > Pavel Machek wrote: > > > > > On Wed 2015-04-01 21:47:43, rhn wrote: > > > > Hello, > > > > > > > > Between kernel 3.16 and 3.17, a regression has been introduced where > > > > the first hibernation after regular shutdown always fails to resume. > > > > Subsequent hibernations succeed. > > > > > > > > The system is a Lenovo x230 with Intel i5, booting with EFI, with the > > > > hibernate partition located on a secondary SSD drive. Installed system > > > > is Fedora 20, hibernation and reboots were issued using the KDE > > > > shutdown dialog. > > > > > > > > I have tracked the problem to first appear in the commit > > > > e67ee10190e69332f929bdd6594a312363321a66Merge branches > > > > 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > > > The problem itself manifests in dmesg as follows (system was first > > > > restarted, then hibernated - this log is from the subsequent > > > resume): > > > > > > Ok, can you try to disable cpufreq and cpuidle, and then try if it > > > reproduces? > > > > > > At that point, this is the candidate: > > > > > > commit e67ee10190e69332f929bdd6594a312363321a66 > > > Merge: 21c806d 84c91b7 39c8bba 372ba8c > > > Author: Rafael J. Wysocki > > > Date: Mon Aug 11 23:19:48 2014 +0200 > > > > > > Merge branches 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > * pm-sleep: > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > ... > > > Alternatively, you can just try to revert > > > > > > commit 84c91b7ae07c62cf6dee7fde3277f4be21331f85 > > > Author: Lee, Chun-Yi > > > Date: Mon Aug 4 23:23:21 2014 +0800 > > > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > When the machine doesn't well handle the e820 persistent when > > > hibernate > > > resuming, then it may cause page fault when writing image to > > > snapshot > > > buffer: > > > > > > > > > ... > > > > > > Thanks, > > > Pavel > > > > I tried to disable CONFIG_CPU_IDLE and CONFIG_CPU_FREQ, however for some > > reason I could only disable CONFIG_CPU_FREQ. > > > > The bug persisted. > > > > Reverting the commit 84c91b7 on top of e67ee10 fixes the problem. > > > > I created a copy of the bug report here: > > https://bugzilla.kernel.org/show_bug.cgi?id=96111 > > Please check if 4.0-rc6 still has the problem and if reverting the commit in > question on top of it fixes the problem too. > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. I think just revert 84c91b7ae until Yinghai Lu's patches merged to v4.1. I will resend 84c91b7ae patch until Yinghai Lu's patches merged. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Unreliable hibernation on Lenovo x230 (regression)
Hi Rafael, On Sat, Apr 04, 2015 at 10:12:43AM +0200, rhn wrote: > On Fri, 03 Apr 2015 23:43:30 +0200 > "Rafael J. Wysocki" wrote: > > > On Friday, April 03, 2015 05:58:25 PM rhn wrote: > > > On Thu, 2 Apr 2015 17:28:05 +0200 > > > Pavel Machek wrote: > > > > > > > On Wed 2015-04-01 21:47:43, rhn wrote: > > > > > Hello, > > > > > > > > > > Between kernel 3.16 and 3.17, a regression has been introduced where > > > > > the first hibernation after regular shutdown always fails to resume. > > > > > Subsequent hibernations succeed. > > > > > > > > > > The system is a Lenovo x230 with Intel i5, booting with EFI, with the > > > > > hibernate partition located on a secondary SSD drive. Installed > > > > > system is Fedora 20, hibernation and reboots were issued using the > > > > > KDE shutdown dialog. > > > > > > > > > > I have tracked the problem to first appear in the commit > > > > > e67ee10190e69332f929bdd6594a312363321a66 Merge branches > > > > > 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > > > > > The problem itself manifests in dmesg as follows (system was first > > > > > restarted, then hibernated - this log is from the subsequent > > > > resume): > > > > > > > > Ok, can you try to disable cpufreq and cpuidle, and then try if it > > > > reproduces? > > > > > > > > At that point, this is the candidate: > > > > > > > > commit e67ee10190e69332f929bdd6594a312363321a66 > > > > Merge: 21c806d 84c91b7 39c8bba 372ba8c > > > > Author: Rafael J. Wysocki > > > > Date: Mon Aug 11 23:19:48 2014 +0200 > > > > > > > > Merge branches 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > > > * pm-sleep: > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > > > ... > > > > Alternatively, you can just try to revert > > > > > > > > commit 84c91b7ae07c62cf6dee7fde3277f4be21331f85 > > > > Author: Lee, Chun-Yi > > > > Date: Mon Aug 4 23:23:21 2014 +0800 > > > > > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > > > When the machine doesn't well handle the e820 persistent when > > > > hibernate > > > > resuming, then it may cause page fault when writing image to > > > > snapshot > > > > buffer: > > > > > > > > > > > > ... > > > > > > > > Thanks, > > > > > > > > Pavel > > > > > > I tried to disable CONFIG_CPU_IDLE and CONFIG_CPU_FREQ, however for some > > > reason I could only disable CONFIG_CPU_FREQ. > > > > > > The bug persisted. > > > > > > Reverting the commit 84c91b7 on top of e67ee10 fixes the problem. > > > > > > I created a copy of the bug report here: > > > https://bugzilla.kernel.org/show_bug.cgi?id=96111 > > > > Please check if 4.0-rc6 still has the problem and if reverting the commit in > > question on top of it fixes the problem too. > > > > > > I took the commit 8f778bbc542ddf8f6243b21d6aca087e709cabdc as the base for > further checking (I started building before I read your message). It's a > descendant of 4.0-rc6, so I hope it's not going to make a difference. > > Results: > 8f778bb : bad > 8f778bb + reverted 84c91b7 : good > 8f778bb + patch [1] : good Thanks for your dmesg on bko#96111. I checked and confirm there have the situation of setup_data reserved as E820_RESERVED_KERN. I will add comment on bugzilla. > > Thanks! > > [1]: > x86: Kill E820_RESERVED_KERN https://lkml.org/lkml/2015/3/4/434 as suggested > in joeyli's other email. I think just revert 84c91b7ae until Yinghai Lu's patches merged to v4.1. I will resend 84c91b7ae patch until Yinghai Lu's patches merged. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Unreliable hibernation on Lenovo x230 (regression)
On Thu, Apr 02, 2015 at 08:12:00PM +0200, rhn wrote: > On Fri, 3 Apr 2015 01:22:21 +0800 > joeyli wrote: > > > On Fri, Apr 03, 2015 at 12:50:54AM +0800, joeyli wrote: > > > Hi, > > > > > > On Thu, Apr 02, 2015 at 05:28:05PM +0200, Pavel Machek wrote: > > > > On Wed 2015-04-01 21:47:43, rhn wrote: > > > > > Hello, > > > > > > > > > > Between kernel 3.16 and 3.17, a regression has been introduced where > > > > > the first hibernation after regular shutdown always fails to resume. > > > > > Subsequent hibernations succeed. > > > > > > > > > > The system is a Lenovo x230 with Intel i5, booting with EFI, with the > > > > > hibernate partition located on a secondary SSD drive. Installed > > > > > system is Fedora 20, hibernation and reboots were issued using the > > > > > KDE shutdown dialog. > > > > > > > > > > I have tracked the problem to first appear in the commit > > > > > e67ee10190e69332f929bdd6594a312363321a66 Merge branches > > > > > 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > > > > > The problem itself manifests in dmesg as follows (system was first > > > > > restarted, then hibernated - this log is from the subsequent > > > > resume): > > > > > > > > Ok, can you try to disable cpufreq and cpuidle, and then try if it > > > > reproduces? > > > > > > > > At that point, this is the candidate: > > > > > > > > commit e67ee10190e69332f929bdd6594a312363321a66 > > > > Merge: 21c806d 84c91b7 39c8bba 372ba8c > > > > Author: Rafael J. Wysocki > > > > Date: Mon Aug 11 23:19:48 2014 +0200 > > > > > > > > Merge branches 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > > > > > * pm-sleep: > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > > > ... > > > > Alternatively, you can just try to revert > > > > > > > > commit 84c91b7ae07c62cf6dee7fde3277f4be21331f85 > > > > Author: Lee, Chun-Yi > > > > Date: Mon Aug 4 23:23:21 2014 +0800 > > > > > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > > > > > When the machine doesn't well handle the e820 persistent when > > > > hibernate > > > > resuming, then it may cause page fault when writing image to > > > > snapshot > > > > buffer: > > > > > > > > > > > > ... > > > > > > > > Thanks, > > > > > > > > Pavel > > > > > > Before revert 84c91b7ae patch, please check does there have log similar as > > > following in dmesg when hibernate resume fail? > > > > > > [ 24.349777] PM: 0xab9bc000 in e820 nosave region: [mem > > > 0xab9bc000-0xab9c2fff] > > > > > > The address may different, by you should see "e820 nosave region" log. > > > Otherwise > > > we got another problem. > > > > > > > Forgot to mention, please add "debug no_console_suspend=1 loglevel=9" to > > kernel > > parameter then try to reproduce issue and look at dmesg. > > > > > > Thanks a lot! > > Joey Lee > > Yes, it's present in dmesg when hibernate fails (default kernel params): > [3.138824] PM: 0x9d3d3000 in e820 nosave region: [mem > 0x9d3d3000-0x9d3d3fff] > OK, then the message means 0x9d3d3000 address used by image kernel but in e820 region of current boot. Need check does this e820 region used by setup_data so reserved as E820_RESERVED_KERN. Need your complete dmesg to verify the e820 table. If the above assumption is true, then Yinghai Lu's patchset could fix this problem: x86: Kill E820_RESERVED_KERN https://lkml.org/lkml/2015/3/4/434 The target kernel version to merge his patches is v4.1 > I probably didn't make it clear - the top dmesg in my original message was > from failed resume. > > Cheers, > rhn On the other hand, Could you please check you are using platform mode to turn off machine for hibernating? $ cat /sys/power/disk [platform] shutdown reboot suspend And, if possible, please file bug on bugzilla.kernel.org and give me the bug number. I prefer collect log and debugging history in bugzilla for further tracking. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Unreliable hibernation on Lenovo x230 (regression)
On Fri, Apr 03, 2015 at 12:50:54AM +0800, joeyli wrote: > Hi, > > On Thu, Apr 02, 2015 at 05:28:05PM +0200, Pavel Machek wrote: > > On Wed 2015-04-01 21:47:43, rhn wrote: > > > Hello, > > > > > > Between kernel 3.16 and 3.17, a regression has been introduced where the > > > first hibernation after regular shutdown always fails to resume. > > > Subsequent hibernations succeed. > > > > > > The system is a Lenovo x230 with Intel i5, booting with EFI, with the > > > hibernate partition located on a secondary SSD drive. Installed system is > > > Fedora 20, hibernation and reboots were issued using the KDE shutdown > > > dialog. > > > > > > I have tracked the problem to first appear in the commit > > > e67ee10190e69332f929bdd6594a312363321a66 Merge branches 'pm-sleep', > > > 'pm-cpufreq' and 'pm-cpuidle' > > > > > > The problem itself manifests in dmesg as follows (system was first > > > restarted, then hibernated - this log is from the subsequent > > resume): > > > > Ok, can you try to disable cpufreq and cpuidle, and then try if it > > reproduces? > > > > At that point, this is the candidate: > > > > commit e67ee10190e69332f929bdd6594a312363321a66 > > Merge: 21c806d 84c91b7 39c8bba 372ba8c > > Author: Rafael J. Wysocki > > Date: Mon Aug 11 23:19:48 2014 +0200 > > > > Merge branches 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > > > * pm-sleep: > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > ... > > Alternatively, you can just try to revert > > > > commit 84c91b7ae07c62cf6dee7fde3277f4be21331f85 > > Author: Lee, Chun-Yi > > Date: Mon Aug 4 23:23:21 2014 +0800 > > > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > > > When the machine doesn't well handle the e820 persistent when > > hibernate > > resuming, then it may cause page fault when writing image to > > snapshot > > buffer: > > > > > > ... > > > > Thanks, > > Pavel > > Before revert 84c91b7ae patch, please check does there have log similar as > following in dmesg when hibernate resume fail? > > [ 24.349777] PM: 0xab9bc000 in e820 nosave region: [mem > 0xab9bc000-0xab9c2fff] > > The address may different, by you should see "e820 nosave region" log. > Otherwise > we got another problem. > Forgot to mention, please add "debug no_console_suspend=1 loglevel=9" to kernel parameter then try to reproduce issue and look at dmesg. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Unreliable hibernation on Lenovo x230 (regression)
Hi, On Thu, Apr 02, 2015 at 05:28:05PM +0200, Pavel Machek wrote: > On Wed 2015-04-01 21:47:43, rhn wrote: > > Hello, > > > > Between kernel 3.16 and 3.17, a regression has been introduced where the > > first hibernation after regular shutdown always fails to resume. Subsequent > > hibernations succeed. > > > > The system is a Lenovo x230 with Intel i5, booting with EFI, with the > > hibernate partition located on a secondary SSD drive. Installed system is > > Fedora 20, hibernation and reboots were issued using the KDE shutdown > > dialog. > > > > I have tracked the problem to first appear in the commit > > e67ee10190e69332f929bdd6594a312363321a66Merge branches 'pm-sleep', > > 'pm-cpufreq' and 'pm-cpuidle' > > > > The problem itself manifests in dmesg as follows (system was first > > restarted, then hibernated - this log is from the subsequent > resume): > > Ok, can you try to disable cpufreq and cpuidle, and then try if it > reproduces? > > At that point, this is the candidate: > > commit e67ee10190e69332f929bdd6594a312363321a66 > Merge: 21c806d 84c91b7 39c8bba 372ba8c > Author: Rafael J. Wysocki > Date: Mon Aug 11 23:19:48 2014 +0200 > > Merge branches 'pm-sleep', 'pm-cpufreq' and 'pm-cpuidle' > > * pm-sleep: > PM / hibernate: avoid unsafe pages in e820 reserved regions > > ... > Alternatively, you can just try to revert > > commit 84c91b7ae07c62cf6dee7fde3277f4be21331f85 > Author: Lee, Chun-Yi > Date: Mon Aug 4 23:23:21 2014 +0800 > > PM / hibernate: avoid unsafe pages in e820 reserved regions > > When the machine doesn't well handle the e820 persistent when > hibernate > resuming, then it may cause page fault when writing image to > snapshot > buffer: > > > ... > > Thanks, > Pavel Before revert 84c91b7ae patch, please check does there have log similar as following in dmesg when hibernate resume fail? [ 24.349777] PM: 0xab9bc000 in e820 nosave region: [mem 0xab9bc000-0xab9c2fff] The address may different, by you should see "e820 nosave region" log. Otherwise we got another problem. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Trusted kernel patchset
On Mon, Mar 16, 2015 at 10:54:54PM +0100, Jiri Kosina wrote: > On Mon, 16 Mar 2015, Matthew Garrett wrote: > > > > - All suspend/resumes allow modifying the kernel. I can boot Linux > > > suspend, boot windows, modify the Linux restore image, boot Linux and > > > own the box. You would need to sign the resume image somehow I think or > > > just disable all suspend/resume > > > > I'm kind of torn on this - yes, there are deployment scenarios where > > hibernation can be used to circumvent the restrictions, but there are > > also scenarios where that can be avoided (eg, the bootloader verifies > > some state with respect to the hibernation image). > > [ adding Joey to CC ] > > Just for completness -- there is a way around this: > > https://lkml.org/lkml/2013/9/14/183 > > The series is not really the optimal one, as Alan Stern later figured out > that symmetric cryptography is strong enough to achieve the same goal, but > I am not sure whether Joey implemented that idea already. > > -- > Jiri Kosina > SUSE Labs About the symmetric key edition, I developed a prototype the codes on github are ugly and still need clear up. But it works for using HMAC to generate signature and verify hibernate image. There still has a situation need to solve: There doesn't have enough random entropy for the FIRST TIME boot to generate HMAC key in EFI stub. And, I need implement a hash function in EFI stub, at least SHA1, to mess entropies from difference sources (rdtsc, RdRand... not too many) for generating new HMAC key. An other idea is sending a random seed from runtime random pool to boot time to be one of the seed of next HMAC key. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
Hi, On Mon, Mar 09, 2015 at 02:10:37PM +0200, Boaz Harrosh wrote: > On 03/06/2015 01:09 AM, Andy Lutomirski wrote: > <> > > > > I will be shocked if a standard of this form ever appears. Modern > > systems *don't have e820*. The BIOSes that are using this type 12 > > hack are awful throwbacks. > > So far the systems we have, with DDR4 NvDIMM(s) (Actual chips arriving soon) > still have BIOS (On by default). The BIOS was yelled "Wolfe" for so long NOW > ;-) > Again these are working system in the field, who will switch them all to UEFI? > > How will the UEFI present them to the system? can you point me to the relevant > code? (Or did you mean BIOS but with a different communication path than > e820?) > Per my understand... With EFI Boot Stub, there have setup_e820() codes in arch/x86/boot/compressed/eboot.c that used to transfer EFI memmap to e820 entries. Currently doesn't have any EFI_MEMORY_TYPE reflects to NvDIMM that will map to e820_type. I wonder what kind of EFI_MEMORY_TYPE reported by UEFI BIOS on those "shipped NvDIMMs motherboards", like supermicro X9DRH-iF-NV. Then we may need add code to setup_e820() for mapping the efi memory type to e820 type 12 region. > > > > --Andy > > > > Thanks > Boaz > Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/8] x86: Kill E820_RESERVED_KERN
On Sat, Mar 07, 2015 at 05:59:14PM -0800, David Rientjes wrote: > On Sat, 7 Mar 2015, Yinghai Lu wrote: > > > Now we are using memblock to do early resource reserver/allocation > > instead of using e820 map directly, and setup_data is reserved in > > memblock early already. > > Also kexec generate setup_data and pass pointer to second kernel, > > so second kernel reserve setup_data by their own. > > (Now kexec-tools create SETUP_EFI and SETUP_E820_EXT). > > > > We can kill E820_RESERVED_KERN and not touch e820 map at all. > > > > That will fix bug in mark_nonsave_region that can not handle that > > case: E820_RAM and E820_RESERVED_KERN ranges are continuous and > > boundary is not page aligned. > > > > Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=913885 > > Is this the bug referenced in the commit message that is fixed? If so, > it's only a bug for resume, correct? I'm not sure if that's clear enough > just from the commit message, I was looking at this patch for an e820 > problem I'm currently facing on 3.3. Yinghai's patches fixed the e820 not page aligned issue that's one of the issues on bug reporter's machine. I found another issue of the BIOS that sometimes it doesn't really keep the e820 table unchanging for hibernate resuming, this BIOS issue causes the total available page number checking fail. I will file another openSUSE bug to separate those 2 issues. > > > Reported-by: "Lee, Chun-Yi" > > Tested-by: "Lee, Chun-Yi" > > Cc: "Lee, Chun-Yi" > > Signed-off-by: Yinghai Lu > > Cc: sta...@vger.kernel.org > > Hmm, although the bug is reported for a 3.12 kernel, I assume this is for > stable 3.10+? If so, it should apply fine with the exception of removing > e820_reserve_setup_data() from setup_arch() rather than > memblock_x86_reserve_range_setup_data(). Or is it for 3.2 as well and > needs to be completely rebased for that kernel? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
Hi Yinghai, On Wed, Mar 04, 2015 at 10:12:58AM -0800, Yinghai Lu wrote: > On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina wrote: > > > > > Also this 15-patch series needs to be separated into two patchsets. The > > whole series is not appropriate for -rc3, but this particular one at least > > is a regression fix that has to go in. > > The first 4 should go v4.0. > > could leave others to v4.1 > > Thanks > > Yinghai After 84c91b7ae merged to v3.17 kernel, hibernate code checks the e280 regions should not be changed when doing hibernate resume. Without your patch 8, the hibernate resume checking will randomly fail on the machines that reserved setup_data in e820 regions. Could you please consider to put "[PATCH v2 08/15] x86: Kill E820_RESERVED_KERN" to v4.0 kernel? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region
On Fri, Jan 30, 2015 at 10:46:48PM +0800, joeyli wrote: > On Fri, Jan 30, 2015 at 12:30:00AM -0800, Yinghai Lu wrote: > > On Thu, Jan 29, 2015 at 7:58 PM, Lee, Chun-Yi > > wrote: > > > The reserve setup_data action break usable regions to not align to > > > page size. As following case: > > > > > > BIOS-e820: [mem 0x00088000-0x000b] reserved > > > BIOS-e820: [mem 0x0010-0x94ca] usable > > > ... > > > e820: update [mem 0x93c5f018-0x93c6f057] usable ==> usable /* > > > reserve setup_data */ > > > e820: update [mem 0x93c4f018-0x93c5e057] usable ==> usable /* > > > reserve setup_data */ > > > ... > > > reserve setup_data: [mem0x00088000-0x000b] reserved > > > reserve setup_data: [mem0x0010-0x93c4f017] usable /* > > > not align */ > > > reserve setup_data: [mem0x93c4f018-0x93c5e057] usable /* > > > not align */ > > > reserve setup_data: [mem0x93c5e058-0x93c5f017] usable /* > > > not align */ > > > reserve setup_data: [mem0x93c5f018-0x93c6f057] usable /* > > > not align */ > > > reserve setup_data: [mem0x93c6f058-0x94ca] usable > > > > > > The codes in e820_mark_nosave_regions() check pfn of each e820 > > > entry to find out the hole between two entries then register it to > > > nosave region. This logic misjudges the non-align but continuous usable > > > region, then register one page in usable region to nosave. As following: > > > > > > PM: Registered nosave memory: [mem 0x000c-0x000f] > > > PM: Registered nosave memory: [mem 0x93c4f000-0x93c4] /* misjudgment > > > */ > > > PM: Registered nosave memory: [mem 0x93c5e000-0x93c5efff] /* misjudgment > > > */ > > > PM: Registered nosave memory: [mem 0x93c5f000-0x93c5] /* misjudgment > > > */ > > > PM: Registered nosave memory: [mem 0x93c6f000-0x93c6] /* misjudgment > > > */ > > > PM: Registered nosave memory: [mem 0x94cb-0x960a] > > > > > > The issue causes some usable pages didn't collect to hibernate snapshot > > > image. > > > And, it also misjudges a usable page in nosave regions then hibernate > > > resuming > > > process interrupted by the unsafe pages checking: > > > https://bugzilla.opensuse.org/show_bug.cgi?id=913885 > > > > > > This patch changed the code in e820_mark_nosave_regions() to check the > > > address instead of pfn to avoid above issue. > > > > [+cc: Ying Huang] > > > > would like to suggest use attached instead: > > > > Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN > > > > Now we are using memblock to do early resource allocation instead of using > > e820 map directly, and setup_data is reserved in memblock early. > > Also kexec pass setup_data pointer to second kernel, so second kernel > > will reserve setup_data by their own. > > > > So we can kill E820_RESERVED_KERN and not touch e820 map at all. > > > > That will fix bug in mark_nonsave_region that can not handle that > > case: E820_RAM and E820_RESERVED_KERN continuous and boundary is > > not page aligned. > > > > Not sure about why tboot need to use this... > > Yes, that's good to totally remove the E820_RESERVED_KERN because we already > use memblock to reserve setup_data ranges. > > > Thanks > Joey Lee This patch works on my Gateway EG50_HW notebook to fix issue in nosave regions. Tested-by: Lee, Chun-Yi Thanks a lot! Joey Lee > > > Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN > > > > Now we are using memblock to do early resource allocation instead of using > > e820 map directly, and setup_data is reserved in memblock early. > > Also kexec pass setup_data pointer to second kernel, so second kernel > > will reserve setup_data by their own. > > > > So we can kill E820_RESERVED_KERN and not touch e820 map at all. > > > > That will fix bug in mark_nonsave_region that can not handle that > > case: E820_RAM and E820_RESERVED_KERN continuous and boundary is > > not page aligned. > > > > Not sure about why tboot need to use this... > > > > Signed-off-by: Yinghai Lu > > > > --- > > arch/x86/include/uapi/asm/e820.h |9 - > > arch/x86/kernel/e820.c |6 ++ > > arch/x86/kernel/setup.c | 26
Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region
On Fri, Jan 30, 2015 at 12:30:00AM -0800, Yinghai Lu wrote:
> On Thu, Jan 29, 2015 at 7:58 PM, Lee, Chun-Yi wrote:
> > The reserve setup_data action break usable regions to not align to
> > page size. As following case:
> >
> > BIOS-e820: [mem 0x00088000-0x000b] reserved
> > BIOS-e820: [mem 0x0010-0x94ca] usable
> > ...
> > e820: update [mem 0x93c5f018-0x93c6f057] usable ==> usable /* reserve
> > setup_data */
> > e820: update [mem 0x93c4f018-0x93c5e057] usable ==> usable /* reserve
> > setup_data */
> > ...
> > reserve setup_data: [mem0x00088000-0x000b] reserved
> > reserve setup_data: [mem0x0010-0x93c4f017] usable /*
> > not align */
> > reserve setup_data: [mem0x93c4f018-0x93c5e057] usable /*
> > not align */
> > reserve setup_data: [mem0x93c5e058-0x93c5f017] usable /*
> > not align */
> > reserve setup_data: [mem0x93c5f018-0x93c6f057] usable /*
> > not align */
> > reserve setup_data: [mem0x93c6f058-0x94ca] usable
> >
> > The codes in e820_mark_nosave_regions() check pfn of each e820
> > entry to find out the hole between two entries then register it to
> > nosave region. This logic misjudges the non-align but continuous usable
> > region, then register one page in usable region to nosave. As following:
> >
> > PM: Registered nosave memory: [mem 0x000c-0x000f]
> > PM: Registered nosave memory: [mem 0x93c4f000-0x93c4] /* misjudgment */
> > PM: Registered nosave memory: [mem 0x93c5e000-0x93c5efff] /* misjudgment */
> > PM: Registered nosave memory: [mem 0x93c5f000-0x93c5] /* misjudgment */
> > PM: Registered nosave memory: [mem 0x93c6f000-0x93c6] /* misjudgment */
> > PM: Registered nosave memory: [mem 0x94cb-0x960a]
> >
> > The issue causes some usable pages didn't collect to hibernate snapshot
> > image.
> > And, it also misjudges a usable page in nosave regions then hibernate
> > resuming
> > process interrupted by the unsafe pages checking:
> > https://bugzilla.opensuse.org/show_bug.cgi?id=913885
> >
> > This patch changed the code in e820_mark_nosave_regions() to check the
> > address instead of pfn to avoid above issue.
>
> [+cc: Ying Huang]
>
> would like to suggest use attached instead:
>
> Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN
>
> Now we are using memblock to do early resource allocation instead of using
> e820 map directly, and setup_data is reserved in memblock early.
> Also kexec pass setup_data pointer to second kernel, so second kernel
> will reserve setup_data by their own.
>
> So we can kill E820_RESERVED_KERN and not touch e820 map at all.
>
> That will fix bug in mark_nonsave_region that can not handle that
> case: E820_RAM and E820_RESERVED_KERN continuous and boundary is
> not page aligned.
>
> Not sure about why tboot need to use this...
Yes, that's good to totally remove the E820_RESERVED_KERN because we already
use memblock to reserve setup_data ranges.
Thanks
Joey Lee
> Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN
>
> Now we are using memblock to do early resource allocation instead of using
> e820 map directly, and setup_data is reserved in memblock early.
> Also kexec pass setup_data pointer to second kernel, so second kernel
> will reserve setup_data by their own.
>
> So we can kill E820_RESERVED_KERN and not touch e820 map at all.
>
> That will fix bug in mark_nonsave_region that can not handle that
> case: E820_RAM and E820_RESERVED_KERN continuous and boundary is
> not page aligned.
>
> Not sure about why tboot need to use this...
>
> Signed-off-by: Yinghai Lu
>
> ---
> arch/x86/include/uapi/asm/e820.h |9 -
> arch/x86/kernel/e820.c |6 ++
> arch/x86/kernel/setup.c | 26 --
> arch/x86/kernel/tboot.c |3 +--
> arch/x86/mm/init_64.c| 11 ---
> 5 files changed, 7 insertions(+), 48 deletions(-)
>
> Index: linux-2.6/arch/x86/include/uapi/asm/e820.h
> ===
> --- linux-2.6.orig/arch/x86/include/uapi/asm/e820.h
> +++ linux-2.6/arch/x86/include/uapi/asm/e820.h
> @@ -33,15 +33,6 @@
> #define E820_NVS 4
> #define E820_UNUSABLE5
>
> -
> -/*
> - * reserved RAM used by kernel itself
> - * if CONFIG_INTEL_TXT is enabled, memory of this type will be
> - * included in the S3 integrity calculation and so should not include
> - * any memory that BIOS might alter over the S3 transition
> - */
> -#define E820_RESERVED_KERN128
> -
> #ifndef __ASSEMBLY__
> #include
> struct e820entry {
> Index: linux-2.6/arch/x86/kernel/e820.c
> ===
> --- linux-2.6.orig/arch/x86/kernel/e820.c
> +++ linux-2.6/arch/x86/kernel/e820.c
> @@ -134,7 +134,6 @@ static void __init e820_print_type(u32 t
> {
> switch (type) {
>
Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region
Hi,
On Thu, Jan 29, 2015 at 11:35:49PM -0800, Yinghai Lu wrote:
> On Thu, Jan 29, 2015 at 7:58 PM, Lee, Chun-Yi wrote:
> >
> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index 49f8864..6eae021 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -687,15 +687,16 @@ void __init parse_e820_ext(u64 phys_addr, u32
> > data_len)
> > void __init e820_mark_nosave_regions(unsigned long limit_pfn)
> > {
> > int i;
> > - unsigned long pfn = 0;
> > + unsigned long pfn = 0, pfnaddr = 0;
> >
> > for (i = 0; i < e820.nr_map; i++) {
> > struct e820entry *ei = &e820.map[i];
> >
> > - if (pfn < PFN_UP(ei->addr))
> > + if (pfnaddr < ei->addr)
> > register_nosave_region(pfn, PFN_UP(ei->addr));
> >
> > - pfn = PFN_DOWN(ei->addr + ei->size);
> > + pfnaddr = ei->addr + ei->size;
> > + pfn = PFN_DOWN(pfnaddr);
> > if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
> > register_nosave_region(PFN_UP(ei->addr), pfn);
> >
> Those changes may not fix the problem.
>
> those E820_RESERVED_KERN and E820_RAM should be continuous.
Yes, they are continuous but not aligned.
>
> So you need to find the real end for those continuous ranges.
>
Sorry for I didn't capture the point for need to find the real end
because those misjudgments are happened in the boundary between
E820_RESERVED_KERN and E820_RAM.
> Thanks
>
> Yinghai
e.g.
reserve setup_data: [mem 0x0010-0x93c4f017] usable
reserve setup_data: [mem 0x93c4f018-0x93c5e057] usable
pfn = PFN_DOWN(0x93c4f017+1) = 0x93c4f
PFN_UP(0x93c4f018) = (0x93c4f018 + 0x1000 - 1 >> PAGE_SHIFT) = 0x93C50
The above case match with "if (pfn < PFN_UP(ei->addr))" logic, it causes
e820_mark_nosave_regions() add one usable page to nosave region:
PM: Registered nosave memory: [mem 0x93c4f000-0x93c4]
Comparing the pfn of regions is not enough so my patch compares the address
instead of pfn. It works to me to avoid those one page area show in nosave
region.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ACPI/osl: speedup grace period in acpi_os_map_cleanup
Hi,
On Sun, Nov 09, 2014 at 01:53:37PM +0400, Konstantin Khlebnikov wrote:
> ACPI maintains cache of ioremap regions to speed up operations and
> access to them from irq context where ioremap() calls aren't allowed.
> This code abuses synchronize_rcu() on unmap path for synchronization
> with fast-path in acpi_os_read/write_memory which uses this cache.
>
> Since v3.10 CPUs are allowed to enter idle state even if they have RCU
> callbacks queued, see commit c0f4dfd4f90f1667d234d21f15153ea09a2eaa66
> ("rcu: Make RCU_FAST_NO_HZ take advantage of numbered callbacks").
> That change caused problems with nvidia proprietary driver which calls
> acpi_os_map/unmap_generic_address several times during initialization.
> Each unmap calls synchronize_rcu and adds significant delay. Totally
> initialization is slowed for a couple of seconds and that is enough to
> trigger timeout in hardware, gpu decides to "fell off the bus". Widely
> spread workaround is reducing "rcu_idle_gp_delay" from 4 to 1 jiffy.
>
> This patch replaces synchronize_rcu() with synchronize_rcu_expedited()
> which is much faster.
>
> Signed-off-by: Konstantin Khlebnikov
> Reported-and-tested-by: Alexander Monakov
> Cc: Tom Boshoven
> Link:
> https://devtalk.nvidia.com/default/topic/567297/linux/linux-3-10-driver-crash/
Please feel free to add Tested-by:
Tested-by: Lee, Chun-Yi
This patch fixed the performance issue on VMWare workstation 10.0.2 with the
virtual machine that has more than 2 CPU and 4G memory:
Mware workstation 10.0.2
BIOS DMI: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference
Platform, BIOS 6.00 07/31/2013
vCPU = 8
vMEM = 4G
mem.hotplug=TRUE
physical CPUs on host machine: Intel(R) Xeon(R) CPU X5670 @ 2.93GHz * 24
I tested this patch with v3.12, v3.17, v3.18-rc4 mainline kernel, those kernel
call can produced issue and all got speedup when acpi initial. I suggest this
patch go to stable kernel patch fixing.
Thanks a lot!
Joey Lee
> ---
> drivers/acpi/osl.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9964f70..217713c 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -436,7 +436,7 @@ static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
> static void acpi_os_map_cleanup(struct acpi_ioremap *map)
> {
> if (!map->refcount) {
> - synchronize_rcu();
> + synchronize_rcu_expedited();
> acpi_unmap(map->phys, map->virt);
> kfree(map);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Hibernate: Do not assume the first e820 area to be RAM
Hi Yinghai,
Thanks for your review, first!
On Tue, Sep 09, 2014 at 11:08:45PM -0700, Yinghai Lu wrote:
> On Mon, Sep 8, 2014 at 3:52 PM, Rafael J. Wysocki wrote:
> > On Monday, August 11, 2014 06:50:52 PM Lee, Chun-Yi wrote:
> >> In arch/x86/kernel/setup.c::trim_bios_range(), the codes introduced
> >> by 1b5576e6 (base on d8a9e6a5), it updates the first 4Kb of memory
> >> to be E820_RESERVED region. That's because it's a BIOS owned area
> >> but generally not listed in the E820 table:
> >>
> >> [0.00] e820: BIOS-provided physical RAM map:
> >> [0.00] BIOS-e820: [mem 0x-0x00096fff]
> >> usable
> >> [0.00] BIOS-e820: [mem 0x00097000-0x00097fff]
> >> reserved
> >> ...
> >> [0.00] e820: update [mem 0x-0x0fff] usable ==> reserved
> >> [0.00] e820: remove [mem 0x000a-0x000f] usable
> >>
> >> But the region of first 4Kb didn't register to nosave memory:
> >>
> >> [0.00] PM: Registered nosave memory: [mem 0x00097000-0x00097fff]
> >> [0.00] PM: Registered nosave memory: [mem 0x000a-0x000f]
> >>
> >> The codes in e820_mark_nosave_regions() assumes the first e820 area to be
> >> RAM, so it causes the first 4Kb E820_RESERVED region ignored when register
> >> to nosave. This patch removed assumption of the first e820 area.
> >>
> >> Cc: "Rafael J. Wysocki"
> >> Cc: Len Brown
> >> Cc: Pavel Machek
> >> Cc: Thomas Gleixner
> >> Cc: Ingo Molnar
> >> Cc: "H. Peter Anvin"
> >> Signed-off-by: Lee, Chun-Yi
> >
> > Thomas, Ingo, Peter, any objections here?
> >
> > If not, do you want to handle it or do you want me to do that?
>
> Did it address any regression?
>
I found this situation when comparing the e820 region with nosave memory
address.
But, I don't know any real machine which has bug report against this.
> >
> >> ---
> >> arch/x86/kernel/e820.c | 7 +++
> >> 1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> >> index 988c00a..d595240 100644
> >> --- a/arch/x86/kernel/e820.c
> >> +++ b/arch/x86/kernel/e820.c
> >> @@ -682,18 +682,17 @@ void __init parse_e820_ext(u64 phys_addr, u32
> >> data_len)
> >> * hibernation (32 bit) or software suspend and suspend to RAM (64 bit).
> >> *
> >> * This function requires the e820 map to be sorted and without any
> >> - * overlapping entries and assumes the first e820 area to be RAM.
> >> + * overlapping entries.
> >> */
> >> void __init e820_mark_nosave_regions(unsigned long limit_pfn)
> >> {
> >> int i;
> >> unsigned long pfn;
> >>
> >> - pfn = PFN_DOWN(e820.map[0].addr + e820.map[0].size);
> >> - for (i = 1; i < e820.nr_map; i++) {
> >> + for (i = 0; i < e820.nr_map; i++) {
> >> struct e820entry *ei = &e820.map[i];
> >>
> >> - if (pfn < PFN_UP(ei->addr))
> >> + if (i > 0 && pfn < PFN_UP(ei->addr))
> >> register_nosave_region(pfn, PFN_UP(ei->addr));
>
> could avoid the i > 0 checking.
>
> >>
> >> pfn = PFN_DOWN(ei->addr + ei->size);
> >>
> >
>
> following would be better ?
>
> @@ -682,15 +682,14 @@ void __init parse_e820_ext(u64 phys_addr, u32 data_len)
> * hibernation (32 bit) or software suspend and suspend to RAM (64 bit).
> *
> * This function requires the e820 map to be sorted and without any
> - * overlapping entries and assumes the first e820 area to be RAM.
> + * overlapping entries.
> */
> void __init e820_mark_nosave_regions(unsigned long limit_pfn)
> {
> int i;
> -unsigned long pfn;
> +unsigned long pfn = 0;
>
> -pfn = PFN_DOWN(e820.map[0].addr + e820.map[0].size);
> -for (i = 1; i < e820.nr_map; i++) {
> +for (i = 0; i < e820.nr_map; i++) {
> struct e820entry *ei = &e820.map[i];
>
> if (pfn < PFN_UP(ei->addr))
Yes, thanks for your suggestion, your change can avoid the i > 0 checking.
I will send v2 patch to add your improvement.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Hibernate: save e820 table to snapshot header for comparison
Hi Pavel,
Thanks for your review, first!
On Tue, Aug 12, 2014 at 11:41:36AM +0200, Pavel Machek wrote:
> Hi!
>
> > [7.374714] e820: Check memory region: [mem
> > 0x0001-0x00187fff] usable
> > [7.378041] PM: Image mismatch: memory map changed
> > [7.381314] PM: Read 2398272 kbytes in 0.27 seconds (8882.48 MB/s)
> > [7.385476] PM: Error -1 resuming
> > [7.388730] PM: Failed to load hibernation image, recovering.
> > [7.688989] PM: Basic memory bitmaps freed
>
> Nice!
>
> > +int save_mem_chk_map(struct mementry *mem_chk_map)
>
> I'd prefer _chk_ -> _check_
>
OK, I will change the naming.
> > +{
> > + int i;
> > +
> > + for (i = 0; i < e820.nr_map; i++) {
> > + struct e820entry *ei = &e820.map[i];
> > +
> > + if (i > MEMCHKMAX)
> > + break;
>
> MEMCHKMAX -> MEM_CHECK_MAX?
>
> What happens when there are more entries?
>
Yes, the number 128 is from E820MAX, the legacy E820 BIOS limitation. My
original thinking is keep
the size of swsusp_info in 4K then don't need change the swap accessing codes
in swsusp_read() and
swsusp_write(). On the other hand, I have no machine that provides E820 entries
more then 128.
hm but, yes, this is an magic number causes we didn't check the entries
bigger than 128.
I will modify this patch to keep the pages of E820 table behind the swsusp_info
header.
> > +bool check_mem_map(int mem_chk_entries, struct mementry *mem_chk_map)
> > +{
> > + int i;
> > + bool ret = true;
> > +
> > + if (mem_chk_entries != e820.nr_map) {
> > + pr_err("PM: memory check entry number %d:%d\n",
> > + mem_chk_entries, e820.nr_map);
> > + ret = false;
> > + goto Print_map;
> > + }
>
> I'd change name to something like mem_map_matches() or mem_map_ok(),
> so that it is clear what true/false means.
>
> Can you reduce ammount of gotos?
>
OK, I will change naming and reduce goto.
> > + for (i = 0; i < mem_chk_entries; i++) {
> > + struct e820entry *ei = &e820.map[i];
> > +
> > + if (i > MEMCHKMAX)
> > + break;
> > +
> > + /* check regions not E820_RAM or E820_RESERVED_KERN */
> > + if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN) {
> > + if (mem_chk_map[i].addr != ei->addr ||
> > + mem_chk_map[i].size != ei->size ||
> > + mem_chk_map[i].type != ei->type) {
> > + ret = false;
> > + goto Print_map;
> > + }
> > + }
>
> Why don't you check RAM and RESERVED_KERN, too? If those changed, we
> don't want to resume, either, right?
>
The RESERVED_KERN is boot_params.hdr.setup_data, reserved by kernel for
PCI RomImage and extend e820 entries. The E820_RAM and E820_RESERVED_KERN
are saved in hibernate snapshot image and will restore to appropriate page
frame. If hibernate can not restore data to the original page frame, then
whole hibernate resume process will be stop. So I think don't need check
RAM and RESERVED_KERN because image saved it.
> (Plus, you only check ei->type; you should check mem_chk_map[].type,
> too AFAICT).
>
Yes, thanks for your suggestion, I will also check type of mem_chk_map.
> Thanks,
> Pavel
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Hibernate: check unsafe page should not in e820 reserved region
On Thu, Aug 07, 2014 at 11:05:33PM +0200, Pavel Machek wrote: > On Thu 2014-08-07 18:17:34, joeyli wrote: > > On Thu, Aug 07, 2014 at 11:39:57AM +0200, Pavel Machek wrote: > > > > > Actually, if you are doing such a check... it makes sense to check for > > > > > _all_ the regions, nosave or not. If e820 map changed at all, it is > > > > > not safe to resume. > > > > > > > > > > Pavel > > > > > > > > Currently nosave region only called register by e820 code, so > > > > hibernate's nosave region included e820 > > > > reserved, ACPI data and ACPI NVS region. > > > > > > > > I thought hashing the start/end pfn of above regions is enough. > > > > > > If ammount of memory changed, for example, it is unsafe to > > > resume. So if you are doing the check, anyway, please hash > > > whole e820 table. > > > > There already have num_physpages in header for check the total physical > > page number. > > Good, but if ammount of memory stayed the same, but offsets > changed (for example), resume is unsafe, too. I agreed > > When I wrote that num_physpages check, I should have checked > whole e820 table, instead. (If anything at all changed there, > "new" kernel is running with wrong e820 info). > > You seem to be in great position to fix that mistake now... > > Pavel Hashing e820 is fine, but it can not provide detail information to user/developer when issue happened. We only know the e820 table changed but not more information for bug tracking, not too many shipping machine have serial console. After checked the space of swsusp_info, I hope can store whole e820 table to snapshot header for compare the range. The maximum space consumed 20 * E820MAX = 20 * 128 = 2560 bytes. Currrent swsusp_info is used 430 bytes in one page, it's enough to us for keep whole e820 table. And, I thought don't need compare the range of E820_RAM and E820_RESERVED_KERN type because they are using by OS and stored in snapshot image. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Hibernate: check unsafe page should not in e820 reserved region
On Thu, Aug 07, 2014 at 11:39:57AM +0200, Pavel Machek wrote: > > > Actually, if you are doing such a check... it makes sense to check for > > > _all_ the regions, nosave or not. If e820 map changed at all, it is > > > not safe to resume. > > > Pavel > > > > Currently nosave region only called register by e820 code, so hibernate's > > nosave region included e820 > > reserved, ACPI data and ACPI NVS region. > > > > I thought hashing the start/end pfn of above regions is enough. > > If ammount of memory changed, for example, it is unsafe to > resume. So if you are doing the check, anyway, please hash > whole e820 table. > Pavel There already have num_physpages in header for check the total physical page number. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Hibernate: check unsafe page should not in e820 reserved region
On Thu, Aug 07, 2014 at 10:00:25AM +0200, Pavel Machek wrote: > Hi! > > > > When the machine doesn't well handle the e820 persistent when hibernate > > > resuming, then it may causes page fault when writing image to snapshot > > > buffer: > > > > > > [ 17.929495] BUG: unable to handle kernel paging request at > > > 880069d4f000 > > > [ 17.933469] IP: [] load_image_lzo+0x810/0xe40 > > > [ 17.933469] PGD 2194067 PUD 77067 PMD 2197067 PTE 0 > > > [ 17.933469] Oops: 0002 [#1] SMP [...] > > > [8.933534] PM: Failed to load hibernation image, recovering. > > > > > > v2: > > > + removed empty check of nosave_regions list. > > > + fixed the typo of "region" in code for error message and patch comment. > > > > > > Cc: "Rafael J. Wysocki" > > > Cc: Len Brown > > > Cc: Takashi Iwai > > > Acked-by: Pavel Machek > > > Signed-off-by: Lee, Chun-Yi > > > > I discussed with Vojtech Pavlik for this patch, he raised a situation is: > > > > Maybe e820 changed but image kernel original pages do not fall into new > > e820 region. > > Then the hibernate will recovery success, but later kernel drivers may got > > problem > > when accessing memory. > > > > My idea is hashing the start/end pfn of each nosave region sequentially, > > put this > > nosave region digest to hibernate header then compare e820 digest in > > check_header() > > when hibernate resuming. > > > > I am developing patch, then we don't need check unsafe page should not in > > unsave(e820) > > regions. > > Actually, if you are doing such a check... it makes sense to check for > _all_ the regions, nosave or not. If e820 map changed at all, it is > not safe to resume. > Pavel Currently nosave region only called register by e820 code, so hibernate's nosave region included e820 reserved, ACPI data and ACPI NVS region. I thought hashing the start/end pfn of above regions is enough. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Hibernate: check unsafe page should not in e820 reserved region
Hi experts,
On Mon, Aug 04, 2014 at 11:23:21PM +0800, Lee, Chun-Yi wrote:
> When the machine doesn't well handle the e820 persistent when hibernate
> resuming, then it may causes page fault when writing image to snapshot
> buffer:
>
> [ 17.929495] BUG: unable to handle kernel paging request at 880069d4f000
> [ 17.933469] IP: [] load_image_lzo+0x810/0xe40
> [ 17.933469] PGD 2194067 PUD 77067 PMD 2197067 PTE 0
> [ 17.933469] Oops: 0002 [#1] SMP
> ...
>
> The 880069d4f000 page is in e820 reserved region of resume boot
> kernel:
>
> [0.00] BIOS-e820: [mem 0x69d4f000-0x69e12fff] reserved
> ...
> [0.00] PM: Registered nosave memory: [mem 0x69d4f000-0x69e12fff]
>
> So snapshot.c mark the pfn to forbidden pages map. But, this
> page is also in the memory bitmap in snapshot image because it's an
> original page used by image kernel, so it will also mark as an
> unsafe(free) page in prepare_image().
>
> That means the page in e820 when resuming mark as "forbidden" and
> "free", it causes get_buffer() treat it as an allocated unsafe page.
> Then snapshot_write_next() return this page to load_image, load_image
> writing content to this address, but this page didn't really allocated
> . So, we got page fault.
>
> Although the root cause is from BIOS, I think aggressive check and
> significant message in kernel will better then a page fault for
> issue tracking, especially when serial console unavailable.
>
> This patch adds code in mark_unsafe_pages() for check does free pages in
> nosave region. If so, then it print message and return fault to stop whole
> S4 resume process:
>
> [8.166004] PM: Image loading progress: 0%
> [8.658717] PM: 0x6796c000 in e820 nosave region: [mem
> 0x6796c000-0x6796cfff]
> [8.918737] PM: Read 2511940 kbytes in 1.04 seconds (2415.32 MB/s)
> [8.926633] PM: Error -14 resuming
> [8.933534] PM: Failed to load hibernation image, recovering.
>
> v2:
> + removed empty check of nosave_regions list.
> + fixed the typo of "region" in code for error message and patch comment.
>
> Cc: "Rafael J. Wysocki"
> Cc: Len Brown
> Cc: Takashi Iwai
> Acked-by: Pavel Machek
> Signed-off-by: Lee, Chun-Yi
I discussed with Vojtech Pavlik for this patch, he raised a situation is:
Maybe e820 changed but image kernel original pages do not fall into new e820
region.
Then the hibernate will recovery success, but later kernel drivers may got
problem
when accessing memory.
My idea is hashing the start/end pfn of each nosave region sequentially, put
this
nosave region digest to hibernate header then compare e820 digest in
check_header()
when hibernate resuming.
I am developing patch, then we don't need check unsafe page should not in
unsave(e820)
regions.
Thanks a lot!
Joey Lee
> ---
> kernel/power/snapshot.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 4fc5c32..c4b8093 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -954,6 +954,25 @@ static void mark_nosave_pages(struct memory_bitmap *bm)
> }
> }
>
> +static bool is_nosave_page(unsigned long pfn)
> +{
> + struct nosave_region *region;
> +
> + list_for_each_entry(region, &nosave_regions, list) {
> + if (pfn >= region->start_pfn && pfn < region->end_pfn) {
> + pr_err("PM: %#010llx in e820 nosave region: "
> +"[mem %#010llx-%#010llx]\n",
> +(unsigned long long) pfn << PAGE_SHIFT,
> +(unsigned long long) region->start_pfn <<
> PAGE_SHIFT,
> +((unsigned long long) region->end_pfn <<
> PAGE_SHIFT)
> + - 1);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> /**
> * create_basic_memory_bitmaps - create bitmaps needed for marking page
> * frames that should not be saved and free page frames. The pointers
> @@ -2015,7 +2034,7 @@ static int mark_unsafe_pages(struct memory_bitmap *bm)
> do {
> pfn = memory_bm_next_pfn(bm);
> if (likely(pfn != BM_END_OF_MAP)) {
> - if (likely(pfn_valid(pfn)))
> + if (likely(pfn_valid(pfn)) && !is_nosave_page(pfn))
> swsusp_set_page_free(pfn_to_page(pfn));
> else
> return -EFAULT;
> --
> 1.8.4.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Hibernate: check unsafe page should not in e820 reserved region
Hi Pavel,
Thanks for your review!
On Mon, Aug 04, 2014 at 03:08:11PM +0200, Pavel Machek wrote:
> > > +
> > > + list_for_each_entry(region, &nosave_regions, list) {
> > > + if (pfn >= region->start_pfn && pfn < region->end_pfn) {
> > > + pr_err("PM: %#010llx in e820 nosave regsion: "
>
> And while looking at it, this should probably be spelled "region".
>
> ...plus maybe past part of changelog here into the comment?
>
Oh, sorry for my typo! I will correct it and comment in next version.
> Otherwise looks good to me,
>
> Acked-by: Pavel Machek
> Pavel
Regards
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Hibernate: check unsafe page should not in e820 reserved region
Hi Takashi,
First, thanks for your review!
On Mon, Aug 04, 2014 at 02:42:32PM +0200, Takashi Iwai wrote:
> At Mon, 4 Aug 2014 18:33:16 +0800,
> Lee, Chun-Yi wrote:
> >
> > When the machine doesn't well handle the e820 persistent when hibernate
> > resuming, then it may causes page fault when writing image to snapshot
> > buffer:
> >
> > [ 17.929495] BUG: unable to handle kernel paging request at
> > 880069d4f000
> > [ 17.933469] IP: [] load_image_lzo+0x810/0xe40
> > [ 17.933469] PGD 2194067 PUD 77067 PMD 2197067 PTE 0
> > [ 17.933469] Oops: 0002 [#1] SMP
> > ...
> >
> > The 880069d4f000 page is in e820 reserved region of resume boot
> > kernel:
> >
> > [0.00] BIOS-e820: [mem 0x69d4f000-0x69e12fff]
> > reserved
> > ...
> > [0.00] PM: Registered nosave memory: [mem 0x69d4f000-0x69e12fff]
> >
> > So snapshot.c mark the pfn to forbidden pages map. But, this
> > page is also in the memory bitmap in snapshot image because it's an
> > original page used by image kernel, so it will also mark as an
> > unsafe(free) page in prepare_image().
> >
> > That means the page in e820 when resuming mark as "forbidden" and
> > "free", it causes get_buffer() treat it as an allocated unsafe page.
> > Then snapshot_write_next() return this page to load_image, load_image
> > writing content to this address, but this page didn't really allocated
> > . So, we got page fault.
> >
> > Although the root cause is from BIOS, I think aggressive check and
> > significant message in kernel will better then a page fault for
> > issue tracking, especially when serial console unavailable.
> >
> > This patch adds code in mark_unsafe_pages() for check does free pages in
> > nosave region. If so, then it print message and return fault to stop whole
> > S4 resume process:
> >
> > [8.166004] PM: Image loading progress: 0%
> > [8.658717] PM: 0x6796c000 in e820 nosave regsion: [mem
> > 0x6796c000-0x6796cfff]
> > [8.918737] PM: Read 2511940 kbytes in 1.04 seconds (2415.32 MB/s)
> > [8.926633] PM: Error -14 resuming
> > [8.933534] PM: Failed to load hibernation image, recovering.
> >
> > Cc: "Rafael J. Wysocki"
> > Cc: Len Brown
> > Cc: Pavel Machek
> > Signed-off-by: Lee, Chun-Yi
> > ---
> > kernel/power/snapshot.c | 24 +++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> > index 98c3b34..e6db5a8 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -730,6 +730,28 @@ static void mark_nosave_pages(struct memory_bitmap *bm)
> > }
> > }
> >
> > +static bool is_nosave_page(unsigned long pfn)
> > +{
> > + struct nosave_region *region;
> > +
> > + if (list_empty(&nosave_regions))
> > + return 0;
>
> Just a nitpicking: this should be "false" for consistency.
> But, looking further at the code:
>
Yes, should using return false;
> > +
> > + list_for_each_entry(region, &nosave_regions, list) {
> > + if (pfn >= region->start_pfn && pfn < region->end_pfn) {
> > + pr_err("PM: %#010llx in e820 nosave regsion: "
> > + "[mem %#010llx-%#010llx]\n",
> > + (unsigned long long) pfn << PAGE_SHIFT,
> > + (unsigned long long) region->start_pfn <<
> > PAGE_SHIFT,
> > + ((unsigned long long) region->end_pfn <<
> > PAGE_SHIFT)
> > + - 1);
> > + return true;
> > + }
> > + }
> > +
> > + return false;
>
> I think the above empty check can be removed completely.
>
>
> Takashi
Thanks for your suggestion, I will remove the empty check.
Regards
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] rtc-efi: check for invalid data coming back from UEFI
On Mon, Jun 02, 2014 at 03:09:16PM +0100, Jan Beulich wrote:
> In particular seeing zero in eft->month is problematic, as it results
> in -1 (converted to unsigned int, i.e. yielding 0x) getting
> passed to rtc_year_days(), where the value gets used as an array index
> (normally resulting in a crash). This was observed with the driver
> enabled on x86 on some Fujitsu system (with possibly not up to date
> firmware, but anyway).
>
> Perhaps efi_read_alarm() should not fail if neither enabled nor pending
> are set, but the returned time is invalid?
>
> Reported-by: Raymund Will
> Signed-off-by: Jan Beulich
Acked-by: Lee, Chun-Yi
Thanks a lot!
Joey Lee
> ---
> drivers/rtc/rtc-efi.c | 32 +++-
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> --- 3.15-rc8/drivers/rtc/rtc-efi.c
> +++ 3.15-rc8-EFI-RTC-check-time/drivers/rtc/rtc-efi.c
> @@ -17,6 +17,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -48,8 +49,8 @@ compute_wday(efi_time_t *eft)
> int y;
> int ndays = 0;
>
> - if (eft->year < 1998) {
> - pr_err("EFI year < 1998, invalid date\n");
> + if (eft->year < EFI_RTC_EPOCH) {
> + pr_err("EFI year < " __stringify(EFI_RTC_EPOCH) ", invalid
> date\n");
> return -1;
> }
>
> @@ -78,19 +79,36 @@ convert_to_efi_time(struct rtc_time *wti
> eft->timezone = EFI_UNSPECIFIED_TIMEZONE;
> }
>
> -static void
> +static bool
> convert_from_efi_time(efi_time_t *eft, struct rtc_time *wtime)
> {
> memset(wtime, 0, sizeof(*wtime));
> +
> + if (eft->second >= 60)
> + return false;
> wtime->tm_sec = eft->second;
> +
> + if (eft->minute >= 60)
> + return false;
> wtime->tm_min = eft->minute;
> +
> + if (eft->hour >= 24)
> + return false;
> wtime->tm_hour = eft->hour;
> +
> + if (!eft->day || eft->day > 31)
> + return false;
> wtime->tm_mday = eft->day;
> +
> + if (!eft->month || eft->month > 12)
> + return false;
> wtime->tm_mon = eft->month - 1;
> wtime->tm_year = eft->year - 1900;
>
> /* day of the week [0-6], Sunday=0 */
> wtime->tm_wday = compute_wday(eft);
> + if (wtime->tm_wday < 0)
> + return false;
>
> /* day in the year [1-365]*/
> wtime->tm_yday = compute_yday(eft);
> @@ -106,6 +124,8 @@ convert_from_efi_time(efi_time_t *eft, s
> default:
> wtime->tm_isdst = -1;
> }
> +
> + return true;
> }
>
> static int efi_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> @@ -122,7 +142,8 @@ static int efi_read_alarm(struct device
> if (status != EFI_SUCCESS)
> return -EINVAL;
>
> - convert_from_efi_time(&eft, &wkalrm->time);
> + if (!convert_from_efi_time(&eft, &wkalrm->time))
> + return -EIO;
>
> return rtc_valid_tm(&wkalrm->time);
> }
> @@ -163,7 +184,8 @@ static int efi_read_time(struct device *
> return -EINVAL;
> }
>
> - convert_from_efi_time(&eft, tm);
> + if (!convert_from_efi_time(&eft, tm))
> + return -EIO;
>
> return rtc_valid_tm(tm);
> }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 四，2014-03-13 於 09:12 +0100，Thomas Gleixner 提到： > On Thu, 13 Mar 2014, joeyli wrote: > > 於 三，2014-03-12 於 20:59 -0700，H. Peter Anvin 提到： > > > On 03/12/2014 08:55 PM, joeyli wrote: > > > > > > > > So do not care "CMOS RTC Not Present", if TAD is present then we use it > > > > instead of CMOS RTC in all kernel code? or we still can use CMOS RTC? > > > > > > > > > > Why would we use *both*!? How would that possibly make sense? > > > > > > -hpa > > > > > > > Yes, it does not make sense for using both. > > > > I switched the code in get_rtc_time() set_rtc_time() to TAD when it > > present, just make sure I'm on the right path. > > No, you're not. get/set_rtc_time() is a complete trainwreck and as I > said before it should move into the rtc subsystem. > > There is no reason at all to keep that stuff in the arch specific > code. It's there for historical reasons and that does not justify to > add more mess to it. Fully understand now. Thanks for your explanation. > > So the right thing to do is: > > 1) Add eventually missing functionality to the RTC subsystem > > 2) Move the arch specific cmos stuff to the rtc subsystem as proper > drivers > > 3) Add TAD there after #2 has been completed. > > Any attempt to add TAD to arch/x86 is NACKed unconditionally. > > Thanks, > > tglx Does there have any people already start the work of #1 and #2? Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 三，2014-03-12 於 20:59 -0700，H. Peter Anvin 提到： > On 03/12/2014 08:55 PM, joeyli wrote: > > > > So do not care "CMOS RTC Not Present", if TAD is present then we use it > > instead of CMOS RTC in all kernel code? or we still can use CMOS RTC? > > > > Why would we use *both*!? How would that possibly make sense? > > -hpa > Yes, it does not make sense for using both. I switched the code in get_rtc_time() set_rtc_time() to TAD when it present, just make sure I'm on the right path. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 三，2014-03-12 於 20:11 -0700，H. Peter Anvin 提到： > On 03/12/2014 07:38 PM, joeyli wrote: > > > > I sent rtc-acpitad driver for RTC subsystem on last month, I will send > > second version. > > > > For using TAD to set wall clock is because in ACPI 5.0 spec, there have > > a "CMOS RTC Not Present" flag in FADT to indicate OSPM should use TAD > > when this flag set: > > > > CMOS RTC Not Present > > > > Bullsh*t. The TAD should be used if it is present, it has nothing to do > with this flag. > > -hpa > > So do not care "CMOS RTC Not Present", if TAD is present then we use it instead of CMOS RTC in all kernel code? or we still can use CMOS RTC? Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 四，2014-03-13 於 00:49 +0100，Thomas Gleixner 提到： > On Thu, 13 Mar 2014, Rafael J. Wysocki wrote: > > I agree, and we need to fix that for 3.14. Patch is appended. > > You beat me by a few minutes. Was about to send out the same, just > with a more spicy changelog :) > > > --- > > From: Rafael J. Wysocki > > Subject: ACPI / init: Invoke early ACPI initialization later > > > > Commit 73f7d1ca3263 (ACPI / init: Run acpi_early_init() before > > timekeeping_init()) optimistically moved the early ACPI initialization > > before timekeeping_init(), but that didn't work, because it broke fast > > TSC calibration for Julian Wollrath on Thinkpad x121e (and most likely > > for others too). The reason is that acpi_early_init() enables the SCI > > and that interferes with the fast TSC calibration mechanism. > > > > Thus follow the original idea to execute acpi_early_init() before > > efi_enter_virtual_mode() to help the EFI people for now and we can > > revisit the other problem that commit 73f7d1ca3263 attempted to > > address in the future (if really necessary). > > Reviewed-by: Thomas Gleixner > > Acked-by: Lee, Chun-Yi Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 四，2014-03-13 於 01:54 +0100，Thomas Gleixner 提到： > On Thu, 13 Mar 2014, Rafael J. Wysocki wrote: > > Thus follow the original idea to execute acpi_early_init() before > > efi_enter_virtual_mode() to help the EFI people for now and we can > > revisit the other problem that commit 73f7d1ca3263 attempted to > > address in the future (if really necessary). > > It's not necessary at all. In fact we really want to get rid of the > arch specific cmos stuff which is an historical leftover. > > I talked to John Stultz earlier today and he agrees that there are > only a few trivial things to add to the RTC subsystem to make this > work. > I sent rtc-acpitad driver for RTC subsystem on last month, I will send second version. For using TAD to set wall clock is because in ACPI 5.0 spec, there have a "CMOS RTC Not Present" flag in FADT to indicate OSPM should use TAD when this flag set: CMOS RTC Not Present If set, indicates that the CMOS RTC is either not implemented, or does not exist at the legacy addresses. OSPM uses the Control Method Time and Alarm Namespace device instead. So, the original thinking of patch is using TAD to replace CMOS interface in kernel for access RTC. The timekeeping_init() is the earliest function to access CMOS RTC, that's why move TAD before it. I hope can discuss about "CMOS RTC Not Present" flag. If hardware vendor set this flag in FADT, should we avoid to access CMOS RTC interface in any kernel code? > >From the timekeeping POV there is absolutely no need to set the wall > clock time early. The kernel boot phase does not care about wall time > at all. We should have it done before we hit userspace, but not even > that is a hard requirement. I agree! If kernel boot phase does not care about wall time, then we don't need parse DSDT for access TAD too early. > > That TAD/EFI time mess is not going to happen before that is solved. > > Thanks, > > tglx > ACPI TAD return local time and timezone information so kernel can adjust wall time then don't need userspace involve. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
Hi Rafael,
於 三，2014-03-12 於 14:30 +0100，Rafael J. Wysocki 提到：
> > But I wonder: Can we simply enable SCI later? In other words, can
> we
> > split acpi_early_init() so that the part before
> acpi_enable_subsystem()
> > is done before timekeeping_init() and the part including and after
> > is done right after anon_vma_init()? Would the TAD initialization
> work
> > then?
>
> Below is a patch implementing that idea. Julian, can you please test
> this one too?
>
> Rafael
>
> ---
> drivers/acpi/bus.c | 20 +++-
> include/linux/acpi.h |1 +
> init/main.c |1 +
> 3 files changed, 17 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/acpi/bus.c
> ===
> --- linux-pm.orig/drivers/acpi/bus.c
> +++ linux-pm/drivers/acpi/bus.c
> @@ -494,11 +494,21 @@ void __init acpi_early_init(void)
> }
>
> status = acpi_load_tables();
> - if (ACPI_FAILURE(status)) {
> - printk(KERN_ERR PREFIX
> - "Unable to load the System Description Tables
> \n");
> - goto error0;
> - }
> + if (ACPI_SUCCESS(status))
> + return;
> +
> + printk(KERN_ERR PREFIX "Unable to load the System Description
> Tables\n");
> +
> + error0:
> + disable_acpi();
> +}
> +
> +void __init acpi_subsystem_init(void)
> +{
> + acpi_status status;
> +
> + if (acpi_disabled)
> + return;
>
After check the DSDT of target machine, I afraid the above patch is not
enough to use _GRT and _SRT because they need opregion support of
SystemMemory and SystemIO for trigger SMM to access RTC.
In my current semifinished patches, I add code to run the
acpi_ev_install_region_handlers() in acpi_enable_subsystem() when
acpi_early_init(), for install region handler to support opregion.
Before install region_handlers, it call acpi_enable() to enable acpi
mode and I think need setup SCI interrupt before run acpi_enable():
acpi_early_init(void)
acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
acpi_enable_subsystem(u32 flags)
acpi_enable(); /* Enable ACPI mode */
acpi_tb_initialize_facs();
acpi_ev_install_region_handlers(); /* Need it! */
For developing ACPI TAD support, I used SystemIO to access CMOS port in
DSDT of Intel DQ57 for simulate ACPI TAD. It also need SystemIO opregion
support as the real machine. I just run your patch on this machine but
it failed.
I think maybe still using ACPI_FADT_NO_CMOS_RTC to check does
acpi_early_init() need run before timekeeping_init().
If there have any future machine that applied ACPI TAD but "Fast TSC
calibration" fail, at least the alternate TSC calibration can work
around issue.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 三，2014-03-12 於 12:20 +0100，Rafael J. Wysocki 提到： > On Wednesday, March 12, 2014 12:00:07 PM joeyli wrote: > > Hi Julian, [...] > > > > > > >From 8ef4fff76dd2f50bef1e8eb9c96f3b0228a38401 Mon Sep 17 00:00:00 2001 > > From: Lee, Chun-Yi > > Date: Wed, 12 Mar 2014 11:36:32 +0800 > > Subject: [PATCH] ACPI / init: Run acpi_early_init() before > > timekeeping_init() when CMOS RTC Not Present bit set > > > > This is a variant patch from Rafael J. Wysocki's > > ACPI / init: Run acpi_early_init() before efi_enter_virtual_mode() > > > > According to Matt Fleming, if acpi_early_init() was executed before > > efi_enter_virtual_mode(), the EFI initialization could benefit from > > it, so Rafael's patch makes that happen. > > > > And, later we want accessing ACPI TAD device to set system clock, so > > move acpi_early_init() before timekeeping_init() when "CMOS RTC Not > > Present" bit set. This position is also before efi_enter_virtual_mode(). > > > > Signed-off-by: Lee, Chun-Yi > > --- > > init/main.c |7 ++- > > 1 files changed, 6 insertions(+), 1 deletions(-) > > > > diff --git a/init/main.c b/init/main.c > > index eb03090..e1b69d2 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -561,7 +561,9 @@ asmlinkage void __init start_kernel(void) > > init_timers(); > > hrtimers_init(); > > softirq_init(); > > - acpi_early_init(); > > + if (acpi_gbl_FADT.header.revision >= 5 && > > + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) > > + acpi_early_init(); > > Sorry, this is not the right way to address that. > > First of all, checking fields in acpi_gbl_FADT from anything in init/main.c > is wrong. If anything, please move that check to acpi_early_init(). And > make it check it if it's been called already. > I see, thanks for your suggestion. > > timekeeping_init(); > > time_init(); > > sched_clock_postinit(); > > @@ -613,6 +615,9 @@ asmlinkage void __init start_kernel(void) > > calibrate_delay(); > > pidmap_init(); > > anon_vma_init(); > > + if (!(acpi_gbl_FADT.header.revision >= 5 && > > + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) > > + acpi_early_init(); > > And then you can call it here again. > > > #ifdef CONFIG_X86 > > if (efi_enabled(EFI_RUNTIME_SERVICES)) > > efi_enter_virtual_mode(); > > > > But I wonder: Can we simply enable SCI later? In other words, can we > split acpi_early_init() so that the part before acpi_enable_subsystem() > is done before timekeeping_init() and the part including and after > is done right after anon_vma_init()? Would the TAD initialization work > then? > The machine that support ACPI TAD doesn't on my hand now, I need find a time go to check it. Per DSDT, it trigger SMI in _GRT and _SRT to get/set time. Looks no SCI involve. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
於 三，2014-03-12 於 11:20 +0100，Julian Wollrath 提到： > > This patch restricts the position to run acpi_early_init() before > > timekeeping_init() when only "CMOS RTC Not Present" bit set in FADT. > > > > Could you please help to test it on your machine? > That patch fixes the problem, thank you. > > Cheers, > Julian Wollrath > > Thanks for your test. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND] Fast TSC calibration fails with v3.14-rc1 and later
Hi Julian, 於 二，2014-03-11 於 18:15 +0100，Julian Wollrath 提到： > Am Tue, 11 Mar 2014 14:56:41 +0100 (CET) > schrieb Thomas Gleixner : > > > Ok, via bisecting I found commit > > > 73f7d1ca32638028e3271f54616773727e2f9f26 (see below) to be the one > > > that introduced this regression. > > > > Interesting. I have no idea what's going on. But maybe can the ACPI > > folks shed some light on it. > My patch moved acpi_early_init() to before timekeeping_init() is for prepare the later using ACPI TAD to set system clock. I think that because acpi_early_init() setup SCI interrupt and enable acpi subsystem, it causes fast TSC calibration fail. > I have absolutely no idea, if it is the right thing to do and why it > works, but the patch below fixes the problem. Thank you for your help. > > > Cheers, > Julian Wollrath > > From 7664f495039d93adfce073e58840a46549904f04 Mon Sep 17 00:00:00 2001 > From: Julian Wollrath > Date: Tue, 11 Mar 2014 18:05:43 +0100 > Subject: [PATCH] Fix fast TSC calibration > > Since commit 73f7d1ca32638028e3271f54616773727e2f9f26 the fast TSC calibration > failed on a Thinkpad X121e with an AMD E450 APU. Moving acpi_early_init() > after > late_time_init() fixes this. > > Signed-off-by: Julian Wollrath > --- > init/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/init/main.c b/init/main.c > index eb03090cdced..bf9d99148bd6 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -561,7 +561,6 @@ asmlinkage void __init start_kernel(void) > init_timers(); > hrtimers_init(); > softirq_init(); > - acpi_early_init(); > timekeeping_init(); > time_init(); > sched_clock_postinit(); > @@ -609,6 +608,7 @@ asmlinkage void __init start_kernel(void) > numa_policy_init(); > if (late_time_init) > late_time_init(); > + acpi_early_init(); > sched_clock_init(); > calibrate_delay(); > pidmap_init(); The late_time_init() dependent on timekeeping_init(), if we don't want move acpi_early_init() before timekeeping_init() then just direct put it before efi_enter_virtual_mode() because we tested this changed. This patch restricts the position to run acpi_early_init() before timekeeping_init() when only "CMOS RTC Not Present" bit set in FADT. Could you please help to test it on your machine? Thanks a lot! Joey Lee >From 8ef4fff76dd2f50bef1e8eb9c96f3b0228a38401 Mon Sep 17 00:00:00 2001 From: Lee, Chun-Yi Date: Wed, 12 Mar 2014 11:36:32 +0800 Subject: [PATCH] ACPI / init: Run acpi_early_init() before timekeeping_init() when CMOS RTC Not Present bit set This is a variant patch from Rafael J. Wysocki's ACPI / init: Run acpi_early_init() before efi_enter_virtual_mode() According to Matt Fleming, if acpi_early_init() was executed before efi_enter_virtual_mode(), the EFI initialization could benefit from it, so Rafael's patch makes that happen. And, later we want accessing ACPI TAD device to set system clock, so move acpi_early_init() before timekeeping_init() when "CMOS RTC Not Present" bit set. This position is also before efi_enter_virtual_mode(). Signed-off-by: Lee, Chun-Yi --- init/main.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/init/main.c b/init/main.c index eb03090..e1b69d2 100644 --- a/init/main.c +++ b/init/main.c @@ -561,7 +561,9 @@ asmlinkage void __init start_kernel(void) init_timers(); hrtimers_init(); softirq_init(); - acpi_early_init(); + if (acpi_gbl_FADT.header.revision >= 5 && + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) + acpi_early_init(); timekeeping_init(); time_init(); sched_clock_postinit(); @@ -613,6 +615,9 @@ asmlinkage void __init start_kernel(void) calibrate_delay(); pidmap_init(); anon_vma_init(); + if (!(acpi_gbl_FADT.header.revision >= 5 && + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) + acpi_early_init(); #ifdef CONFIG_X86 if (efi_enabled(EFI_RUNTIME_SERVICES)) efi_enter_virtual_mode(); -- 1.6.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] crypto: avoid module request when lookup crypto larval of template
Hi Herbert, 於 一，2014-03-10 於 19:57 +0800，Herbert Xu 提到： > On Mon, Mar 10, 2014 at 05:22:51PM +0800, Lee, Chun-Yi wrote: > > When allocate crypto algorithms, e.g. crypto_alloc_shash(), using > > template model will run into the path that call module_request(). > > But there have no any module alias that match with the template name. > > There aren't any but there could be. For example, as it is > all direct implementations of cbc(aes) rely on the fact that > they also happen to provide aes to autoload themselves. > > We may well have a case where it makes sense for a module to > provide just cbc(aes) for example, in which case it would need > such an alias for it to be loaded automatically. > > Thanks, Thanks for your quick explanation! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MODSIGN: Fix including certificate twice when the signing_key.x509
於 四，2014-01-16 於 12:31 +，David Howells 提到： > > Are you asking for this to go upstream or into my devel-pekey branch? > > If upstream you want it to go upstream, I presume commit > d7ec435fdd03cfee70dba934ee384acc87bd6d00 doesn't fix the problem? > > David You are right. I tried the d7ec435 patch fixed issue. Please ignore my patch. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MODSIGN: Fix including certificate twice when the signing_key.x509
於 三，2014-01-15 於 15:09 +1030，Rusty Russell 提到： > > > > v2: > > Using '$(shell /bin/pwd)' instead of '$(shell pwd)' for more > reliable > > between different shells > > Hmm, that's not a great test for equality. How about: > > ifneq ($(realpath .), $(realpath $(srctree))) > > That should cover all the cases. > > Cheers, > Rusty. Yes, it's better! Thanks for your suggestion, I will send new version with changelog updated. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT][PATCH] ACPI / init: Run acpi_early_init() before efi_enter_virtual_mode()
於 二，2014-01-14 於 13:32 -0700，Toshi Kani 提到：
> > > + acpi_early_init();
> > > timekeeping_init();
> > > time_init();
> > > sched_clock_postinit();
> > > @@ -641,7 +642,6 @@ asmlinkage void __init start_kernel(void)
> > >
> > > check_bugs();
> > >
> > > - acpi_early_init(); /* before LAPIC and SMP init */
> > > sfi_init_late();
> > >
> > > if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> > >
> >
> > Hi Toshi,
> >
> > Could you try this variant, too? If this works as well then we end
> up
> > solving two problems in one patch...
>
> Hi Peter,
>
> Yes, this version works fine as well.
>
> Tested-by: Toshi Kani
>
> Thanks,
> -Toshi
Thanks a lot for your testing.
I will re-send a formal patch with changelog to everybody.
Regards
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT][PATCH] ACPI / init: Run acpi_early_init() before efi_enter_virtual_mode() (was: Re: [RFC PATCH 00/14] Support timezone of ACPI TAD and EFI TIME)
於 日，2014-01-12 於 01:30 +0100，Rafael J. Wysocki 提到：
> OK
>
> I don't see any adverse effects of the patch below on a couple of my
> test
> boxes, but (a) they are Intel-based and (b) they are non-EFI, so it
> would be
> good to give it a go on as many machines as reasonably possible.
>
> Thanks,
> Rafael
>
> ---
> From: Rafael J. Wysocki
> Subject: ACPI / init: Run acpi_early_init() before
> efi_enter_virtual_mode()
>
> According to Matt Fleming, if acpi_early_init() was executed befpre
> efi_enter_virtual_mode(), the EFI initialization could benefit from
> it, so make that happen.
>
> Signed-off-by: Rafael J. Wysocki
> ---
> init/main.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-pm/init/main.c
> ===
> --- linux-pm.orig/init/main.c
> +++ linux-pm/init/main.c
> @@ -615,6 +615,7 @@ asmlinkage void __init start_kernel(void
> calibrate_delay();
> pidmap_init();
> anon_vma_init();
> + acpi_early_init();
> #ifdef CONFIG_X86
> if (efi_enabled(EFI_RUNTIME_SERVICES))
> efi_enter_virtual_mode();
> @@ -641,7 +642,6 @@ asmlinkage void __init start_kernel(void
>
> check_bugs();
>
> - acpi_early_init(); /* before LAPIC and SMP init */
> sfi_init_late();
>
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>
>
>
This patch works to me on Acer Gateway Z5WT2 UEFI notebook and Intel
UEFI development board.
Does it possible move acpi_early_init() to before timekeeping_init()?
The position is also before efi_enter_virtual_mode() and that will be
useful for parsing ACPI TAD to set system clock:
diff --git a/init/main.c b/init/main.c
index febc511..b6d93c8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -565,6 +565,7 @@ asmlinkage void __init start_kernel(void)
init_timers();
hrtimers_init();
softirq_init();
+ acpi_early_init();
timekeeping_init();
time_init();
sched_clock_postinit();
@@ -641,7 +642,6 @@ asmlinkage void __init start_kernel(void)
check_bugs();
- acpi_early_init(); /* before LAPIC and SMP init */
sfi_init_late();
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 三，2014-01-08 於 09:56 -0800，H. Peter Anvin 提到： [...] > > Document of Windows XP: > > > http://www.freelists.org/post/windows_errors/what-error-messages-really-mean-WinXP-IO-Ports-Blocked-from-Bios-AML-on-Windows-XP > > > > If just for ACPI TAD testing, we can remove the port protection > check of > > RTC ports in hwvalid.c. I have read 0x70/0x71 port success after > removed > > the checking in acpica/hwvalid.c. > > > > I will try to write RTC port in AML after remove acpica check, maybe > > have other unpredictable situation. > > > > Now *THERE* is a good use of the "no RTC bit". In the case that bit > is > set we should presumably remove these ports from the block list. Thanks for your suggestion, I will put a testing patch on this. > > Otherwise we should use the CMOS address space, not the I/O port > address > space. > > -hpa Unfortunately current acpica leaks the SystemCMOS handler: ACPI Error: Region SystemCMOS (ID=5) has no handler (20131115/exfldio-299) Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 二，2014-01-07 於 08:35 -0800，H. Peter Anvin 提到：
> On 01/07/2014 02:40 AM, joeyli wrote:
> >
> > Due to accessing CMOS through ASL need enable SMM support in OVMF,
>
> Why? The CMOS is its own ASL address space, and you need that anyway to
> be able to access the RTC proper. If you don't want to use it because
> you don't want to export any indication of a legacy RTC you should be
> able to just do I/O port references directly in your ASL.
>
> -hpa
>
>
ACPICA denied AML access RTC ports.
I tried to access 0x70, 0x71 ports in ASL on a real machine, ACPICA
denied AML access to those ports. I got the following dmesg:
hwvalid-0188 hw_validate_io_request: Denied AML access to port
0x0071/1
The code in acpica denied it:
linux/drivers/acpi/acpica/hwvalid.c
* This provides ACPICA with the desired port protections and
* Microsoft compatibility.
*
* Description of port entries:
[...]
* RTC: Real-time clock
* CMOS: Extended CMOS
[...]
*/
static const struct acpi_port_info acpi_protected_ports[] = {
[...]
{"RTC", 0x0070, 0x0071, ACPI_OSI_WIN_XP},
{"CMOS", 0x0074, 0x0076, ACPI_OSI_WIN_XP},
Document of Windows XP:
http://www.freelists.org/post/windows_errors/what-error-messages-really-mean-WinXP-IO-Ports-Blocked-from-Bios-AML-on-Windows-XP
If just for ACPI TAD testing, we can remove the port protection check of
RTC ports in hwvalid.c. I have read 0x70/0x71 port success after removed
the checking in acpica/hwvalid.c.
I will try to write RTC port in AML after remove acpica check, maybe
have other unpredictable situation.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 一，2014-01-06 於 21:37 -0800，H. Peter Anvin 提到： > On 01/06/2014 12:58 AM, joeyli wrote: > > 於 二，2013-12-31 於 16:42 -0800，H. Peter Anvin 提到： > >> On 12/19/2013 09:41 PM, joeyli wrote: > >>>> > >>>> What platform do you have that has TAD support? I am wondering how this > >>>> was tested. > >>>> > >>> > >>> It's a testing platform that's only support get/set time functions of > >>> ACPI TAD. > >>> > >> > >> It would be really, really good to get this into Qemu (either SeaBIOS or > >> OVMF, or ideally both) so we can have anyone test. > >> > >>-hpa > > > > I will try to add to OVMF first. > > > > For the record, I posted a patch to Qemu about a year ago to store the > timezone in the CMOS, which might be useful for this implementation. It > was rejected because of no firmware support, so if you implement it for > OVMF we can (update and) push this patch again. > > -hpa > Thanks for your patch. Due to accessing CMOS through ASL need enable SMM support in OVMF, I am looking Laszlo's OVMF S3 patches: https://www.mail-archive.com/edk2-devel@lists.sourceforge.net/msg04969.html Honestly I am not good on this area, I need to spend more time to understand it. Regards Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 四，2014-01-02 於 16:09 +0800，Lan Tianyu 提到： > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > > index bba9b72..3f7a075 100644 > > --- a/drivers/acpi/bus.c > > +++ b/drivers/acpi/bus.c > > @@ -689,6 +689,9 @@ static int __init acpi_init(void) > > pci_mmcfg_late_init(); > > acpi_scan_init(); > > acpi_ec_init(); > > +#ifdef CONFIG_X86 > > + acpi_tad_init(); > > +#endif > > Why calling acpi_tad_init() directly here rather than using > module_initcall? > Is there dependency? > The rtc-acpitad RTC driver depend on acpi_read/set_time functions in acpi_tad. On the other hand, if we adapt to ACPI time device when "CMOS RTC Not Present" set, then acpi_read/set_time will used to replace CMOS functions that's called by other drivers. So, I direct call acpi_tad_init() here. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 二，2013-12-31 於 16:42 -0800，H. Peter Anvin 提到： > On 12/19/2013 09:41 PM, joeyli wrote: > >> > >> What platform do you have that has TAD support? I am wondering how this > >> was tested. > >> > > > > It's a testing platform that's only support get/set time functions of > > ACPI TAD. > > > > It would be really, really good to get this into Qemu (either SeaBIOS or > OVMF, or ideally both) so we can have anyone test. > > -hpa > I will try to add to OVMF first. Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 06/14] rtc-efi: register rtc-efi device when EFI enabled
於 五，2013-12-20 於 17:51 -0800，H. Peter Anvin 提到： > On 12/20/2013 05:24 PM, joeyli wrote: > > 於 五，2013-12-20 於 13:04 -0800，H. Peter Anvin 提到： > >> > >> Actually, it doesn't have to reprogram the clock ... it just needs to > >> know if another OS has already done so. All Linux needs to do is to be > >> able to derive UTC from whatever the RTC is set to and to be able to > >> keep it consistent. > >> > > > > It's dependent on a right boot initial priority of distribution. > > -ENOPARSE > > > Here have a discussion of adjusting system clock by TZ (from ACPI or > > UEFI): > > Discussion on BIOS/CMOS/UEFI clock in local time > > http://www.spinics.net/lists/util-linux-ng/msg07639.html > > > > and, > > from Ted Ts'o in the mail thread > > https://lkml.org/lkml/2008/1/8/195 > > > > If kernel use the TZ field from ACPI TAD or EFI to adjust system > > clock when booting, then it can avoid buggy distributions adjust > > system clock AFTER e2fsck is run. > > > > Using ACPI TAD should after DSDT parsing in subsystem initial stage, > > so I choice EFI time services before we can move DSDT parser to > > start_kernel(). > > > > Yes, of course. That is irrelevant to needing to reprogram the clock, > though. > > My argument is very simple: if we have to rely on EFI, we can get the > offset in the boot stub before ExitBootServices(), and then simply never > change it. That way we still pick up if another operating system has > changed it, and it will still reflect the proper UTC time. > > -hpa > OK, I will add this part to next version for the UEFI system doesn't have ACPI TAD. Thanks for your suggestion! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 07/14] rtc-efi: add GMTOFF support to rtc_efi
於 五，2013-12-20 於 15:11 +，Matthew Garrett 提到： > On Thu, 2013-12-19 at 15:51 +0800, Lee, Chun-Yi wrote: > > This patch adds 2 new iotrl: RTC_RD_GMTOFF and RTC_SET_GMTOFF to > > rtc_efi support get/set gmt offset that mapping to the GUN's tm_gmtoff > > extension (Seconds east of UTC). > > Shouldn't this information also be exported via the rtc-sysfs interface? > Thanks for your suggestion, I will also export it to rtc-sysfs. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/14] Support timezone of ACPI TAD and EFI TIME
於 五，2013-12-20 於 22:45 +0100，Rafael J. Wysocki 提到： > On Friday, December 20, 2013 01:10:26 PM H. Peter Anvin wrote: > > On 12/19/2013 09:38 PM, joeyli wrote: > > > > > > If don't use EFI time, then the first priority is using ACPI TAD if it > > > present. Due to ACPI TAD is a generic acpi device that's need OS parsing > > > DSDT table before set system time. > > > > > > Either move DSDT parser from subsystem initial stage to start_kernel or > > > move timekeeping initial to after DSDT be parsed. Which one you think is > > > more possible and risk less? Then I will try that way. > > > > > > > I discussed the DSDT/SSDT parsing issue with Rafael and he claims it > > would require a lot of restructuring. Unfortunately ACPI is at this > > point done rather late, as I understand. All of this is a big problem. > > My understanding, however, is that to use the TAD, we don't actually need to > create a struct acpi_device for it. We just need a handle to the ACPICA > object which can be found using acpi_get_devices() as soon as the namespace > has been extracted from the DSDT and friends. That in turn happens in > acpi_early_init(), which is called from start_kernel() right before > efi_late_init(). > > Is that early enough? > > Rafael > Thanks for your good suggestion, then I have a direction on using ACPI TAD earlier. Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 06/14] rtc-efi: register rtc-efi device when EFI enabled
於 五，2013-12-20 於 13:04 -0800，H. Peter Anvin 提到： > On 12/20/2013 07:14 AM, Matthew Garrett wrote: > > On Fri, 2013-12-20 at 11:37 +0100, Borislav Petkov wrote: > >> On Thu, Dec 19, 2013 at 08:30:48PM -0800, H. Peter Anvin wrote: > >>> On 12/19/2013 08:24 PM, joeyli wrote: > >>>> I agreed, but userspace application should not be too often to access > >>>> RTC. Maybe only when system boot and set timezone. > >>> > >>> This is, quite frankly, an idiotic argument. > >> > >> TBH, I've been struggling with the question too - and it might even be a > >> stupid question - but what is that absolute need to be able to get the > >> TZ in userspace? Why should I care? > >> > >> Can we get some use cases for stupid people like me please? > > > > Dual-boot environments will tend to have the RTC in local time, not UTC. > > That means that userspace has to reprogram the clock over daylight > > savings changes, and to do that it must know whether another OS has > > already done so. > > > > Actually, it doesn't have to reprogram the clock ... it just needs to > know if another OS has already done so. All Linux needs to do is to be > able to derive UTC from whatever the RTC is set to and to be able to > keep it consistent. > > -hpa > > It's dependent on a right boot initial priority of distribution. Here have a discussion of adjusting system clock by TZ (from ACPI or UEFI): Discussion on BIOS/CMOS/UEFI clock in local time http://www.spinics.net/lists/util-linux-ng/msg07639.html and, from Ted Ts'o in the mail thread https://lkml.org/lkml/2008/1/8/195 If kernel use the TZ field from ACPI TAD or EFI to adjust system clock when booting, then it can avoid buggy distributions adjust system clock AFTER e2fsck is run. Using ACPI TAD should after DSDT parsing in subsystem initial stage, so I choice EFI time services before we can move DSDT parser to start_kernel(). Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 04/14] ACPI: Add ACPI 5.0 Time and Alarm Device driver
於 四，2013-12-19 於 07:22 -0800，H. Peter Anvin 提到： > On 12/18/2013 11:51 PM, Lee, Chun-Yi wrote: > > This patch add the driver of Time and Alarm Device in ACPI 5.0. > > Currently it only implemented get/set time functions and grab > > the capabilities of device when driver initial. > > > > This driver also register rtc-acpitad platform device for RTC ACPITAD > > stub driver using. > > > > Signed-off-by: Lee, Chun-Yi > > What platform do you have that has TAD support? I am wondering how this > was tested. > > -hpa > It's a testing platform that's only support get/set time functions of ACPI TAD. Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/14] Support timezone of ACPI TAD and EFI TIME
於 四，2013-12-19 於 20:22 -0800，H. Peter Anvin 提到： > On 12/19/2013 08:05 PM, joeyli wrote: > > > > Then that means the priority of PNP0B0x is higher then "CMOS RTC Not > > Present" flag. ACPI spec doesn't have clear definition on this. > > > > According to the Microsoft requirements documents, such a platform is > broken and shouldn't exist. OK~~ > > > I look forward to Borislav's "EFI runtime mapping" to fix the physical > > address accessing issue of EFI time service on x86_64 machines. I tested > > his patches on a issue machine and it works for walk around BIOS bug. > > > > Can we use EFI time services on x86_64 after Borislav's patches accepted > > to mainline? > > > > No. > > -hpa If don't use EFI time, then the first priority is using ACPI TAD if it present. Due to ACPI TAD is a generic acpi device that's need OS parsing DSDT table before set system time. Either move DSDT parser from subsystem initial stage to start_kernel or move timekeeping initial to after DSDT be parsed. Which one you think is more possible and risk less? Then I will try that way. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 06/14] rtc-efi: register rtc-efi device when EFI enabled
於 四，2013-12-19 於 14:09 +，Matt Fleming 提到： > On Thu, 19 Dec, at 03:51:47PM, Lee, Chun-Yi wrote: > > UEFI time services, GetTime(), SetTime(), GetWakeupTime(), SetWakeupTime() > > are also > > supported by other non-IA64 architecutre with UEFI BIOS, e.g. x86. > > > > This patch changed RTC_DRV_EFI configuration to depend on EFI but not just > > IA64. It > > checks efi_enabled flag and efi-rtc driver should enabled. > > > > Cc: Matt Fleming > > Cc: H. Peter Anvin > > Cc: Matthew Garrett > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: Jan Beulich > > Signed-off-by: Lee, Chun-Yi > > --- > > arch/x86/platform/efi/efi.c | 17 + > > drivers/rtc/Kconfig |2 +- > > 2 files changed, 18 insertions(+), 1 deletions(-) > > This patch needs to be justified. Enabling the EFI runtime *Time > functions just because they're available isn't good enough. We need to > know why this patch improves things, what use case does it solve? > The main purpose of enable efi-rtc driver is providing interface to userspace access timezone field in EFI time services. Currently have two BIOS interfaces provided the timezone field accessing ability, ACPI TAD and EFI. I hope can enable them on x86_64 machines. > The general attitude has been that we want to invoke the runtime > services less, not more, due to the huge variety of runtime > implementation bugs. > I agreed, but userspace application should not be too often to access RTC. Maybe only when system boot and set timezone. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/14] Support timezone of ACPI TAD and EFI TIME
於 四，2013-12-19 於 06:59 -0800，H. Peter Anvin 提到： > On 12/18/2013 11:43 PM, Lee, Chun-Yi wrote: > > This patchset add the timezone support of ACPI TAD and EFI TIME, it > > also add codes for adjusting system time base on the timezone value > > from EFI TIME services when system boot. > > EFI time is something we used to support and ripped out, because it > doesn't work well, *at all*. > > I am hyper-skeptical to reintroducing them, and *definitely* will veto > reintroducing them on a system which has PNP0B0x devices present. > > -hpa > Then that means the priority of PNP0B0x is higher then "CMOS RTC Not Present" flag. ACPI spec doesn't have clear definition on this. I look forward to Borislav's "EFI runtime mapping" to fix the physical address accessing issue of EFI time service on x86_64 machines. I tested his patches on a issue machine and it works for walk around BIOS bug. Can we use EFI time services on x86_64 after Borislav's patches accepted to mainline? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/14] rtc: block registration of rtc-cmos when CMOS RTC Not Present
Hi hpa,
於 四，2013-12-19 於 06:38 -0800，H. Peter Anvin 提到：
> Where did you find a platform with "no CMOS" set and a PNP RTC? I find the
> expect behavior in that case to be quite ambiguous and it is not at all clear
> to me that what you have here is the right thing.
Actually there doesn't have the box both with "No CMOS" and PNP device.
I choice to totally block rtc-cmos driver when "No CMOS RTC" because the
definition in ACPI spec:
CMOS RTC Not Present
If set, indicates that the CMOS RTC is either not implemented, or
does not exist at the legacy addresses. OSPM uses the Control
Method Time and Alarm Namespace device instead.
^^
It suggest us using ACPI TAD interface when this flag present. But, I
agreed your point for this is ambiguous due to ACPI spec didn't clear
define the relationship between PNP0B0x.
Maybe we can do more detail check in cmos_init when "No CMOS RTC" set:
+ check if have ACPI TAD device, then block rtc-cmos
+ check if no ACPI TAD device, but have PNP0B0x, then we use PNP0b0x.
>
> "Lee, Chun-Yi" wrote:
> >We should not acess CMOS address when CMOS RTC Not Present bit set in
> >FADT. The ee5872be patch didn't avoid rtc-cmos driver loaded when
> >system support
> >ACPI PNP PNP0B0* devices.
> >So this patch block the registion of rtc-cmos driver to avoid
> >user space access RTC through CMOS interface.
> >
> >Signed-off-by: Lee, Chun-Yi
> >---
> > arch/x86/kernel/rtc.c | 20
> > drivers/rtc/rtc-cmos.c |9 +
> > 2 files changed, 25 insertions(+), 4 deletions(-)
> >
...
> >--- a/drivers/rtc/rtc-cmos.c
> >+++ b/drivers/rtc/rtc-cmos.c
> >@@ -28,6 +28,9 @@
> > * interrupts disabled, holding the global rtc_lock, to exclude those
> > * other drivers and utilities on correctly configured systems.
> > */
> >+
> >+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >+
> > #include
> > #include
> > #include
> >@@ -1144,6 +1147,12 @@ static int __init cmos_init(void)
> > {
> > int retval = 0;
> >
> >+if (acpi_gbl_FADT.header.revision >= 5 &&
> >+acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) {
> >+pr_info("ACPI CMOS RTC Not Present detected - not loading\n");
> >+return 0;
> >+}
> >+
> > #ifdef CONFIG_PNP
> > retval = pnp_register_driver(&cmos_pnp_driver);
> > if (retval == 0)
>
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/14] x86-64/efi: Use EFI to deal with platform wall clock (again)
於 四，2013-12-19 於 10:49 +，Matt Fleming 提到：
> On Thu, 19 Dec, at 06:20:16PM, Lee, Chun-Yi wrote:
> > From: Jan Beulich
> >
> > Other than ix86, x86-64 on EFI so far didn't set the
> > {g,s}et_wallclock accessors to the EFI routines, thus
> > incorrectly using raw RTC accesses instead.
> >
> > Simply removing the #ifdef around the respective code isn't
> > enough, however: While so far early get-time calls were done in
> > physical mode, this doesn't work properly for x86-64, as virtual
> > addresses would still need to be set up for all runtime regions
> > (which wasn't the case on the system I have access to), so
> > instead the patch moves the call to efi_enter_virtual_mode()
> > ahead (which in turn allows to drop all code related to calling
> > efi-get-time in physical mode).
> >
> > Additionally the earlier calling of efi_set_executable()
> > requires the CPA code to cope, i.e. during early boot it must be
> > avoided to call cpa_flush_array(), as the first thing this
> > function does is a BUG_ON(irqs_disabled()).
> >
> > Also make the two EFI functions in question here static -
> > they're not being referenced elsewhere.
> >
> > History:
> >
> > This commit was originally merged as bacef661acdb ("x86-64/efi:
> > Use EFI to deal with platform wall clock") but it resulted in some
> > ASUS machines no longer booting due to a firmware bug, and so was
> > reverted in f026cfa82f62.
> >
> > Then a pre-emptive fix for the buggy ASUS firmware was merged in
> > 03a1c254975e ("x86, efi: 1:1 pagetable mapping for virtual EFI
> > calls") but it causes odd bootup problems on x86-64. So this patch
> > revoked again by 11520e5e7c1.
> >
> > Now Borislav Petkov's "EFI runtime services virtual mapping" is
> > merged to EFI 'next' branch. So this patch can be reapplied again.
> >
> > Signed-off-by: Jan Beulich
> > Tested-by: Matt Fleming
> > Acked-by: Matthew Garrett
> > Cc: Ingo Molnar
> > Cc: Peter Zijlstra
> > Cc: H. Peter Anvin
> > Signed-off-by: Matt Fleming [added commit history]
> > Acked-by: Lee, Chun-Yi
> > ---
> > arch/x86/mm/pageattr.c | 10 ++
> > arch/x86/platform/efi/efi.c | 35 +++
> > include/linux/efi.h |2 --
> > init/main.c |8
> > 4 files changed, 21 insertions(+), 34 deletions(-)
>
> Lee, you can't just simply resend this patch with all the tags - I
> haven't tested this version with any recent changes and I'm pretty sure
> Matthew isn't going to Ack it.
I am very sorry for I didn't remove those tags before send it. I will
remove it when send second version.
>
> Do you know if anyone has tested this patch with Borislav's recent
> changes?
>
I tested Borislav's patch set on a issue BIOS and make sure it works,
but I have no chance to test this patch with it. I will find a time to
test it.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, efi: change name of efi_no_storage_paranoia parameter to efi_storage_paranoia
於 四，2013-11-21 於 17:53 +0800，joeyli 提到： > 於 四，2013-11-21 於 18:13 +0900，Yasuaki Ishimatsu 提到： > > (2013/11/20 17:08), joeyli wrote: > > > 於 三，2013-11-20 於 15:26 +0900，Yasuaki Ishimatsu 提到： > > >> (2013/11/19 12:16), Madper Xie wrote: > > >>> > > >>> isimatu.yasu...@jp.fujitsu.com writes: > > >>> > > >>>> Hi Matt, > > >>>> > > >>>> Sorry for late the reply. > > >>>> > > >>>> > > >>>> (2013/11/11 19:54), Matt Fleming wrote: > > >>>>> On Mon, 11 Nov, at 05:52:59PM, Yasuaki Ishimatsu wrote: > > >>>>>> Hi Matt, > > >>>>>> > > >>>>>> I uses FUJITSU's x86 box. > > >>>>>> This does not become bricked even if I use all efi variable storage. > > >>>>>> Thus I want a way to not need to specify efi_no_storage_paranoia > > >>>>>> parameter. > > >>>>> > > >>>>> The efi_no_storage_paranoia parameter was introduced because some > > >>>>> machines do not initiate garbage collection of the NVRAM until you > > >>>>> allocate all space - basically it's a switch to turn off the "save 5KB > > >>>>> of stoarge at all times" workaround that is needed to avoid bricking > > >>>>> some machines. > > >>>>> > > >>>>> The intention of the switch is not to allow you to fill your NVRAM > > >>>>> just > > >>>>> because you can. If that is something you want to do then I think it's > > >>>>> fair to require you to explicitly turn on efi_no_storage_paranoia. But > > >>>>> I'm assuming here that you are doing something like writing lots and > > >>>>> lots of pstore entries and just want to write as many as your variable > > >>>>> storage will allow? Or are you doing something more fundamental like > > >>>>> creating Boot entries? > > >>>>> > > >>>>> What are you doing to run into the 5KB reserve? How much NVRAM does > > >>>>> your > > >>>>> machine come with? > > >>>> > > >>>> I just add boot entry to NVRAM by efibootmgr command. But when Linux > > >>>> boots up, > > >>>> the remaining NVRAM is less than 5Kbyte. So I cannnot add new entry. > > >>>> > > >>> Howdy Yasuaki, > > >>> If the remaining NVRAM is less than 5Kb, your writing will trigger a > > >>> NVRAM storage reclamation. However you still failed creating entry. > > >>> So > > >>> I'm just curious what itmes occupy lots of nvram storage space. > > >> > > >> Even if we got EFI_OUT_OF_RESOURCES while running Linux, gc does not run. > > >> Trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS environment > > >> with > > >> UEFI. So on my system, if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte > > >> threshold, > > >> we cannot use nvram storage until EFI_OUT_OF_RESOURCES occurs on pre OS > > >> environment with UEFI. > > >> > > >> Thanks, > > >> Yasuaki Ishimatsu > > > > > > Can we try to trigger gc by EFI_OUT_OF_RESOURCE in EFI stub kernel or > > > EFI boot loader to recover NVRAM space? Does work with the BIOS on this > > > machine? > > > > Yes. I can trigger gc by EFI_OUT_OF_RESOUCE in EFI shell on my machine. > > > > Thanks, > > Yasuaki Ishimatu > > > > OK, then maybe can try to trigger gc in EFI stub before > ExitBootService(). > > Another problem is what's the reasonable threshold. The threshold should > bigger then 5Kbyte, then we write a dummy BootTime NV variable to > trigger gc. > > > Thanks > Joey Lee hm... after review your mail, if the free NVRAM space of issue machine is REALLY less then 5K even after gc. Then there have no way to write any variable unless user remove some NV variable. Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, efi: change name of efi_no_storage_paranoia parameter to efi_storage_paranoia
於 四，2013-11-21 於 18:13 +0900，Yasuaki Ishimatsu 提到： > (2013/11/20 17:08), joeyli wrote: > > 於 三，2013-11-20 於 15:26 +0900，Yasuaki Ishimatsu 提到： > >> (2013/11/19 12:16), Madper Xie wrote: > >>> > >>> isimatu.yasu...@jp.fujitsu.com writes: > >>> > >>>> Hi Matt, > >>>> > >>>> Sorry for late the reply. > >>>> > >>>> > >>>> (2013/11/11 19:54), Matt Fleming wrote: > >>>>> On Mon, 11 Nov, at 05:52:59PM, Yasuaki Ishimatsu wrote: > >>>>>> Hi Matt, > >>>>>> > >>>>>> I uses FUJITSU's x86 box. > >>>>>> This does not become bricked even if I use all efi variable storage. > >>>>>> Thus I want a way to not need to specify efi_no_storage_paranoia > >>>>>> parameter. > >>>>> > >>>>> The efi_no_storage_paranoia parameter was introduced because some > >>>>> machines do not initiate garbage collection of the NVRAM until you > >>>>> allocate all space - basically it's a switch to turn off the "save 5KB > >>>>> of stoarge at all times" workaround that is needed to avoid bricking > >>>>> some machines. > >>>>> > >>>>> The intention of the switch is not to allow you to fill your NVRAM just > >>>>> because you can. If that is something you want to do then I think it's > >>>>> fair to require you to explicitly turn on efi_no_storage_paranoia. But > >>>>> I'm assuming here that you are doing something like writing lots and > >>>>> lots of pstore entries and just want to write as many as your variable > >>>>> storage will allow? Or are you doing something more fundamental like > >>>>> creating Boot entries? > >>>>> > >>>>> What are you doing to run into the 5KB reserve? How much NVRAM does your > >>>>> machine come with? > >>>> > >>>> I just add boot entry to NVRAM by efibootmgr command. But when Linux > >>>> boots up, > >>>> the remaining NVRAM is less than 5Kbyte. So I cannnot add new entry. > >>>> > >>> Howdy Yasuaki, > >>> If the remaining NVRAM is less than 5Kb, your writing will trigger a > >>> NVRAM storage reclamation. However you still failed creating entry. So > >>> I'm just curious what itmes occupy lots of nvram storage space. > >> > >> Even if we got EFI_OUT_OF_RESOURCES while running Linux, gc does not run. > >> Trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS environment > >> with > >> UEFI. So on my system, if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte > >> threshold, > >> we cannot use nvram storage until EFI_OUT_OF_RESOURCES occurs on pre OS > >> environment with UEFI. > >> > >> Thanks, > >> Yasuaki Ishimatsu > > > > Can we try to trigger gc by EFI_OUT_OF_RESOURCE in EFI stub kernel or > > EFI boot loader to recover NVRAM space? Does work with the BIOS on this > > machine? > > Yes. I can trigger gc by EFI_OUT_OF_RESOUCE in EFI shell on my machine. > > Thanks, > Yasuaki Ishimatu > OK, then maybe can try to trigger gc in EFI stub before ExitBootService(). Another problem is what's the reasonable threshold. The threshold should bigger then 5Kbyte, then we write a dummy BootTime NV variable to trigger gc. Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, efi: change name of efi_no_storage_paranoia parameter to efi_storage_paranoia
於 三，2013-11-20 於 15:26 +0900，Yasuaki Ishimatsu 提到： > (2013/11/19 12:16), Madper Xie wrote: > > > > isimatu.yasu...@jp.fujitsu.com writes: > > > >> Hi Matt, > >> > >> Sorry for late the reply. > >> > >> > >> (2013/11/11 19:54), Matt Fleming wrote: > >>> On Mon, 11 Nov, at 05:52:59PM, Yasuaki Ishimatsu wrote: > Hi Matt, > > I uses FUJITSU's x86 box. > This does not become bricked even if I use all efi variable storage. > Thus I want a way to not need to specify efi_no_storage_paranoia > parameter. > >>> > >>> The efi_no_storage_paranoia parameter was introduced because some > >>> machines do not initiate garbage collection of the NVRAM until you > >>> allocate all space - basically it's a switch to turn off the "save 5KB > >>> of stoarge at all times" workaround that is needed to avoid bricking > >>> some machines. > >>> > >>> The intention of the switch is not to allow you to fill your NVRAM just > >>> because you can. If that is something you want to do then I think it's > >>> fair to require you to explicitly turn on efi_no_storage_paranoia. But > >>> I'm assuming here that you are doing something like writing lots and > >>> lots of pstore entries and just want to write as many as your variable > >>> storage will allow? Or are you doing something more fundamental like > >>> creating Boot entries? > >>> > >>> What are you doing to run into the 5KB reserve? How much NVRAM does your > >>> machine come with? > >> > >> I just add boot entry to NVRAM by efibootmgr command. But when Linux boots > >> up, > >> the remaining NVRAM is less than 5Kbyte. So I cannnot add new entry. > >> > > Howdy Yasuaki, > >If the remaining NVRAM is less than 5Kb, your writing will trigger a > >NVRAM storage reclamation. However you still failed creating entry. So > >I'm just curious what itmes occupy lots of nvram storage space. > > Even if we got EFI_OUT_OF_RESOURCES while running Linux, gc does not run. > Trigger of gc is when EFI_OUT_OF_RESOURCES occurs on pre OS environment with > UEFI. So on my system, if EFI_OUT_OF_RESOURCES occurs by the 5Kbyte threshold, > we cannot use nvram storage until EFI_OUT_OF_RESOURCES occurs on pre OS > environment with UEFI. > > Thanks, > Yasuaki Ishimatsu Can we try to trigger gc by EFI_OUT_OF_RESOURCE in EFI stub kernel or EFI boot loader to recover NVRAM space? Does work with the BIOS on this machine? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 四，2013-09-26 於 14:22 +0200，Vojtech Pavlik 提到： > On Thu, Sep 26, 2013 at 02:06:21PM +0200, Pavel Machek wrote: > > > > For the symmetric key solution, I will try HMAC (Hash Message > > > Authentication Code). It's already used in networking, hope the > > > performance is not too bad to a big image. > > > > Kernel already supports crc32 of the hibernation image, you may want > > to take a look how that is done. > > > > Maybe you want to replace crc32 with cryptographics hash (sha1?) and > > then use only hash for more crypto? That way speed of whatever > crypto > > you do should not be an issue. > > Well, yes, one could skip the CRC when the signing is enabled to gain > a > little speedup. In current kernel, CRC is for check the integrity of LZO compressed image, the purpose is different to check the integrity of snapshot image. So, CRC will not in non-compress hibernate or userspace hibernate code path On the other hand, attacker can easily change the CRC code in the header of LZO hibernate image. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 四，2013-09-26 於 14:06 +0200，Pavel Machek 提到： > Hi! > > > For the symmetric key solution, I will try HMAC (Hash Message > > Authentication Code). It's already used in networking, hope the > > performance is not too bad to a big image. > > Kernel already supports crc32 of the hibernation image, you may want > to take a look how that is done. In current kernel design, The crc32 is only for the LZO in-kernel hibernate, doesn't apply to non-compress hibernate and userspace hibernate. Put signature to snapshot header can support any kind of caller that's trigger hibernate. Any userspace hibernate tool will take the snapshot image from kernel, so, we need put the signature(or hash result) to snapshot header before userspace write it to anywhere. > > Maybe you want to replace crc32 with cryptographics hash (sha1?) and > then use only hash for more crypto? That way speed of whatever crypto > you do should not be an issue. That speed of hash is calculated from non-compress snapshot image, does not overlap with crc32. > > Actually... > > Is not it as simple as storing hash of hibernation image into NVRAM > and then verifying the hash matches the value in NVRAM on next > startup? No encryption needed. > > And that may even be useful for non-secure-boot people, as it ensures > you boot right image after resume, boot it just once, etc... > > Pavel The HMAC approach will not encrypt, just put the key of HMAC to boottime variable. If user doesn't enable UEFI secure boot, that's fine, the key of HMAC still cannot access in OS runtime. If user enable UEFI secure boot, then that's better! Because all EFI file will signed by the manufacturers or OSVs to make sure the code is secure, will not pass the key to runtime. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 四，2013-09-26 於 10:19 +0800，joeyli 提到： > 於 三，2013-09-25 於 17:25 -0400，Alan Stern 提到： > > On Wed, 25 Sep 2013, David Howells wrote: > > > > > I have pushed some keyrings patches that will likely affect this to: > > > > > > > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-devel > > > > > > I intend to ask James to pull these into his next branch. If he's happy > > > to do > > > so, I can look at pulling at least your asymmetric keys patch on top of > > > them. > > > > This suggests a point that I raised at the Linux Plumbers conference: > > > > Why are asymmetric keys used for verifying the hibernation image? It > > seems that a symmetric key would work just as well. And it would be a > > lot quicker to generate, because it wouldn't need any high-precision > > integer computations. > > > > Alan Stern > > > > > > Per my understood, it's like add salt to snapshot when generate > signature, then remove the salt when store the snapshot to swap. (or > pass snapshot to userland). > > Let me explain the symmetric key solution base on my understand: > > + EFI stub kernel generate a hash value from a random seed, then store > it to EFi boot varaible. It should protected by UEFI secure boot > environment. > > + When hibernate launched: > - Kernel create the snapshot image of memory. It's included the > random hash value(salt) that generated in EFI stub stage. > - Then kernel hash the snapshot image, put the hash to snapshot > header, just like current asymmetric keys solution. > - Kernel erase the salt in snapshot image before it go to swap or > pass to userspace tool. > > + When hibernate resume: > - Kernel or userspace tool load the snapshot(without salt) from swap > to temporary memory space. > - Kernel fill the salt back to snapshot image in memory, hash it. > - Kernel compare the hash with the hash that put in snapshot header. > - Verification done! The follow-up action as current solution. > > Please current me if I missed anything. > > > Thanks a lot! > Joey Lee > For the symmetric key solution, I will try HMAC (Hash Message Authentication Code). It's already used in networking, hope the performance is not too bad to a big image. Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
Hi Phil,
First! Thanks for your time to review my patch!
於 一，2013-09-23 於 19:49 +0300，Phil Carmody 提到：
> On Sun, Sep 15, 2013 at 08:56:48AM +0800, Lee, Chun-Yi wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when
> > look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference:
> > http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
>
> You're now at V4.
The V4 is for whole patchset, I didn't do any modify in this patch in
this version.
The version define maybe confuse between separate and whole patchset, I
will avoid it.
>
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
> > ---
> > crypto/asymmetric_keys/rsa.c | 163
> > +-
> > include/crypto/public_key.h |2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding
> > operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow
> > kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
> > + ret = -ENOTSUPP;
>
> ...
>
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value
> > H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
>
> The earlier "ret = -ENOTSUPP;" is either unused because you return at the
> IS_ERR,
> or unused because you overwrite it here. I'm a little disappointed that
> the compiler didn't recognise that something was assigned to a value that
> is never used.
>
> Phil
Yes, Dmitry also pointed out this issue, I should not go on the hash
process if the hash algorithm didn't support.
I will change fix this problem in next version.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 四，2013-09-26 於 02:27 +0200，Pavel Machek 提到： > On Wed 2013-09-25 15:16:54, James Bottomley wrote: > > On Wed, 2013-09-25 at 17:25 -0400, Alan Stern wrote: > > > On Wed, 25 Sep 2013, David Howells wrote: > > > > > > > I have pushed some keyrings patches that will likely affect this to: > > > > > > > > > > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-devel > > > > > > > > I intend to ask James to pull these into his next branch. If he's > > > > happy to do > > > > so, I can look at pulling at least your asymmetric keys patch on top of > > > > them. > > > > > > This suggests a point that I raised at the Linux Plumbers conference: > > > > > > Why are asymmetric keys used for verifying the hibernation image? It > > > seems that a symmetric key would work just as well. And it would be a > > > lot quicker to generate, because it wouldn't need any high-precision > > > integer computations. > > > > The reason is the desire to validate that the previous kernel created > > something which it passed on to the current kernel (in this case, the > > hibernation image) untampered with. To do that, something must be > > passed to the prior kernel that can be validated but *not* recreated by > > the current kernel. > > I don't get this. Why is it important that current kernel can't > recreate the signature? > > Current kernel is not considered malicious (if it were, you have worse > problems). > Current boot kernel should not malicious especially when UEFI secure boot enabled. > Pavel > > PS: And yes, it would be nice to have > Documentation/power/swsusp-uefi.txt (or something) explaining the > design. > Thanks for your suggestion, I will write the swsusp-uefi.txt to explaining the design in next version. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 三，2013-09-25 於 17:25 -0400，Alan Stern 提到： > On Wed, 25 Sep 2013, David Howells wrote: > > > I have pushed some keyrings patches that will likely affect this to: > > > > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-devel > > > > I intend to ask James to pull these into his next branch. If he's happy to > > do > > so, I can look at pulling at least your asymmetric keys patch on top of > > them. > > This suggests a point that I raised at the Linux Plumbers conference: > > Why are asymmetric keys used for verifying the hibernation image? It > seems that a symmetric key would work just as well. And it would be a > lot quicker to generate, because it wouldn't need any high-precision > integer computations. > > Alan Stern > > Per my understood, it's like add salt to snapshot when generate signature, then remove the salt when store the snapshot to swap. (or pass snapshot to userland). Let me explain the symmetric key solution base on my understand: + EFI stub kernel generate a hash value from a random seed, then store it to EFi boot varaible. It should protected by UEFI secure boot environment. + When hibernate launched: - Kernel create the snapshot image of memory. It's included the random hash value(salt) that generated in EFI stub stage. - Then kernel hash the snapshot image, put the hash to snapshot header, just like current asymmetric keys solution. - Kernel erase the salt in snapshot image before it go to swap or pass to userspace tool. + When hibernate resume: - Kernel or userspace tool load the snapshot(without salt) from swap to temporary memory space. - Kernel fill the salt back to snapshot image in memory, hash it. - Kernel compare the hash with the hash that put in snapshot header. - Verification done! The follow-up action as current solution. Please current me if I missed anything. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4 13/15] Hibernate: introduced SNAPSHOT_SIG_HASH config for select hash algorithm
於 三，2013-09-18 於 15:45 +0200，Pavel Machek 提到： > On Sun 2013-09-15 08:56:59, Lee, Chun-Yi wrote: > > This patch introduced SNAPSHOT_SIG_HASH config for user to select which > > hash algorithm will be used during signature generation of snapshot. > > This series is big enough already... and who is going to test it? The hash config not just for testing, it's relate to the performance and secure between different hash algorithms. There have person raised in LPC say he don't like SHA algorithm. > There's no need to make hash configurable. Just select one that works. > > Pavel > SHA1 has good performance, and SHA512 has better security, which one you like it? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC V4 PATCH 00/15] Signature verification of hibernate snapshot
於 三，2013-09-25 於 22:04 +0100，David Howells 提到： > I have pushed some keyrings patches that will likely affect this to: > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-devel > Thanks for your point out, I will respin my asymmetric keys patch base on this tree. > I intend to ask James to pull these into his next branch. If he's happy to do > so, I can look at pulling at least your asymmetric keys patch on top of them. > > It'd be helpful if you could see if you need to make any updates. > > David > In LPC, Alan and Vojtech raised another thinking is using symmetric key to protect the hash of snapshot. It's simpler then using RSA private key to sign it. Even finally we use the symmetric key solution, I will still respin and resent the patch for add the leading zero byte: [PATCH V4 07/15] asymmetric keys: explicitly add the leading zero byte to encoded message I think keys-devel tree need it. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] X.509: Remove validity check of certificate date
於 三，2013-09-25 於 11:03 +0200，Alexander Holler 提到： > (I've resend this message, because the one I've replied to contained > contained kernel@vger... and not linux-kernel@vger... as addressee) > > I've already sent exactly the same patch here: > > http://lkml.org/lkml/2013/3/27/449 > > and here: > > https://lkml.org/lkml/2013/6/6/207 Ah! Thanks for your point out, I didn't search out it! > > but for some unspoken reason it always got ignored. Yes, looks your patch is waiting for merge... > > It would make wonder if it would be accepted this time, just because of > another author name. I think just ignore my patch. > > Regards, > > Alexander Holler Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when
> > look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference:
> > http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
> > ---
> > crypto/asymmetric_keys/rsa.c | 163
> > +-
> > include/crypto/public_key.h |2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding
> > operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow
> > kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash
> > value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hi
Re: [PATCH 00/12] One more attempt at useful kernel lockdown
於 二，2013-09-10 於 18:26 +，Matthew Garrett 提到：
> On Tue, 2013-09-10 at 14:23 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 10 Sep 2013, Matthew Garrett wrote:
> > > That's why modern systems require signed firmware updates.
> >
> > Linux doesn't. Is someone working on adding signature support to the
> > runtime firmware loader?
>
> It'd be simple to do so, but so far the model appears to be that devices
> that expect signed firmware enforce that themselves.
>
> --
> Matthew Garrett
> NrybXǧv^){.n+{y^nrzh& Gh(階ݢj"mzޖfh~m
Takashi has a implementation of firmware check:
[PATCH RFC v2 0/4] Add firmware signature file check
https://lkml.org/lkml/2012/11/8/343
Thanks
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到： > On Thu, 05 Sep, at 06:13:36PM, joeyli wrote: > > This S4WakeKey is a VOLATILE variable that could not modify by > > SetVariable() at runtime. So, it's read only even through efivars. > > > > Does it what your concern? > > No, the UEFI spec probibits certain runtime functions from being > executed concurrently on separate cpus and the spinlock used in the > efivars code ensures that we adhere to that restriction. See table 31 in > section 7.1 of the UEFI 2.4 spec for the list of services that are > non-reentrant. > > The problem isn't that we want to avoid simultaneous access to > S4WakeKey, it's that we can't invoke any of the variable runtime service > functions concurrently. > I see! I will use efivar api to access EFI variable. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
Hi Matt,
First, thanks for your review!
於 四，2013-09-05 於 09:53 +0100，Matt Fleming 提到：
> On Thu, 22 Aug, at 07:01:50PM, Lee, Chun-Yi wrote:
> > +static int efi_status_to_err(efi_status_t status)
> > +{
> > + int err;
> > +
> > + switch (status) {
> > + case EFI_INVALID_PARAMETER:
> > + err = -EINVAL;
> > + break;
> > + case EFI_OUT_OF_RESOURCES:
> > + err = -ENOSPC;
> > + break;
> > + case EFI_DEVICE_ERROR:
> > + err = -EIO;
> > + break;
> > + case EFI_WRITE_PROTECTED:
> > + err = -EROFS;
> > + break;
> > + case EFI_SECURITY_VIOLATION:
> > + err = -EACCES;
> > + break;
> > + case EFI_NOT_FOUND:
> > + err = -ENODATA;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
> Please don't reimplement this. Instead make the existing function
> global.
>
OK, I will make the function to global.
> [...]
>
> > +static void *load_wake_key_data(unsigned long *datasize)
> > +{
> > + u32 attr;
> > + void *wkey_data;
> > + efi_status_t status;
> > +
> > + if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > + return ERR_PTR(-EPERM);
> > +
> > + /* obtain the size */
> > + *datasize = 0;
> > + status = efi.get_variable(EFI_S4_WAKE_KEY_NAME, &EFI_HIBERNATE_GUID,
> > + NULL, datasize, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL) {
> > + wkey_data = ERR_PTR(efi_status_to_err(status));
> > + pr_err("PM: Couldn't get wake key data size: 0x%lx\n", status);
> > + goto error;
> > + }
>
> Is it safe to completely bypass the efivars interface and access
> efi.get_variable() directly? I wouldn't have thought so, unless you can
> guarantee that the kernel isn't going to access any of the EFI runtime
> services while you execute this function.
>
This S4WakeKey is a VOLATILE variable that could not modify by
SetVariable() at runtime. So, it's read only even through efivars.
Does it what your concern?
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode
於 二，2013-09-03 於 19:50 -0400，Matthew Garrett 提到： > UEFI Secure Boot provides a mechanism for ensuring that the firmware will > only load signed bootloaders and kernels. Certain use cases may also > require that all kernel modules also be signed. Add a configuration option > that enforces this automatically when enabled. > > Signed-off-by: Matthew Garrett > --- > Documentation/x86/zero-page.txt | 2 ++ > arch/x86/Kconfig | 10 ++ > arch/x86/boot/compressed/eboot.c | 36 > +++ > arch/x86/include/uapi/asm/bootparam.h | 3 ++- > arch/x86/kernel/setup.c | 6 ++ > include/linux/module.h| 6 ++ > kernel/module.c | 7 +++ > 7 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt > index 199f453..ec38acf 100644 > --- a/Documentation/x86/zero-page.txt > +++ b/Documentation/x86/zero-page.txt > @@ -30,6 +30,8 @@ Offset Proto NameMeaning > 1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below) > 1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in > edd_mbr_sig_buffer > (below) > +1EB/001 ALL kbd_status Numlock is enabled > +1EC/001 ALL secure_boot Secure boot is enabled in the firmware > 1EF/001 ALL sentinelUsed to detect broken bootloaders > 290/040 ALL edd_mbr_sig_buffer EDD MBR signatures > 2D0/A00 ALL e820_mapE820 memory map table > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index b32ebf9..6a6c19b 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -1581,6 +1581,16 @@ config EFI_STUB > > See Documentation/x86/efi-stub.txt for more information. > > +config EFI_SECURE_BOOT_SIG_ENFORCE > +def_bool n Maybe need add "select MODULE_SIG" to here for auto enable kernel module signature check when user select this option? > + prompt "Force module signing when UEFI Secure Boot is enabled" > + ---help--- > + UEFI Secure Boot provides a mechanism for ensuring that the > + firmware will only load signed bootloaders and kernels. Certain > + use cases may also require that all kernel modules also be signed. > + Say Y here to automatically enable module signature enforcement > + when a system boots with UEFI Secure Boot enabled. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 09/11] uswsusp: Disable when module loading is restricted
於 二，2013-09-03 於 19:50 -0400，Matthew Garrett 提到：
> uswsusp allows a user process to dump and then restore kernel state, which
> makes it possible to avoid module loading restrictions. Prevent this when
> any restrictions have been imposed on loading modules.
>
> Signed-off-by: Matthew Garrett
Tested-by: Lee, Chun-Yi
> ---
> kernel/power/user.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 4ed81e7..15cb72f 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -48,6 +49,9 @@ static int snapshot_open(struct inode *inode, struct file
> *filp)
> struct snapshot_data *data;
> int error;
>
> + if (secure_modules())
> + return -EPERM;
> +
> lock_system_sleep();
>
> if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
Thanks
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 11/11] Add option to automatically enforce module signatures when in Secure Boot mode
於 二，2013-09-03 於 19:50 -0400，Matthew Garrett 提到：
> UEFI Secure Boot provides a mechanism for ensuring that the firmware will
> only load signed bootloaders and kernels. Certain use cases may also
> require that all kernel modules also be signed. Add a configuration option
> that enforces this automatically when enabled.
>
> Signed-off-by: Matthew Garrett
Tested-by: Lee, Chun-Yi
Thanks
Joey Lee
> ---
> Documentation/x86/zero-page.txt | 2 ++
> arch/x86/Kconfig | 10 ++
> arch/x86/boot/compressed/eboot.c | 36
> +++
> arch/x86/include/uapi/asm/bootparam.h | 3 ++-
> arch/x86/kernel/setup.c | 6 ++
> include/linux/module.h| 6 ++
> kernel/module.c | 7 +++
> 7 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
> index 199f453..ec38acf 100644
> --- a/Documentation/x86/zero-page.txt
> +++ b/Documentation/x86/zero-page.txt
> @@ -30,6 +30,8 @@ Offset Proto NameMeaning
> 1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below)
> 1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in
> edd_mbr_sig_buffer
> (below)
> +1EB/001 ALL kbd_status Numlock is enabled
> +1EC/001 ALL secure_boot Secure boot is enabled in the firmware
> 1EF/001 ALL sentinelUsed to detect broken bootloaders
> 290/040 ALL edd_mbr_sig_buffer EDD MBR signatures
> 2D0/A00 ALL e820_mapE820 memory map table
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b32ebf9..6a6c19b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1581,6 +1581,16 @@ config EFI_STUB
>
> See Documentation/x86/efi-stub.txt for more information.
>
> +config EFI_SECURE_BOOT_SIG_ENFORCE
> +def_bool n
> + prompt "Force module signing when UEFI Secure Boot is enabled"
> + ---help---
> + UEFI Secure Boot provides a mechanism for ensuring that the
> + firmware will only load signed bootloaders and kernels. Certain
> + use cases may also require that all kernel modules also be signed.
> + Say Y here to automatically enable module signature enforcement
> + when a system boots with UEFI Secure Boot enabled.
> +
> config SECCOMP
> def_bool y
> prompt "Enable seccomp to safely compute untrusted bytecode"
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index b7388a4..53bfe4f 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #undef memcpy/* Use memcpy from misc.c */
>
> @@ -861,6 +862,37 @@ fail:
> return status;
> }
>
> +static int get_secure_boot(void)
> +{
> + u8 sb, setup;
> + unsigned long datasize = sizeof(sb);
> + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> + efi_status_t status;
> +
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + L"SecureBoot", &var_guid, NULL, &datasize, &sb);
> +
> + if (status != EFI_SUCCESS)
> + return 0;
> +
> + if (sb == 0)
> + return 0;
> +
> +
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + L"SetupMode", &var_guid, NULL, &datasize,
> + &setup);
> +
> + if (status != EFI_SUCCESS)
> + return 0;
> +
> + if (setup == 1)
> + return 0;
> +
> + return 1;
> +}
> +
> +
> /*
> * Because the x86 boot code expects to be passed a boot_params we
> * need to create one ourselves (usually the bootloader would create
> @@ -1169,6 +1201,10 @@ struct boot_params *efi_main(void *handle,
> efi_system_table_t *_table,
> if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> goto fail;
>
> + sanitize_boot_params(boot_params);
> +
> + boot_params->secure_boot = get_secure_boot();
> +
> setup_graphics(boot_params);
>
> setup_efi_pci(boot_params);
> diff --git a/arch/x86/include/uapi/asm/bootparam.h
> b/arch/x86/include/uapi/asm/bootparam.h
> index c15ddaf..85d7685 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -131,7 +131,8 @@ struct boot_params {
> __u8 eddbuf_entries; /* 0x1e9 */
> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */
> __u8 kbd_status; /* 0x1eb */
> - __u8 _pad5[3]; /* 0x1ec */
> + __u8 secure_boot; /* 0x1ec */
> + __u8 _pad5[2]; /* 0x1ed */
> /*
>* The sentinel is set to a nonzero va
Re: [PATCH V3 01/11] Add secure_modules() call
於 二，2013-09-03 於 19:50 -0400，Matthew Garrett 提到：
> Provide a single call to allow kernel code to determine whether the system
> has been configured to either disable module loading entirely or to load
> only modules signed with a trusted key.
>
> Signed-off-by: Matthew Garrett
Tested-by: Lee, Chun-Yi
> ---
> include/linux/module.h | 7 +++
> kernel/module.c| 10 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 46f1ea0..0c266b2 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -509,6 +509,8 @@ int unregister_module_notifier(struct notifier_block *
> nb);
>
> extern void print_modules(void);
>
> +extern bool secure_modules(void);
> +
> #else /* !CONFIG_MODULES... */
>
> /* Given an address, look for it in the exception tables. */
> @@ -619,6 +621,11 @@ static inline int unregister_module_notifier(struct
> notifier_block * nb)
> static inline void print_modules(void)
> {
> }
> +
> +static inline bool secure_modules(void)
> +{
> + return false;
> +}
> #endif /* CONFIG_MODULES */
>
> #ifdef CONFIG_SYSFS
> diff --git a/kernel/module.c b/kernel/module.c
> index 2069158..0e94acf 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3852,3 +3852,13 @@ void module_layout(struct module *mod,
> }
> EXPORT_SYMBOL(module_layout);
> #endif
> +
> +bool secure_modules(void)
> +{
> +#ifdef CONFIG_MODULE_SIG
> + return (sig_enforce || modules_disabled);
> +#else
> + return modules_disabled;
> +#endif
> +}
> +EXPORT_SYMBOL(secure_modules);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode
於 三，2013-09-04 於 08:01 -0400，Josh Boyer 提到：
> On Wed, Sep 4, 2013 at 6:51 AM, joeyli wrote:
> > 於 五，2013-08-30 於 19:41 -0400，Josh Boyer 提到：
> >> On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
> >> > On 08/29/2013 11:37 AM, Josh Boyer wrote:
> >> > >> setup_efi_pci(boot_params);
> >> > >> diff --git a/arch/x86/include/uapi/asm/bootparam.h
> >> > >> b/arch/x86/include/uapi/asm/bootparam.h
> >> > >> index c15ddaf..d35da96 100644
> >> > >> --- a/arch/x86/include/uapi/asm/bootparam.h
> >> > >> +++ b/arch/x86/include/uapi/asm/bootparam.h
> >> > >> @@ -131,7 +131,8 @@ struct boot_params {
> >> > >> __u8 eddbuf_entries; /* 0x1e9 */
> >> > >> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */
> >> > >> __u8 kbd_status; /* 0x1eb */
> >> > >> -__u8 _pad5[3]; /* 0x1ec */
> >> > >> +__u8 secure_boot; /* 0x1ec */
> >> > >> +__u8 _pad5[2]; /* 0x1ec */
> >> > >> /*
> >> > >> * The sentinel is set to a nonzero value (0xff) in header.S.
> >> > >> *
> >> > >
> >> > > You need to include the following chunk of code with this, otherwise
> >> > > the
> >> > > secure_boot variable gets cleared.
> >> > >
> >> >
> >> > Not really.
> >> >
> >> > There are three cases:
> >> >
> >> > 1. Boot stub only. Here we do the right thing with the bootparams.
> >> > 2. Boot loader bypasses the boot stub completely. Here we MUST NOT do
> >> >what you suggest above.
> >> > 3. Boot stub with a boot_params structure passed in. Here we should
> >> >run sanitize_boot_params() (an inline for a reason) in the boot
> >> >stub, before we set the secure_boot field. Once that is done, we
> >> >again don't need that modification.
> >>
> >> OK. If 3 works, then great. All I know is that Fedora has been
> >> carrying the above hunk for months and it was missing in this patch set.
> >> So when I went to test it, the patches didn't do anything because the
> >> secure_boot field was getting cleared.
> >>
> >> I'm more than happy to try option 3, and I'll poke at it next week
> >> unless someone beats me to it.
> >>
> >> josh
> >
> > The secure_boot field cleaned by sanitize_boot_params() when using grub2
> > linuxefi to load efi stub kernel.
> > I printed the boot_params->sentinel value, confirm this value is NOT 0
> > when running grub2 linuxefi path, the entry point is efi_stub_entry.
> >
> > On the other hand,
> > the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
> > the secure_boot field can keep.
> >
> > Does that mean grub2 should clean the sentinel value? or we move the get
> > secure_boot value to efi_init()?
>
> See V3 of this patch that Matthew sent yesterday. It calls
> sanitize_boot_params in efi_main before calling get_secure_boot. I
> tested that yesterday and it worked fine.
>
> josh
Ah!
Thanks for you point out, I missed his v3 patch.
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode
於 五，2013-08-30 於 19:41 -0400，Josh Boyer 提到：
> On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
> > On 08/29/2013 11:37 AM, Josh Boyer wrote:
> > >> setup_efi_pci(boot_params);
> > >> diff --git a/arch/x86/include/uapi/asm/bootparam.h
> > >> b/arch/x86/include/uapi/asm/bootparam.h
> > >> index c15ddaf..d35da96 100644
> > >> --- a/arch/x86/include/uapi/asm/bootparam.h
> > >> +++ b/arch/x86/include/uapi/asm/bootparam.h
> > >> @@ -131,7 +131,8 @@ struct boot_params {
> > >> __u8 eddbuf_entries; /* 0x1e9 */
> > >> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */
> > >> __u8 kbd_status; /* 0x1eb */
> > >> -__u8 _pad5[3]; /* 0x1ec */
> > >> +__u8 secure_boot; /* 0x1ec */
> > >> +__u8 _pad5[2]; /* 0x1ec */
> > >> /*
> > >> * The sentinel is set to a nonzero value (0xff) in header.S.
> > >> *
> > >
> > > You need to include the following chunk of code with this, otherwise the
> > > secure_boot variable gets cleared.
> > >
> >
> > Not really.
> >
> > There are three cases:
> >
> > 1. Boot stub only. Here we do the right thing with the bootparams.
> > 2. Boot loader bypasses the boot stub completely. Here we MUST NOT do
> >what you suggest above.
> > 3. Boot stub with a boot_params structure passed in. Here we should
> >run sanitize_boot_params() (an inline for a reason) in the boot
> >stub, before we set the secure_boot field. Once that is done, we
> >again don't need that modification.
>
> OK. If 3 works, then great. All I know is that Fedora has been
> carrying the above hunk for months and it was missing in this patch set.
> So when I went to test it, the patches didn't do anything because the
> secure_boot field was getting cleared.
>
> I'm more than happy to try option 3, and I'll poke at it next week
> unless someone beats me to it.
>
> josh
The secure_boot field cleaned by sanitize_boot_params() when using grub2
linuxefi to load efi stub kernel.
I printed the boot_params->sentinel value, confirm this value is NOT 0
when running grub2 linuxefi path, the entry point is efi_stub_entry.
On the other hand,
the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
the secure_boot field can keep.
Does that mean grub2 should clean the sentinel value? or we move the get
secure_boot value to efi_init()?
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/10] Add additional security checks when module loading is restricted
於 三，2013-08-28 於 16:07 -0700，Kees Cook 提到： > On Wed, Aug 28, 2013 at 3:58 PM, Lenny Szubowicz wrote: > > > > > > - Original Message - > >> From: "Matthew Garrett" > >> To: "Lenny Szubowicz" > >> Cc: linux-kernel@vger.kernel.org, linux-...@vger.kernel.org, > >> jwbo...@redhat.com, keesc...@chromium.org > >> Sent: Wednesday, August 28, 2013 6:41:55 PM > >> Subject: Re: [PATCH 0/10] Add additional security checks when module > >> loading is restricted > >> > >> On Wed, 2013-08-28 at 18:37 -0400, Lenny Szubowicz wrote: > >> > >> > Did you purposely exclude similar checks for hibernate that were covered > >> > by earlier versions of your patch set? > >> > >> Yes, I think it's worth tying it in with the encrypted hibernation > >> support. The local attack is significantly harder in the hibernation > >> case - in the face of unknown hardware it basically involves a > >> pre-generated memory image corresponding to your system or the ability > >> to force a reboot into an untrusted environment. I think it's probably > >> more workable to just add a configuration option for forcing encrypted > >> hibernation when secure boot is in use. > >> > >> -- > >> Matthew Garrett > > > > I'm root. So I can write anything I want to the swap file that looks > > like a valid hibernate image but is code of my choosing. I can read > > anything I need from /dev/mem or /dev/kmem to help me do that. > > I can then immediately initiate a reboot. > > Strictly speaking, RAM contents are not available via /dev/*mem, even > to root. However, you can request a suspend image be written, but to > not enter hibernation. Then modify the image, and request a resume > from it. > > -Kees > I agreed! As a userland hibernate tool, it possible trigger kernel to generate a snapshot image of current memory, read the snapshot, modify and upload it back to the temporary memory space of snapshot, trigger S4 resume to restore it. The signature check of S4 snapshot can prevent this attack because the patches put the signature of snapshot image to snapshot header. Even attacker change the signature of header or modified the data page in snapshot. The modified snapshot image will not pass by signature check. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/18 v3] Signature verification of hibernate snapshot
於 日，2013-09-01 於 18:40 +0200，Florian Weimer 提到： > * Matthew Garrett: > > > On Sun, Sep 01, 2013 at 12:41:22PM +0200, Florian Weimer wrote: > > > >> But if you don't generate fresh keys on every boot, the persistent > >> keys are mor exposed to other UEFI applications. Correct me if I'm > >> wrong, but I don't think UEFI variables are segregated between > >> different UEFI applications, so if anyone gets a generic UEFI variable > >> dumper (or setter) signed by the trusted key, this cryptographic > >> validation of hibernate snapshots is bypassable. > > > > If anyone can execute arbitrary code in your UEFI environment then > > you've already lost. > > This is not about arbitrary code execution. The problematic > applications which conflict with this proposed functionality are not > necessarily malicious by themselves and even potentially useful. > > For example, if you want to provision a bunch of machines and you have > to set certain UEFI variables, it might be helpful to do so in an > unattended fashion, just by booting from a USB stick with a suitable > UEFI application. Is this evil? I don't think so. > -- Yes, if there have the kind of UEFI tools like you said, and it leak to attacker, then we lost. Even we re-generate key-pair for every S4, the tool, if it can set variable, means it can replace the public key that was stored by efi bootloader in bootservices variable. Then we still lost. When the tool can only dump variable but not set, then re-generate key-pair to every S4 can prevent it. If the tool can also set variable, then I don't think there have any way to protect key-pair in UEFI variables. Using TPM is a way to protect key-pair, but user need key-in password when generate and use key to sign stuff. It noises to user, but the best way to keep the password is in brain. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/18 v3] Signature verification of hibernate snapshot
於 四，2013-08-29 於 23:32 +0200，Pavel Machek 提到： > Hi! > > > > >- Bootloader store the public key to EFI boottime variable by itself > > > >- Bootloader put The private key to S4SignKey EFI variable for > > > > forward to > > > > kernel. > > > > > > Is the UEFI NVRAM really suited for such regular updates? > > > > > > > Yes, Matthew raised this concern at before. I modified patch to load > > private key in efi stub kernel, before ExitBootServices(), that means we > > don't need generate key-pair at every system boot. So, the above > > procedure of efi bootloader will only run one time. > > > > User can enable SNAPSHOT_REGEN_KEYS kernel config to notify efi > > booloader regenerate key-pair for every S4 to improve security if he > > want. So, the key-pair re-generate procedure will only launched when S4 > > resume, not every system boot. > > How many writes can UEFI NVRAM survive? (Is it NOR?) Currently doesn't have enough information for normal. Yes, I don't know. > > "every S4 resume" may be approximately "every boot" for some users... > Pavel Yes, it's possible. So, this option will be disabled by default. Default will only generate one key-pair for every hibernate. If "re-generate key-pair for every S4" is still hurt lift of UEFI NVRAM too much, then another thinking for re-generate key-pair are: + Re-generate key-pair after a number of hibernates. e.g. after 5, 10, 20... times or + Random re-generate key-pair? On the other hand... In current design, GenS4Key EFI variable could be write by userland hibernate tool, kernel will respect GenS4Key value from userland when hibernate launch. So, userland can tell bootloader to lunch the key-pair regeneration procedure. Thanks a lot! Joey LEe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 00/18 v3] Signature verification of hibernate snapshot
Hi Florian, Thanks for your response. 於 三，2013-08-28 於 23:01 +0200，Florian Weimer 提到： > * Chun-Yi Lee: > > > + EFI bootloader must generate RSA key-pair when system boot: I should add more information on this sentence for mention need GenS4Key runtime variable then re-generate key-pair. Thanks! > >- Bootloader store the public key to EFI boottime variable by itself > >- Bootloader put The private key to S4SignKey EFI variable for forward to > > kernel. > > Is the UEFI NVRAM really suited for such regular updates? > Yes, Matthew raised this concern at before. I modified patch to load private key in efi stub kernel, before ExitBootServices(), that means we don't need generate key-pair at every system boot. So, the above procedure of efi bootloader will only run one time. User can enable SNAPSHOT_REGEN_KEYS kernel config to notify efi booloader regenerate key-pair for every S4 to improve security if he want. So, the key-pair re-generate procedure will only launched when S4 resume, not every system boot. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
於 二，2013-08-27 於 13:29 +0200，Pavel Machek 提到： > > > > @@ -1205,6 +1290,10 @@ struct boot_params *efi_main(void *handle, > > > > efi_system_table_t *_table, > > > > > > > > setup_efi_pci(boot_params); > > > > > > > > +#ifdef CONFIG_SNAPSHOT_VERIFICATION > > > > + setup_s4_keys(boot_params); > > > > +#endif > > > > + > > > > > > Move ifdef inside the function? > > > > OK, I will define a dummy function for non-verification situation. > > IIRC you can just put the #ifdef inside the function body. > Pavel I want use inline dummy function like saveable_highmem_page() in snapshot.c, maybe it's better then additional function call? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 17/18] Hibernate: introduced SNAPSHOT_SIG_HASH config for select hash algorithm
於 二，2013-08-27 於 13:30 +0200，Pavel Machek 提到： > On Tue 2013-08-27 18:22:17, joeyli wrote: > > 於 日，2013-08-25 於 18:43 +0200，Pavel Machek 提到： > > > On Thu 2013-08-22 19:01:56, Lee, Chun-Yi wrote: > > > > This patch introduced SNAPSHOT_SIG_HASH config for user to select which > > > > hash algorithm will be used during signature generation of snapshot. > > > > > > > > v2: > > > > Add define check of oCONFIG_SNAPSHOT_VERIFICATION in snapshot.c before > > > > declare pkey_hash(). > > > > > > > > Reviewed-by: Jiri Kosina > > > > Signed-off-by: Lee, Chun-Yi > > > > --- > > > > kernel/power/Kconfig| 46 > > > > ++ > > > > kernel/power/snapshot.c | 27 ++- > > > > 2 files changed, 68 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > > > > index b592d88..79b34fa 100644 > > > > --- a/kernel/power/Kconfig > > > > +++ b/kernel/power/Kconfig > > > > @@ -78,6 +78,52 @@ config SNAPSHOT_VERIFICATION > > > > dependent on UEFI environment. EFI bootloader should generate > > > > the > > > > key-pair. > > > > > > > > +choice > > > > + prompt "Which hash algorithm should snapshot be signed with?" > > > > +depends on SNAPSHOT_VERIFICATION > > > > +help > > > > + This determines which sort of hashing algorithm will be used > > > > during > > > > + signature generation of snapshot. This algorithm _must_ be > > > > built into > > > > + the kernel directly so that signature verification can take > > > > place. > > > > + It is not possible to load a signed snapshot containing the > > > > algorithm > > > > + to check the signature on that module. > > > > > > Like if 1000 ifdefs you already added to the code are not enough, you > > > make some new ones? > > > Pavel > > > > > > > This SNAPSHOT_SIG_HASH kernel config is to select which SHA algorithms > > used for generate digest of snapshot. The configuration will captured by > > a const char* in code: > > > > +static const char *snapshot_hash = CONFIG_SNAPSHOT_SIG_HASH; > > + > > +static int pkey_hash(void) > > > > So, there doesn't have any ifdef block derived from this new config. > > I'd say select one hash function, and use it. There's no need to make > it configurable. > Pavel There have better performance when SHA algorithm output shorter hash result. On the other hand, longer hash result provide better security. And, on 64-bits system, the SHA512 has better performance then SHA256. Due to user have different use case and different hardware, why not give them this option to make decision? Thanks a lot! Joey LEe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 17/18] Hibernate: introduced SNAPSHOT_SIG_HASH config for select hash algorithm
於 日，2013-08-25 於 18:43 +0200，Pavel Machek 提到： > On Thu 2013-08-22 19:01:56, Lee, Chun-Yi wrote: > > This patch introduced SNAPSHOT_SIG_HASH config for user to select which > > hash algorithm will be used during signature generation of snapshot. > > > > v2: > > Add define check of oCONFIG_SNAPSHOT_VERIFICATION in snapshot.c before > > declare pkey_hash(). > > > > Reviewed-by: Jiri Kosina > > Signed-off-by: Lee, Chun-Yi > > --- > > kernel/power/Kconfig| 46 > > ++ > > kernel/power/snapshot.c | 27 ++- > > 2 files changed, 68 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > > index b592d88..79b34fa 100644 > > --- a/kernel/power/Kconfig > > +++ b/kernel/power/Kconfig > > @@ -78,6 +78,52 @@ config SNAPSHOT_VERIFICATION > > dependent on UEFI environment. EFI bootloader should generate the > > key-pair. > > > > +choice > > + prompt "Which hash algorithm should snapshot be signed with?" > > +depends on SNAPSHOT_VERIFICATION > > +help > > + This determines which sort of hashing algorithm will be used > > during > > + signature generation of snapshot. This algorithm _must_ be built > > into > > + the kernel directly so that signature verification can take place. > > + It is not possible to load a signed snapshot containing the algorithm > > + to check the signature on that module. > > Like if 1000 ifdefs you already added to the code are not enough, you > make some new ones? > Pavel > This SNAPSHOT_SIG_HASH kernel config is to select which SHA algorithms used for generate digest of snapshot. The configuration will captured by a const char* in code: +static const char *snapshot_hash = CONFIG_SNAPSHOT_SIG_HASH; + +static int pkey_hash(void) So, there doesn't have any ifdef block derived from this new config. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 15/18] Hibernate: adapt to UEFI secure boot with signature check
於 日，2013-08-25 於 18:42 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:54, Lee, Chun-Yi wrote:
> > In current solution, the snapshot signature check used the RSA key-pair
> > that are generated by bootloader(e.g. shim) and pass the key-pair to
> > kernel through EFI variables. I choice to binding the snapshot
> > signature check mechanism with UEFI secure boot for provide stronger
> > protection of hibernate. Current behavior is following:
> >
> > + UEFI Secure Boot ON, Kernel found key-pair from shim:
> >Will do the S4 signature check.
> >
> > + UEFI Secure Boot ON, Kernel didn't find key-pair from shim:
> >Will lock down S4 function.
> >
> > + UEFI Secure Boot OFF
> >Will NOT do the S4 signature check.
> >Ignore any keys from bootloader.
> >
> > v2:
> > Replace sign_key_data_loaded() by skey_data_available() to check sign key
> > data
> > is available for hibernate.
> >
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
> > ---
> > kernel/power/hibernate.c | 36 +-
> > kernel/power/main.c | 11 +-
> > kernel/power/snapshot.c | 95
> > ++
> > kernel/power/swap.c |4 +-
> > kernel/power/user.c | 11 +
> > 5 files changed, 112 insertions(+), 45 deletions(-)
> >
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index c545b15..0f19f3d 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -29,6 +29,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "power.h"
> >
> > @@ -632,7 +633,14 @@ static void power_down(void)
> > int hibernate(void)
> > {
> > int error;
> > - int skey_error;
> > +
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + if (!capable(CAP_COMPROMISE_KERNEL) && !skey_data_available()) {
> > +#else
> > + if (!capable(CAP_COMPROMISE_KERNEL)) {
> > +#endif
> > + return -EPERM;
> > + }
> >
> > lock_system_sleep();
> > /* The snapshot device should not be opened while we're running */
> > @@ -799,6 +807,15 @@ static int software_resume(void)
> > if (error)
> > goto Unlock;
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + if (!capable(CAP_COMPROMISE_KERNEL) && !wkey_data_available()) {
> > +#else
> > + if (!capable(CAP_COMPROMISE_KERNEL)) {
> > +#endif
> > + mutex_unlock(&pm_mutex);
> > + return -EPERM;
> > + }
> > +
> > /* The snapshot device should not be opened while we're running */
> > if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
> > error = -EBUSY;
> > @@ -892,6 +909,15 @@ static ssize_t disk_show(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > int i;
> > char *start = buf;
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + if (efi_enabled(EFI_SECURE_BOOT) && !skey_data_available()) {
> > +#else
> > + if (efi_enabled(EFI_SECURE_BOOT)) {
> > +#endif
> > + buf += sprintf(buf, "[%s]\n", "disabled");
> > + return buf-start;
> > + }
> > +
> > for (i = HIBERNATION_FIRST; i <= HIBERNATION_MAX; i++) {
> > if (!hibernation_modes[i])
> > continue;
> > @@ -926,6 +952,14 @@ static ssize_t disk_store(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > char *p;
> > int mode = HIBERNATION_INVALID;
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + if (!capable(CAP_COMPROMISE_KERNEL) && !skey_data_available()) {
> > +#else
> > + if (!capable(CAP_COMPROMISE_KERNEL)) {
> > +#endif
> > + return -EPERM;
> > + }
> > +
> > p = memchr(buf, '\n', n);
> > len = p ? p - buf : n;
> >
>
> You clearly need some helper function.
> Pavel
>
I will use a help function to replace those ifdef block.
Thanks for your suggestion!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
Hi Pavel,
於 日，2013-08-25 於 18:25 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:50, Lee, Chun-Yi wrote:
> > Introduced a hibernate_key.c file to query the key pair from EFI variables
> > and maintain key pair for check signature of S4 snapshot image. We
> > loaded the private key when snapshot image stored success.
> >
> > This patch introduced 2 EFI variables for store the key to sign S4 image and
> > verify signature when S4 wake up. The names and GUID are:
> > S4SignKey-fe141863-c070-478e-b8a3-878a5dc9ef21
> > S4WakeKey-fe141863-c070-478e-b8a3-878a5dc9ef21
> >
> > S4SignKey is used by EFI bootloader to pass the RSA private key that
> > packaged
> > by PKCS#8 format, kernel will read and parser it when system boot and reload
> > it when S4 resume. EFI bootloader need gnerate a new private key when every
> > time system boot.
> >
> > S4WakeKey is used to pass the RSA public key that packaged by X.509
> > certificate, kernel will read and parser it for check the signature of
> > S4 snapshot image when S4 resume.
> >
> > The follow-up patch will remove S4SignKey and S4WakeKey after load them
> > to kernel for avoid anyone can access it through efivarfs.
> >
> > v3:
> > - Load S4 sign key before ExitBootServices.
> > Load private key before ExitBootServices() then bootloader doesn't need
> > generate key-pair for each booting:
> >+ Add setup_s4_keys() to eboot.c to load S4 sign key before
> > ExitBootServices.
> >+ Reserve the memory block of sign key data blob in efi.c
> > - In Makefile, moved hibernate_keys.o before hibernate.o for load S4 sign
> > key before check hibernate image. It makes sure the new sign key will be
> > transfer to resume target kernel.
> > - Set "depends on EFI_STUB" in Kconfig
> >
> > v2:
> > Add CONFIG_SNAPSHOT_VERIFICATION for build of hibernate_keys.c depend on
> > Kconfig.
> >
> > Cc: Matthew Garrett
> > Cc: Takashi Iwai
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
>
>
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -368,6 +368,91 @@ free_handle:
> > return status;
> > }
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > +static efi_status_t setup_s4_keys(struct boot_params *params)
> > +{
> > + struct setup_data *data;
> > + unsigned long datasize;
> > + u32 attr;
> > + struct efi_s4_key *s4key;
> > + efi_status_t status;
> > +
> > + data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
>
> A bit too many casts.
Thanks.
Yes, here is my mistake, I will remove "unsigned long" cast.
>
> > @@ -1205,6 +1290,10 @@ struct boot_params *efi_main(void *handle,
> > efi_system_table_t *_table,
> >
> > setup_efi_pci(boot_params);
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + setup_s4_keys(boot_params);
> > +#endif
> > +
>
> Move ifdef inside the function?
OK, I will define a dummy function for non-verification situation.
>
> > @@ -729,6 +792,11 @@ void __init efi_init(void)
> >
> > set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
> >
> > +#ifdef CONFIG_SNAPSHOT_VERIFICATION
> > + /* keep s4 key from setup_data */
> > + efi_reserve_s4_skey_data();
> > +#endif
> > +
>
> Here too.
>
I will also use dummy function here.
Thanks
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 13/18] Hibernate: Avoid S4 sign key data included in snapshot image
於 日，2013-08-25 於 18:39 +0200，Pavel Machek 提到： > On Thu 2013-08-22 19:01:52, Lee, Chun-Yi wrote: > > This patch add swsusp_page_is_sign_key() method to hibernate_key.c and > > check the page is S4 sign key data when collect saveable page in > > snapshot.c to avoid sign key data included in snapshot image. > > > > Reviewed-by: Jiri Kosina > > Signed-off-by: Lee, Chun-Yi > > --- > > kernel/power/snapshot.c |6 ++ > > 1 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > > index 72e2911..9e4c94b 100644 > > --- a/kernel/power/snapshot.c > > +++ b/kernel/power/snapshot.c > > @@ -860,6 +860,9 @@ static struct page *saveable_highmem_page(struct zone > > *zone, unsigned long pfn) > > > > BUG_ON(!PageHighMem(page)); > > > > + if (swsusp_page_is_sign_key(page)) > > + return NULL; > > + > > if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page) || > > PageReserved(page)) > > return NULL; > > Just do set_page_forbidden() on your page? Before call swsusp_unset_page_forbidden(), we need make sure the create_basic_memory_bitmaps() function already called to initial forbidden_pages_map. That means need careful the timing, otherwise the page of private key will not register to forbidden pages map. So, I choice to refuse the page of private key when snapshot finding which page is saveable. It has clearer code and we don't need worry the future change of hibernate.c or user.c for when they call create_basic_memory_bitmaps() and when the code clear forbidden pages map. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 12/18] Hibernate: generate and verify signature of snapshot
Hi Pavel,
Thanks for your time to review my patches.
於 日，2013-08-25 於 18:36 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:51, Lee, Chun-Yi wrote:
> > This patch add the code for generate/verify signature of snapshot, it
> > put the signature to snapshot header. This approach can support both
> > on userspace hibernate and in-kernel hibernate.
> >
> > v2:
> > - Due to loaded S4 sign key before ExitBootServices, we need forward key
> > from
> > boot kernel to resume target kernel. So this patch add a empty page in
> > snapshot image, then we keep the pfn of this empty page in snapshot
> > header.
> > When system resume from hibernate, we fill new sign key to this empty page
> > space after snapshot image checked pass. This mechanism let boot kernel
> > can
> > forward new sign key to resume target kernel but don't need write new
> > private
> > key to any other storage, e.g. swap.
> >
> > Cc: Matthew Garrett
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
> > ---
> > kernel/power/power.h|6 +
> > kernel/power/snapshot.c | 280
> > +-
> > kernel/power/swap.c | 14 +++
> > kernel/power/user.c |9 ++
> > 4 files changed, 302 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index 69a81d8..84e0b06 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -3,6 +3,9 @@
> > #include
> > #include
> >
> > +/* The maximum length of snapshot signature */
> > +#define SIG_LENG 512
> > +
> > struct swsusp_info {
> > struct new_utsname uts;
> > u32 version_code;
> > @@ -11,6 +14,8 @@ struct swsusp_info {
> > unsigned long image_pages;
> > unsigned long pages;
> > unsigned long size;
> > + unsigned long skey_data_buf_pfn;
> > + u8 signature[SIG_LENG];
> > } __attribute__((aligned(PAGE_SIZE)));
>
> SIG_LEN or SIG_LENGTH. Select one.
>
I will use SIG_LEN at next version, thanks!
>
> > +static int
> > copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap
> > *orig_bm)
> > {
> > struct zone *zone;
> > - unsigned long pfn;
> > + unsigned long pfn, dst_pfn;
> ...
> > + tfm = crypto_alloc_shash(SNAPSHOT_HASH, 0, 0);
> > + if (IS_ERR(tfm)) {
> > + pr_err("IS_ERR(tfm): %ld", PTR_ERR(tfm));
> > + return PTR_ERR(tfm);
> > + }
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
>
> Are you sure GFP_KERNEL allocation is okay at this phase of
> hibernation?
>
> Could the hashing be done at later phase, when writing the image to
> disk?
>
Thanks for you point out!
Yes, call memory allocate here is not a good design due to it causes
garbage in snapshot that will not released by resumed kernel.
I just finished another implementation, the respin patch extracts the
signature generation code to another function then call the function in
swsusp_save() after copy_data_pages() finished. We can write to memory
at that stage.
> >
> > +void **h_buf;
>
> helpfully named.
>
I will change the name to handle_buffers;
> > + ret = verify_signature(s4_wake_key, pks);
> > + if (ret) {
> > + pr_err("snapshot S4 signature verification fail: %d\n", ret);
> > + goto error_verify;
> > + } else
> > + pr_info("snapshot S4 signature verification pass!\n");
> > +
> > + if (pks->rsa.s)
> > + mpi_free(pks->rsa.s);
> > + kfree(pks);
>
> ret = 0 and fall through?
>
When verification success, verify_signature() will return 0.
Yes, here have duplicate code, I will clear up it.
> > + return 0;
> > +
> > +error_verify:
> > + if (pks->rsa.s)
> > + mpi_free(pks->rsa.s);
> > +error_mpi:
> > + kfree(pks);
> > + return ret;
> > +}
>
>
> > + ret = crypto_shash_final(desc, digest);
> > + if (ret)
> > + goto error_shash;
> > +
> > + ret = snapshot_verify_signature(digest, digest_size);
> > + if (ret)
> > + goto error_verify;
> > +
> > + kfree(h_buf);
> > + kfree(digest);
> > + crypto_free_shash(tfm);
> > + return 0;
>
> These four lines can be deleted.
>
Yes, here also duplicate, I will remove.
> > +
> > +error_verify:
> > +error_shash:
> > + kfree(h_buf);
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> Pavel
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/18] asymmetric keys: separate the length checking of octet string from RSA_I2OSP
於 日，2013-08-25 於 18:01 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:42, Lee, Chun-Yi wrote:
> > Due to RSA_I2OSP is not only used by signature verification path but also
> > used
> > in signature generation path. So, separate the length checking of octet
> > string
> > because it's not for generate 0x00 0x01 leading string when used in
> > signature
> > generation.
> >
> > Reviewed-by: Jiri Kosina
> > Signed-off-by: Lee, Chun-Yi
>
> > +static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > +{
> > + unsigned x_size;
> > + unsigned X_size;
> > + u8 *X = NULL;
>
> Is this kernel code or entry into obfuscated C code contest? This is not
> funny.
>
> Pavel
The small "x" is the input integer that will transfer to big "X" that is
a octet sting.
Sorry for I direct give variable name to match with spec, maybe I need
use big_X or
Do you have good suggest for the naming?
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/18] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
Hi Pavel,
First, thanks for your review!
於 日，2013-08-25 於 17:53 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:41, Lee, Chun-Yi wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation
> > (RSASSA-PKCS1-v1_5-SIGN).
>
> Is this your own code, or did you copy it from somewhere?
>
It's my own code, development base on RSA PKCS#1 spec. So the naming of
variables are match with PKCS#1 spec.
> > + if (!T)
> > + goto error_T;
> > +
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data,
> > RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest,
> > pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message
> > length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = EINVAL;
> > + goto error_emLen;
> > + }
>
> Normal kernel calling convention is 0 / -EINVAL.
Yes, here is my mistake, I will modify it.
>
> > + memcpy(EM + 2, PS, emLen - tLen - 3);
> > + EM[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM + 2 + emLen - tLen - 3 + 1, T, tLen);
>
> ThisDoesNotLookLikeKernelCode, NoCamelCase, please.
>
Thanks for you point out, I will change it.
> > + *_EM = EM;
>
> And we don't usually use _ prefix like this.
>
Thanks! I will change it.
>
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
>
> u8 *signature?
Yes, this 'S' is signature. I put the naming full match with spec for
development, I will change it to match kernel rule. e.g. signature_S
>
> > + size_t k; /* length k of signature S */
>
> u8 *signature_length.
>
I will use signature_leng_k to also match with PKCS#1 spec, I think it's
better for review source code with the spec for debugging.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] asymmetric keys: explicitly add the leading zero byte to encoded message
Hi all experts, Does there have any suggestions or comments for this patch to asymmetric keys? Thanks a lot! Joey Lee 於 五，2013-07-12 於 11:11 +0800，Lee, Chun-Yi 提到： > From: Chun-Yi Lee > > Per PKCS1 spec, the EMSA-PKCS1-v1_5 encoded message is leading by 0x00 0x01 in > its first 2 bytes. The leading zero byte is suppressed by MPI so we pass a > pointer to the _preceding_ byte to RSA_verify() in original code, but it has > risk for the byte is not zero because it's not in EM buffer's scope, neither > RSA_verify() nor mpi_get_buffer() didn't take care the leading byte. > > To avoid the risk, that's better we explicitly add the leading zero byte to EM > for pass to RSA_verify(). This patch allocate a _EM buffer to capture the > result from RSA_I2OSP(), then set the first byte to zero in EM and copy the > remaining bytes from _EM. > > Cc: Rusty Russell > Cc: Josh Boyer > Cc: Randy Dunlap > Cc: Herbert Xu > Cc: "David S. Miller" > Cc: David Howells > Signed-off-by: Chun-Yi Lee > --- > crypto/asymmetric_keys/rsa.c | 14 ++ > 1 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c > index ca1a4f3..7bc99d2 100644 > --- a/crypto/asymmetric_keys/rsa.c > +++ b/crypto/asymmetric_keys/rsa.c > @@ -303,6 +303,7 @@ static int RSA_verify_signature(const struct public_key > *key, > /* Variables as per RFC3447 sec 8.2.2 */ > const u8 *H = sig->digest; > u8 *EM = NULL; > + u8 *_EM = NULL; > MPI m = NULL; > size_t k; > > @@ -337,14 +338,19 @@ static int RSA_verify_signature(const struct public_key > *key, > /* (2c) Convert the message representative (m) to an encoded message >* (EM) of length k octets. >* > - * NOTE! The leading zero byte is suppressed by MPI, so we pass a > - * pointer to the _preceding_ byte to RSA_verify()! > + * NOTE! The leading zero byte is suppressed by MPI, so we add it > + * back to EM before input to RSA_verify()! >*/ > - ret = RSA_I2OSP(m, k, &EM); > + ret = RSA_I2OSP(m, k, &_EM); > if (ret < 0) > goto error; > > - ret = RSA_verify(H, EM - 1, k, sig->digest_size, > + EM = kmalloc(k, GFP_KERNEL); > + memset(EM, 0, 1); > + memcpy(EM + 1, _EM, k-1); > + kfree(_EM); > + > + ret = RSA_verify(H, EM, k, sig->digest_size, >RSA_ASN1_templates[sig->pkey_hash_algo].data, >RSA_ASN1_templates[sig->pkey_hash_algo].size); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 4/4] x86, efi: Map runtime services 1:1
於 一，2013-06-17 於 19:50 +0200，Borislav Petkov 提到： > +#ifdef CONFIG_X86_64 > + efi_scratch.pgt11 = (pgd_t *)(unsigned > long)real_mode_header->trampoline_pgd; > +#endif > + efi_use_11_map = true; > + I think "efi_use_11_map = true" should moved to before "#endif", otherwise kernel will build fail on i586 architecture. I got build error: arch/x86/platform/efi/efi.c:1074: error: ‘efi_use_11_map’ undeclared (first use in this function) arch/x86/platform/efi/efi.c:1074: error: (Each undeclared identifier is reported only once arch/x86/platform/efi/efi.c:1074: error: for each function it appears in.) Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] asymmetric keys: explicitly add the leading zero byte to encoded message
From: Chun-Yi Lee Per PKCS1 spec, the EMSA-PKCS1-v1_5 encoded message is leading by 0x00 0x01 in its first 2 bytes. The leading zero byte is suppressed by MPI so we pass a pointer to the _preceding_ byte to RSA_verify() in original code, but it has risk for the byte is not zero because it's not in EM buffer's scope, neither RSA_verify() nor mpi_get_buffer() didn't take care the leading byte. To avoid the risk, that's better we explicitly add the leading zero byte to EM for pass to RSA_verify(). This patch allocate a _EM buffer to capture the result from RSA_I2OSP(), then set the first byte to zero in EM and copy the remaining bytes from _EM. Cc: Rusty Russell Cc: Josh Boyer Cc: Randy Dunlap Cc: Herbert Xu Cc: "David S. Miller" Cc: David Howells Signed-off-by: Chun-Yi Lee --- crypto/asymmetric_keys/rsa.c | 14 ++ 1 files changed, 10 insertions(+), 4 deletions(-) diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c index ca1a4f3..7bc99d2 100644 --- a/crypto/asymmetric_keys/rsa.c +++ b/crypto/asymmetric_keys/rsa.c @@ -303,6 +303,7 @@ static int RSA_verify_signature(const struct public_key *key, /* Variables as per RFC3447 sec 8.2.2 */ const u8 *H = sig->digest; u8 *EM = NULL; + u8 *_EM = NULL; MPI m = NULL; size_t k; @@ -337,14 +338,19 @@ static int RSA_verify_signature(const struct public_key *key, /* (2c) Convert the message representative (m) to an encoded message * (EM) of length k octets. * -* NOTE! The leading zero byte is suppressed by MPI, so we pass a -* pointer to the _preceding_ byte to RSA_verify()! +* NOTE! The leading zero byte is suppressed by MPI, so we add it +* back to EM before input to RSA_verify()! */ - ret = RSA_I2OSP(m, k, &EM); + ret = RSA_I2OSP(m, k, &_EM); if (ret < 0) goto error; - ret = RSA_verify(H, EM - 1, k, sig->digest_size, + EM = kmalloc(k, GFP_KERNEL); + memset(EM, 0, 1); + memcpy(EM + 1, _EM, k-1); + kfree(_EM); + + ret = RSA_verify(H, EM, k, sig->digest_size, RSA_ASN1_templates[sig->pkey_hash_algo].data, RSA_ASN1_templates[sig->pkey_hash_algo].size); -- 1.6.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, efi: retry ExitBootServices() on failure
Thanks for Zach's clarify, then I think that makes sense BIOS has problem if kernel need call ExitBootServices() more then 2 times. Thanks Joey Lee 於 二，2013-06-18 於 04:20 +，Zachary Bobroff 提到： > The timer is shutdown before callbacks on exitbootservices are called. The > bios should be entirely single threaded at this point unless Linux has > started some other CPUs. So exitbootservices will not return until each > until each callback is complete. In short, then it would return the status > of failure if the memory map has changed, success otherwise. The callbacks > are not called on any subsequent call to exitbootservices, so the map should > stay the same then. > > Sent from my iPhone > > On Jun 17, 2013, at 10:48 PM, "joeyli" wrote: > > > Hi Zach, > > > > 於 二，2013-06-18 於 00:18 +，Zachary Bobroff 提到： > >> All, > >> > >>>> Why a single retry is having reasonable guarantees to work when the > >> original one failed (nothing prevents an event handler to do another > >> allocation the next time through). > >> > >> This patch is being submitted because of the potential issues that > >> occur when 3rd party option ROMs are signaled by the ExitBootServices > >> event. At the time they are signaled, they can perform any number of > >> actions that would change the EFI Memory map. This wasn't actually > >> seen with our default implementation of our firmware, it was seen when > >> this plug-in card's option rom was run. > >> > >> We only need to retry once because of what is in the UEFI > >> specification 2.3.1 Errata C. The exact verbiage is as follows: > >> > >> The ExitBootServices() function is called by the currently executing > >> EFI OS loader image to terminate all boot services. On success, the > >> loader becomes responsible for the continued operation of the system. > >> All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled > >> before ExitBootServices() returns EFI_SUCCESS. The events are only > >> signaled once even if ExitBootServices() is called multiple times. > >> > >> Since the firmware will only signal the ExitBootServices event once, > >> the code that has the potential to change the memory map will only be > >> signaled on the first call to exit boot services. > > > > Does it possible have any delay time of 3rd party ROMs to change EFI > > memory map after they are signaled? > > or ExitBootServices() will wait all 3rd party ROMs updated memory map > > finished then return true/false to kernel? > > > > > > Thanks a lot! > > Joey Lee > > > > The information contained in this message may be confidential and proprietary > to American Megatrends, Inc. This communication is intended to be read only > by the individual or entity to whom it is addressed or by their designee. If > the reader of this message is not the intended recipient, you are on notice > that any distribution of this message, in any form, is strictly prohibited. > Please promptly notify the sender by reply e-mail or by telephone at > 770-246-8600, and then delete or destroy all copies of the transmission. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] x86, efi: retry ExitBootServices() on failure
Hi Zach, 於 二，2013-06-18 於 00:18 +，Zachary Bobroff 提到： > All, > > >> Why a single retry is having reasonable guarantees to work when the > original one failed (nothing prevents an event handler to do another > allocation the next time through). > > This patch is being submitted because of the potential issues that > occur when 3rd party option ROMs are signaled by the ExitBootServices > event. At the time they are signaled, they can perform any number of > actions that would change the EFI Memory map. This wasn't actually > seen with our default implementation of our firmware, it was seen when > this plug-in card's option rom was run. > > We only need to retry once because of what is in the UEFI > specification 2.3.1 Errata C. The exact verbiage is as follows: > > The ExitBootServices() function is called by the currently executing > EFI OS loader image to terminate all boot services. On success, the > loader becomes responsible for the continued operation of the system. > All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled > before ExitBootServices() returns EFI_SUCCESS. The events are only > signaled once even if ExitBootServices() is called multiple times. > > Since the firmware will only signal the ExitBootServices event once, > the code that has the potential to change the memory map will only be > signaled on the first call to exit boot services. Does it possible have any delay time of 3rd party ROMs to change EFI memory map after they are signaled? or ExitBootServices() will wait all 3rd party ROMs updated memory map finished then return true/false to kernel? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, efi: retry ExitBootServices() on failure
於 一，2013-06-17 於 11:17 +0100，Matt Fleming 提到：
> On Mon, 17 Jun, at 10:46:28AM, Jan Beulich wrote:
> > To me, all this looks like it is being done on phenomenological basis,
> > taking a particular build of a particular firmware implementation as
> > the reference. Imo we shouldn't change the code in this way. This
> > also applies to the fact that the step is being doubled rather than
> > e.g. tripled: With it ending up a "again" anyway (see below), what's
> > the point of avoiding one more of the iterations?
> >
> > Generic considerations would result in the increment being at least
> > 3 * element size; twice the element size assumes that the allocator
> > would behave in certain ways (like returning the head or tail part of
> > a larger piece of memory).
>
> I have no issue with changing the multiplier. But let's get
> clarification from Zach as to what exactly is going on here.
>
> > I agree that there ought to be an upper limit. But a single retry here
> > again looks like a tailored solution to a particular observed (mis-)
> > behavior, rather than something resulting from general considerations.
>
> What value would you suggest for the retry?
>
Currently grub2 retry unlimited times like attached patch.
But, I also agree need have a upper limit.
Thanks a lot!
Joey Lee
revno: 4778
committer: Vladimir 'phcoder' Serbinenko
branch nick: grub
timestamp: Tue 2013-03-26 11:34:56 +0100
message:
* grub-core/kern/efi/mm.c (grub_efi_finish_boot_services):
Try terminating EFI services several times due to quirks in some
implementations.
diff:
=== modified file 'ChangeLog'
--- ChangeLog 2013-03-26 10:29:52 +
+++ ChangeLog 2013-03-26 10:34:56 +
@@ -1,3 +1,9 @@
+2013-03-26 Vladimir Serbinenko
+
+ * grub-core/kern/efi/mm.c (grub_efi_finish_boot_services):
+ Try terminating EFI services several times due to quirks in some
+ implementations.
+
2013-03-26 Colin Watson
* grub-core/commands/acpihalt.c (skip_ext_op): Add support for
=== modified file 'grub-core/kern/efi/mm.c'
--- grub-core/kern/efi/mm.c 2013-01-13 01:10:41 +
+++ grub-core/kern/efi/mm.c 2013-03-26 10:34:56 +
@@ -160,27 +160,41 @@
apple, sizeof (apple)) == 0);
#endif
- if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, &finish_key,
- &finish_desc_size, &finish_desc_version) < 0)
-return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
-
- if (outbuf && *outbuf_size < finish_mmap_size)
-return grub_error (GRUB_ERR_IO, "memory map buffer is too small");
-
- finish_mmap_buf = grub_malloc (finish_mmap_size);
- if (!finish_mmap_buf)
-return grub_errno;
-
- if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, &finish_key,
- &finish_desc_size, &finish_desc_version) <= 0)
-return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
-
- b = grub_efi_system_table->boot_services;
- status = efi_call_2 (b->exit_boot_services, grub_efi_image_handle,
- finish_key);
- if (status != GRUB_EFI_SUCCESS)
-return grub_error (GRUB_ERR_IO, "couldn't terminate EFI services");
-
+ while (1)
+{
+ if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, &finish_key,
+ &finish_desc_size, &finish_desc_version) < 0)
+ return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
+
+ if (outbuf && *outbuf_size < finish_mmap_size)
+ return grub_error (GRUB_ERR_IO, "memory map buffer is too small");
+
+ finish_mmap_buf = grub_malloc (finish_mmap_size);
+ if (!finish_mmap_buf)
+ return grub_errno;
+
+ if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, &finish_key,
+ &finish_desc_size, &finish_desc_version) <= 0)
+ {
+ grub_free (finish_mmap_buf);
+ return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
+ }
+
+ b = grub_efi_system_table->boot_services;
+ status = efi_call_2 (b->exit_boot_services, grub_efi_image_handle,
+ finish_key);
+ if (status == GRUB_EFI_SUCCESS)
+ break;
+
+ if (status != GRUB_EFI_INVALID_PARAMETER)
+ {
+ grub_free (finish_mmap_buf);
+ return grub_error (GRUB_ERR_IO, "couldn't terminate EFI services");
+ }
+
+ grub_free (finish_mmap_buf);
+ grub_printf ("Trying to terminate EFI services again\n");
+}
grub_efi_is_finished = 1;
if (outbuf_size)
*outbuf_size = finish_mmap_size;
Re: [PATCH] x86, efi: retry ExitBootServices() on failure
Hi Zach,
於 二，2013-06-11 於 07:52 +0100，Matt Fleming 提到：
> From: Zach Bobroff
>
> ExitBootServices is absolutely supposed to return a failure if any
> ExitBootServices event handler changes the memory map. Basically the
> get_map loop should run again if ExitBootServices returns an error the
> first time. I would say it would be fair that if ExitBootServices gives
> an error the second time then Linux would be fine in returning control
> back to BIOS.
>
> The second change is the following line:
>
> again:
> size += sizeof(*mem_map) * 2;
>
> Originally you were incrementing it by the size of one memory map entry.
> The issue here is all related to the low_alloc routine you are using.
> In this routine you are making allocations to get the memory map itself.
> Doing this allocation or allocations can affect the memory map by more
> than one record.
>
> [ mfleming - changelog, code style ]
> Signed-off-by: Zach Bobroff
> Cc:
> Signed-off-by: Matt Fleming
> ---
> arch/x86/boot/compressed/eboot.c | 20 +---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 35ee62f..7c6e5d9 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1037,18 +1037,20 @@ static efi_status_t exit_boot(struct boot_params
> *boot_params,
> efi_memory_desc_t *mem_map;
> efi_status_t status;
> __u32 desc_version;
> + bool called_exit = false;
> u8 nr_entries;
> int i;
>
> size = sizeof(*mem_map) * 32;
>
> again:
> - size += sizeof(*mem_map);
> + size += sizeof(*mem_map) * 2;
> _size = size;
> status = low_alloc(size, 1, (unsigned long *)&mem_map);
Can we know why increased the size of double *mem_map is big enough?
Does there have any real guarantee to be sufficient?
And, why would doubling the increment be necessary here, but not in
__get_map()?
> if (status != EFI_SUCCESS)
> return status;
>
> +get_map:
The get_map label is being placed such that the size doesn't
get adjusted anymore, yet it is supposed to deal with an allocation
having happened during ExitBootServices(), which could have
grown the map.
Why doesn't direct goto 'again' label?
> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
> mem_map, &key, &desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> @@ -1074,8 +1076,20 @@ again:
> /* Might as well exit boot services now */
> status = efi_call_phys2(sys_table->boottime->exit_boot_services,
> handle, key);
> - if (status != EFI_SUCCESS)
> - goto free_mem_map;
> + if (status != EFI_SUCCESS) {
> + /*
> + * ExitBootServices() will fail if any of the event
> + * handlers change the memory map. In which case, we
> + * must be prepared to retry, but only once so that
> + * we're guaranteed to exit on repeated failures instead
> + * of spinning forever.
> + */
> + if (called_exit)
> + goto free_mem_map;
> +
> + called_exit = true;
Why a single retry is having reasonable guarantees to work when the
original one failed (nothing prevents an event handler to do another
allocation the next time through).
Why not retry 3, 4, 5 or even unlimited times?
> + goto get_map;
> + }
>
> /* Historic? */
> boot_params->alt_mem_k = 32 * 1024;
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] i915: Don't provide ACPI backlight interface if firmware expects Windows 8
於 日，2013-06-09 於 19:01 -0400，Matthew Garrett 提到： > Windows 8 leaves backlight control up to individual graphics drivers rather > than making ACPI calls itself. There's plenty of evidence to suggest that > the Intel driver for Windows doesn't use the ACPI interface, including the > fact that it's broken on a bunch of machines when the OS claims to support > Windows 8. The simplest thing to do appears to be to disable the ACPI > backlight interface on these systems. > > Signed-off-by: Matthew Garrett > --- > drivers/gpu/drm/i915/i915_dma.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 3b315ba..23b6292 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned > long flags) > /* Must be done after probing outputs */ > intel_opregion_init(dev); > acpi_video_register(); > + /* Don't use ACPI backlight functions on Windows 8 platforms */ > + if (acpi_osi_version() >= ACPI_OSI_WIN_8) > + acpi_video_backlight_unregister(); > } > > if (IS_GEN5(dev)) This patch set works to me on Acer Aspire V3 notebook for unregister the backlight interface of acpi video driver when i915 loaded. Acer Aspire V3 has the Windows8 support in DSDT. Tested-by: Lee, Chun-Yi Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Modify UEFI anti-bricking code
於 四，2013-06-06 於 05:42 +，Matthew Garrett 提到：
> On Thu, 2013-06-06 at 13:05 +0800, joeyli wrote:
>
> > + if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
> > + return EFI_OUT_OF_RESOURCES;
>
> I'd move this up to the top of the function, and just return 0 - there's
> no risk of the firmware causing problems if it's a volatile variable, so
> we should probably just pass it down to the firmware and return an error
> from there.
>
OK, I moved volatile checking to the top of the function.
New version, version 3 diff result like the following.
Thanks a lot for reviewing
Joey Lee
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..5ae2eb0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@
#define EFI_DEBUG 1
+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0,
0x9f, 0x92, 0xa9)
@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);
/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+EFI_VARIABLE_NON_VOLATILE |
+EFI_VARIABLE_BOOTSERVICE_ACCESS |
+EFI_VARIABLE_RUNTIME_ACCESS,
+0, NULL);
}
/*
@@ -1040,6 +1046,9 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
efi_status_t status;
u64 storage_size, remaining_size, max_size;
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return 0;
+
status = efi.query_variable_info(attributes, &storage_size,
&remaining_size, &max_size);
if (status != EFI_SUCCESS)
@@ -1051,7 +1060,9 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);
if (status == EFI_SUCCESS) {
/*
@@ -1069,7 +1083,10 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
* that we delete it...
*/
efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
-attributes, 0, dummy);
+EFI_VARIABLE_NON_VOLATILE |
+EFI_VARIABLE_BOOTSERVICE_ACCESS |
+EFI_VARIABLE_RUNTIME_ACCESS,
+0, dummy);
}
/*
@@ -1085,7 +1102,7 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Modify UEFI anti-bricking code
於 四，2013-06-06 於 13:05 +0800，joeyli 提到：
> 於 三，2013-06-05 於 16:59 +0100，Matt Fleming 提到：
> > On Wed, 05 Jun, at 02:53:27PM, Matthew Garrett wrote:
> > > On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:
> > >
> > > > Folks, what do you want me to do with this? Merge it with Matthew's
> > > > patch?
> > >
> > > Do that and add Joey's signed-off-by?
> >
> > Right, this is what I've got queued up.
> >
> > ---
> >
> > >From 380dcc12ba82f4e10feb6a72207b2e4771d16d8d Mon Sep 17 00:00:00 2001
> > From: Matthew Garrett
> > Date: Sat, 1 Jun 2013 16:06:20 -0400
> > Subject: [PATCH] Modify UEFI anti-bricking code
> >
> > This patch reworks the UEFI anti-bricking code, including an effective
> > reversion of cc5a080c and 31ff2f20. It turns out that calling
> > QueryVariableInfo() from boot services results in some firmware
> > implementations jumping to physical addresses even after entering virtual
> > mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> > going to work so well.
> [...]
>
> The follow diff change is base on 380dcc12 patch queued in efi git tree,
> it included Matthew and hpa's suggestions. I fix the attributes of DUMMY
> object to NV/BS/RT and introduced a #define of the minimum reserve flash
> space.
>
> This change works to me on OVMF.
>
>
>
> Thanks a lot!
> Joey Lee
>
Sorry for attached a wrong diff result, it lost a NV/BS/RT attributes
changed in efi_query_variable_store(). The right diff change is
following.
Thanks a lot!
Joey Lee
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..ec8ac97 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@
#define EFI_DEBUG 1
+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0,
0x9f, 0x92, 0xa9)
@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);
/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+EFI_VARIABLE_NON_VOLATILE |
+EFI_VARIABLE_BOOTSERVICE_ACCESS |
+EFI_VARIABLE_RUNTIME_ACCESS,
+0, NULL);
}
/*
@@ -1051,7 +1057,12 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_OUT_OF_RESOURCES;
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);
if (status == EFI_SUCCESS) {
/*
@@ -1069,7 +1083,10 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
* that we delete it...
*/
efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
-attributes, 0, dummy);
+EFI_VARIABLE_NON_VOLATILE |
+EFI_VARIABLE_BOOTSERVICE_ACCESS |
+EFI_VARIABLE_RUNTIME_ACCESS,
+0, dummy);
}
/*
@@ -1085,7 +1102,7 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Modify UEFI anti-bricking code
於 三，2013-06-05 於 16:59 +0100，Matt Fleming 提到：
> On Wed, 05 Jun, at 02:53:27PM, Matthew Garrett wrote:
> > On Wed, 2013-06-05 at 15:49 +0100, Fleming, Matt wrote:
> >
> > > Folks, what do you want me to do with this? Merge it with Matthew's patch?
> >
> > Do that and add Joey's signed-off-by?
>
> Right, this is what I've got queued up.
>
> ---
>
> >From 380dcc12ba82f4e10feb6a72207b2e4771d16d8d Mon Sep 17 00:00:00 2001
> From: Matthew Garrett
> Date: Sat, 1 Jun 2013 16:06:20 -0400
> Subject: [PATCH] Modify UEFI anti-bricking code
>
> This patch reworks the UEFI anti-bricking code, including an effective
> reversion of cc5a080c and 31ff2f20. It turns out that calling
> QueryVariableInfo() from boot services results in some firmware
> implementations jumping to physical addresses even after entering virtual
> mode, so until we have 1:1 mappings for UEFI runtime space this isn't
> going to work so well.
[...]
The follow diff change is base on 380dcc12 patch queued in efi git tree,
it included Matthew and hpa's suggestions. I fix the attributes of DUMMY
object to NV/BS/RT and introduced a #define of the minimum reserve flash
space.
This change works to me on OVMF.
Thanks a lot!
Joey Lee
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index cc3cfe8..2617675 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,8 @@
#define EFI_DEBUG 1
+#define EFI_MIN_RESERVE 5120
+
#define EFI_DUMMY_GUID \
EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0,
0x9f, 0x92, 0xa9)
@@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void)
kfree(new_memmap);
/* clean DUMMY object */
- efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL);
+ efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
+EFI_VARIABLE_NON_VOLATILE |
+EFI_VARIABLE_BOOTSERVICE_ACCESS |
+EFI_VARIABLE_RUNTIME_ACCESS,
+0, NULL);
}
/*
@@ -1051,7 +1057,12 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
* write if permitting it would reduce the available space to under
* 5KB. This figure was provided by Samsung, so should be safe.
*/
- if ((remaining_size - size < 5120) && !efi_no_storage_paranoia) {
+ if ((remaining_size - size < EFI_MIN_RESERVE) &&
+ !efi_no_storage_paranoia) {
+
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_OUT_OF_RESOURCES;
+
/*
* Triggering garbage collection may require that the firmware
* generate a real EFI_OUT_OF_RESOURCES error. We can force
@@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
- attributes, dummy_size, dummy);
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ dummy_size, dummy);
if (status == EFI_SUCCESS) {
/*
@@ -1085,7 +1099,7 @@ efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size)
/*
* There still isn't enough room, so return an error
*/
- if (remaining_size - size < 5120)
+ if (remaining_size - size < EFI_MIN_RESERVE)
return EFI_OUT_OF_RESOURCES;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Modify UEFI anti-bricking code
於 三，2013-06-05 於 16:08 +，Matthew Garrett 提到： > On Wed, 2013-06-05 at 16:59 +0100, Matt Fleming wrote: > > > + /* clean DUMMY object */ > > + efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID, 0, 0, NULL); > > Hm. Actually, is that going to work? From the spec: > The patch I tested on OVMF, it can delete DUMMY object when system boot. > If a preexisting variable is rewritten with different attributes, > SetVariable()shall not modify the variable and shall return > EFI_INVALID_PARAMETER. > > So I think we probably need to fix the attributes to NV|RT|BS for both > this call and the one in query_variable_store. We should probably also > only do the workaround if the NV bit is set in the original query. > > -- > Matthew Garrett | mj...@srcf.ucam.org Yes, I think that more safe for fix the attributes. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
