linux-kernel - Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201312060259.48259.arnd@arndb.de>
Date:	Fri, 6 Dec 2013 02:59:48 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	Leif Lindholm <leif.lindholm@...aro.org>,
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux@....linux.org.uk, mark.rutland@....com,
	linaro-uefi@...aro.org, matt.fleming@...el.com, patches@...aro.org,
	roy.franz@...aro.org, msalter@...hat.com, grant.likely@...aro.org
Subject: Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support

On Thursday 28 November 2013, Leif Lindholm wrote:
> @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p)
>  	sanity_check_meminfo();
>  	arm_memblock_init(&meminfo, mdesc);
>  
> +#ifdef CONFIG_EFI
> +	uefi_memblock_arm_reserve_range();
> +#endif
> +

Better use 

	if (IS_ENABLED(CONFIG_EFI))

here for readability and better build-time checking of the interface when
CONFIG_EFI is disabled.

> +/*
> + * Returns 1 if 'facility' is enabled, 0 otherwise.
> + */
> +int efi_enabled(int facility)
> +{
> +	return test_bit(facility, &arm_uefi_facility) != 0;
> +}
> +EXPORT_SYMBOL(efi_enabled);

I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why
a symbol should be available to GPL-incompatible modules.

> +static __init int is_discardable_region(efi_memory_desc_t *md)
> +{
> +#ifdef KEEP_ALL_REGIONS
> +	return 0;
> +#endif

IS_ENABLED() again.

> +	if (md->attribute & EFI_MEMORY_RUNTIME)
> +		return 0;
> +
> +	switch (md->type) {
> +#ifdef KEEP_BOOT_SERVICES_REGIONS
> +	case EFI_BOOT_SERVICES_CODE:
> +	case EFI_BOOT_SERVICES_DATA:
> +#endif

and I think it can be used here too:

	switch (md->type) {
	case EFI_BOOT_SERVICES_CODE:
	case EFI_BOOT_SERVICES_DATA:
		if (IS_ENABLED(KEEP_BOOT_SERVICES_REGIONS))
			return 1;
		/* fallthrough */
	case EFI_ACPI_RECLAIM_MEMORY:

> +	memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap,
> +					 uefi_boot_mmap_size);
> +	if (!memmap.phys_map)
> +		return 1;
> +
> +	p = memmap.phys_map;
> +	e = (void *)((u32)p + uefi_boot_mmap_size);

You are doing a lot of type casts here, which is normally an indication
that you have the types wrong in some way. I can't spot a mistake
here, but maybe you can give it some more thought and see if it can be
changed.

> +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry)
> +{
> +	u64 va;
> +	u64 paddr;
> +	u64 size;
> +
> +	*entry = *md;
> +	paddr = entry->phys_addr;
> +	size = entry->num_pages << EFI_PAGE_SHIFT;
> +
> +	/*
> +	 * Map everything writeback-capable as coherent memory,
> +	 * anything else as device.
> +	 */
> +	if (md->attribute & EFI_MEMORY_WB)
> +		va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL);
> +	else
> +		va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL);

Same here. Why is 'va' a u64?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/