linux-kernel - Re: [PATCH 1/4] x86/efi: show actual ending addresses in efi_print

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: <20151223124711.GB2471@codeblueprint.co.uk>
Date:	Wed, 23 Dec 2015 12:47:11 +0000
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	"Elliott, Robert (Persistent Memory)" <elliott@....com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Leif Lindholm <leif.lindholm@...aro.org>
Subject: Re: [PATCH 1/4] x86/efi: show actual ending addresses in
 efi_print_memmap

On Mon, 21 Dec, at 04:44:19PM, Elliott, Robert (Persistent Memory) wrote:
> 
> The format of that line is architecture-specific and only appears
> under the efi=debug kernel parameter, so I don't know how much
> anyone relies on the specific format.  Good question for the lists.
 
Both kernel and non-kernel developers are consumers of these debug
statements, and people do come to rely on the format of the text.

I have certainly become acustomed to the current format we have,
particularly when debugging issues via other users, i.e. when I'm not
the person running the kernel being debugged.

Just because it's a debug option doesn't mean it's completely open to
modification. Reasonable Justification must be provided. Having said
that, I think you provide a good argument in your other email,

  https://lkml.kernel.org/r/94D0CD8314A33A4D9D801C0FE68B40295BEC74CF@G9W0745.americas.hpqcorp.net

> arch/ia64/kernel/efi.c shares the range=[...) format, but prints
> the range after the bitmask rather than before:
> 	printk("mem%02d: %s "
> 		"range=[0x%016lx-0x%016lx) (%4lu%s)\n",
> 		i, efi_md_typeattr_format(buf, sizeof(buf), md),
> 		md->phys_addr,
> 		md->phys_addr + efi_md_size(md), size, unit);
> 
> arch/arm64/kernel/efi.c has no mem prefix, or range=[...) text
> surrounding the range:
> 	pr_info("  0x%012llx-0x%012llx %s",
> 		paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1,
> 		efi_md_typeattr_format(buf, sizeof(buf), md));
> 	pr_cont("*");
> 	pr_cont("\n");
> 
> The x86 code is inside ifdef EFI_DEBUG, which is always
> defined in that file.  I wonder if it was supposed to change
> to efi_enabled(EFI_DBG) to be based off the efi=debug kernel
> parameter?  efi_init() qualified the call to this function
> based on that:
>         if (efi_enabled(EFI_DBG))
>                 efi_print_memmap();
 
The EFI_DEBUG symbol should just be deleted. It's been enabled since
forever and really provides no value, because as you point out,
printing of the memory map is guarded by EFI_DBG anyway.

I'll send out a patch for ripping that out.

> In arch/ia64/kernel/efi.c efi_init(), EFI_DEBUG is set to 0
> so the print doesn't happen at all without editing the
> source code.  It doesn't use efi_enabled(EFI_DBG).
> 
> arch/arm64/kernel/efi.c uses efi_enabled(EFI_DBG) exclusively.
--
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/