Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support
* Matthew Garrett mj...@srcf.ucam.org wrote: On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote: Also, the main question would be, what is the typical value for si-lfb_depth. 32 on almost all EFI systems? All around the map? Depends on what graphics state the EFI bootloader passes us? Microsoft require that it be 32, so in practice I suspect it'll always be 32. [...] Ok, cool - and it's a rare sight: Microsoft standing up against bloat and complexity. [...] It's possible that Itanium decides to be different. That would be a feature! Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] EFI: Runtime services virtual mapping
On 10/10/13 at 01:34pm, Matt Fleming wrote: On Thu, 10 Oct, at 10:58:28AM, Borislav Petkov wrote: On Thu, Oct 10, 2013 at 04:14:34PM +0800, Dave Young wrote: Even though I still have no idea why kernel text overlap with efi boot region, anyway map the un-overlapped part is necessary though. I can post the kexec related patches after your mapping patches settle down Right, settle down being the key here. Matt just mentioned on IRC that we might not need boot services mappings by the time we have to start the kexec kernel, which would mean, you don't have to do anything in efi_reserve_boot_services(). Dave, apologies for not discussing the whole Boot Services thing sooner. I missed your questions. No problem, Thanks for clarifying the boot service issue. We really should not be passing the EFI Boot Service regions via the memmap to kexec at all, because by the time the kexec'd kernel is running those pages that previously contained Boot Service code/data will have likely been reused for something else. Which, to answer your question, is why the Boot Service regions overlap the kernel text in the kexec'd kernel - those regions have been reallocated by the first kernel and now happen to contain the kernel text of the kexec kernel. Ok, then I understand passing boot service regions to 2nd kernel make no sense. But for current implementation from Boris, getting same mapping between diffrent kernel depends on same md order (same start and size for each one) How about using this mapping solution but at the same time for kexec kernel we also pass the virtual mappings via setup_data, only thing diffrent is we only need map the non boot region and just use the boot region size to ensure the other regions are mapped with same virtual address. OTOH, if we only passing ioremapped data without Boris's current patch the problem I worry about is how can we ensure the addresses are not used by other code before we mapping the in 2nd kernel efi_init. For the boot efi_reserve_boot_services code, it's mainly for the SetVirtualAddressMap callback use, so boot regions should not be reused before SetVirtualAddressMap, but the overlapping happens before the efi_reserve_boot_services, isn't it a problem? The reason that we don't keep the Boot Service regions around forever is because they can take up a considerable amount of memory, so the current situation of free'ing them after we're sure the firmware isn't going to reference them is still the right way to go, and simply not including any Boot Service entries in the memory map passed to kexec should make everything work OK. The question which needs answering first though is, how the whole efi thing is going to handle any functionality like calling into efi boot regions from runtime functions and such. Which hasn't really been tested and fw vendors don't really want to support that. But this is all bits and pieces I heard yesterday so it is all pretty wet and I'll let efi guys, i.e. the Matts and a couple of others :-), figure out this whole issue. We currently treat the scenario where Runtime Services reference Boot Service regions as a bug and either work around it (where we do efi_reserve_boot_services() and efi_free_boot_services() around SetVirtualAddressMap()) or we avoid calling those services altogether. The spec is pretty clear that runtime drivers shouldn't be doing this, and so far this approach has served us well. There are only two reasons why we keep the Boot Services regions around (for a short period) at all, 1) To work around the aforementioned runtime firmware bugs 2) To copy a ACPI BGRT image into kernel memory I'm not sure whether the kexec kernel would care about the BGRT? I have no idea about BGRT previously, it's Boot Graphics Resource Table so it's only for boot time use, I guess kexec can safely ignore it. Thanks Dave -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support
On Thu, Oct 10, 2013 at 8:09 PM, Peter Jones pjo...@redhat.com wrote:
INTN
GetPixelElementSize (
IN EFI_PIXEL_BITMASK *PixelBits
)
{
INTN HighestPixel = -1;
INTN BluePixel;
INTN RedPixel;
INTN GreenPixel;
INTN RsvdPixel;
BluePixel = FindHighestSetBit (PixelBits-BlueMask);
RedPixel = FindHighestSetBit (PixelBits-RedMask);
GreenPixel = FindHighestSetBit (PixelBits-GreenMask);
RsvdPixel = FindHighestSetBit (PixelBits-ReservedMask);
HighestPixel = max (BluePixel, RedPixel);
HighestPixel = max (HighestPixel, GreenPixel);
HighestPixel = max (HighestPixel, RsvdPixel);
return HighestPixel;
}
EFI_PHYSICAL_ADDRESS NewPixelAddress;
EFI_PHYSICAL_ADDRESS CurrentPixelAddress;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION OutputInfo;
INTN PixelElementSize;
switch (OutputInfo.PixelFormat) {
case PixelBitMask:
PixelElementSize = GetPixelElementSize
(OutputInfo.PixelInformation);
So this can be less than 32.
break;
case PixelBlueGreenRedReserved8BitPerColor:
case PixelRedGreenBlueReserved8BitPerColor:
PixelElementSize = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
break;
}
Which makes this painfully clear.
Also, the main question would be, what is the typical value for
si-lfb_depth. 32 on almost all EFI systems? All around the map? Depends
on what graphics state the EFI bootloader passes us?
Yes, 32 on almost all systems that implement a framebuffer console at
all.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] EFI: Runtime services virtual mapping
On Fri, Oct 11, 2013 at 02:24:37PM +0800, Dave Young wrote: But for current implementation from Boris, getting same mapping between diffrent kernel depends on same md order (same start and size for each one) How about using this mapping solution but at the same time for kexec kernel we also pass the virtual mappings via setup_data, only thing diffrent is we only need map the non boot region and just use the boot region size to ensure the other regions are mapped with same virtual address. Actually, as hpa suggested, we will need to be passing the explicit virtual addresses to the kexec kernel in case we change the mapping algorithm in the future. So all should go through setup_data. OTOH, if we only passing ioremapped data without Boris's current patch the problem I worry about is how can we ensure the addresses are not used by other code before we mapping the in 2nd kernel efi_init. Right, the old method of mapping EFI runtime regions used ioremap and was mapping the regions in the same address space. Now we have reserved a 64G in the VA space ending at -4G (i.e. 0x___) which is reserved only for EFI RT usage. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] EFI: Runtime services virtual mapping
On Fri, 11 Oct, at 02:24:37PM, Dave Young wrote: For the boot efi_reserve_boot_services code, it's mainly for the SetVirtualAddressMap callback use, so boot regions should not be reused before SetVirtualAddressMap, but the overlapping happens before the efi_reserve_boot_services, isn't it a problem? Hang on, which kernel are you referring to here? The boot kernel or the kexec'd kernel? I thought you were saying you noticed the overlap when running in the second (kexec'd) kernel? The only reason that you would see this overlap in the first (boot) kernel is if the bootloader messed up and allocated the kernel text as EfiBootServicesCode/Data. I'd like to believe no bootloaders are still doing that. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] EFI: Runtime services virtual mapping
Matt, The kernel I referring is the boot kernel aka the 1st kernel, the boot loader is grub2 from Fedora 19. [sorry for top reply because of using webmail] - Original Message - From: Matt Fleming m...@console-pimps.org To: Dave Young dyo...@redhat.com Cc: Borislav Petkov b...@alien8.de, X86 ML x...@kernel.org, LKML linux-ker...@vger.kernel.org, Borislav Petkov b...@suse.de, Matthew Garrett mj...@srcf.ucam.org, H. Peter Anvin h...@zytor.com, James Bottomley james.bottom...@hansenpartnership.com, Vivek Goyal vgo...@redhat.com, linux-efi@vger.kernel.org, fwts-de...@lists.ubuntu.com Sent: Friday, October 11, 2013 6:27:06 PM Subject: Re: [PATCH 12/12] EFI: Runtime services virtual mapping On Fri, 11 Oct, at 02:24:37PM, Dave Young wrote: For the boot efi_reserve_boot_services code, it's mainly for the SetVirtualAddressMap callback use, so boot regions should not be reused before SetVirtualAddressMap, but the overlapping happens before the efi_reserve_boot_services, isn't it a problem? Hang on, which kernel are you referring to here? The boot kernel or the kexec'd kernel? I thought you were saying you noticed the overlap when running in the second (kexec'd) kernel? The only reason that you would see this overlap in the first (boot) kernel is if the bootloader messed up and allocated the kernel text as EfiBootServicesCode/Data. I'd like to believe no bootloaders are still doing that. -- Matt Fleming, Intel Open Source Technology Center -- 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-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support
On Thu, 10 Oct, at 07:28:44PM, Ingo Molnar wrote:
Btw., could we perhaps remap the whole framebuffer at init time, or is it
too large? If early_ioremap() fails for whatever reason then that will
emit a WARN_ON(), which will recurse in a fairly nasty way ...
The framebuffer memory will be quite large, so I don't think it makes
sense to map it all this early, because it's likely we'll run out of
fixmap entries.
+ if (!dst)
+ return;
+
+ memset(dst, 0, len);
+ early_iounmap(dst, len);
+}
+
+static __init void early_efi_clear_screen(void)
+{
+ struct screen_info *si;
+ int y;
+
+ si = boot_params.screen_info;
+ for (y = 0; y si-lfb_height; y++)
+ early_efi_clear_line(y);
Looks a bit superfluous to introduce 'si' just for that single use?
I did this to reduce the length of the for (y = 0... line.
+}
+
+static void early_efi_write_char(void *dst, char c, int h)
+{
+ const u8 *src;
+ u32 fgcolor = 0xaa;
That's RGB grey, right? Why not 0xff for a very visible white?
The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
picked this value.
+ u8 s8;
+ int m;
+
+ src = font-data + (c * font-height);
here too the multiplication does not need parantheses.
+ s8 = *(src + h);
We normally only care about the ASCII range, but doesn't 'c' want to be
'unsigned char', to make sure we do the right thing, should anyone use
above 0x7f characters in printk, accidentally or intentionally?
Yeah, this should be unsigned.
+
+ for (m = 0; m 8; m++) {
+ u32 val, mask = 0;
+
+ if ((s8 (7 - m)) 1)
+ mask = 0x;
+
+ val = mask fgcolor;
Hm, because this is really about 32-bit pixel framebuffer, is that mask
code a _really_ roundabout way of saying:
const u32 color_grey = 0x00aa;
const u32 color_black = 0x;
...
if ((s8 (7 - m)) 1)
pixel = color_grey;
else
pixel = color_black;
*dst = pixel;
?
+ memcpy(dst, val, 4);
Also, why not pass in dst as 'u32 *' and replace the memcpy with:
*dst = val;
?
+ dst += 4;
and this with:
dst++;
?
Yeah, that's a nice cleanup.
+ }
+
+ early_iounmap(dst, len);
+ }
+
+ num -= count;
+ efi_x += count * font-width;
+ str += count;
+
+ if (num 0 *s == '\n') {
+ efi_x = 0;
+ efi_y += font-height;
btw., it's a fixed-width, fixed-height font, right?
Correct.
+ str++;
+ num--;
+ }
+
+ if (efi_x = si-lfb_width) {
+ efi_x = 0;
+ efi_y += font-height;
+ }
+
+ if (efi_y + font-height = si-lfb_height) {
+ early_efi_clear_screen();
+ efi_y = 0;
So, if I understand it correctly this clears the screen and starts at the
top when we scroll off the bottom, right?
That might make the recovery of oopses hard when the number of log lines
is unlucky.
Would scrolling a few lines up instead, via a well-calculated memcpy and
memset be doable?
Yeah we can do that. I thought about this originally but decided against
it because I figured it would complicate the code unnecessarily. But it
turned out to be fairly trivial.
+ if (!font)
+ return -ENODEV;
+
+ early_efi_clear_screen();
Assuming we implement scrolling above, here too it might make sense to
scroll up the framebuffer - if the crash is early enough then some
firmware and boot stub info might still be present in the framebuffer?
It's possible that some info will be in the framebuffer, but we can't
begin writing immediately after the boot stub info because we don't know
the last xy coordinates the firmware wrote to.
But yeah, leaving it intact and beginning our output from the last line
of the framebuffer makes more sense than clearing the screen entirely.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] EFI earlyprintk support
From: Matt Fleming matt.flem...@intel.com These two patches cleanup the #include linux/efi.h duplication and add support for earlyprintk=efi, which is the only way users can debug early boot crashes without special hardware. Matt Fleming (2): x86/efi: Include linux/efi.h in asm/efi.h x86/efi: Add EFI framebuffer earlyprintk support Documentation/kernel-parameters.txt | 8 +- arch/x86/Kconfig.debug | 9 ++ arch/x86/boot/compressed/eboot.c | 1 - arch/x86/include/asm/efi.h | 4 + arch/x86/kernel/early_printk.c | 6 ++ arch/x86/kernel/setup.c | 1 - arch/x86/platform/efi/Makefile | 1 + arch/x86/platform/efi/early_printk.c | 191 +++ arch/x86/platform/efi/efi.c | 1 - arch/x86/platform/efi/efi_32.c | 1 - arch/x86/platform/efi/efi_64.c | 1 - arch/x86/platform/uv/bios_uv.c | 1 - 12 files changed, 216 insertions(+), 9 deletions(-) create mode 100644 arch/x86/platform/efi/early_printk.c -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] x86/efi: Include linux/efi.h in asm/efi.h
From: Matt Fleming matt.flem...@intel.com Every file that includes asm/efi.h also includes linux/efi.h. Just include linux/efi.h directly and avoid the duplication. Cc: H. Peter Anvin h...@zytor.com Cc: Thomas Gleixner t...@linutronix.de Suggested-by: Ingo Molnar mi...@kernel.org Signed-off-by: Matt Fleming matt.flem...@intel.com --- arch/x86/boot/compressed/eboot.c | 1 - arch/x86/include/asm/efi.h | 2 ++ arch/x86/kernel/setup.c | 1 - arch/x86/platform/efi/efi.c | 1 - arch/x86/platform/efi/efi_32.c | 1 - arch/x86/platform/efi/efi_64.c | 1 - arch/x86/platform/uv/bios_uv.c | 1 - 7 files changed, 2 insertions(+), 6 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index b7388a4..3f1dae2 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -7,7 +7,6 @@ * * --- */ -#include linux/efi.h #include linux/pci.h #include asm/efi.h #include asm/setup.h diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 0062a01..b10ea9e 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -1,6 +1,8 @@ #ifndef _ASM_X86_EFI_H #define _ASM_X86_EFI_H +#include linux/efi.h + #ifdef CONFIG_X86_32 #define EFI_LOADER_SIGNATURE EL32 diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index f0de629..35e9883 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -37,7 +37,6 @@ #include linux/root_dev.h #include linux/highmem.h #include linux/module.h -#include linux/efi.h #include linux/init.h #include linux/edd.h #include linux/iscsi_ibft.h diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c7e22ab..543a4d9 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -30,7 +30,6 @@ #include linux/kernel.h #include linux/init.h -#include linux/efi.h #include linux/efi-bgrt.h #include linux/export.h #include linux/bootmem.h diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c index 40e4469..dd566d1 100644 --- a/arch/x86/platform/efi/efi_32.c +++ b/arch/x86/platform/efi/efi_32.c @@ -22,7 +22,6 @@ #include linux/kernel.h #include linux/types.h #include linux/ioport.h -#include linux/efi.h #include asm/io.h #include asm/desc.h diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 39a0e7f..f146de9 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -23,7 +23,6 @@ #include linux/bootmem.h #include linux/ioport.h #include linux/module.h -#include linux/efi.h #include linux/uaccess.h #include linux/io.h #include linux/reboot.h diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index 7666121..e55b074 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -19,7 +19,6 @@ * Copyright (c) Russ Anderson r...@sgi.com */ -#include linux/efi.h #include linux/export.h #include asm/efi.h #include linux/io.h -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] x86/efi: Include linux/efi.h in asm/efi.h
The patch description doesn't match what the patch does. We do not normally have the asm file include the linux file, which is what the patch seems to do. Matt Fleming m...@console-pimps.org wrote: From: Matt Fleming matt.flem...@intel.com Every file that includes asm/efi.h also includes linux/efi.h. Just include linux/efi.h directly and avoid the duplication. Cc: H. Peter Anvin h...@zytor.com Cc: Thomas Gleixner t...@linutronix.de Suggested-by: Ingo Molnar mi...@kernel.org Signed-off-by: Matt Fleming matt.flem...@intel.com --- arch/x86/boot/compressed/eboot.c | 1 - arch/x86/include/asm/efi.h | 2 ++ arch/x86/kernel/setup.c | 1 - arch/x86/platform/efi/efi.c | 1 - arch/x86/platform/efi/efi_32.c | 1 - arch/x86/platform/efi/efi_64.c | 1 - arch/x86/platform/uv/bios_uv.c | 1 - 7 files changed, 2 insertions(+), 6 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index b7388a4..3f1dae2 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -7,7 +7,6 @@ * * --- */ -#include linux/efi.h #include linux/pci.h #include asm/efi.h #include asm/setup.h diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 0062a01..b10ea9e 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -1,6 +1,8 @@ #ifndef _ASM_X86_EFI_H #define _ASM_X86_EFI_H +#include linux/efi.h + #ifdef CONFIG_X86_32 #define EFI_LOADER_SIGNATURE EL32 diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index f0de629..35e9883 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -37,7 +37,6 @@ #include linux/root_dev.h #include linux/highmem.h #include linux/module.h -#include linux/efi.h #include linux/init.h #include linux/edd.h #include linux/iscsi_ibft.h diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c7e22ab..543a4d9 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -30,7 +30,6 @@ #include linux/kernel.h #include linux/init.h -#include linux/efi.h #include linux/efi-bgrt.h #include linux/export.h #include linux/bootmem.h diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c index 40e4469..dd566d1 100644 --- a/arch/x86/platform/efi/efi_32.c +++ b/arch/x86/platform/efi/efi_32.c @@ -22,7 +22,6 @@ #include linux/kernel.h #include linux/types.h #include linux/ioport.h -#include linux/efi.h #include asm/io.h #include asm/desc.h diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 39a0e7f..f146de9 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -23,7 +23,6 @@ #include linux/bootmem.h #include linux/ioport.h #include linux/module.h -#include linux/efi.h #include linux/uaccess.h #include linux/io.h #include linux/reboot.h diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c index 7666121..e55b074 100644 --- a/arch/x86/platform/uv/bios_uv.c +++ b/arch/x86/platform/uv/bios_uv.c @@ -19,7 +19,6 @@ * Copyright (c) Russ Anderson r...@sgi.com */ -#include linux/efi.h #include linux/export.h #include asm/efi.h #include linux/io.h -- Sent from my mobile phone. Please pardon brevity and lack of formatting. -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] x86/efi: Include linux/efi.h in asm/efi.h
On Fri, 11 Oct, at 08:25:26AM, H. Peter Anvin wrote:
The patch description doesn't match what the patch does.
Oh, bah. I see I wrote the patch description in a weird way. It should
have said something like,
Every file that includes asm/efi.h also includes linux/efi.h. Move
the inclusion of the linux file into asm/efi.h so that users only
need to include one header.
We do not normally have the asm file include the linux file, which is
what the patch seems to do.
Ingo suggested this because no file in arch/x86 includes asm/efi.h
without also including linux/efi.h, because asm/efi.h makes use of the
definitions in linux/efi.h. Admittedly this whole thing is a little
backwards.
I know that the usual idiom is to include the linux file, but x86 is the
only architecture to provide an asm/efi.h header, which means that to
stick with the usual idiom, we'd need an #ifdef CONFIG_X86 in
linux/efi.h to automatically include the asm file.
Unless we have an empty efi.h in include/asm-generic?
In fact, now I think about it, that would be a much better solution
because there's already a bunch of x86-specific defintions in
linux/efi.h, so the asm-generic/efi.h wouldn't be empty because we could
split out things like...
#ifdef CONFIG_X86
extern void efi_late_init(void);
extern void efi_free_boot_services(void);
extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long
size);
#else
static inline void efi_late_init(void) {}
static inline void efi_free_boot_services(void) {}
static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned
long size)
{
return EFI_SUCCESS;
}
#endif
Thoughts?
[ Patch included because I dropped Ingo from original Cc list ]
Matt Fleming m...@console-pimps.org wrote:
From: Matt Fleming matt.flem...@intel.com
Every file that includes asm/efi.h also includes linux/efi.h. Just
include linux/efi.h directly and avoid the duplication.
Cc: H. Peter Anvin h...@zytor.com
Cc: Thomas Gleixner t...@linutronix.de
Suggested-by: Ingo Molnar mi...@kernel.org
Signed-off-by: Matt Fleming matt.flem...@intel.com
---
arch/x86/boot/compressed/eboot.c | 1 -
arch/x86/include/asm/efi.h | 2 ++
arch/x86/kernel/setup.c | 1 -
arch/x86/platform/efi/efi.c | 1 -
arch/x86/platform/efi/efi_32.c | 1 -
arch/x86/platform/efi/efi_64.c | 1 -
arch/x86/platform/uv/bios_uv.c | 1 -
7 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c
b/arch/x86/boot/compressed/eboot.c
index b7388a4..3f1dae2 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -7,7 +7,6 @@
*
*
---
*/
-#include linux/efi.h
#include linux/pci.h
#include asm/efi.h
#include asm/setup.h
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0062a01..b10ea9e 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -1,6 +1,8 @@
#ifndef _ASM_X86_EFI_H
#define _ASM_X86_EFI_H
+#include linux/efi.h
+
#ifdef CONFIG_X86_32
#define EFI_LOADER_SIGNATUREEL32
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f0de629..35e9883 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -37,7 +37,6 @@
#include linux/root_dev.h
#include linux/highmem.h
#include linux/module.h
-#include linux/efi.h
#include linux/init.h
#include linux/edd.h
#include linux/iscsi_ibft.h
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c7e22ab..543a4d9 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -30,7 +30,6 @@
#include linux/kernel.h
#include linux/init.h
-#include linux/efi.h
#include linux/efi-bgrt.h
#include linux/export.h
#include linux/bootmem.h
diff --git a/arch/x86/platform/efi/efi_32.c
b/arch/x86/platform/efi/efi_32.c
index 40e4469..dd566d1 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -22,7 +22,6 @@
#include linux/kernel.h
#include linux/types.h
#include linux/ioport.h
-#include linux/efi.h
#include asm/io.h
#include asm/desc.h
diff --git a/arch/x86/platform/efi/efi_64.c
b/arch/x86/platform/efi/efi_64.c
index 39a0e7f..f146de9 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -23,7 +23,6 @@
#include linux/bootmem.h
#include linux/ioport.h
#include linux/module.h
-#include linux/efi.h
#include linux/uaccess.h
#include linux/io.h
#include linux/reboot.h
diff --git a/arch/x86/platform/uv/bios_uv.c
b/arch/x86/platform/uv/bios_uv.c
index 7666121..e55b074 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -19,7 +19,6 @@
* Copyright (c) Russ Anderson r...@sgi.com
*/
-#include linux/efi.h
#include linux/export.h
#include asm/efi.h
#include linux/io.h
--
Sent from my
[PATCH v3] efivars,efi-pstore: Hold off deletion of sysfs entry until, the scan is completed
Change from v2:
- Move dynamic memory allocation to efi_pstore_read() before holding
efivars-lock to protect entry-var.Data.
- Access to entry-scanning while holding efivars-lock.
- Move a comment about a returned value from efi_pstore_read_func() to
efi_pstore_read() because size 0 case may happen in efi_pstore_read().
Currently, when mounting pstore file system, a read callback of efi_pstore
driver runs mutiple times as below.
- In the first read callback, scan efivar_sysfs_list from head and pass
a kmsg buffer of a entry to an upper pstore layer.
- In the second read callback, rescan efivar_sysfs_list from the entry and pass
another kmsg buffer to it.
- Repeat the scan and pass until the end of efivar_sysfs_list.
In this process, an entry is read across the multiple read function calls.
To avoid race between the read and erasion, the whole process above is
protected by a spinlock, holding in open() and releasing in close().
At the same time, kmemdup() is called to pass the buffer to pstore filesystem
during it.
And then, it causes a following lockdep warning.
To make the dynamic memory allocation runnable without taking spinlock,
holding off a deletion of sysfs entry if it happens while scanning it
via efi_pstore, and deleting it after the scan is completed.
To implement it, this patch introduces two flags, scanning and deleting,
to efivar_entry.
[1.143710] [ cut here ]
[1.144058] WARNING: CPU: 1 PID: 1 at kernel/lockdep.c:2740
lockdep_trace_alloc+0x104/0x110()
[1.144058] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
[1.144058] Modules linked in:
[1.144058] CPU: 1 PID: 1 Comm: systemd Not tainted 3.11.0-rc5 #2
[1.144058] 0009 8800797e9ae0 816614a5
8800797e9b28
[1.144058] 8800797e9b18 8105510d 0080
0046
[1.144058] 00d0 03af 81ccd0c0
8800797e9b78
[1.144058] Call Trace:
[1.144058] [816614a5] dump_stack+0x54/0x74
[1.144058] [8105510d] warn_slowpath_common+0x7d/0xa0
[1.144058] [8105517c] warn_slowpath_fmt+0x4c/0x50
[1.144058] [8131290f] ? vsscanf+0x57f/0x7b0
[1.144058] [810bbd74] lockdep_trace_alloc+0x104/0x110
[1.144058] [81192da0] __kmalloc_track_caller+0x50/0x280
[1.144058] [815147bb] ?
efi_pstore_read_func.part.1+0x12b/0x170
[1.144058] [8115b260] kmemdup+0x20/0x50
[1.144058] [815147bb] efi_pstore_read_func.part.1+0x12b/0x170
[1.144058] [81514800] ?
efi_pstore_read_func.part.1+0x170/0x170
[1.144058] [815148b4] efi_pstore_read_func+0xb4/0xe0
[1.144058] [81512b7b] __efivar_entry_iter+0xfb/0x120
[1.144058] [8151428f] efi_pstore_read+0x3f/0x50
[1.144058] [8128d7ba] pstore_get_records+0x9a/0x150
[1.158207] [812af25c] ? selinux_d_instantiate+0x1c/0x20
[1.158207] [8128ce30] ? parse_options+0x80/0x80
[1.158207] [8128ced5] pstore_fill_super+0xa5/0xc0
[1.158207] [811ae7d2] mount_single+0xa2/0xd0
[1.158207] [8128ccf8] pstore_mount+0x18/0x20
[1.158207] [811ae8b9] mount_fs+0x39/0x1b0
[1.158207] [81160550] ? __alloc_percpu+0x10/0x20
[1.158207] [811c9493] vfs_kern_mount+0x63/0xf0
[1.158207] [811cbb0e] do_mount+0x23e/0xa20
[1.158207] [8115b51b] ? strndup_user+0x4b/0xf0
[1.158207] [811cc373] SyS_mount+0x83/0xc0
[1.158207] [81673cc2] system_call_fastpath+0x16/0x1b
[1.158207] ---[ end trace 61981bc62de9f6f4 ]---
Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
drivers/firmware/efi/efi-pstore.c | 143 +++---
drivers/firmware/efi/efivars.c| 12 ++--
drivers/firmware/efi/vars.c | 12 +++-
include/linux/efi.h | 2 +
4 files changed, 153 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/efi/efi-pstore.c
b/drivers/firmware/efi/efi-pstore.c
index 5002d50..ebd5dbc 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -18,14 +18,12 @@ module_param_named(pstore_disable, efivars_pstore_disable,
bool, 0644);
static int efi_pstore_open(struct pstore_info *psi)
{
- efivar_entry_iter_begin();
psi-data = NULL;
return 0;
}
static int efi_pstore_close(struct pstore_info *psi)
{
- efivar_entry_iter_end();
psi-data = NULL;
return 0;
}
@@ -91,19 +89,125 @@ static int efi_pstore_read_func(struct efivar_entry
*entry, void *data)
__efivar_entry_get(entry, entry-var.Attributes,
entry-var.DataSize, entry-var.Data);
size = entry-var.DataSize;
+ memcpy(*cb_data-buf, entry-var.Data, (size_t)min_t(unsigned long,
+1024, size));
- *cb_data-buf =
RE: [RFC][PATCH v2] efivars,efi-pstore: Hold off deletion of sysfs entry until the scan is completed
Matt,
I submitted a v3 patch based on my comment below..
Seiji
-Original Message-
From: linux-efi-ow...@vger.kernel.org
[mailto:linux-efi-ow...@vger.kernel.org] On Behalf Of Seiji Aguchi
Sent: Wednesday, October 09, 2013 12:37 PM
To: Matt Fleming
Cc: linux-ker...@vger.kernel.org; linux-efi@vger.kernel.org;
tony.l...@intel.com; matt.flem...@intel.com; dle-
deve...@lists.sourceforge.net; Tomoki Sekiyama
Subject: RE: [RFC][PATCH v2] efivars,efi-pstore: Hold off deletion of sysfs
entry until the scan is completed
Thank you for reviewing.
In my understanding, your point is that all accesses to efivar_entry should
be done while holding __efivars-lock.
@@ -88,8 +103,9 @@ static int efi_pstore_read_func(struct efivar_entry
*entry, void *data)
return 0;
entry-var.DataSize = 1024;
- __efivar_entry_get(entry, entry-var.Attributes,
-entry-var.DataSize, entry-var.Data);
+ efivar_entry_get(entry, entry-var.Attributes,
+ entry-var.DataSize, entry-var.Data);
+
size = entry-var.DataSize;
*cb_data-buf = kmemdup(entry-var.Data, size, GFP_KERNEL);
This isn't safe to do without holding the __efivars-lock, because
there's the potential for someone else to update entry-var.Data and
entry-var.DataSize while you're in the middle of copying the data in
kmemdup(). This could leak to an information leak, though I think you're
safe from an out-of-bounds access because DataSize is never 1024.
I see...
Bu, kmemdup() cannot be called while holding the spinlock.
So, for protecting efivar_entry, I will call kzalloc() before holding the
lock in efi_pstore_read().
and use memcpy() in efi_pstore_read_func().
The pseudo code is as below.
static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
struct pstore_info *psi)
{
*data.buf = kzalloc(1024, GFP_KERNEL);
if (!*data.buf)
return -ENOMEM;
efivar_entry_iter_begin();
size = efi_pstore_sysfs_entry_iter(data,
(struct efivar_entry
**)psi-data);
efivar_entry_iter_end();
if (size = 0)
kfree(*data.buf);
return size;
}
static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
{
[...]
entry-var.DataSize = 1024;
__efivar_entry_get(entry, entry-var.Attributes,
entry-var.DataSize, entry-var.Data);
size = entry-var.DataSize;
memcpy(*cb_data-buf, entry-var.Data, (size_t)min_t(unsigned
long,
1024,
size));
return size;
}
This doesn't look correct to me. You can't access 'entry' outside of the
*_iter_begin() and *_iter_end() blocks. You can't do,
efivar_entry_iter_end():
if (!entry-scanning)
efivar_unregister(entry);
because 'entry' may have already been freed by another CPU.
I will fix it as follows.
if (!entry-scanning) {
efivar_entry_iter_end();
efivar_unregister(entry);
} else
efivar_entry_iter_end();
(efivar_unregister(entry) still runs concurrently.
But, it cannot move inside spinlock because kzalloc() may run while freeing
kobject.)
Is it your expectation?
Seiji
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] EFI: Runtime services virtual mapping
CCing Peter Jones .., Peter, any idea about the grub related problem? On 10/11/13 at 09:42am, Dave Young wrote: Matt, The kernel I referring is the boot kernel aka the 1st kernel, the boot loader is grub2 from Fedora 19. [sorry for top reply because of using webmail] - Original Message - From: Matt Fleming m...@console-pimps.org To: Dave Young dyo...@redhat.com Cc: Borislav Petkov b...@alien8.de, X86 ML x...@kernel.org, LKML linux-ker...@vger.kernel.org, Borislav Petkov b...@suse.de, Matthew Garrett mj...@srcf.ucam.org, H. Peter Anvin h...@zytor.com, James Bottomley james.bottom...@hansenpartnership.com, Vivek Goyal vgo...@redhat.com, linux-efi@vger.kernel.org, fwts-de...@lists.ubuntu.com Sent: Friday, October 11, 2013 6:27:06 PM Subject: Re: [PATCH 12/12] EFI: Runtime services virtual mapping On Fri, 11 Oct, at 02:24:37PM, Dave Young wrote: For the boot efi_reserve_boot_services code, it's mainly for the SetVirtualAddressMap callback use, so boot regions should not be reused before SetVirtualAddressMap, but the overlapping happens before the efi_reserve_boot_services, isn't it a problem? Hang on, which kernel are you referring to here? The boot kernel or the kexec'd kernel? I thought you were saying you noticed the overlap when running in the second (kexec'd) kernel? The only reason that you would see this overlap in the first (boot) kernel is if the bootloader messed up and allocated the kernel text as EfiBootServicesCode/Data. I'd like to believe no bootloaders are still doing that. -- Matt Fleming, Intel Open Source Technology Center -- 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-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
