What changes can be upstreamed?

[crass]

Now that this project is being deprecated, it would be a shame not to salvage useful changes to send to upstream GRUB2. I'm thinking at least the map module could be nice. I would be willing to assist in this process, but I'd like your help in determining what might be useful to send to upstream. Does this interest you at all?

[a1ive]

I'm not sure which changes will be accepted upstream, so if you'd like to submit them that would be great.
The map module may not be easy to separate from this branch on its own.
Maybe you could consider the efiload module? (for loading UEFI drivers like CrScreenshotDxe)
and this commit 812d122 ?

[crass]

I'm not sure which changes will be accepted upstream, so if you'd like to submit them that would be great.

Yeah, I'm not entirely sure either what will be accepted, but I do have more experience and intuition about it. I'm willing to submit patches to the grub-devel list and handle feedback. Since I'm not as familiar with your code and I believe less knowledgeable than you, I expect that there may come criticisms of the patches on the list that I may not know how to respond to. My hope is that you'll be able to help resolve them.

What is a good way to communicate with you about issues? Just open an issue here? or open a discussion? or something else?

How do you think I should handle the SOB, (Signed-off-by)? The most ideal solution is to have you as the author of the patch, with your SOB. Then I'll add my SOB and potentially a Co-developed-by if I've modified the patch. The issue with this ideal solution is that your SOB should have your real name. Will this be a problem for you? I believe the email 10670106+a1ive@users.noreply.github.com you use for the commits to this repo is fine, unless you'd like to use a different email.

The map module may not be easy to separate from this branch on its own. Maybe you could consider the efiload module? (for loading UEFI drivers like CrScreenshotDxe)

This is mostly good, but I see a few issues before its ready to be sent to the list. As above, what's a good way to discuss this?

The biggest issue is the connect all functionality. I think you're more familiar with EFI, but can't we just connect that one handle of the loaded image? Someone recently submitted a patch to connect all EFI handles, but it was rejected because apparently that can take a very long time and seems to be frowned upon by the EFI gurus. Perhaps the efiload command should only connect the handle to the driver that was just loaded. I think its still useful to have efi_connect_all command though, or perhaps better an efi_connect command that connects all handles when given no arguments. But when given arguments they are a list of handle ids to connect. Thoughts?

We should also probably use grub_efi_allocate_pages_real() instead of calling efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES, GRUB_EFI_LOADER_CODE, pages, &address); directly and also grub_efi_free_pages() instead of the call to b->free_pages().

Should b->start_image() error handling be done more like in chainloader:

grub/grub-core/loader/efi/chainloader.c

Lines 80 to 102 in 635ef55

	status = efi_call_3 (b->start_image, image_handle, &exit_data_size, &exit_data);
	if (status != GRUB_EFI_SUCCESS)
	{
	if (exit_data)
	{
	char *buf;
	buf = grub_malloc (exit_data_size * 4 + 1);
	if (buf)
	{
	*grub_utf16_to_utf8 ((grub_uint8_t *) buf,
	exit_data, exit_data_size) = 0;
	grub_error (GRUB_ERR_BAD_OS, buf);
	grub_free (buf);
	}
	}
	else
	grub_error (GRUB_ERR_BAD_OS, "unknown error");
	}
	if (exit_data)
	efi_call_1 (b->free_pool, exit_data);

Other issues, like improper spacing and improper return code/error handling for grub_cmd_efiload, I can take care of.

and this commit 812d122 ?

I think I remember seeing this too a while back, but coming to the conclusion that its more tricky than this. I think its because this only works if the file will be read sequentially, which probably none of the filesystem drivers do. But its been a while since I've looked at it. Have you tested that this works? Are you only reading sequentially from a loopback device backed by a compressed file?

[crass]

Also, it looks like there's some reasonable patches in your patches repo under the grub_gnu directory. Are these your patches or were they gotten from somewhere else? (eg. picked from a distro's patches).

[a1ive]

What is a good way to communicate with you about issues? Just open an issue here? or open a discussion? or something else?

Just open an issue here.

