[PATCH] efi: fix semihosting EFI payload booting

[Heinrich Schuchardt]

On 5/10/23 19:26, Andre Przywara wrote:
> 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

This does not tell me which arguments I should pass to qemu-system-aarch64.

>
> 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.

If it is missing in Ubuntu's QEMU, please, indicate how I can build a
QEMU that allows testing. This information needs to go into semihosting.rst.

>
> Regardless I was doing those experiments with the FVP fastmodel, which is

It seems FVP's are not open source
(https://developer.arm.com/Tools%20and%20Software/Fixed%20Virtual%20Platforms).

We need some open source solution for testing the suggested changes.

Best regards

Heinrich

> 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);
>>
>