Re: [PATCH] efi: fix semihosting EFI payload booting

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 11 May 2023 17:23:13 +0200
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

Hi Heinrich, Ilias,

> On 5/11/23 10:59, Andre Przywara wrote:
> > On Thu, 11 May 2023 08:22:30 +0200
> > Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >  
> >> On 5/11/23 02:00, Andre Przywara wrote:  
> >>> On Wed, 10 May 2023 23:19:33 +0200
> >>> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>
> >>> Hi,
> >>>  
> >>>> On 5/10/23 19:26, Andre Przywara wrote:  
> >>>>> On Wed, 10 May 2023 17:58:06 +0200
> >>>>> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>  
> >>>>>> On 5/10/23 16:13, Andre Przywara wrote:  
> >>>>>>> At the moment any naive attempt to boot an EFI payload that has just
> >>>>>>> been loaded via "hostfs" (sandbox or semihosting) is met by a rather
> >>>>>>> confusing error message:
> >>>>>>> ===========
> >>>>>>> VExpress64# load hostfs - $kernel_addr_r Image
> >>>>>>> 52752896 bytes read in 8 ms (6.1 GiB/s)
> >>>>>>> VExpress64# bootefi $kernel_addr_r
> >>>>>>> No UEFI binary known at 0x80080000
> >>>>>>> ===========
> >>>>>>> Actually explicitly providing the filesize:
> >>>>>>> VExpress64# bootefi $kernel_addr_r:$filesize
> >>>>>>> works around that problem, but the issue lies deeper: the call to
> >>>>>>> efi_set_bootdev() (as done by the generic load code) bails out at some
> >>>>>>> point, leaving the image_addr and image_size variables unset, which
> >>>>>>> triggers this message. The problem seems to be that "-" is not
> >>>>>>> understood by the code creating an UEFI device path. We could try to fix
> >>>>>>> just that, but actually semihosting seems to have some explicit support
> >>>>>>> in UEFI (at least it does in EDK II): there is a separate GUID for it,
> >>>>>>> and hostfs is significantly different in some aspects to justify special
> >>>>>>> handling.
> >>>>>>>
> >>>>>>> Check for the device name being "hostfs" and create a specific UEFI device
> >>>>>>> path for semihosting in this case. This uses the GUID used by EDK II for
> >>>>>>> almost 15 years.
> >>>>>>> This fixes the above load/bootefi sequence without requiring an explicit
> >>>>>>> file size argument.
> >>>>>>>
> >>>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> >>>>>>

....

> Now I am able to debug the code.
> 
> I wonder if the file system should be exposed as EFI simple file system
> so that EFI applications can access it, e.g. for GRUB to load initrd and
> the kernel.

Yes, this would be the ultimate goal. There is no file listing
functionality in semihosting, which is a bummer, but it should still work
if you know the file name. On top of the EDK-II shell or grub being able to
load an image, this would also allow to easily specify an initrd via the
kernel command line. This works with EDK-II's semihosting support.

But I see that this requires more work, IIUC we need to support
hostfs explicitly via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
At the moment this seems to assume that there is some underlying block
device, so this might need some refactoring.

As it stands right now, your much simpler patch seems to provide the same
functionality as mine, so I wonder if we should take this as a kind of
quick fix, to allow starting EFI apps loaded via hostfs (including
sandbox, btw).

Eventually, when introducing proper filesystem protocol support, this
patch probably would need to come back.

So I leave this up to you, but am fine with your simpler patch.

Cheers,
Andre

> 
> If we don't want to expose it, this change will be enough:
> 
> diff --git a/lib/efi_loader/efi_device_path.c
> b/lib/efi_loader/efi_device_path.c
> index e2e98a39be..058bdc1ee5 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -1203,7 +1203,7 @@ efi_status_t efi_dp_from_name(const char *dev,
> const char *devnr,
>          } else if (!strcmp(dev, "Uart")) {
>                  if (device)
>                          *device = efi_dp_from_uart();
> -       } else if (!strcmp(dev, "Mem")) {
> +       } else if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs") ) {
>                  efi_get_image_parameters(&image_addr, &image_size);
> 
>                  if (device)
> 
> Best regards
> 
> Heinrich
>