How do you think I should handle the SOB, (Signed-off-by)? The most ideal solution is to have you as the author of the patch, with your SOB. Then I'll add my SOB and potentially a Co-developed-by if I've modified the patch. The issue with this ideal solution is that your SOB should have your real name. Will this be a problem for you? I believe the email 10670106+a1ive@users.noreply.github.com you use for the commits to this repo is fine, unless you'd like to use a different email.

Li Gen <ligenlive@gmail.com>

[a1ive]

The biggest issue is the connect all functionality. I think you're more familiar with EFI, but can't we just connect that one handle of the loaded image? Someone recently submitted a patch to connect all EFI handles, but it was rejected because apparently that can take a very long time and seems to be frowned upon by the EFI gurus. Perhaps the efiload command should only connect the handle to the driver that was just loaded. I think its still useful to have efi_connect_all command though, or perhaps better an efi_connect command that connects all handles when given no arguments. But when given arguments they are a list of handle ids to connect. Thoughts?

According to EDK2's code, it connects all handles.
https://github.com/tianocore/edk2/blob/4d83ee04f44a8dc9e6425a719b39c9d378730ca1/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c#L167
I'm also not sure what the reason is for doing this inside EDK2.

We should also probably use grub_efi_allocate_pages_real() instead of calling efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES, GRUB_EFI_LOADER_CODE, pages, &address); directly and also grub_efi_free_pages() instead of the call to b->free_pages().

Yes, you are right.

Should b->start_image() error handling be done more like in chainloader

We need to unload the image if b->start_image() failed, I forgot to do it.

[a1ive]

I think I remember seeing this too a while back, but coming to the conclusion that its more tricky than this. I think its because this only works if the file will be read sequentially, which probably none of the filesystem drivers do. But its been a while since I've looked at it. Have you tested that this works? Are you only reading sequentially from a loopback device backed by a compressed file?

gzio, xzio and other decompressor allows random reads (but it's slow). If the file size is relatively small, this won't cause too much of a problem.

grub/grub-core/io/xzio.c

Lines 240 to 250 in 5b20b2f

	/* If seek backward need to reset decoder and start from beginning of file.
	TODO Possible improvement by jumping blocks. */
	if (file->offset < xzio->saved_offset)
	{
	xz_dec_reset (xzio->dec);
	xzio->saved_offset = 0;
	xzio->buf.out_pos = 0;
	xzio->buf.in_pos = 0;
	xzio->buf.in_size = 0;
	grub_file_seek (xzio->file, 0);
	}

[a1ive]

Also, it looks like there's some reasonable patches in your patches repo under the grub_gnu directory. Are these your patches or were they gotten from somewhere else? (eg. picked from a distro's patches).

These are my patches, but then someone else has submitted similar patches and most of them have been accepted by upstream (e.g. https://github.com/a1ive/patches/blob/master/grub_gnu/0002-msdospart-add-missing-parameter.patch).

https://github.com/a1ive/patches/blob/master/grub_gnu/0004-gptsync-fix-bug-in-CHS-calculation.patch
This patch should not be sent to upstream GRUB2 because it does not follow the EDK2 Spec requirements for GPT.
However GRUB4DOS doesn't accept CHS FE FF FF so I have to do this.

https://github.com/a1ive/patches/blob/master/grub_gnu/0001-efi-fix-wrong-def-for-simple_text_input_ex_interface.patch
https://github.com/a1ive/patches/blob/master/grub_gnu/0003-read-fix-overflow-in-grub_getline.patch
I think these two patches are OK.

[crass]

Also, it looks like there's some reasonable patches in your patches repo under the grub_gnu directory. Are these your patches or were they gotten from somewhere else? (eg. picked from a distro's patches).

