Re: [PATCH 1/4] x86/efi: show actual ending addresses in efi_print_memmap

From: Matt Fleming <matt@codeblueprint.co.uk>
To: "Elliott, Robert (Persistent Memory)" <elliott@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, linux-efi@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [PATCH 1/4] x86/efi: show actual ending addresses in efi_print_memmap
Date: Wed, 23 Dec 2015 12:47:11 +0000	[thread overview]
Message-ID: <20151223124711.GB2471@codeblueprint.co.uk> (raw)
In-Reply-To: <94D0CD8314A33A4D9D801C0FE68B40295BEC3C9C@G9W0745.americas.hpqcorp.net>

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.