[PATCH] efi: fix semihosting EFI payload booting

[Andre Przywara]

On Wed, 10 May 2023 17:58:06 +0200
Heinrich Schuchardt <xypron.glpk at 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 at arm.com>  
> 
> Could you, please, indicate how to invoke QEMU with semihosting enabled.
> This information needs to be added to doc/usage/semihosting.rst.

It's already there:
https://u-boot.readthedocs.io/en/latest/usage/semihosting.html#qemu

It's mostly just "-semihosting" on the QEMU side, but it's not enabled by
default in the QEMU defconfig, because it *was* needed to be provided in sync
with the QEMU option. It's a debug feature, so there is no good discovery
mechanism, really. I think Sean made this work, with this autodetect via
trap feature, so we might want to enable this option now, at least for the
filesystem part.

Regardless I was doing those experiments with the FVP fastmodel, which is
the most original semihosting provider, I think, and always enables
semihosting.

> Can there be multiple semihosting block devices?

There is no semihosting *block device*, it's a pure filesystem interface.
And there is conceptually only one instance of that: it's basically a
hook in the debugger (or emulation software) to provide some kind of
syscall interface, triggered by a certain instruction ("brk" on AArch64).
This allows some very limited access to the host filesystem - read files,
write files, but no listing, for instance.
It is super convenient to launch the model and load the kernel (or
whatever) directly from your build machine's filesystem - especially if
you do bisects or trial-and-error experiments.

> > ---
> >   lib/efi_loader/efi_device_path.c | 34 ++++++++++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index e2e98a39be1..b6d2074dd70 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -1079,6 +1079,35 @@ struct efi_device_path *efi_dp_from_uart(void)
> >   	return buf;
> >   }
> >
> > +#define SEMIHOSTING_GUID \
> > +	EFI_GUID(0xc5b9c74a, 0x6d72, 0x4719, \
> > +		 0x99, 0xab, 0xc5, 0x9f, 0x19, 0x90, 0x91, 0xeb)  
> 
> Can semihosting be moved to the driver model?

Mmh, I am not sure this is a thing, since there is no block device, it's a
filesystem. Which is part of the problem, I believe, and probably also a
good reason to treat it separately.

> Then we could create a generic catch all for device path nodes for all
> uclasses.

Need to digest that idea ....

Cheers,
Andre

> 
> Best regards
> 
> Heinrich
> 
> > +
> > +struct efi_device_path *efi_dp_from_semihosting(void)
> > +{
> > +	const struct efi_device_path_vendor smh_vendor = {
> > +		.dp = {
> > +			.type     = DEVICE_PATH_TYPE_HARDWARE_DEVICE,
> > +			.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR,
> > +			.length   = sizeof(smh_vendor),
> > +		},
> > +		.guid = SEMIHOSTING_GUID,
> > +	};
> > +	void *buf, *pos;
> > +	size_t dpsize = sizeof(smh_vendor) + sizeof(END);
> > +
> > +	buf = efi_alloc(dpsize);
> > +	if (!buf)
> > +		return NULL;
> > +	pos = buf;
> > +	memcpy(pos, &smh_vendor, sizeof(smh_vendor));
> > +	pos += sizeof(smh_vendor);
> > +
> > +	memcpy(pos, &END, sizeof(END));
> > +
> > +	return buf;
> > +}
> > +
> >   #ifdef CONFIG_NETDEVICES
> >   struct efi_device_path *efi_dp_from_eth(void)
> >   {
> > @@ -1210,6 +1239,11 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> >   			*device = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >   						  (uintptr_t)image_addr,
> >   						  image_size);
> > +	} else if (!strcmp(dev, "hostfs")) {
> > +		efi_get_image_parameters(&image_addr, &image_size);
> > +
> > +		if (device)
> > +			*device = efi_dp_from_semihosting();
> >   	} else {
> >   		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> >   					       1);  
>