These are my patches, but then someone else has submitted similar patches and most of them have been accepted by upstream (e.g. https://github.com/a1ive/patches/blob/master/grub_gnu/0002-msdospart-add-missing-parameter.patch).

https://github.com/a1ive/patches/blob/master/grub_gnu/0004-gptsync-fix-bug-in-CHS-calculation.patch This patch should not be sent to upstream GRUB2 because it does not follow the EDK2 Spec requirements for GPT. However GRUB4DOS doesn't accept CHS FE FF FF so I have to do this.

Ok, good to know that this should not be upstreamed.

https://github.com/a1ive/patches/blob/master/grub_gnu/0001-efi-fix-wrong-def-for-simple_text_input_ex_interface.patch https://github.com/a1ive/patches/blob/master/grub_gnu/0003-read-fix-overflow-in-grub_getline.patch I think these two patches are OK.

Ok, thanks for confirming that. I'm looking at the getline patch again and wondering exactly whatproblem that this solves. All I see is maybe it allows getline to be multiline if modifiers are used when pressing enter. And I'm not even sure that's true.

What do you think about 0007-efi-chainloader-get-the-device-path-from-filename.patch and 0005-cmp-add-return-value.patch? Both of those don't seem to be upstream and seem like worth upstreaming, with perhaps some modifications. I don't completely understand the second changeset of the chainloader, why is this change needed?

[a1ive]

I'm looking at the getline patch again and wondering exactly whatproblem that this solves. All I see is maybe it allows getline to be multiline if modifiers are used when pressing enter. And I'm not even sure that's true.

Oh sorry. This patch is not correct.
It should be:

int c;
...
while (1)
{
  c = grub_getkey ();
  if ((c == '\n') || (c == '\r'))
    break;
  if (!grub_isprint (c))
    continue;
  line[i] = c;
  if (!silent)
    grub_printf ("%c", c);
...

If the user accidentally presses the arrow keys, char c will overflow and the terminal will print a strange character.
We should therefore use int c and make sure it is a printable character. (#49)

[a1ive]

I don't completely understand the second changeset of the chainloader, why is this change needed?

When GRUB2 loads an EFI application, it can pass a device path to the EFI application so that the EFI application knows its own path. (This is optional)
For example Microsoft's bootmgfw.efi can load the BCD configuration file in the same directory based on this.
If the EFI application is on a 'real' disk (e.g. (hd0,gpt4)/efi/microsoft/boot/bootmgfw.efi), then everything is fine.
If we want to boot an EFI application from a loopback device (e.g. (loopback)/efi/boot/shellx64.efi), then we can only pass an empty device path.
But Microsoft's bootmgfw.efi has a lot of bugs. If it is on a CD, then it will report an error when we boot it that it cannot read the BCD file. This is because bootmgfw.efi needs the device path to point to the El Torito FAT image of the CD and not the CD itself.
I haven't figured out how to fix this perfectly, so I'm inclined not to submit this patch.

[a1ive]

0005-cmp-add-return-value.patch

Add a return value to the cmp command so that it can be used in an if statement.
Maybe we should also add a --quiet option to it so that it won't print anything?

[crass]

0005-cmp-add-return-value.patch

Add a return value to the cmp command so that it can be used in an if statement. Maybe we should also add a --quiet option to it so that it won't print anything?

Yes, I like having cmp have a better return code. Adding --quiet would be nice. Its a separate patch and not that hard, but I'm not as interested in that right now.

[crass]

I don't completely understand the second changeset of the chainloader, why is this change needed?

When GRUB2 loads an EFI application, it can pass a device path to the EFI application so that the EFI application knows its own path. (This is optional) For example Microsoft's bootmgfw.efi can load the BCD configuration file in the same directory based on this. If the EFI application is on a 'real' disk (e.g. (hd0,gpt4)/efi/microsoft/boot/bootmgfw.efi), then everything is fine. If we want to boot an EFI application from a loopback device (e.g. (loopback)/efi/boot/shellx64.efi), then we can only pass an empty device path. But Microsoft's bootmgfw.efi has a lot of bugs. If it is on a CD, then it will report an error when we boot it that it cannot read the BCD file. This is because bootmgfw.efi needs the device path to point to the El Torito FAT image of the CD and not the CD itself. I haven't figured out how to fix this perfectly, so I'm inclined not to submit this patch.

Ok, thanks for the explanation. I've reworked the patch and have something like the second changeset. I think the author of the previous code thought that a NULL device path could not be passed to LoadImage, but it definitely can because we are using an in-memory image.

I think the issue with bootmgfw.efi is not exactly related. I think its possible but requires loading some UEFI drivers.