Re: [PATCH 4/4] kvx: add support for elf loading using bootm

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Clement,

On 6/29/20 10:23 AM, Clément Leger wrote:
> Hi Ahmad
> 
> ----- On 29 Jun, 2020, at 10:15, Ahmad Fatoum a.fatoum@pengutronix.de wrote:
> 
>> Hello,
>>
>> On 6/24/20 9:15 AM, Clément Leger wrote:
>>>> You already flushed out the dcache contents to the point of unification.
>>>> What do you gain by invalidating it?
>>>
>>> Indeed, this is not strictly necessary, it just allow to boot the elf without a
>>> dirty D-cache. But actually, the booted program should invalidate its D-cache
>>> before fetching any data to avoid using cached data. I can remove it since
>>> this is only for some buggy programs.
>>
>> Why would use of cached data hurt here? If you keep the MMU throughout, you
>> should only need to invalidate the data cache when doing DMA. Am I missing
>> something?
> 
> You are actually right, since barebox loaded the program by itself, the D-cache
> contains the expected data even for the loaded program.
> BTW, MMU is not enabled yet in barebox.

Ah, I assumed the MMU would have been enabled, so that MMIO isn't cached/speculated
into. If the MMU is not enabled, you should only need to invalidate if the cache
contents are indeterminate on reset.

> 
>>
>> Either way, I've no preference (except for naming it the same as on ARM,
>> see previous mail).
> 
> Sorry, I missed the points you made on my previous patchset, my mailer marked
> all the mail has read :/. I will make another more through review.

No worries.

Cheers
Ahmad

> 
> Regards,
> 
> Clément
> 
>>
>> Cheers
>> Ahmad
>>
>>>
>>>>
>>>>> +
>>>>> +	/**
>>>>> +	 * Parameters passing
>>>>> +	 * r0: boot magic
>>>>> +	 * r1: device tree pointer
>>>>> +	 */
>>>>> +	entry(LINUX_BOOT_PARAM_MAGIC, (void *) fdt_load_addr);
>>>>> +
>>>>> +	/* should never return ! */
>>>>> +	panic("Returned from boot program !\n");
>>>>> +
>>>>> +	return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static int do_boot_elf(struct image_data *data, struct elf_image *elf)
>>>>> +{
>>>>> +	int ret;
>>>>> +	void *fdt;
>>>>> +	boot_func_entry entry;
>>>>> +	unsigned long load_addr, initrd_address;
>>>>> +
>>>>> +	/* load initrd after the elf */
>>>>> +	load_addr = PAGE_ALIGN((unsigned long) elf->high_addr);
>>>>> +	if (bootm_has_initrd(data)) {
>>>>> +		if (data->initrd_address != UIMAGE_INVALID_ADDRESS)
>>>>> +			initrd_address = data->initrd_address;
>>>>> +		else
>>>>> +			initrd_address = load_addr;
>>>>> +
>>>>> +		printf("Loading initrd at 0x%lx\n", initrd_address);
>>>>> +		ret = bootm_load_initrd(data, initrd_address);
>>>>> +		if (ret) {
>>>>> +			printf("Failed to load initrd\n");
>>>>> +			return ret;
>>>>> +		}
>>>>> +
>>>>> +		if (data->initrd_address == UIMAGE_INVALID_ADDRESS) {
>>>>> +			load_addr += resource_size(data->initrd_res);
>>>>> +			load_addr = PAGE_ALIGN(load_addr);
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	fdt = bootm_get_devicetree(data);
>>>>> +	if (IS_ERR(fdt)) {
>>>>> +		printf("Failed to load dtb\n");
>>>>> +		return PTR_ERR(fdt);
>>>>> +	}
>>>>> +
>>>>> +	printf("Loading device tree at %lx\n", load_addr);
>>>>> +	/* load device tree after the initrd if any */
>>>>> +	ret = bootm_load_devicetree(data, fdt, load_addr);
>>>>> +	if (ret) {
>>>>> +		printf("Failed to load device tree: %d\n", ret);
>>>>> +		goto err_free_fdt;
>>>>> +	}
>>>>> +
>>>>> +	entry = (boot_func_entry) data->os_address;
>>>>> +
>>>>> +	ret = do_boot_entry(data, entry, fdt);
>>>>> +
>>>>> +err_free_fdt:
>>>>> +	free(fdt);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int do_bootm_elf(struct image_data *data)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = bootm_load_os(data, data->os_address);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	return do_boot_elf(data, data->elf);
>>>>> +}
>>>>> +
>>>>> +static struct image_handler elf_handler = {
>>>>> +	.name = "ELF",
>>>>> +	.bootm = do_bootm_elf,
>>>>> +	.filetype = filetype_elf,
>>>>> +};
>>>>> +
>>>>> +static struct binfmt_hook binfmt_elf_hook = {
>>>>> +	.type = filetype_elf,
>>>>> +	.exec = "bootm",
>>>>> +};
>>>>> +
>>>>> +static int kvx_register_image_handler(void)
>>>>> +{
>>>>> +	register_image_handler(&elf_handler);
>>>>> +
>>>>> +	binfmt_register(&binfmt_elf_hook);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +late_initcall(kvx_register_image_handler);
>>>>>
>>>>
>>>> --
>>>> Pengutronix e.K.                           |                             |
>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>>
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox