[PATCH -v4] x86: only load initrd above 4g on second try

[PATCH -v4] x86: only load initrd above 4g on second try

[Yinghai Lu]

Mantas found that after commit 4bf7111f5016 ("x86/efi: Support initrd
loaded above 4G"), the kernel freezes at the earliest possible moment
when trying to boot via UEFI on Asus laptop.

Revert to old way to load initrd under 4G on first try,
second try will use above 4G buffer when initrd is too big
and does not fit under 4G.

-v2: add print out for second try, and print out files buf address.
-v3: can not snprintf
-v4: drop address print out for files buf

Reported-by: Mantas Mikulėnas <grawity-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Anders Darander <anders-7UjN0b3lYz2SbKU13Z4Etw@public.gmane.org>
Signed-off-by: Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

---
 arch/x86/boot/compressed/eboot.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/boot/compressed/eboot.c
===================================================================
--- linux-2.6.orig/arch/x86/boot/compressed/eboot.c
+++ linux-2.6/arch/x86/boot/compressed/eboot.c
@@ -1032,7 +1032,6 @@ struct boot_params *make_boot_params(str
 	int i;
 	unsigned long ramdisk_addr;
 	unsigned long ramdisk_size;
-	unsigned long initrd_addr_max;
 
 	efi_early = c;
 	sys_table = (efi_system_table_t *)(unsigned long)efi_early->table;
@@ -1095,15 +1094,20 @@ struct boot_params *make_boot_params(str
 
 	memset(sdt, 0, sizeof(*sdt));
 
-	if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
-		initrd_addr_max = -1UL;
-	else
-		initrd_addr_max = hdr->initrd_addr_max;
-
 	status = handle_cmdline_files(sys_table, image,
 				      (char *)(unsigned long)hdr->cmd_line_ptr,
-				      "initrd=", initrd_addr_max,
+				      "initrd=", hdr->initrd_addr_max,
 				      &ramdisk_addr, &ramdisk_size);
+
+	if (status != EFI_SUCCESS &&
+	    hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G) {
+		efi_printk(sys_table, "Trying to load files to higher address\n");
+		status = handle_cmdline_files(sys_table, image,
+				      (char *)(unsigned long)hdr->cmd_line_ptr,
+				      "initrd=", -1UL,
+				      &ramdisk_addr, &ramdisk_size);
+	}
+
 	if (status != EFI_SUCCESS)
 		goto fail2;
 	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[H. Peter Anvin]

On 09/04/2014 03:01 AM, Matt Fleming wrote:
> On Wed, 03 Sep, at 09:50:07PM, Yinghai Lu wrote:
>> Mantas found that after commit 4bf7111f5016 ("x86/efi: Support initrd
>> loaded above 4G"), the kernel freezes at the earliest possible moment
>> when trying to boot via UEFI on Asus laptop.
>>
>> Revert to old way to load initrd under 4G on first try,
>> second try will use above 4G buffer when initrd is too big
>> and does not fit under 4G.
>>
>> -v2: add print out for second try, and print out files buf address.
>> -v3: can not snprintf
>> -v4: drop address print out for files buf
>>
>> Reported-by: Mantas Mikulėnas <grawity@gmail.com>
>> Tested-by: Anders Darander <anders@chargestorm.se>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  arch/x86/boot/compressed/eboot.c |   18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> Thanks, I've picked this up and expanded on the commit message because a
> lot of useful information was discussed on the mailing list that we
> should definitely include.
> 
>   http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=urgent&id=b524e05df6d466af2f7eea1f044369109e48e641
> 

I am fine with this patch, but at the same time I do want to note that
there is an alternative to double-buffer the patch and/or (if that
applies to the buggy BIOS) round up the size of the target buffer.

	-hpa

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Matt Fleming]

On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
> 
> I am fine with this patch, but at the same time I do want to note that
> there is an alternative to double-buffer the patch and/or (if that
> applies to the buggy BIOS) round up the size of the target buffer.

I'm not sure that rounding up the size of the target buffer will
workaround this issue correctly.

As far as I know, the only thing that Mantas tried was rounding up the
size of the source file, by padding it.

Laszlo, is there anything we can do in the kernel to avoid the EDK2 bug
you fixed here,

  https://github.com/tianocore/edk2/commit/4e39b75e

such as lying about the number of bytes we're trying to read or
allocating a bigger target buffer than we actually need?

Assuming of course, that the bug that Laszlo fixed and the bug that
Mantas is hitting are one and the same.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Thu, Sep 4, 2014 at 2:29 PM, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
>>
>> I am fine with this patch, but at the same time I do want to note that
>> there is an alternative to double-buffer the patch and/or (if that
>> applies to the buggy BIOS) round up the size of the target buffer.
>
> I'm not sure that rounding up the size of the target buffer will
> workaround this issue correctly.
>
> As far as I know, the only thing that Mantas tried was rounding up the
> size of the source file, by padding it.

Hi Mantas,

Can you try attached patch on top of linus tree?

Thanks

Yinghai

[-- Attachment #2: round_up_chunksize.patch --]
[-- Type: text/x-patch, Size: 662 bytes --]

---
 drivers/firmware/efi/libstub/efi-stub-helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/firmware/efi/libstub/efi-stub-helper.c
===================================================================
--- linux-2.6.orig/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ linux-2.6/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -426,7 +426,7 @@ efi_status_t handle_cmdline_files(efi_sy
 				if (size > EFI_READ_CHUNK_SIZE)
 					chunksize = EFI_READ_CHUNK_SIZE;
 				else
-					chunksize = size;
+					chunksize = round_up(size, EFI_PAGE_SIZE);
 
 				status = efi_file_read(files[j].handle,
 						       &chunksize,

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Anders Darander]

* Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [140905 03:19]:


> On Thu, Sep 4, 2014 at 2:29 PM, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> > On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:

> >> I am fine with this patch, but at the same time I do want to note that
> >> there is an alternative to double-buffer the patch and/or (if that
> >> applies to the buggy BIOS) round up the size of the target buffer.

> > I'm not sure that rounding up the size of the target buffer will
> > workaround this issue correctly.

> > As far as I know, the only thing that Mantas tried was rounding up the
> > size of the source file, by padding it.

> Can you try attached patch on top of linus tree?

I took the liberty to test the patch on my Dell XPS13 9333, and
unfortunately I got the old hang back. 

This was tested on the current Linus' tree.

Cheers,
Anders Darander

> -					chunksize = size;
> +					chunksize = round_up(size, EFI_PAGE_SIZE);



-- 
It usually takes more than three weeks to prepare a good impromptu speech.
		-- Mark Twain

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Laszlo Ersek]

On 09/05/14 07:47, Anders Darander wrote:
> * Yinghai Lu <yinghai@kernel.org> [140905 03:19]:
> 
> 
>> On Thu, Sep 4, 2014 at 2:29 PM, Matt Fleming <matt@console-pimps.org> wrote:
>>> On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
> 
>>>> I am fine with this patch, but at the same time I do want to note that
>>>> there is an alternative to double-buffer the patch and/or (if that
>>>> applies to the buggy BIOS) round up the size of the target buffer.
> 
>>> I'm not sure that rounding up the size of the target buffer will
>>> workaround this issue correctly.
> 
>>> As far as I know, the only thing that Mantas tried was rounding up the
>>> size of the source file, by padding it.
> 
>> Can you try attached patch on top of linus tree?
> 
> I took the liberty to test the patch on my Dell XPS13 9333, and
> unfortunately I got the old hang back. 
> 
> This was tested on the current Linus' tree.

Assuming that the UEFI implementation on Mantas's and Anders's machines
use edk2's FatPkg to read the file from an EFI System Partition:

this kernel patch will have no effect, because FatPkg (the ESP
filesystem driver) checks the buffer size against the remainder of the
file, and clips the input buffer if it would overshoot the file size.

In other words, the rounding up of the kernel will be undone in a
"somewhat higher level" driver in the firmware, and the request size
that reaches DiskIo (the "lowel level driver") remains the same.

https://github.com/tianocore/edk2-FatPkg/blob/master/EnhancedFatDxe/ReadWrite.c#L306

    } else {
      //
      // Access a file
      //
      EndPosition = IFile->Position + *BufferSize;
      if (EndPosition > OFile->FileSize) {
        //
        // The position goes beyond the end of file
        //
        if (IoMode == READ_DATA) {
          //
          // Adjust the actual size read
          //
          *BufferSize -= (UINTN) EndPosition - OFile->FileSize;

Thanks
Laszlo

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Yinghai Lu]

On Thu, Sep 4, 2014 at 11:38 PM, Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/05/14 07:47, Anders Darander wrote:
> In other words, the rounding up of the kernel will be undone in a
> "somewhat higher level" driver in the firmware, and the request size
> that reaches DiskIo (the "lowel level driver") remains the same.

Thanks for explanation.

Is there any way to check if EFI firmware have your fix from some calling?

Thanks

Yinghai

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Laszlo Ersek]

On 09/05/14 19:03, Yinghai Lu wrote:
> On Thu, Sep 4, 2014 at 11:38 PM, Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 09/05/14 07:47, Anders Darander wrote:
>> In other words, the rounding up of the kernel will be undone in a
>> "somewhat higher level" driver in the firmware, and the request size
>> that reaches DiskIo (the "lowel level driver") remains the same.
> 
> Thanks for explanation.
> 
> Is there any way to check if EFI firmware have your fix from some calling?

I can't think of any, alas.

It might make sense to contact Mantas's and Anders's UEFI vendors; after
all a reliable reproducer is available.

Thanks,
Laszlo

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Matt Fleming]

On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
> 
> I am fine with this patch, but at the same time I do want to note that
> there is an alternative to double-buffer the patch and/or (if that
> applies to the buggy BIOS) round up the size of the target buffer.

I took a whack at the double-buffer strategy, and I think the patch is
conceptually pretty straight forward.

Could someone try out the following patch? It works around the problem
on my ASUS machine.

---

From 89e7fdaeb04f79d9834678e486215974986d4ac8 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Fri, 5 Sep 2014 23:15:11 +0100
Subject: [PATCH] x86/efi: Workaround firmware bug with double-buffer read
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Mantas found that after commit 4bf7111f5016 ("x86/efi: Support initrd
loaded above 4G"), the kernel freezes at the earliest possible moment
when trying to boot via UEFI on Asus laptop.

The cause of the freeze appears to be a firmware bug when reading file
data into buffers above 4GB, though the exact reason is unknown.  Mantas
reports that the hang can be avoided if the file size is a multiple of
512 bytes, but I've seen some ASUS firmware simply corrupting the file
data rather than freezing.

Since the bug has only been reported when reading into a buffer above
4GB, we can workaround the problem by doing a double-buffer read, where
we read a partial file chunk into a buffer < 4GB then copy it to the
buffer (potentially) above 4GB.

Laszlo fixed an issue in the upstream EDK2 DiskIO code in Aug 2013 which
may possibly be related, commit 4e39b75e ("MdeModulePkg/DiskIoDxe: fix
source/destination pointer of overrun transfer"). Whatever the cause,
it's unlikely that a fix will be forthcoming from the vendor, hence the
workaround.

Cc: Yinghai Lu <yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reported-by: Mantas Mikulėnas <grawity-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reported-by: Harald Hoyer <harald-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 28 ++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 32d5cca30f49..470793ce2617 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -297,6 +297,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 {
 	struct file_info *files;
 	unsigned long file_addr;
+	unsigned long chunkbuf;
 	u64 file_size_total;
 	efi_file_handle_t *fh = NULL;
 	efi_status_t status;
@@ -416,6 +417,24 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 			goto free_file_total;
 		}
 
+		/*
+		 * Allocate a bounce buffer below 4GB.
+		 *
+		 * Some firmware implementations cannot perform file
+		 * reads into buffers above 4G and at best corrupt the
+		 * buffer, at worst they hang completely.
+		 *
+		 * Pass chunkbuf to the firmware to perform reads in
+		 * chunksize bytes and copy them to the target buffer
+		 * 'file_addr'.
+		 */
+		status = efi_high_alloc(sys_table_arg, EFI_READ_CHUNK_SIZE,
+					0x1000, &chunkbuf, 0xffffffff);
+		if (status != EFI_SUCCESS) {
+			pr_efi_err(sys_table_arg, "Failed to alloc file chunk buffer\n");
+			goto free_chunk;
+		}
+
 		addr = file_addr;
 		for (j = 0; j < nr_files; j++) {
 			unsigned long size;
@@ -430,11 +449,13 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 
 				status = efi_file_read(files[j].handle,
 						       &chunksize,
-						       (void *)addr);
+						       (void *)chunkbuf);
 				if (status != EFI_SUCCESS) {
 					pr_efi_err(sys_table_arg, "Failed to read file\n");
-					goto free_file_total;
+					goto free_chunk;
 				}
+
+				memcpy((void *)addr, (void *)chunkbuf, chunksize);
 				addr += chunksize;
 				size -= chunksize;
 			}
@@ -442,6 +463,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 			efi_file_close(files[j].handle);
 		}
 
+		efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
 	}
 
 	efi_call_early(free_pool, files);
@@ -451,6 +473,8 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 
 	return status;
 
+free_chunk:
+	efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
 free_file_total:
 	efi_free(sys_table_arg, file_size_total, file_addr);
 
-- 
1.9.3

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Anders Darander]

* Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> [140906 00:16]:

> On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:

> > I am fine with this patch, but at the same time I do want to note that
> > there is an alternative to double-buffer the patch and/or (if that
> > applies to the buggy BIOS) round up the size of the target buffer.

> I took a whack at the double-buffer strategy, and I think the patch is
> conceptually pretty straight forward.

> Could someone try out the following patch? It works around the problem
> on my ASUS machine.

Your patch works around the problem on my Dell machine also. With that
one applied, it boots fine!

Thus, 

Tested-by: Anders Darander <anders-7UjN0b3lYz2SbKU13Z4Etw@public.gmane.org>

Cheers,
Anders

-- 
All the troubles you have will pass away very quickly.

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Mantas Mikulėnas]

On Sat, Sep 6, 2014 at 1:16 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
>>
>> I am fine with this patch, but at the same time I do want to note that
>> there is an alternative to double-buffer the patch and/or (if that
>> applies to the buggy BIOS) round up the size of the target buffer.
>
> I took a whack at the double-buffer strategy, and I think the patch is
> conceptually pretty straight forward.
>
> Could someone try out the following patch? It works around the problem
> on my ASUS machine.
>
> ---
>
> From 89e7fdaeb04f79d9834678e486215974986d4ac8 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Fri, 5 Sep 2014 23:15:11 +0100
> Subject: [PATCH] x86/efi: Workaround firmware bug with double-buffer read
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Mantas found that after commit 4bf7111f5016 ("x86/efi: Support initrd
> loaded above 4G"), the kernel freezes at the earliest possible moment
> when trying to boot via UEFI on Asus laptop.
>
> The cause of the freeze appears to be a firmware bug when reading file
> data into buffers above 4GB, though the exact reason is unknown.  Mantas
> reports that the hang can be avoided if the file size is a multiple of
> 512 bytes, but I've seen some ASUS firmware simply corrupting the file
> data rather than freezing.
>
> Since the bug has only been reported when reading into a buffer above
> 4GB, we can workaround the problem by doing a double-buffer read, where
> we read a partial file chunk into a buffer < 4GB then copy it to the
> buffer (potentially) above 4GB.
>
> Laszlo fixed an issue in the upstream EDK2 DiskIO code in Aug 2013 which
> may possibly be related, commit 4e39b75e ("MdeModulePkg/DiskIoDxe: fix
> source/destination pointer of overrun transfer"). Whatever the cause,
> it's unlikely that a fix will be forthcoming from the vendor, hence the
> workaround.
>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Reported-by: Mantas Mikulėnas <grawity@gmail.com>
> Reported-by: Harald Hoyer <harald@redhat.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 28 ++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 32d5cca30f49..470793ce2617 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -297,6 +297,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>  {
>         struct file_info *files;
>         unsigned long file_addr;
> +       unsigned long chunkbuf;
>         u64 file_size_total;
>         efi_file_handle_t *fh = NULL;
>         efi_status_t status;
> @@ -416,6 +417,24 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>                         goto free_file_total;
>                 }
>
> +               /*
> +                * Allocate a bounce buffer below 4GB.
> +                *
> +                * Some firmware implementations cannot perform file
> +                * reads into buffers above 4G and at best corrupt the
> +                * buffer, at worst they hang completely.
> +                *
> +                * Pass chunkbuf to the firmware to perform reads in
> +                * chunksize bytes and copy them to the target buffer
> +                * 'file_addr'.
> +                */
> +               status = efi_high_alloc(sys_table_arg, EFI_READ_CHUNK_SIZE,
> +                                       0x1000, &chunkbuf, 0xffffffff);
> +               if (status != EFI_SUCCESS) {
> +                       pr_efi_err(sys_table_arg, "Failed to alloc file chunk buffer\n");
> +                       goto free_chunk;
> +               }
> +
>                 addr = file_addr;
>                 for (j = 0; j < nr_files; j++) {
>                         unsigned long size;
> @@ -430,11 +449,13 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>
>                                 status = efi_file_read(files[j].handle,
>                                                        &chunksize,
> -                                                      (void *)addr);
> +                                                      (void *)chunkbuf);
>                                 if (status != EFI_SUCCESS) {
>                                         pr_efi_err(sys_table_arg, "Failed to read file\n");
> -                                       goto free_file_total;
> +                                       goto free_chunk;
>                                 }
> +
> +                               memcpy((void *)addr, (void *)chunkbuf, chunksize);
>                                 addr += chunksize;
>                                 size -= chunksize;
>                         }
> @@ -442,6 +463,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>                         efi_file_close(files[j].handle);
>                 }
>
> +               efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
>         }
>
>         efi_call_early(free_pool, files);
> @@ -451,6 +473,8 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>
>         return status;
>
> +free_chunk:
> +       efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
>  free_file_total:
>         efi_free(sys_table_arg, file_size_total, file_addr);
>
> --
> 1.9.3
>
> --
> Matt Fleming, Intel Open Source Technology Center

