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: <20130921152139.GG2737@rocoto.smurfnet.nu>
Date:	Sat, 21 Sep 2013 17:21:39 +0200
From:	Leif Lindholm <leif.lindholm@...aro.org>
To:	Matt Fleming <matt@...sole-pimps.org>
Cc:	Mark Salter <msalter@...hat.com>, Borislav Petkov <bp@...en8.de>,
	X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	Borislav Petkov <bp@...e.de>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	Vivek Goyal <vgoyal@...hat.com>,
	Dave Young <dyoung@...hat.com>, linux-efi@...r.kernel.org,
	Roy Franz <roy.franz@...aro.org>
Subject: Re: [PATCH 02/11] efi: Remove EFI_PAGE_SHIFT and EFI_PAGE_SIZE

On Fri, Sep 20, 2013 at 11:42:49AM +0100, Matt Fleming wrote:
> On Thu, 19 Sep, at 04:54:45PM, Borislav Petkov wrote:
> > From: Borislav Petkov <bp@...e.de>
> > 
> > ... and use the good old standard defines which we all know. Also,
> > simplify math to shift by PAGE_SHIFT instead of multiplying by
> > PAGE_SIZE.
> > 
> > Signed-off-by: Borislav Petkov <bp@...e.de>
> > ---
> >  arch/x86/boot/compressed/eboot.c | 12 ++++++------
> >  arch/x86/boot/compressed/eboot.h |  1 -
> >  arch/x86/platform/efi/efi.c      | 22 +++++++++++-----------
> >  include/linux/efi.h              |  6 ++----
> >  4 files changed, 19 insertions(+), 22 deletions(-)
> 
> I'm pulling in Leif and Roy just so they're aware of this change,
> because while PAGE_SHIFT is always 12 on x86, that's not true for arm64.

Thank you.
Also adding Mark.
 
> However, I imagine that much work would be needed to allow for page
> sizes other than 4K, so I am definitely going to take this patch.
 
This could actually be a problem for us pretty much from day 1.
UEFI mandates the use of 4K pages on arm64, but Fedora will be using
64K - so this aspect of this patch will actually break the default
usage model of Linux on arm64.

It will probably not be a problem on the stub side, and it's not used
in many places but it would break efi_lookup_mapped_address(),
efi_range_is_wc() and memrange_efi_to_native() for use by arm64.
At least the first of these would be a problem.

/
    Leif

> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index b7388a425f09..5c440bf769a8 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -96,7 +96,7 @@ static efi_status_t high_alloc(unsigned long size, unsigned long align,
> >  	if (status != EFI_SUCCESS)
> >  		goto fail;
> >  
> > -	nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> > +	nr_pages = round_up(size, PAGE_SIZE) / PAGE_SIZE;
> >  again:
> >  	for (i = 0; i < map_size / desc_size; i++) {
> >  		efi_memory_desc_t *desc;
> > @@ -111,7 +111,7 @@ again:
> >  			continue;
> >  
> >  		start = desc->phys_addr;
> > -		end = start + desc->num_pages * (1UL << EFI_PAGE_SHIFT);
> > +		end = start + (desc->num_pages << PAGE_SHIFT);
> >  
> >  		if ((start + size) > end || (start + size) > max)
> >  			continue;
> > @@ -173,7 +173,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
> >  	if (status != EFI_SUCCESS)
> >  		goto fail;
> >  
> > -	nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> > +	nr_pages = round_up(size, PAGE_SIZE) / PAGE_SIZE;
> >  	for (i = 0; i < map_size / desc_size; i++) {
> >  		efi_memory_desc_t *desc;
> >  		unsigned long m = (unsigned long)map;
> > @@ -188,7 +188,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
> >  			continue;
> >  
> >  		start = desc->phys_addr;
> > -		end = start + desc->num_pages * (1UL << EFI_PAGE_SHIFT);
> > +		end = start + (desc->num_pages << PAGE_SHIFT);
> >  
> >  		/*
> >  		 * Don't allocate at 0x0. It will confuse code that
> > @@ -224,7 +224,7 @@ static void low_free(unsigned long size, unsigned long addr)
> >  {
> >  	unsigned long nr_pages;
> >  
> > -	nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> > +	nr_pages = round_up(size, PAGE_SIZE) / PAGE_SIZE;
> >  	efi_call_phys2(sys_table->boottime->free_pages, addr, nr_pages);
> >  }
> >  
> > @@ -1128,7 +1128,7 @@ static efi_status_t relocate_kernel(struct setup_header *hdr)
> >  	 * possible.
> >  	 */
> >  	start = hdr->pref_address;
> > -	nr_pages = round_up(hdr->init_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> > +	nr_pages = round_up(hdr->init_size, PAGE_SIZE) / PAGE_SIZE;
> >  
> >  	status = efi_call_phys4(sys_table->boottime->allocate_pages,
> >  				EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> > diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
> > index e5b0a8f91c5f..786398c1bb9a 100644
> > --- a/arch/x86/boot/compressed/eboot.h
> > +++ b/arch/x86/boot/compressed/eboot.h
> > @@ -11,7 +11,6 @@
> >  
> >  #define DESC_TYPE_CODE_DATA	(1 << 0)
> >  
> > -#define EFI_PAGE_SIZE		(1UL << EFI_PAGE_SHIFT)
> >  #define EFI_READ_CHUNK_SIZE	(1024 * 1024)
> >  
> >  #define EFI_CONSOLE_OUT_DEVICE_GUID    \
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 7cec1e9e5494..538c1e6b7b2c 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -339,7 +339,7 @@ static void __init do_add_efi_memmap(void)
> >  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> >  		efi_memory_desc_t *md = p;
> >  		unsigned long long start = md->phys_addr;
> > -		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
> > +		unsigned long long size = md->num_pages << PAGE_SHIFT;
> >  		int e820_type;
> >  
> >  		switch (md->type) {
> > @@ -416,8 +416,8 @@ static void __init print_efi_memmap(void)
> >  		pr_info("mem%02u: type=%u, attr=0x%llx, "
> >  			"range=[0x%016llx-0x%016llx) (%lluMB)\n",
> >  			i, md->type, md->attribute, md->phys_addr,
> > -			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
> > -			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> > +			md->phys_addr + (md->num_pages << PAGE_SHIFT),
> > +			(md->num_pages >> (20 - PAGE_SHIFT)));
> >  	}
> >  #endif  /*  EFI_DEBUG  */
> >  }
> > @@ -429,7 +429,7 @@ void __init efi_reserve_boot_services(void)
> >  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> >  		efi_memory_desc_t *md = p;
> >  		u64 start = md->phys_addr;
> > -		u64 size = md->num_pages << EFI_PAGE_SHIFT;
> > +		u64 size = md->num_pages << PAGE_SHIFT;
> >  
> >  		if (md->type != EFI_BOOT_SERVICES_CODE &&
> >  		    md->type != EFI_BOOT_SERVICES_DATA)
> > @@ -473,7 +473,7 @@ void __init efi_free_boot_services(void)
> >  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> >  		efi_memory_desc_t *md = p;
> >  		unsigned long long start = md->phys_addr;
> > -		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
> > +		unsigned long long size = md->num_pages << PAGE_SHIFT;
> >  
> >  		if (md->type != EFI_BOOT_SERVICES_CODE &&
> >  		    md->type != EFI_BOOT_SERVICES_DATA)
> > @@ -825,7 +825,7 @@ void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
> >  		return NULL;
> >  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> >  		efi_memory_desc_t *md = p;
> > -		u64 size = md->num_pages << EFI_PAGE_SHIFT;
> > +		u64 size = md->num_pages << PAGE_SHIFT;
> >  		u64 end = md->phys_addr + size;
> >  		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> >  		    md->type != EFI_BOOT_SERVICES_CODE &&
> > @@ -843,7 +843,7 @@ void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
> >  
> >  void efi_memory_uc(u64 addr, unsigned long size)
> >  {
> > -	unsigned long page_shift = 1UL << EFI_PAGE_SHIFT;
> > +	unsigned long page_shift = 1UL << PAGE_SHIFT;
> >  	u64 npages;
> >  
> >  	npages = round_up(size, page_shift) / page_shift;
> > @@ -896,7 +896,7 @@ void __init efi_enter_virtual_mode(void)
> >  			continue;
> >  		}
> >  
> > -		prev_size = prev_md->num_pages << EFI_PAGE_SHIFT;
> > +		prev_size = prev_md->num_pages << PAGE_SHIFT;
> >  
> >  		if (md->phys_addr == (prev_md->phys_addr + prev_size)) {
> >  			prev_md->num_pages += md->num_pages;
> > @@ -914,7 +914,7 @@ void __init efi_enter_virtual_mode(void)
> >  		    md->type != EFI_BOOT_SERVICES_DATA)
> >  			continue;
> >  
> > -		size = md->num_pages << EFI_PAGE_SHIFT;
> > +		size = md->num_pages << PAGE_SHIFT;
> >  		end = md->phys_addr + size;
> >  
> >  		start_pfn = PFN_DOWN(md->phys_addr);
> > @@ -1011,7 +1011,7 @@ u32 efi_mem_type(unsigned long phys_addr)
> >  		md = p;
> >  		if ((md->phys_addr <= phys_addr) &&
> >  		    (phys_addr < (md->phys_addr +
> > -				  (md->num_pages << EFI_PAGE_SHIFT))))
> > +				  (md->num_pages << PAGE_SHIFT))))
> >  			return md->type;
> >  	}
> >  	return 0;
> > @@ -1026,7 +1026,7 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> >  		md = p;
> >  		if ((md->phys_addr <= phys_addr) &&
> >  		    (phys_addr < (md->phys_addr +
> > -				  (md->num_pages << EFI_PAGE_SHIFT))))
> > +				  (md->num_pages << PAGE_SHIFT))))
> >  			return md->attribute;
> >  	}
> >  	return 0;
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 5f8f176154f7..fa47d80ab4b5 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -95,8 +95,6 @@ typedef	struct {
> >  #define EFI_MEMORY_RUNTIME	((u64)0x8000000000000000ULL)	/* range requires runtime mapping */
> >  #define EFI_MEMORY_DESCRIPTOR_VERSION	1
> >  
> > -#define EFI_PAGE_SHIFT		12
> > -
> >  typedef struct {
> >  	u32 type;
> >  	u32 pad;
> > @@ -611,7 +609,7 @@ static inline int efi_range_is_wc(unsigned long start, unsigned long len)
> >  {
> >  	unsigned long i;
> >  
> > -	for (i = 0; i < len; i += (1UL << EFI_PAGE_SHIFT)) {
> > +	for (i = 0; i < len; i += PAGE_SIZE) {
> >  		unsigned long paddr = __pa(start + i);
> >  		if (!(efi_mem_attributes(paddr) & EFI_MEMORY_WC))
> >  			return 0;
> > @@ -728,7 +726,7 @@ struct efi_generic_dev_path {
> >  
> >  static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
> >  {
> > -	*npages = PFN_UP(*addr + (*npages<<EFI_PAGE_SHIFT)) - PFN_DOWN(*addr);
> > +	*npages = PFN_UP(*addr + (*npages << PAGE_SHIFT)) - PFN_DOWN(*addr);
> >  	*addr &= PAGE_MASK;
> >  }
> >  
> > -- 
> > 1.8.4
> > 
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