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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <201312061859.40224.arnd@arndb.de>
Date:	Fri, 6 Dec 2013 18:59:40 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Leif Lindholm <leif.lindholm@...aro.org>
Cc:	matt.fleming@...el.com, linux-arm-kernel@...ts.infradead.org,
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux@....linux.org.uk, mark.rutland@....com,
	linaro-uefi@...ts.linaro.org, 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 Friday 06 December 2013, Leif Lindholm wrote:
> 
> On Fri, Dec 06, 2013 at 02:59:48AM +0100, Arnd Bergmann wrote:
> > 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.
> 
> I think I'll do what Grant suggested and stub it out in asm/uefi.h.

That's ok, too. I usually prefer the IS_ENABLED() method if there is only
one caller, since it's harder to get wrong, but the result is similar.

> > > +/*
> > > + * 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.
> 
> So ... this is duplicating x86 code which Matt Fleming has promised
> to make generic. I'd prefer to keep it identical to x86 until this
> copy goes away.

Ok, makes sense.

> > > +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?
> 
> Because the field in the structure (defined by UEFI) is 64-bit:
> 
>         entry->virt_addr = va;

I see. Maybe put the typecast in that final assignment then?

	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/