[PATCH V2] x86/efi: Free allocated memory if remap fails
From: Sai Praneeth
efi_memmap_alloc(), as the name suggests, allocates memory for a new efi
memory map. It's referenced from couple of places, namely,
efi_arch_mem_reserve() and efi_free_boot_services(). These callers,
after allocating memory, remap it for further use. As usual, a routine
check is performed to confirm successful remap. If the remap fails,
ideally, the allocated memory should be freed but presently we just
return without freeing it up. Hence, fix this bug by freeing up the
memory appropriately.
As efi_memmap_alloc() allocates memory depending on whether mm_init()
has already been invoked or not, similarly, while freeing use
memblock_free() to free memory allocated before invoking mm_init() and
__free_pages() to free memory allocated after invoking mm_init().
It's a fact that memremap() and early_memremap() might never fail and
this code might never get a chance to run but to maintain good kernel
programming semantics, we might need this patch.
Signed-off-by: Sai Praneeth Prakhya
Cc: Lee Chun-Yi
Cc: Borislav Petkov
Cc: Tony Luck
Cc: Dave Hansen
Cc: Bhupesh Sharma
Cc: Ricardo Neri
Cc: Peter Zijlstra
Cc: Ravi Shankar
Cc: Matt Fleming
Cc: Ard Biesheuvel
---
I found this bug when working on a different patch set which uses
efi_memmap_alloc() and then noticed that I never freed the allocated
memory. I found it weird, in the sense that, memory is allocated but is
not freed (upon returning from an error). So, wasn't sure if that should
be treated as a bug or should I just leave it as is because everything
works fine even without this patch. Since the effort for the patch is
very minimal, I just went ahead and posted one, so that I could know
your thoughts on it.
Changes from V1 to V2:
--
1. Fix the bug of freeing memory map that was just installed by correctly
calling free_pages().
2. Call memblock_free() and __free_pages() directly from the appropriate
places instead of efi_memmap_free().
Note: Patch based on Linus's mainline tree V4.18-rc1
arch/x86/platform/efi/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 36c1f8b9f7e0..cfa93af97def 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -285,6 +285,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
new = early_memremap(new_phys, new_size);
if (!new) {
pr_err("Failed to map new boot services memmap\n");
+ memblock_free(new_phys, new_size);
return;
}
@@ -429,6 +430,7 @@ void __init efi_free_boot_services(void)
new = memremap(new_phys, new_size, MEMREMAP_WB);
if (!new) {
pr_err("Failed to map new EFI memmap\n");
+ __free_pages(pfn_to_page(PHYS_PFN(new_phys)),
get_order(new_size));
return;
}
@@ -452,6 +454,7 @@ void __init efi_free_boot_services(void)
if (efi_memmap_install(new_phys, num_entries)) {
pr_err("Could not install new EFI memmap\n");
+ __free_pages(pfn_to_page(PHYS_PFN(new_phys)),
get_order(new_size));
return;
}
}
--
2.7.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] x86/efi: Free allocated memory if remap fails
> > > > +void __init efi_memmap_free(phys_addr_t mem, unsigned int
> > > > num_entries)
> > > > +{
> > > > + unsigned long size = num_entries * efi.memmap.desc_size;
> > > > + unsigned int order = get_order(size);
> > > > + phys_addr_t end = mem + size - 1;
> > > > +
> > > > + if (slab_is_available()) {
> > > > + __free_pages(pfn_to_page(PHYS_PFN(mem)), order);
> > > How do you know that the memory you are freeing was allocated when
> > > slab_is_available() was already true?
> > >
> > efi_memmap_free() should be used *only* in conjunction
> > with efi_memmap_alloc()(As I explicitly didn't mention this, maybe it
> > might
> > have confused you).
> >
> > When allocating memory efi_memmap_alloc() does similar check
> > for slab_is_available() and if so, it allocates memory using
> > alloc_pages().
> > So, to free pages allocated using alloc_pages(), efi_memmap_free()
> > uses __free_pages().
> >
> I understand that. But by abstracting away the free() routine as well
> as the alloc() routine, you are hiding this fact.
>
> What is preventing me from using efi_memmap_alloc() to allocate space
> for the memmap, and using efi_memmap_free() in another place? How are
> you preventing that this does not happen in a way where mm_init() may
> be called in the mean time?
>
> Whether __free_pages() should be used or memblock_free() is a property
> of the *allocation* itself, not of whether mm_init() has already been
> called. So if (!slab_is_available()), you can use memblock_free().
> However, if (slab_is_available()), you cannot use __free_pages()
> because the allocation could have been made before mm_init() was
> called.
>
Aahh.. Thanks a lot! for making it clear. I see the bug now
(efi_memmap_alloc() could be called before mm_init() in which case it uses
memblock_alloc() where as efi_memmap_free() could be called after mm_init() in
which case it uses __free_pages()).
I will fix this.
Regards,
Sai
--
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: Free allocated memory if remap fails
On 18 June 2018 at 19:20, Sai Praneeth Prakhya
wrote:
>
>> > It's a fact that memremap() and early_memremap() might never fail and
>> > this code might never get a chance to run but to maintain good kernel
>> > programming semantics, we might need this patch.
>> >
>> > Signed-off-by: Sai Praneeth Prakhya
>> > Reviewed-by: Ricardo Neri
>> Please don't include tags for reviews that did not happen on-list.
>>
>
> Sure! Thanks for letting me know.
>
>> > @@ -450,10 +451,11 @@ void __init efi_free_boot_services(void)
>> >
>> > memunmap(new);
>> >
>> > - if (efi_memmap_install(new_phys, num_entries)) {
>> > + if (efi_memmap_install(new_phys, num_entries))
>> > pr_err("Could not install new EFI memmap\n");
>> > - return;
>> > - }
>> > +
>> > +free_mem:
>> > + efi_memmap_free(new_phys, num_entries);
>> Doesn't this free the memory map that you just installed?
>>
>
> That's true! It's a bug. I will fix it.
>
>> >
>> > }
>> >
>> > /**
>> > + * efi_memmap_free - Free memory allocated by efi_memmap_alloc()
>> > + * @mem: Physical address allocated by efi_memmap_alloc()
>> > + * @num_entries: Number of entries in the allocated map.
>> > + *
>> > + * efi_memmap_alloc() allocates memory depending on whether mm_init()
>> > + * has already been invoked or not. It uses either memblock or "normal"
>> > + * page allocation. Use this function to free the memory allocated by
>> > + * efi_memmap_alloc(). Since the allocation is done in two different
>> > + * ways, similarly, we free it in two different ways.
>> > + *
>> > + */
>> > +void __init efi_memmap_free(phys_addr_t mem, unsigned int num_entries)
>> > +{
>> > + unsigned long size = num_entries * efi.memmap.desc_size;
>> > + unsigned int order = get_order(size);
>> > + phys_addr_t end = mem + size - 1;
>> > +
>> > + if (slab_is_available()) {
>> > + __free_pages(pfn_to_page(PHYS_PFN(mem)), order);
>> How do you know that the memory you are freeing was allocated when
>> slab_is_available() was already true?
>>
>
> efi_memmap_free() should be used *only* in conjunction
> with efi_memmap_alloc()(As I explicitly didn't mention this, maybe it might
> have confused you).
>
> When allocating memory efi_memmap_alloc() does similar check
> for slab_is_available() and if so, it allocates memory using alloc_pages().
> So, to free pages allocated using alloc_pages(), efi_memmap_free()
> uses __free_pages().
>
I understand that. But by abstracting away the free() routine as well
as the alloc() routine, you are hiding this fact.
What is preventing me from using efi_memmap_alloc() to allocate space
for the memmap, and using efi_memmap_free() in another place? How are
you preventing that this does not happen in a way where mm_init() may
be called in the mean time?
Whether __free_pages() should be used or memblock_free() is a property
of the *allocation* itself, not of whether mm_init() has already been
called. So if (!slab_is_available()), you can use memblock_free().
However, if (slab_is_available()), you cannot use __free_pages()
because the allocation could have been made before mm_init() was
called.
>> >
>> > + return;
>> > + }
>> > +
>> > + if (memblock_free(mem, size))
>> > + pr_err("Failed to free mem from %pa to %pa\n", ,
>> > );
>> > +}
>> > +
>
> Regards,
> Sai
--
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: Free allocated memory if remap fails
> > It's a fact that memremap() and early_memremap() might never fail and
> > this code might never get a chance to run but to maintain good kernel
> > programming semantics, we might need this patch.
> >
> > Signed-off-by: Sai Praneeth Prakhya
> > Reviewed-by: Ricardo Neri
> Please don't include tags for reviews that did not happen on-list.
>
Sure! Thanks for letting me know.
> > @@ -450,10 +451,11 @@ void __init efi_free_boot_services(void)
> >
> > memunmap(new);
> >
> > - if (efi_memmap_install(new_phys, num_entries)) {
> > + if (efi_memmap_install(new_phys, num_entries))
> > pr_err("Could not install new EFI memmap\n");
> > - return;
> > - }
> > +
> > +free_mem:
> > + efi_memmap_free(new_phys, num_entries);
> Doesn't this free the memory map that you just installed?
>
That's true! It's a bug. I will fix it.
> >
> > }
> >
> > /**
> > + * efi_memmap_free - Free memory allocated by efi_memmap_alloc()
> > + * @mem: Physical address allocated by efi_memmap_alloc()
> > + * @num_entries: Number of entries in the allocated map.
> > + *
> > + * efi_memmap_alloc() allocates memory depending on whether mm_init()
> > + * has already been invoked or not. It uses either memblock or "normal"
> > + * page allocation. Use this function to free the memory allocated by
> > + * efi_memmap_alloc(). Since the allocation is done in two different
> > + * ways, similarly, we free it in two different ways.
> > + *
> > + */
> > +void __init efi_memmap_free(phys_addr_t mem, unsigned int num_entries)
> > +{
> > + unsigned long size = num_entries * efi.memmap.desc_size;
> > + unsigned int order = get_order(size);
> > + phys_addr_t end = mem + size - 1;
> > +
> > + if (slab_is_available()) {
> > + __free_pages(pfn_to_page(PHYS_PFN(mem)), order);
> How do you know that the memory you are freeing was allocated when
> slab_is_available() was already true?
>
efi_memmap_free() should be used *only* in conjunction
with efi_memmap_alloc()(As I explicitly didn't mention this, maybe it might
have confused you).
When allocating memory efi_memmap_alloc() does similar check
for slab_is_available() and if so, it allocates memory using alloc_pages().
So, to free pages allocated using alloc_pages(), efi_memmap_free()
uses __free_pages().
> >
> > + return;
> > + }
> > +
> > + if (memblock_free(mem, size))
> > + pr_err("Failed to free mem from %pa to %pa\n", ,
> > );
> > +}
> > +
Regards,
Sai
--
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] efi: cper: avoid using get_seconds()
On 18 June 2018 at 17:54, Arnd Bergmann wrote:
> On Mon, Jun 18, 2018 at 5:50 PM, Ard Biesheuvel
> wrote:
>> On 18 June 2018 at 17:49, Arnd Bergmann wrote:
>>> On Mon, Jun 18, 2018 at 5:47 PM, Ard Biesheuvel
>>> wrote:
On 18 June 2018 at 16:17, Arnd Bergmann wrote:
>>>
> - atomic64_set(, ((u64)get_seconds()) << 32);
> + if (!atomic64_read()) {
> + time64_t time = ktime_get_real_seconds();
> +
> + /*
> +* This code is unlikely to still be needed in year 2106,
> +* but just in case, let's use a few more bits for
> timestamps
> +* after y2038 to be sure they keep increasing
> monotonically
> +* for the next few hundred years...
> +*/
> + if (time < 0x8000)
> + atomic64_set(, (ktime_get_real_seconds()) <<
> 32);
> + else
> + atomic64_set(, 0x8000ull |
> + ktime_get_real_seconds() <<
> 24);
> + }
Given that these values are never decoded and interpreted as
timestamps, can't we simply switch to the second flavour immediately?
>>>
>>> I considered that, but the downside would be that all future filenames would
>>> come before all past file names.
>>
>> Won't we have that same problem in 2038?
>
> No, it goes from 0x7fff to 0x8000, followed
> by 0x8100.
>
Ah, right. I'm with you now :-)
I'll queue this in the efi tree.
--
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] efi: cper: avoid using get_seconds()
On Mon, Jun 18, 2018 at 5:50 PM, Ard Biesheuvel
wrote:
> On 18 June 2018 at 17:49, Arnd Bergmann wrote:
>> On Mon, Jun 18, 2018 at 5:47 PM, Ard Biesheuvel
>> wrote:
>>> On 18 June 2018 at 16:17, Arnd Bergmann wrote:
>>
- atomic64_set(, ((u64)get_seconds()) << 32);
+ if (!atomic64_read()) {
+ time64_t time = ktime_get_real_seconds();
+
+ /*
+* This code is unlikely to still be needed in year 2106,
+* but just in case, let's use a few more bits for
timestamps
+* after y2038 to be sure they keep increasing
monotonically
+* for the next few hundred years...
+*/
+ if (time < 0x8000)
+ atomic64_set(, (ktime_get_real_seconds()) <<
32);
+ else
+ atomic64_set(, 0x8000ull |
+ ktime_get_real_seconds() << 24);
+ }
>>>
>>> Given that these values are never decoded and interpreted as
>>> timestamps, can't we simply switch to the second flavour immediately?
>>
>> I considered that, but the downside would be that all future filenames would
>> come before all past file names.
>
> Won't we have that same problem in 2038?
No, it goes from 0x7fff to 0x8000, followed
by 0x8100.
Arnd
--
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] efi: cper: avoid using get_seconds()
On 18 June 2018 at 17:49, Arnd Bergmann wrote:
> On Mon, Jun 18, 2018 at 5:47 PM, Ard Biesheuvel
> wrote:
>> On 18 June 2018 at 16:17, Arnd Bergmann wrote:
>
>>> - atomic64_set(, ((u64)get_seconds()) << 32);
>>> + if (!atomic64_read()) {
>>> + time64_t time = ktime_get_real_seconds();
>>> +
>>> + /*
>>> +* This code is unlikely to still be needed in year 2106,
>>> +* but just in case, let's use a few more bits for
>>> timestamps
>>> +* after y2038 to be sure they keep increasing monotonically
>>> +* for the next few hundred years...
>>> +*/
>>> + if (time < 0x8000)
>>> + atomic64_set(, (ktime_get_real_seconds()) <<
>>> 32);
>>> + else
>>> + atomic64_set(, 0x8000ull |
>>> + ktime_get_real_seconds() << 24);
>>> + }
>>
>> Given that these values are never decoded and interpreted as
>> timestamps, can't we simply switch to the second flavour immediately?
>
> I considered that, but the downside would be that all future filenames would
> come before all past file names.
Won't we have that same problem in 2038?
> I don't know if the order is important at
> all, but the current implementation at least looks like it's intended to keep
> all file names strictly sorted across boots.
>
> Arnd
--
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] efi: cper: avoid using get_seconds()
On Mon, Jun 18, 2018 at 5:47 PM, Ard Biesheuvel
wrote:
> On 18 June 2018 at 16:17, Arnd Bergmann wrote:
>> - atomic64_set(, ((u64)get_seconds()) << 32);
>> + if (!atomic64_read()) {
>> + time64_t time = ktime_get_real_seconds();
>> +
>> + /*
>> +* This code is unlikely to still be needed in year 2106,
>> +* but just in case, let's use a few more bits for timestamps
>> +* after y2038 to be sure they keep increasing monotonically
>> +* for the next few hundred years...
>> +*/
>> + if (time < 0x8000)
>> + atomic64_set(, (ktime_get_real_seconds()) << 32);
>> + else
>> + atomic64_set(, 0x8000ull |
>> + ktime_get_real_seconds() << 24);
>> + }
>
> Given that these values are never decoded and interpreted as
> timestamps, can't we simply switch to the second flavour immediately?
I considered that, but the downside would be that all future filenames would
come before all past file names. I don't know if the order is important at
all, but the current implementation at least looks like it's intended to keep
all file names strictly sorted across boots.
Arnd
--
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] efi: cper: avoid using get_seconds()
On 18 June 2018 at 16:17, Arnd Bergmann wrote:
> get_seconds() is deprecated because of the 32-bit time overflow
> in y2038/y2106 on 32-bit architectures. The way it is used in
> cper_next_record_id() causes an overflow in 2106 when unsigned UTC
> seconds overflow, even on 64-bit architectures.
>
> This starts using ktime_get_real_seconds() to give us more than 32 bits
> of timestamp on all architectures, and then changes the algorithm to use
> 39 bits for the timestamp after the y2038 wrap date, plus an always-1
> bit at the top. This gives us another 127 epochs of 136 years, with
> strictly monotonically increasing sequence numbers across boots.
>
> This is almost certainly overkill, but seems better than just extending
> the deadline from 2038 to 2106.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/firmware/efi/cper.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 3bf0dca378a6..b73fc4cab083 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -48,8 +48,21 @@ u64 cper_next_record_id(void)
> {
> static atomic64_t seq;
>
> - if (!atomic64_read())
> - atomic64_set(, ((u64)get_seconds()) << 32);
> + if (!atomic64_read()) {
> + time64_t time = ktime_get_real_seconds();
> +
> + /*
> +* This code is unlikely to still be needed in year 2106,
> +* but just in case, let's use a few more bits for timestamps
> +* after y2038 to be sure they keep increasing monotonically
> +* for the next few hundred years...
> +*/
> + if (time < 0x8000)
> + atomic64_set(, (ktime_get_real_seconds()) << 32);
> + else
> + atomic64_set(, 0x8000ull |
> + ktime_get_real_seconds() << 24);
> + }
Given that these values are never decoded and interpreted as
timestamps, can't we simply switch to the second flavour immediately?
>
> return atomic64_inc_return();
> }
> --
> 2.9.0
>
--
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 v2 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
On systems where fbcon is configured for deferred console takeover, the
intend is for the framebuffer to show the boot graphics (e.g a vendor
logo) until some message (e.g. an error) is printed or a graphical
session takes over.
Some firmware relies on the OS to show the boot graphics.
This patch adds support to efifb to show the boot graphics and
automatically enables this when fbcon is configured for deferred
console takeover.
Signed-off-by: Hans de Goede
---
Changes in v2:
-Simplify the comment about acpi_bgrt.status
-Clear the parts of the screen which don't contain the logo to black
(memset to 0), rather then leaving them as is
---
drivers/video/fbdev/efifb.c | 140
1 file changed, 140 insertions(+)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 46a4484e3da7..fa01eecc0a55 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -9,16 +9,39 @@
#include
#include
+#include
#include
#include
#include
#include
+#include
#include
#include
#include
#include /* For drm_get_panel_orientation_quirk */
#include /* For DRM_MODE_PANEL_ORIENTATION_* */
+struct bmp_file_header {
+ u16 id;
+ u32 file_size;
+ u32 reserved;
+ u32 bitmap_offset;
+} __packed;
+
+struct bmp_dib_header {
+ u32 dib_header_size;
+ s32 width;
+ s32 height;
+ u16 planes;
+ u16 bpp;
+ u32 compression;
+ u32 bitmap_size;
+ u32 horz_resolution;
+ u32 vert_resolution;
+ u32 colors_used;
+ u32 colors_important;
+} __packed;
+
static bool request_mem_succeeded = false;
static bool nowc = false;
@@ -66,6 +89,121 @@ static int efifb_setcolreg(unsigned regno, unsigned red,
unsigned green,
return 0;
}
+/*
+ * If fbcon deffered console takeover is configured, the intent is for the
+ * framebuffer to show the boot graphics (e.g. vendor logo) until there is some
+ * (error) message to display. But the boot graphics may have been destroyed by
+ * e.g. option ROM output, detect this and restore the boot graphics.
+ */
+#if defined CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER && \
+defined CONFIG_ACPI_BGRT
+static void efifb_copy_bmp(u8 *src, u32 *dst, int width, struct screen_info
*si)
+{
+ u8 r, g, b;
+
+ while (width--) {
+ b = *src++;
+ g = *src++;
+ r = *src++;
+ *dst++ = (r << si->red_pos) |
+(g << si->green_pos) |
+(b << si->blue_pos);
+ }
+}
+
+static void efifb_show_boot_graphics(struct fb_info *info)
+{
+ u32 bmp_width, bmp_height, bmp_pitch, screen_pitch, dst_x, y, src_y;
+ struct screen_info *si = _info;
+ struct bmp_file_header *file_header;
+ struct bmp_dib_header *dib_header;
+ void *bgrt_image = NULL;
+ u8 *dst = info->screen_base;
+
+ if (!bgrt_tab.image_address) {
+ pr_info("efifb: No BGRT, not showing boot graphics\n");
+ return;
+ }
+
+ /* Avoid flashing the logo if we're going to print std probe messages */
+ if (console_loglevel > CONSOLE_LOGLEVEL_QUIET)
+ return;
+
+ /* bgrt_tab.status is unreliable, so we don't check it */
+
+ if (si->lfb_depth != 32) {
+ pr_info("efifb: not 32 bits, not showing boot graphics\n");
+ return;
+ }
+
+ bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+ MEMREMAP_WB);
+ if (!bgrt_image) {
+ pr_warn("efifb: Ignoring BGRT: failed to map image memory\n");
+ return;
+ }
+
+ if (bgrt_image_size < (sizeof(*file_header) + sizeof(*dib_header)))
+ goto error;
+
+ file_header = bgrt_image;
+ if (file_header->id != 0x4d42 || file_header->reserved != 0)
+ goto error;
+
+ dib_header = bgrt_image + sizeof(*file_header);
+ if (dib_header->dib_header_size != 40 || dib_header->width < 0 ||
+ dib_header->planes != 1 || dib_header->bpp != 24 ||
+ dib_header->compression != 0)
+ goto error;
+
+ bmp_width = dib_header->width;
+ bmp_height = abs(dib_header->height);
+ bmp_pitch = round_up(3 * bmp_width, 4);
+ screen_pitch = si->lfb_linelength;
+
+ if ((file_header->bitmap_offset + bmp_pitch * bmp_height) >
+ bgrt_image_size)
+ goto error;
+
+ if ((bgrt_tab.image_offset_x + bmp_width) > si->lfb_width ||
+ (bgrt_tab.image_offset_y + bmp_height) > si->lfb_height)
+ goto error;
+
+ pr_info("efifb: showing boot graphics\n");
+
+ for (y = 0; y < si->lfb_height; y++, dst += si->lfb_linelength) {
+ /* Only background? */
+ if (y < bgrt_tab.image_offset_y ||
+ y >= (bgrt_tab.image_offset_y + bmp_height))
[PATCH v2 1/2] efi/bgrt: Drop __initdata from bgrt_image_size
bgrt_image_size is necessary to (optionally) show the boot graphics from
the efifb code. The efifb driver is a platform driver, using a normal
driver probe() driver callback. So even though it is always builtin it
cannot reference __initdata.
Acked-by: Ard Biesheuvel
Signed-off-by: Hans de Goede
---
drivers/firmware/efi/efi-bgrt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
index 50793fda7819..b22ccfb0c991 100644
--- a/drivers/firmware/efi/efi-bgrt.c
+++ b/drivers/firmware/efi/efi-bgrt.c
@@ -20,7 +20,7 @@
#include
struct acpi_table_bgrt bgrt_tab;
-size_t __initdata bgrt_image_size;
+size_t bgrt_image_size;
struct bmp_header {
u16 id;
--
2.17.1
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] efi: cper: avoid using get_seconds()
get_seconds() is deprecated because of the 32-bit time overflow
in y2038/y2106 on 32-bit architectures. The way it is used in
cper_next_record_id() causes an overflow in 2106 when unsigned UTC
seconds overflow, even on 64-bit architectures.
This starts using ktime_get_real_seconds() to give us more than 32 bits
of timestamp on all architectures, and then changes the algorithm to use
39 bits for the timestamp after the y2038 wrap date, plus an always-1
bit at the top. This gives us another 127 epochs of 136 years, with
strictly monotonically increasing sequence numbers across boots.
This is almost certainly overkill, but seems better than just extending
the deadline from 2038 to 2106.
Signed-off-by: Arnd Bergmann
---
drivers/firmware/efi/cper.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 3bf0dca378a6..b73fc4cab083 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -48,8 +48,21 @@ u64 cper_next_record_id(void)
{
static atomic64_t seq;
- if (!atomic64_read())
- atomic64_set(, ((u64)get_seconds()) << 32);
+ if (!atomic64_read()) {
+ time64_t time = ktime_get_real_seconds();
+
+ /*
+* This code is unlikely to still be needed in year 2106,
+* but just in case, let's use a few more bits for timestamps
+* after y2038 to be sure they keep increasing monotonically
+* for the next few hundred years...
+*/
+ if (time < 0x8000)
+ atomic64_set(, (ktime_get_real_seconds()) << 32);
+ else
+ atomic64_set(, 0x8000ull |
+ ktime_get_real_seconds() << 24);
+ }
return atomic64_inc_return();
}
--
2.9.0
--
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 0/2] efi: add support for cacheable efifb mappings
On 15 June 2018 at 18:39, Ard Biesheuvel wrote: > On 15 June 2018 at 18:38, Laszlo Ersek wrote: >> On 06/15/18 12:48, Ard Biesheuvel wrote: >>> KVM guests on ARM have special requirements when it comes to mapping >>> framebuffers: given that the host [emulating the framebuffer] uses >>> cacheable accesses to read from the framebuffer region, the guest >>> should uses cacheable accesses as well, or coherency is lost, i.e., >>> the host does not see what the guest writes. >>> >>> Modifying PCI drivers to take this into account just for KVM on ARM is >>> unreasonable, given that mapping BARs cacheable and still expecting >>> side effects does not make any sense. However, doing the same for >>> regions of system memory does make sense, since a framebuffer in DRAM >>> could be accessed via DMA by a coherent master, and so it makes sense >>> to take the memory attributes described by the UEFI memory map into >>> account if it covers the efifb region. >>> >>> Patch #2 implements this. Patch #1 is a preparatory patch that makes >>> efi_mem_desc_lookup() usable for us in #2. >>> >>> Question is how to best test this. I tried Gerd's ramfb patches against >>> QEMU with a recent ArmVirtQemu build but I am having trouble getting my >>> console to use the EFI framebuffer. >>> >>> Ard Biesheuvel (2): >>> efi: drop type and attribute checks in efi_mem_desc_lookup() >>> fbdev/efifb: honour UEFI memory map attributes when mapping the fb >>> >>> arch/x86/platform/efi/quirks.c | 3 +- >>> drivers/firmware/efi/efi.c | 8 +-- >>> drivers/firmware/efi/esrt.c| 5 +- >>> drivers/video/fbdev/efifb.c| 52 >>> 4 files changed, 49 insertions(+), 19 deletions(-) >>> >> >> I reproduced the issue using the latest stock F28 kernel in the KVM >> guest, on my Mustang A3 host. Then I built this set on top of >> v4.17-12074-g4c5e8fc62d6a, using the F28 kernel config. I didn't pass >> any parameters to efifb. The series works perfectly for me. >> >> Tested-by: Laszlo Ersek >> > > Thanks! Very happy to hear that ... Peter, any objections? If not, I will resend them with Laszlo's T-b and cc the fbdev list and maintainer to get this into v4.19 Thanks, Ard. -- 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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
On 18 June 2018 at 11:13, Hans de Goede wrote:
> Hi,
>
>
> On 18-06-18 10:43, Ard Biesheuvel wrote:
>>
>> On 18 June 2018 at 10:30, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 18-06-18 09:36, Ard Biesheuvel wrote:
Hallo Hans,
On 17 June 2018 at 17:32, Hans de Goede wrote:
>
>
> On systems where fbcon is configured for deferred console takeover, the
> intend is for the framebuffer to show the boot graphics (e.g a vendor
> logo) until some message (e.g. an error) is printed or a graphical
> session takes over.
>
> Some firmware however relies on the OS to show the boot graphics
> (indicated by bgrt_tab.status being 0) and the boot graphics may have
> been destroyed by e.g. the grub boot menu.
>
> This patch adds support to efifb to show the boot graphics and
> automatically enables this when fbcon is configured for deferred
> console takeover.
>
> Signed-off-by: Hans de Goede
I have tested this code on ARM QEMU/mach-virt, and with a little tweak
(which I will post separately), the code works as expected, i.e., it
redraws the boot logo based on the contents of the BGRT table.
>>>
>>>
>>>
>>> That is great.
>>>
However, what it doesn't do is clear the screen, which means the logo
is drawn on top of whatever the boot environment left behind, and I
end up with something like this.
http://people.linaro.org/~ard.biesheuvel/mach-virt-bgrt-logo-redrawn.png
>>>
>>>
>>>
>>> Hmm, less great. I'm not sure how to deal with this, on x86 it is more
>>> or less guaranteed that the screen is already cleared when we load and
>>> clearing a 4k screen means writing about 32MB, which I guess with modern
>>> RAM speeds is not that bad actually.
>>>
>>> I see that you got this picture by manual booting from the EFI shell,
>>> in what state does the firmware / bootloader normally leave the
>>> framebuffer?
>>
>>
>> UEFI does not usually clear the screen when it boots the default
>> entry, so it is entirely dependent on the boot loader that runs next.
>> I guess GRUB usually clears the screen unconditionally?
>
>
> It depends, GRUB either leaves it completely alone (leaving the
> BGRT graphics which the firmware hopefully has put up already
> in place) or if it actually draws anything, then it clears iit
> before starting the kernel.
>
>> In any case, I think it is reasonable to clear the screen if you
>> redraw the logo, but I do wonder if it is safe to assume that the
>> background color is black.
>>
>> Instead, we may decide to clear the screen before ExitBootServices()
>> [using EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen()].
>
>
> On most x86 machines (but not all, GRR) the firmware itself will
> draw the logo already. I actually have grub patches pending to make
> it not do any text-output related calls at all unless it actually
> has a message / menu it wants to display.
>
> Calling EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen() will cause
> a bootup sequence like this:
>
> firmware draws logo
> clearscreen
> redraw logo
>
> Which will cause an ugly flash to black. Where as the purpose
> is to have a smooth boot with the logo being on screen all
> the time without any flickers / flashes.
>
> I've never seen a machine where the background is not black,
> the background not being black would also break the spinning
> dots which microsofts draws on top of the background while
> booting, so a memset to 0 seems sensible to me.
>
>
>>> I'm asking because if normally it is either cleared
>>> to black, or already showing the logo I wonder if we should take the
>>> (small) penalty of clearing ?
>>>
>>> Given that we are talking about only 32 MB I could do a v2 which just
>>> memsets the rest of the screen to 0.
>>>
>>> So we get:
>>>
>>> for (y= 0; y < height; y++) {
>>> if (line_part_of_bgrt) {
>>> memset(left-of-bgrt);
>>> draw_bgrt_line(y);
>>> memset(right-of-bgrt);
>>> } else {
>>> memset(line);
>>> }
>>> }
>>>
>>> Note I've deliberately done the if on a per line
>>> base to keep the actual blit part of the loop
>>> efficient and without any extra conditionals in
>>> there. I also don't simply first memset the entire
>>> fb to 0 to avoid a flash / tearing if the bgrt
>>> image is already in place (which happens often on
>>> x86).
>>>
>>> Implementing this is easy and as said the extra execution time should
>>> be quite small, still I wonder what others think about this?
>>>
>>> I'm leaning towards doing the clearing / memsets since I've seen
>>> some firmwares leave some artifacts from not completely clearing
>>> things like a "Press F2 to enter setup" message, missing a few
>>> pixels and leaving those on screen.
>>>
>>
>> I think the overhead of doing the clearing is not going to be the
>> bottleneck. But redrawing
Re: [PATCH] x86/efi: Free allocated memory if remap fails
Hello Sai,
On 16 June 2018 at 03:09, Sai Praneeth Prakhya
wrote:
> From: Sai Praneeth
>
> efi_memmap_alloc(), as the name suggests, allocates memory for a new efi
> memory map. It's referenced from couple of places, namely,
> efi_arch_mem_reserve() and efi_free_boot_services(). These callers,
> after allocating memory, remap it for further use. As usual, a routine
> check is performed to confirm successful remap. If the remap fails,
> ideally, the allocated memory should be freed but presently we just
> return without freeing it up. Hence, fix this bug by introducing
> efi_memmap_free() which frees memory allocated by efi_memmap_alloc().
>
> As efi_memmap_alloc() allocates memory depending on whether mm_init()
> has already been invoked or not, similarly efi_memmap_free() frees
> memory accordingly.
>
> efi_fake_memmap() also references efi_memmap_alloc() but it frees
> memory correctly using memblock_free(), but replace it with
> efi_memmap_free() to maintain consistency, as in, allocate memory with
> efi_memmap_alloc() and free memory with efi_memmap_free().
>
> It's a fact that memremap() and early_memremap() might never fail and
> this code might never get a chance to run but to maintain good kernel
> programming semantics, we might need this patch.
>
> Signed-off-by: Sai Praneeth Prakhya
> Reviewed-by: Ricardo Neri
Please don't include tags for reviews that did not happen on-list.
> Cc: Lee Chun-Yi
> Cc: Borislav Petkov
> Cc: Tony Luck
> Cc: Dave Hansen
> Cc: Bhupesh Sharma
> Cc: Ricardo Neri
> Cc: Peter Zijlstra
> Cc: Ravi Shankar
> Cc: Matt Fleming
> Cc: Ard Biesheuvel
> ---
>
> I found this bug when working on a different patch set which uses
> efi_memmap_alloc() and then noticed that I never freed the allocated
> memory. I found it weird, in the sense that, memory is allocated but is
> not freed (upon returning from an error). So, wasn't sure if that should
> be treated as a bug or should I just leave it as is because everything
> works fine even without this patch. Since the effort for the patch is
> very minimal, I just went ahead and posted one, so that I could know
> your thoughts on it.
>
> Note: Patch based on Linus's mainline tree V4.17
>
> arch/x86/platform/efi/quirks.c | 10 ++
> drivers/firmware/efi/fake_mem.c | 2 +-
> drivers/firmware/efi/memmap.c | 27 +++
> include/linux/efi.h | 1 +
> 4 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 36c1f8b9f7e0..f223093f2df7 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -285,6 +285,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64
> size)
> new = early_memremap(new_phys, new_size);
> if (!new) {
> pr_err("Failed to map new boot services memmap\n");
> + efi_memmap_free(new_phys, num_entries);
> return;
> }
>
> @@ -429,7 +430,7 @@ void __init efi_free_boot_services(void)
> new = memremap(new_phys, new_size, MEMREMAP_WB);
> if (!new) {
> pr_err("Failed to map new EFI memmap\n");
> - return;
> + goto free_mem;
> }
>
> /*
> @@ -450,10 +451,11 @@ void __init efi_free_boot_services(void)
>
> memunmap(new);
>
> - if (efi_memmap_install(new_phys, num_entries)) {
> + if (efi_memmap_install(new_phys, num_entries))
> pr_err("Could not install new EFI memmap\n");
> - return;
> - }
> +
> +free_mem:
> + efi_memmap_free(new_phys, num_entries);
Doesn't this free the memory map that you just installed?
> }
>
> /*
> diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> index 6c7d60c239b5..63edcedee25b 100644
> --- a/drivers/firmware/efi/fake_mem.c
> +++ b/drivers/firmware/efi/fake_mem.c
> @@ -79,7 +79,7 @@ void __init efi_fake_memmap(void)
> new_memmap = early_memremap(new_memmap_phy,
> efi.memmap.desc_size * new_nr_map);
> if (!new_memmap) {
> - memblock_free(new_memmap_phy, efi.memmap.desc_size *
> new_nr_map);
> + efi_memmap_free(new_memmap_phy, new_nr_map);
> return;
> }
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 5fc70520e04c..27d28cb4652d 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -50,6 +50,33 @@ phys_addr_t __init efi_memmap_alloc(unsigned int
> num_entries)
> }
>
> /**
> + * efi_memmap_free - Free memory allocated by efi_memmap_alloc()
> + * @mem: Physical address allocated by efi_memmap_alloc()
> + * @num_entries: Number of entries in the allocated map.
> + *
> + * efi_memmap_alloc() allocates memory depending on whether mm_init()
> + * has already been invoked or not. It uses either memblock or "normal"
> + * page
Re: [PATCH 0/2] efifb: Copy the ACPI BGRT boot graphics to the
Hi, On 18-06-18 11:23, Daniel Vetter wrote: On Sun, Jun 17, 2018 at 05:32:33PM +0200, Hans de Goede wrote: Hi All, Here is a patch-set to make sure that the efifb contains the boot graphics from the ACPI BGRT extension when the kernel is configured to use the (new) deferred fbcon console takeover support. Let me explain why this is desirable (same reason as for the deferred fbcon console takeover support itself): Various (desktop oriented) Linux distributions have spend a lot of time to not show way too technial boot messages to end users during bootup. What we would really like for the boot experience is something like MacOS X / Windows 10 do. The (EFI) firmware boots up a logo and we leave that in place until the login-manager (e.g. gdm) starts and then the login-manager takes over the framebuffer including the current logo contents and fades that into the login screen. The deferred fbcon console takeover (combined with shim and grub) patches makes the desired boot experience possible, but this assumes that the firmware starts shim with the framebuffer containing the boot graphics. This is not always the case, this patch ensures that the boot graphics are in place. Since the bgrt.status field is not exactly reliable, this commit simply always copies over the bootgraphics. If they are already there this effectively is a no-op. The first patch in this series makes a trivial change to drivers/firmware/efi/efi-bgrt.c, dropping __initdata from bgrt_image_size. Ard, since the second patch depends on the first and the change is really trivial, can we please have your ack for merging the efi-bgrt.c change through the fbdev tree? Random side-comment ... plans to roll out the same for drm drivers? With the client infrastructure Noralf is working on doing that should be fairly straight-forward. Interim step would be to add it to the shared fbdev emulation layer (but that's a bit a hack, and precludes the use of this on fbcon-less systems). I had not really thought about this yet. AFAICT the ACPI BGRT table is part of UEFI, so having it also means having an UEFI framebuffer and I expect us to always use that to be able to show error messages initializing the real drm/kms driver. But I guess in the future the plan it to stop using the efifb linux driver and instead use simple drm, then we will certainly want this in drm. And thinking more about this, currently I'm relying (for a flickerfree experience) on the kms driver taking over the fb setup by the firmware. But I guess it may not always succeed and if it does not succeed, then restoring the bootgraphics (on a quiet boot) would be good too. Once I've everything upstream to make flickerfree work for i915 I plan to look at the amd / nouveau cases next. For those adding BGRT graphics restoration to the drm drivers might make for a good quick fix. We would still get a flicker from the modeset but at least the screen would not be just black until the gui loads if we restore the boot graphics from the drm driver and I guess we could prime the fb with the bootgraphics before the modeset to make the flicker as small as possible. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4.16 274/279] efi/libstub/arm64: Handle randomized TEXT_OFFSET
4.16-stable review patch. If anyone has any objections, please let me know.
--
From: Mark Rutland
[ Upstream commit 4f74d72aa7067e75af92fbab077e6d7d0210be66 ]
When CONFIG_RANDOMIZE_TEXT_OFFSET=y, TEXT_OFFSET is an arbitrary
multiple of PAGE_SIZE in the interval [0, 2MB).
The EFI stub does not account for the potential misalignment of
TEXT_OFFSET relative to EFI_KIMG_ALIGN, and produces a randomized
physical offset which is always a round multiple of EFI_KIMG_ALIGN.
This may result in statically allocated objects whose alignment exceeds
PAGE_SIZE to appear misaligned in memory. This has been observed to
result in spurious stack overflow reports and failure to make use of
the IRQ stacks, and theoretically could result in a number of other
issues.
We can OR in the low bits of TEXT_OFFSET to ensure that we have the
necessary offset (and hence preserve the misalignment of TEXT_OFFSET
relative to EFI_KIMG_ALIGN), so let's do that.
Reported-by: Kim Phillips
Tested-by: Kim Phillips
[ardb: clarify comment and commit log, drop unneeded parens]
Signed-off-by: Mark Rutland
Signed-off-by: Ard Biesheuvel
Cc: Linus Torvalds
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: linux-efi@vger.kernel.org
Fixes: 6f26b3671184c36d ("arm64: kaslr: increase randomization granularity")
Link: http://lkml.kernel.org/r/20180518140841.9731-2-ard.biesheu...@linaro.org
Signed-off-by: Ingo Molnar
Signed-off-by: Sasha Levin
Signed-off-by: Greg Kroah-Hartman
---
drivers/firmware/efi/libstub/arm64-stub.c | 10 ++
1 file changed, 10 insertions(+)
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -98,6 +98,16 @@ efi_status_t handle_kernel_image(efi_sys
(phys_seed >> 32) & mask : TEXT_OFFSET;
/*
+* With CONFIG_RANDOMIZE_TEXT_OFFSET=y, TEXT_OFFSET may not
+* be a multiple of EFI_KIMG_ALIGN, and we must ensure that
+* we preserve the misalignment of 'offset' relative to
+* EFI_KIMG_ALIGN so that statically allocated objects whose
+* alignment exceeds PAGE_SIZE appear correctly aligned in
+* memory.
+*/
+ offset |= TEXT_OFFSET % EFI_KIMG_ALIGN;
+
+ /*
* If KASLR is enabled, and we have some randomness available,
* locate the kernel at a randomized offset in physical memory.
*/
--
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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Hi,
On 18-06-18 10:43, Ard Biesheuvel wrote:
On 18 June 2018 at 10:30, Hans de Goede wrote:
Hi,
On 18-06-18 09:36, Ard Biesheuvel wrote:
Hallo Hans,
On 17 June 2018 at 17:32, Hans de Goede wrote:
On systems where fbcon is configured for deferred console takeover, the
intend is for the framebuffer to show the boot graphics (e.g a vendor
logo) until some message (e.g. an error) is printed or a graphical
session takes over.
Some firmware however relies on the OS to show the boot graphics
(indicated by bgrt_tab.status being 0) and the boot graphics may have
been destroyed by e.g. the grub boot menu.
This patch adds support to efifb to show the boot graphics and
automatically enables this when fbcon is configured for deferred
console takeover.
Signed-off-by: Hans de Goede
I have tested this code on ARM QEMU/mach-virt, and with a little tweak
(which I will post separately), the code works as expected, i.e., it
redraws the boot logo based on the contents of the BGRT table.
That is great.
However, what it doesn't do is clear the screen, which means the logo
is drawn on top of whatever the boot environment left behind, and I
end up with something like this.
http://people.linaro.org/~ard.biesheuvel/mach-virt-bgrt-logo-redrawn.png
Hmm, less great. I'm not sure how to deal with this, on x86 it is more
or less guaranteed that the screen is already cleared when we load and
clearing a 4k screen means writing about 32MB, which I guess with modern
RAM speeds is not that bad actually.
I see that you got this picture by manual booting from the EFI shell,
in what state does the firmware / bootloader normally leave the
framebuffer?
UEFI does not usually clear the screen when it boots the default
entry, so it is entirely dependent on the boot loader that runs next.
I guess GRUB usually clears the screen unconditionally?
It depends, GRUB either leaves it completely alone (leaving the
BGRT graphics which the firmware hopefully has put up already
in place) or if it actually draws anything, then it clears iit
before starting the kernel.
In any case, I think it is reasonable to clear the screen if you
redraw the logo, but I do wonder if it is safe to assume that the
background color is black.
Instead, we may decide to clear the screen before ExitBootServices()
[using EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen()].
On most x86 machines (but not all, GRR) the firmware itself will
draw the logo already. I actually have grub patches pending to make
it not do any text-output related calls at all unless it actually
has a message / menu it wants to display.
Calling EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen() will cause
a bootup sequence like this:
firmware draws logo
clearscreen
redraw logo
Which will cause an ugly flash to black. Where as the purpose
is to have a smooth boot with the logo being on screen all
the time without any flickers / flashes.
I've never seen a machine where the background is not black,
the background not being black would also break the spinning
dots which microsofts draws on top of the background while
booting, so a memset to 0 seems sensible to me.
I'm asking because if normally it is either cleared
to black, or already showing the logo I wonder if we should take the
(small) penalty of clearing ?
Given that we are talking about only 32 MB I could do a v2 which just
memsets the rest of the screen to 0.
So we get:
for (y= 0; y < height; y++) {
if (line_part_of_bgrt) {
memset(left-of-bgrt);
draw_bgrt_line(y);
memset(right-of-bgrt);
} else {
memset(line);
}
}
Note I've deliberately done the if on a per line
base to keep the actual blit part of the loop
efficient and without any extra conditionals in
there. I also don't simply first memset the entire
fb to 0 to avoid a flash / tearing if the bgrt
image is already in place (which happens often on
x86).
Implementing this is easy and as said the extra execution time should
be quite small, still I wonder what others think about this?
I'm leaning towards doing the clearing / memsets since I've seen
some firmwares leave some artifacts from not completely clearing
things like a "Press F2 to enter setup" message, missing a few
pixels and leaving those on screen.
I think the overhead of doing the clearing is not going to be the
bottleneck. But redrawing a logo on black background that was designed
to be displayed over another color is going to look really ugly ...
Do you know of any examples of this ?
There seems to be no known way to get the background color, so black /
all 0 seems the be the best bet.
I would expect any non black background logos to simply be screen
filling.
Regards,
Hans
--
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
Re: [PATCH 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Hi,
On 18-06-18 09:36, Ard Biesheuvel wrote:
Hallo Hans,
On 17 June 2018 at 17:32, Hans de Goede wrote:
On systems where fbcon is configured for deferred console takeover, the
intend is for the framebuffer to show the boot graphics (e.g a vendor
logo) until some message (e.g. an error) is printed or a graphical
session takes over.
Some firmware however relies on the OS to show the boot graphics
(indicated by bgrt_tab.status being 0) and the boot graphics may have
been destroyed by e.g. the grub boot menu.
This patch adds support to efifb to show the boot graphics and
automatically enables this when fbcon is configured for deferred
console takeover.
Signed-off-by: Hans de Goede
I have tested this code on ARM QEMU/mach-virt, and with a little tweak
(which I will post separately), the code works as expected, i.e., it
redraws the boot logo based on the contents of the BGRT table.
That is great.
However, what it doesn't do is clear the screen, which means the logo
is drawn on top of whatever the boot environment left behind, and I
end up with something like this.
http://people.linaro.org/~ard.biesheuvel/mach-virt-bgrt-logo-redrawn.png
Hmm, less great. I'm not sure how to deal with this, on x86 it is more
or less guaranteed that the screen is already cleared when we load and
clearing a 4k screen means writing about 32MB, which I guess with modern
RAM speeds is not that bad actually.
I see that you got this picture by manual booting from the EFI shell,
in what state does the firmware / bootloader normally leave the
framebuffer? I'm asking because if normally it is either cleared
to black, or already showing the logo I wonder if we should take the
(small) penalty of clearing ?
Given that we are talking about only 32 MB I could do a v2 which just
memsets the rest of the screen to 0.
So we get:
for (y= 0; y < height; y++) {
if (line_part_of_bgrt) {
memset(left-of-bgrt);
draw_bgrt_line(y);
memset(right-of-bgrt);
} else {
memset(line);
}
}
Note I've deliberately done the if on a per line
base to keep the actual blit part of the loop
efficient and without any extra conditionals in
there. I also don't simply first memset the entire
fb to 0 to avoid a flash / tearing if the bgrt
image is already in place (which happens often on
x86).
Implementing this is easy and as said the extra execution time should
be quite small, still I wonder what others think about this?
I'm leaning towards doing the clearing / memsets since I've seen
some firmwares leave some artifacts from not completely clearing
things like a "Press F2 to enter setup" message, missing a few
pixels and leaving those on screen.
Regards,
Hans
--
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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Hi, On 18-06-18 10:53, Môshe van der Sterre wrote: Hi Hans, On 06/17/2018 05:32 PM, Hans de Goede wrote: On systems where fbcon is configured for deferred console takeover, the intend is for the framebuffer to show the boot graphics (e.g a vendor logo) until some message (e.g. an error) is printed or a graphical session takes over. Some firmware however relies on the OS to show the boot graphics (indicated by bgrt_tab.status being 0) and the boot graphics may have been destroyed by e.g. the grub boot menu. It may be clearer to just say that the boot graphics may have been destroyed. The reference to the status field and firmware expectations only confuses the intention of this patch, imho. (This ties in to what I say below) This patch adds support to efifb to show the boot graphics and automatically enables this when fbcon is configured for deferred console takeover. + /* +* We do not check bgrt_tab.status here because this seems to only +* reflect if the firmware has shown the boot graphics at all, if it +* later got destroyed by something status will still be 1. +* Since we draw the exact same graphic at the exact same place this +* will not lead to any tearing if the boot graphic is already there. +*/ I agree that ignoring bgrt_tab.status is the absolute best option. The status (valid-bit) can, in the real world, be any value with any meaning. I checked this on a few machines as part of commit 66dbe99cfe30. - My workstation always has 0. - An old server that I checked always has 1. - My laptop has 1 if the boot is uninterrupted, 0 if I request the UEFI boot menu. So, I have the same reservation about this comment as I have about the commit message. Imho, simply mentioning that the status field cannot be relied upon (in any case), would get the point across. Ok, I will simplify both the commit message and comment bits to just state that the status field is unreliable. Regards, Hans -- 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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Hi Hans, On 06/17/2018 05:32 PM, Hans de Goede wrote: > On systems where fbcon is configured for deferred console takeover, the > intend is for the framebuffer to show the boot graphics (e.g a vendor > logo) until some message (e.g. an error) is printed or a graphical > session takes over. > > Some firmware however relies on the OS to show the boot graphics > (indicated by bgrt_tab.status being 0) and the boot graphics may have > been destroyed by e.g. the grub boot menu. It may be clearer to just say that the boot graphics may have been destroyed. The reference to the status field and firmware expectations only confuses the intention of this patch, imho. (This ties in to what I say below) > This patch adds support to efifb to show the boot graphics and > automatically enables this when fbcon is configured for deferred > console takeover. > > + /* > + * We do not check bgrt_tab.status here because this seems to only > + * reflect if the firmware has shown the boot graphics at all, if it > + * later got destroyed by something status will still be 1. > + * Since we draw the exact same graphic at the exact same place this > + * will not lead to any tearing if the boot graphic is already there. > + */ I agree that ignoring bgrt_tab.status is the absolute best option. The status (valid-bit) can, in the real world, be any value with any meaning. I checked this on a few machines as part of commit 66dbe99cfe30. - My workstation always has 0. - An old server that I checked always has 1. - My laptop has 1 if the boot is uninterrupted, 0 if I request the UEFI boot menu. So, I have the same reservation about this comment as I have about the commit message. Imho, simply mentioning that the status field cannot be relied upon (in any case), would get the point across. -- 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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
On 18 June 2018 at 10:30, Hans de Goede wrote:
> Hi,
>
> On 18-06-18 09:36, Ard Biesheuvel wrote:
>>
>> Hallo Hans,
>>
>> On 17 June 2018 at 17:32, Hans de Goede wrote:
>>>
>>> On systems where fbcon is configured for deferred console takeover, the
>>> intend is for the framebuffer to show the boot graphics (e.g a vendor
>>> logo) until some message (e.g. an error) is printed or a graphical
>>> session takes over.
>>>
>>> Some firmware however relies on the OS to show the boot graphics
>>> (indicated by bgrt_tab.status being 0) and the boot graphics may have
>>> been destroyed by e.g. the grub boot menu.
>>>
>>> This patch adds support to efifb to show the boot graphics and
>>> automatically enables this when fbcon is configured for deferred
>>> console takeover.
>>>
>>> Signed-off-by: Hans de Goede
>>
>>
>> I have tested this code on ARM QEMU/mach-virt, and with a little tweak
>> (which I will post separately), the code works as expected, i.e., it
>> redraws the boot logo based on the contents of the BGRT table.
>
>
> That is great.
>
>> However, what it doesn't do is clear the screen, which means the logo
>> is drawn on top of whatever the boot environment left behind, and I
>> end up with something like this.
>>
>> http://people.linaro.org/~ard.biesheuvel/mach-virt-bgrt-logo-redrawn.png
>
>
> Hmm, less great. I'm not sure how to deal with this, on x86 it is more
> or less guaranteed that the screen is already cleared when we load and
> clearing a 4k screen means writing about 32MB, which I guess with modern
> RAM speeds is not that bad actually.
>
> I see that you got this picture by manual booting from the EFI shell,
> in what state does the firmware / bootloader normally leave the
> framebuffer?
UEFI does not usually clear the screen when it boots the default
entry, so it is entirely dependent on the boot loader that runs next.
I guess GRUB usually clears the screen unconditionally?
In any case, I think it is reasonable to clear the screen if you
redraw the logo, but I do wonder if it is safe to assume that the
background color is black.
Instead, we may decide to clear the screen before ExitBootServices()
[using EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.ClearScreen()].
> I'm asking because if normally it is either cleared
> to black, or already showing the logo I wonder if we should take the
> (small) penalty of clearing ?
>
> Given that we are talking about only 32 MB I could do a v2 which just
> memsets the rest of the screen to 0.
>
> So we get:
>
> for (y= 0; y < height; y++) {
> if (line_part_of_bgrt) {
> memset(left-of-bgrt);
> draw_bgrt_line(y);
> memset(right-of-bgrt);
> } else {
> memset(line);
> }
> }
>
> Note I've deliberately done the if on a per line
> base to keep the actual blit part of the loop
> efficient and without any extra conditionals in
> there. I also don't simply first memset the entire
> fb to 0 to avoid a flash / tearing if the bgrt
> image is already in place (which happens often on
> x86).
>
> Implementing this is easy and as said the extra execution time should
> be quite small, still I wonder what others think about this?
>
> I'm leaning towards doing the clearing / memsets since I've seen
> some firmwares leave some artifacts from not completely clearing
> things like a "Press F2 to enter setup" message, missing a few
> pixels and leaving those on screen.
>
I think the overhead of doing the clearing is not going to be the
bottleneck. But redrawing a logo on black background that was designed
to be displayed over another color is going to look really ugly ...
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] efi/arm: preserve early mapping of UEFI memory map longer for BGRT
The BGRT code validates the contents of the table against the UEFI
memory map, and so it expects it to be mapped when the code runs.
On ARM, this is currently not the case, since we tear down the early
mapping after efi_init() completes, and only create the permanent
mapping in arm_enable_runtime_services(), which executes as an early
initcall, but still leaves a window where the UEFI memory map is not
mapped.
So move the call to efi_memmap_unmap() from efi_init() to
arm_enable_runtime_services().
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/arm-init.c| 1 -
drivers/firmware/efi/arm-runtime.c | 2 ++
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 80d1a885def5..a7c522eac640 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -259,7 +259,6 @@ void __init efi_init(void)
reserve_regions();
efi_esrt_init();
- efi_memmap_unmap();
memblock_reserve(params.mmap & PAGE_MASK,
PAGE_ALIGN(params.mmap_size +
diff --git a/drivers/firmware/efi/arm-runtime.c
b/drivers/firmware/efi/arm-runtime.c
index 5889cbea60b8..59a8c0ec94d5 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -115,6 +115,8 @@ static int __init arm_enable_runtime_services(void)
return 0;
}
+ efi_memmap_unmap();
+
if (efi_runtime_disabled()) {
pr_info("EFI runtime services will be disabled.\n");
return 0;
--
2.17.1
--
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 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Hallo Hans,
On 17 June 2018 at 17:32, Hans de Goede wrote:
> On systems where fbcon is configured for deferred console takeover, the
> intend is for the framebuffer to show the boot graphics (e.g a vendor
> logo) until some message (e.g. an error) is printed or a graphical
> session takes over.
>
> Some firmware however relies on the OS to show the boot graphics
> (indicated by bgrt_tab.status being 0) and the boot graphics may have
> been destroyed by e.g. the grub boot menu.
>
> This patch adds support to efifb to show the boot graphics and
> automatically enables this when fbcon is configured for deferred
> console takeover.
>
> Signed-off-by: Hans de Goede
I have tested this code on ARM QEMU/mach-virt, and with a little tweak
(which I will post separately), the code works as expected, i.e., it
redraws the boot logo based on the contents of the BGRT table.
However, what it doesn't do is clear the screen, which means the logo
is drawn on top of whatever the boot environment left behind, and I
end up with something like this.
http://people.linaro.org/~ard.biesheuvel/mach-virt-bgrt-logo-redrawn.png
> ---
> drivers/video/fbdev/efifb.c | 147
> 1 file changed, 147 insertions(+)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 46a4484e3da7..b041d936a438 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -9,16 +9,39 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include /* For drm_get_panel_orientation_quirk */
> #include /* For DRM_MODE_PANEL_ORIENTATION_* */
>
> +struct bmp_file_header {
> + u16 id;
> + u32 file_size;
> + u32 reserved;
> + u32 bitmap_offset;
> +} __packed;
> +
> +struct bmp_dib_header {
> + u32 dib_header_size;
> + s32 width;
> + s32 height;
> + u16 planes;
> + u16 bpp;
> + u32 compression;
> + u32 bitmap_size;
> + u32 horz_resolution;
> + u32 vert_resolution;
> + u32 colors_used;
> + u32 colors_important;
> +} __packed;
> +
> static bool request_mem_succeeded = false;
> static bool nowc = false;
>
> @@ -66,6 +89,128 @@ static int efifb_setcolreg(unsigned regno, unsigned red,
> unsigned green,
> return 0;
> }
>
> +/*
> + * If fbcon deffered console takeover is configured, the intent is for the
> + * framebuffer to show the boot graphics (e.g. vendor logo) until there is
> some
> + * (error) message to display. But the boot graphics may have been destroyed
> by
> + * e.g. option ROM output, detect this and restore the boot graphics.
> + */
> +#if defined CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER && \
> +defined CONFIG_ACPI_BGRT
> +static void efifb_show_boot_graphics(struct fb_info *info)
> +{
> + u32 *dst, bmp_width, bmp_height, bmp_pitch, screen_pitch;
> + struct screen_info *si = _info;
> + struct bmp_file_header *file_header;
> + struct bmp_dib_header *dib_header;
> + void *bgrt_image = NULL;
> + u8 *src, r, g, b;
> + s32 x, y;
> +
> + if (!bgrt_tab.image_address) {
> + pr_info("efifb: No BGRT, not showing boot graphics\n");
> + return;
> + }
> +
> + /* Avoid flashing the logo if we're going to print std probe messages
> */
> + if (console_loglevel > CONSOLE_LOGLEVEL_QUIET)
> + return;
> +
> + /*
> +* We do not check bgrt_tab.status here because this seems to only
> +* reflect if the firmware has shown the boot graphics at all, if it
> +* later got destroyed by something status will still be 1.
> +* Since we draw the exact same graphic at the exact same place this
> +* will not lead to any tearing if the boot graphic is already there.
> +*/
> +
> + if (si->lfb_depth != 32) {
> + pr_info("efifb: not 32 bits, not showing boot graphics\n");
> + return;
> + }
> +
> + bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
> + MEMREMAP_WB);
> + if (!bgrt_image) {
> + pr_warn("efifb: Ignoring BGRT: failed to map image memory\n");
> + return;
> + }
> +
> + if (bgrt_image_size < (sizeof(*file_header) + sizeof(*dib_header)))
> + goto error;
> +
> + file_header = bgrt_image;
> + if (file_header->id != 0x4d42 || file_header->reserved != 0)
> + goto error;
> +
> + dib_header = bgrt_image + sizeof(*file_header);
> + if (dib_header->dib_header_size != 40 || dib_header->width < 0 ||
> + dib_header->planes != 1 || dib_header->bpp != 24 ||
> + dib_header->compression != 0)
> + goto error;
> +
> + bmp_width = dib_header->width;
> + bmp_height = abs(dib_header->height);
> + bmp_pitch =
Re: [PATCH 1/2] efi/bgrt: Drop __initdata from bgrt_image_size
On 17 June 2018 at 17:32, Hans de Goede wrote:
> bgrt_image_size is necessary to (optionally) show the boot graphics from
> the efifb code. The efifb driver is a platform driver, using a normal
> driver probe() driver callback. So even though it is always builtin it
> cannot reference __initdata.
>
> Signed-off-by: Hans de Goede
Acked-by: Ard Biesheuvel
> ---
> drivers/firmware/efi/efi-bgrt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> index 50793fda7819..b22ccfb0c991 100644
> --- a/drivers/firmware/efi/efi-bgrt.c
> +++ b/drivers/firmware/efi/efi-bgrt.c
> @@ -20,7 +20,7 @@
> #include
>
> struct acpi_table_bgrt bgrt_tab;
> -size_t __initdata bgrt_image_size;
> +size_t bgrt_image_size;
>
> struct bmp_header {
> u16 id;
> --
> 2.17.1
>
--
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