Finally got around to testing the patches here. Sorry for no replies earlier.

This seemed like it would work, but... it hangs at the memcpy?...

(If I add a printk before memcpy and a printk after memcpy, I never
see the second one. I guess that memory location is just cursed, or
something.)

Going to test Yinghai's 6aacad3513bf now, just in case.

-- 
Mantas Mikulėnas <grawity@gmail.com>

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Mantas Mikulėnas]

On Sat, Sep 6, 2014 at 11:09 PM, Mantas Mikulėnas <grawity@gmail.com> wrote:
> On Sat, Sep 6, 2014 at 1:16 AM, Matt Fleming <matt@console-pimps.org> wrote:
>> On Thu, 04 Sep, at 01:59:05PM, H. Peter Anvin wrote:
>>>
>>> I am fine with this patch, but at the same time I do want to note that
>>> there is an alternative to double-buffer the patch and/or (if that
>>> applies to the buggy BIOS) round up the size of the target buffer.
>>
>> I took a whack at the double-buffer strategy, and I think the patch is
>> conceptually pretty straight forward.
>>
>> Could someone try out the following patch? It works around the problem
>> on my ASUS machine.
>>
>> ---
>>
>> From 89e7fdaeb04f79d9834678e486215974986d4ac8 Mon Sep 17 00:00:00 2001
>> From: Matt Fleming <matt.fleming@intel.com>
>> Date: Fri, 5 Sep 2014 23:15:11 +0100
>> Subject: [PATCH] x86/efi: Workaround firmware bug with double-buffer read
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> Mantas found that after commit 4bf7111f5016 ("x86/efi: Support initrd
>> loaded above 4G"), the kernel freezes at the earliest possible moment
>> when trying to boot via UEFI on Asus laptop.
>>
>> The cause of the freeze appears to be a firmware bug when reading file
>> data into buffers above 4GB, though the exact reason is unknown.  Mantas
>> reports that the hang can be avoided if the file size is a multiple of
>> 512 bytes, but I've seen some ASUS firmware simply corrupting the file
>> data rather than freezing.
>>
>> Since the bug has only been reported when reading into a buffer above
>> 4GB, we can workaround the problem by doing a double-buffer read, where
>> we read a partial file chunk into a buffer < 4GB then copy it to the
>> buffer (potentially) above 4GB.
>>
>> Laszlo fixed an issue in the upstream EDK2 DiskIO code in Aug 2013 which
>> may possibly be related, commit 4e39b75e ("MdeModulePkg/DiskIoDxe: fix
>> source/destination pointer of overrun transfer"). Whatever the cause,
>> it's unlikely that a fix will be forthcoming from the vendor, hence the
>> workaround.
>>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Reported-by: Mantas Mikulėnas <grawity@gmail.com>
>> Reported-by: Harald Hoyer <harald@redhat.com>
>> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> ---
>>  drivers/firmware/efi/libstub/efi-stub-helper.c | 28 ++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index 32d5cca30f49..470793ce2617 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -297,6 +297,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>>  {
>>         struct file_info *files;
>>         unsigned long file_addr;
>> +       unsigned long chunkbuf;
>>         u64 file_size_total;
>>         efi_file_handle_t *fh = NULL;
>>         efi_status_t status;
>> @@ -416,6 +417,24 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>>                         goto free_file_total;
>>                 }
>>
>> +               /*
>> +                * Allocate a bounce buffer below 4GB.
>> +                *
>> +                * Some firmware implementations cannot perform file
>> +                * reads into buffers above 4G and at best corrupt the
>> +                * buffer, at worst they hang completely.
>> +                *
>> +                * Pass chunkbuf to the firmware to perform reads in
>> +                * chunksize bytes and copy them to the target buffer
>> +                * 'file_addr'.
>> +                */
>> +               status = efi_high_alloc(sys_table_arg, EFI_READ_CHUNK_SIZE,
>> +                                       0x1000, &chunkbuf, 0xffffffff);
>> +               if (status != EFI_SUCCESS) {
>> +                       pr_efi_err(sys_table_arg, "Failed to alloc file chunk buffer\n");
>> +                       goto free_chunk;
>> +               }
>> +
>>                 addr = file_addr;
>>                 for (j = 0; j < nr_files; j++) {
>>                         unsigned long size;
>> @@ -430,11 +449,13 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>>
>>                                 status = efi_file_read(files[j].handle,
>>                                                        &chunksize,
>> -                                                      (void *)addr);
>> +                                                      (void *)chunkbuf);
>>                                 if (status != EFI_SUCCESS) {
>>                                         pr_efi_err(sys_table_arg, "Failed to read file\n");
>> -                                       goto free_file_total;
>> +                                       goto free_chunk;
>>                                 }
>> +
>> +                               memcpy((void *)addr, (void *)chunkbuf, chunksize);
>>                                 addr += chunksize;
>>                                 size -= chunksize;
>>                         }
>> @@ -442,6 +463,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>>                         efi_file_close(files[j].handle);
>>                 }
>>
>> +               efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
>>         }
>>
>>         efi_call_early(free_pool, files);
>> @@ -451,6 +473,8 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>>
>>         return status;
>>
>> +free_chunk:
>> +       efi_free(sys_table_arg, EFI_READ_CHUNK_SIZE, chunkbuf);
>>  free_file_total:
>>         efi_free(sys_table_arg, file_size_total, file_addr);
>>
>> --
>> 1.9.3
>>
>> --
>> Matt Fleming, Intel Open Source Technology Center
>
> Finally got around to testing the patches here. Sorry for no replies earlier.
>
> This seemed like it would work, but... it hangs at the memcpy?...
>
> (If I add a printk before memcpy and a printk after memcpy, I never
> see the second one. I guess that memory location is just cursed, or
> something.)
>
> Going to test Yinghai's 6aacad3513bf now, just in case.

Err, I meant b524e05df6d4 (the one in mfleming/urgent).

-- 
Mantas Mikulėnas <grawity@gmail.com>

Re: [PATCH -v4] x86: only load initrd above 4g on second try

[Matt Fleming]

On Sat, 06 Sep, at 11:09:04PM, Mantas Mikulėnas wrote:
> 
> Finally got around to testing the patches here. Sorry for no replies earlier.
> 
> This seemed like it would work, but... it hangs at the memcpy?...
> 
> (If I add a printk before memcpy and a printk after memcpy, I never
> see the second one. I guess that memory location is just cursed, or
> something.)
 
Thanks for testing. Let's go ahead with Yinghai's patch ("x86/efi: Only
load initrd above 4g on second try").

-- 
Matt Fleming, Intel Open Source Technology Center