The current debug print in EFI does
[ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
and rounds off the size to 0MB and isn't very useful. We should print this in
Kib. After applying this patch we get better info with
[ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
Signed-off-by: Prarit Bhargava <[email protected]>
Cc: [email protected]
Cc: Matt Fleming <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/x86/platform/efi/efi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 87fc96b..3875090 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -384,10 +384,10 @@ static void __init print_efi_memmap(void)
p < memmap.map_end;
p += memmap.desc_size, i++) {
md = p;
- pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
+ pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluKiB)\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->num_pages << 2));
}
#endif /* EFI_DEBUG */
}
--
1.7.9.3
On Tue, 29 Jul 2014, Prarit Bhargava wrote:
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 87fc96b..3875090 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -384,10 +384,10 @@ static void __init print_efi_memmap(void)
> p < memmap.map_end;
> p += memmap.desc_size, i++) {
> md = p;
> - pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
> + pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluKiB)\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->num_pages << 2));
> }
> #endif /* EFI_DEBUG */
> }
If EFI_PAGE_SHIFT were to ever change from its hard-coded 12, then this
would be wrong. Any reason to not simply print EFI_PAGE_SIZE and
md->num_pages?
On Tue, 2014-07-29 at 13:29 -0700, David Rientjes wrote:
> On Tue, 29 Jul 2014, Prarit Bhargava wrote:
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
[]
> > @@ -384,10 +384,10 @@ static void __init print_efi_memmap(void)
> > p < memmap.map_end;
> > p += memmap.desc_size, i++) {
> > md = p;
> > - pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
> > + pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluKiB)\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->num_pages << 2));
> > }
> > #endif /* EFI_DEBUG */
> > }
>
> If EFI_PAGE_SHIFT were to ever change from its hard-coded 12, then( this
> would be wrong. Any reason to not simply print EFI_PAGE_SIZE and
> md->num_pages?
Or maybe ((u64)EFI_PAGE_SIZE * md->num_pages) >> 10
On 07/29/2014 04:29 PM, David Rientjes wrote:
> On Tue, 29 Jul 2014, Prarit Bhargava wrote:
>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index 87fc96b..3875090 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -384,10 +384,10 @@ static void __init print_efi_memmap(void)
>> p < memmap.map_end;
>> p += memmap.desc_size, i++) {
>> md = p;
>> - pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
>> + pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluKiB)\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->num_pages << 2));
>> }
>> #endif /* EFI_DEBUG */
>> }
>
> If EFI_PAGE_SHIFT were to ever change from its hard-coded 12, then this
> would be wrong. Any reason to not simply print EFI_PAGE_SIZE and
> md->num_pages?
Hmm ... maybe it might be better to print out (md->num_pages * EFI_PAGE_SIZE) >>
10 ? I'll just double check that in a kernel and repost.
Thanks for the suggestion David :)
P.
>
On 07/29/2014 06:02 PM, Joe Perches wrote:
> On Tue, 2014-07-29 at 13:29 -0700, David Rientjes wrote:
>> On Tue, 29 Jul 2014, Prarit Bhargava wrote:
>>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> []
>>> @@ -384,10 +384,10 @@ static void __init print_efi_memmap(void)
>>> p < memmap.map_end;
>>> p += memmap.desc_size, i++) {
>>> md = p;
>>> - pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
>>> + pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluKiB)\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->num_pages << 2));
>>> }
>>> #endif /* EFI_DEBUG */
>>> }
>>
>> If EFI_PAGE_SHIFT were to ever change from its hard-coded 12, then( this
>> would be wrong. Any reason to not simply print EFI_PAGE_SIZE and
>> md->num_pages?
>
> Or maybe ((u64)EFI_PAGE_SIZE * md->num_pages) >> 10
Heh :) See my just sent email. I'll give this a shot :)
P.
>
>
On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
> The current debug print in EFI does
>
> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
>
> and rounds off the size to 0MB and isn't very useful. We should print this in
> Kib. After applying this patch we get better info with
>
> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
Turning this into kiB unconditionally won't always work ok:
First of all, there might be something which parses that output so I'd
make sure I'm not breaking that. Maybe fwts... Matt will know.
Then, I have an UEFI region which is > 13G:
[ 0.000000] efi: mem42: type=7, attr=0xf, range=[0x0000000100000000-0x0000000450000000) (13568MB)
With your patch it is even worse:
[ 0.000000] efi: mem42: type=7, attr=0xf, range=[0x0000000100000000-0x0000000450000000) (13893632kiB)
I'd guess you'll have to go all out and do this properly. :-)
Something like showing the highest unit which is still > 1. In the above
case, this should be (13GB) or maybe even introduce fractions.
Depends on how involved this output should be.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/29/2014 06:29 PM, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
>> The current debug print in EFI does
>>
>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
>>
>> and rounds off the size to 0MB and isn't very useful. We should print this in
>> Kib. After applying this patch we get better info with
>>
>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
>
> Turning this into kiB unconditionally won't always work ok:
>
> First of all, there might be something which parses that output so I'd
> make sure I'm not breaking that. Maybe fwts... Matt will know.
>
> Then, I have an UEFI region which is > 13G:
>
> [ 0.000000] efi: mem42: type=7, attr=0xf, range=[0x0000000100000000-0x0000000450000000) (13568MB)
>
>
> With your patch it is even worse:
>
> [ 0.000000] efi: mem42: type=7, attr=0xf, range=[0x0000000100000000-0x0000000450000000) (13893632kiB)
>
>
> I'd guess you'll have to go all out and do this properly. :-)
>
> Something like showing the highest unit which is still > 1. In the above
> case, this should be (13GB) or maybe even introduce fractions.
>
> Depends on how involved this output should be.
Yeah, I thought about doing something like that but figured if I'm at this level
I can do some math ;), and it was best to keep the code simple with a KiB.
Matt -- will anything be broken here if we change the output?
P.
>
On Tue, Jul 29, 2014 at 06:32:56PM -0400, Prarit Bhargava wrote:
> and it was best to keep the code simple with a KiB.
You're missing the point - the output doesn't get simple with KiB. Read
the example I just gave you!
(13893632kiB) is actively *not* helping when one looks at it!
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, 30 Jul 2014, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 06:32:56PM -0400, Prarit Bhargava wrote:
> > and it was best to keep the code simple with a KiB.
>
> You're missing the point - the output doesn't get simple with KiB. Read
> the example I just gave you!
>
> (13893632kiB) is actively *not* helping when one looks at it!
>
/proc/meminfo must be frustrating then, too.
If Prarit is going to implement this suggested reverse memparse() then it
would be nice to also have it as a generic function that others can use.
On 07/29/2014 06:36 PM, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 06:32:56PM -0400, Prarit Bhargava wrote:
>> and it was best to keep the code simple with a KiB.
>
> You're missing the point - the output doesn't get simple with KiB. Read
> the example I just gave you!
>
> (13893632kiB) is actively *not* helping when one looks at it!
I did get your point and I'm (politely) disagreeing with it. Your case ONLY
works if the number is _exactly_ 13GB. What if it is 13.5? Then we're still
rounding off and reporting 14GB. Which, if this code is meant for debug, makes
no sense to me. Why round it off?
P.
>
On Tue, Jul 29, 2014 at 03:42:55PM -0700, David Rientjes wrote:
> If Prarit is going to implement this suggested reverse memparse() ...
The only meaningful argument about the formatting here IMO is what
people staring at this output are going to understand from it.
And since those people most often tend to be the poor souls dealing with
UEFI, we should ask them whether this needs fixing and if so, how.
That's it.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/29/2014 07:01 PM, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 03:42:55PM -0700, David Rientjes wrote:
>> If Prarit is going to implement this suggested reverse memparse() ...
>
> The only meaningful argument about the formatting here IMO is what
> people staring at this output are going to understand from it.
>
> And since those people most often tend to be the poor souls dealing with
> UEFI, we should ask them whether this needs fixing and if so, how.
>
... I did do a get_maintainers.pl on this before sending it. Did I miss someone
or some list? :(
P.
> That's it.
>
On Tue, 2014-07-29 at 15:42 -0700, David Rientjes wrote:
> On Wed, 30 Jul 2014, Borislav Petkov wrote:
>
> > On Tue, Jul 29, 2014 at 06:32:56PM -0400, Prarit Bhargava wrote:
> > > and it was best to keep the code simple with a KiB.
> >
> > You're missing the point - the output doesn't get simple with KiB. Read
> > the example I just gave you!
> >
> > (13893632kiB) is actively *not* helping when one looks at it!
> >
>
> /proc/meminfo must be frustrating then, too.
>
> If Prarit is going to implement this suggested reverse memparse() then it
> would be nice to also have it as a generic function that others can use.
Maybe yet another vsprintf extension?
Maybe %pH<vartype> where vartype is one of [hh, h, u, ul, ull]
with something like
u64 t1 = (u64)*(appropriate cast)vartype;
u64 t2 = t1;
int index = 0;
while ((t1 >>= 10)) {
index++;
t2 >>= 10;
}
to output the equivalent of
%llu%s, t2, "bkmgtpezy"[index]
ala dump_pagetables
On Tue, 29 Jul 2014, Joe Perches wrote:
> Maybe yet another vsprintf extension?
>
> Maybe %pH<vartype> where vartype is one of [hh, h, u, ul, ull]
> with something like
> u64 t1 = (u64)*(appropriate cast)vartype;
> u64 t2 = t1;
> int index = 0;
> while ((t1 >>= 10)) {
> index++;
> t2 >>= 10;
> }
>
> to output the equivalent of
> %llu%s, t2, "bkmgtpezy"[index]
> ala dump_pagetables
>
Prarit has a good point about unnecessarily rounding the value, which is
exactly what the patch is addressing, and it wouldn't be good to be
changing the units depending on the value if anybody is using it for
anything other than reading it with their own two eyes. Since
EFI_PAGE_SIZE will always be in KB, it makes sense to export it in KB.
On Tue, Jul 29, 2014 at 3:54 PM, Prarit Bhargava <[email protected]> wrote:
>
>
> On 07/29/2014 06:36 PM, Borislav Petkov wrote:
>> On Tue, Jul 29, 2014 at 06:32:56PM -0400, Prarit Bhargava wrote:
>>> and it was best to keep the code simple with a KiB.
>>
>> You're missing the point - the output doesn't get simple with KiB. Read
>> the example I just gave you!
>>
>> (13893632kiB) is actively *not* helping when one looks at it!
>
> I did get your point and I'm (politely) disagreeing with it. Your case ONLY
> works if the number is _exactly_ 13GB. What if it is 13.5? Then we're still
> rounding off and reporting 14GB. Which, if this code is meant for debug, makes
> no sense to me. Why round it off?
>
I think if it was being represented in procfs or sysfs, we'd need to
be extremely specific to make it machine-readable, but for a
human-readable printk, it makes sense to me to print it in the smaller
unit size until the value is in tens of the next higher unit size
(e.g. print in KiB until 10+MiB, print in MiB until 10+GiB).
On Wed, 30 Jul, at 12:29:32AM, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
> > The current debug print in EFI does
> >
> > [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
> >
> > and rounds off the size to 0MB and isn't very useful. We should print this in
> > Kib. After applying this patch we get better info with
> >
> > [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
>
> Turning this into kiB unconditionally won't always work ok:
>
> First of all, there might be something which parses that output so I'd
> make sure I'm not breaking that. Maybe fwts... Matt will know.
I'm not aware of anything that parses the dmesg output, but I'm
including Colin in case he has any insight.
--
Matt Fleming, Intel Open Source Technology Center
On 30/07/14 15:48, Matt Fleming wrote:
> On Wed, 30 Jul, at 12:29:32AM, Borislav Petkov wrote:
>> On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
>>> The current debug print in EFI does
>>>
>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
>>>
>>> and rounds off the size to 0MB and isn't very useful. We should print this in
>>> Kib. After applying this patch we get better info with
>>>
>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
>>
>> Turning this into kiB unconditionally won't always work ok:
>>
>> First of all, there might be something which parses that output so I'd
>> make sure I'm not breaking that. Maybe fwts... Matt will know.
>
> I'm not aware of anything that parses the dmesg output, but I'm
> including Colin in case he has any insight.
>
That won't break fwts.
Colin
On 07/30/2014 11:05 AM, Colin Ian King wrote:
> On 30/07/14 15:48, Matt Fleming wrote:
>> On Wed, 30 Jul, at 12:29:32AM, Borislav Petkov wrote:
>>> On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
>>>> The current debug print in EFI does
>>>>
>>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
>>>>
>>>> and rounds off the size to 0MB and isn't very useful. We should print this in
>>>> Kib. After applying this patch we get better info with
>>>>
>>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
>>>
>>> Turning this into kiB unconditionally won't always work ok:
>>>
>>> First of all, there might be something which parses that output so I'd
>>> make sure I'm not breaking that. Maybe fwts... Matt will know.
>>
>> I'm not aware of anything that parses the dmesg output, but I'm
>> including Colin in case he has any insight.
>>
> That won't break fwts.
Matt -- could you make a decision on whether or not this should be strictly in
KiB or if it should be a rolling KiB, Mib, and GiB?
My issue is simply that I'd like to know *exactly* how big each range is and not
have it rounded off. Others think that it is easier to read Mib & Gib ...
P.
>
> Colin
>
On 07/30/14 10:04, Prarit Bhargava wrote:
>
>
> On 07/30/2014 11:05 AM, Colin Ian King wrote:
>> On 30/07/14 15:48, Matt Fleming wrote:
>>> On Wed, 30 Jul, at 12:29:32AM, Borislav Petkov wrote:
>>>> On Tue, Jul 29, 2014 at 01:09:21PM -0400, Prarit Bhargava wrote:
>>>>> The current debug print in EFI does
>>>>>
>>>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (0MB)
>>>>>
>>>>> and rounds off the size to 0MB and isn't very useful. We should print this in
>>>>> Kib. After applying this patch we get better info with
>>>>>
>>>>> [ 0.000000] efi: mem84: type=3, attr=0xf, range=[0x00000000645b5000-0x00000000645fb000) (280kiB)
>>>>
>>>> Turning this into kiB unconditionally won't always work ok:
>>>>
>>>> First of all, there might be something which parses that output so I'd
>>>> make sure I'm not breaking that. Maybe fwts... Matt will know.
>>>
>>> I'm not aware of anything that parses the dmesg output, but I'm
>>> including Colin in case he has any insight.
>>>
>> That won't break fwts.
>
> Matt -- could you make a decision on whether or not this should be strictly in
> KiB or if it should be a rolling KiB, Mib, and GiB?
>
> My issue is simply that I'd like to know *exactly* how big each range is and not
> have it rounded off. Others think that it is easier to read Mib & Gib ...
It is easier to read in MiB or GiB IMHO, but since it is debug info,
exact is good. It would be even better [more readable] to use commas
(or periods/dots) to separate it into groups of 3 digits.
--
~Randy
On 07/30/2014 10:10 AM, Randy Dunlap wrote:
>>
>> My issue is simply that I'd like to know *exactly* how big each range is and not
>> have it rounded off. Others think that it is easier to read Mib & Gib ...
>
> It is easier to read in MiB or GiB IMHO, but since it is debug info,
> exact is good. It would be even better [more readable] to use commas
> (or periods/dots) to separate it into groups of 3 digits.
>
Arguably the exactness is available in the range...
-hpa
On Wed, Jul 30, 2014 at 10:21:26AM -0700, H. Peter Anvin wrote:
> Arguably the exactness is available in the range...
... and the size too. FWIW, other region dumps don't even print size:
[ 0.000000] e820: BIOS-provided physical RAM map:
[ 0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009e7ff] usable
[ 0.000000] BIOS-e820: [mem 0x000000000009e800-0x000000000009ffff] reserved
[ 0.000000] BIOS-e820: [mem 0x00000000000e0000-0x00000000000fffff] reserved
...
If we do that too for UEFI, we can certainly put an end to the
bikeshedding... provided everyone is tired of it by now. :-P
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/30/14 12:58, Borislav Petkov wrote:
> On Wed, Jul 30, 2014 at 10:21:26AM -0700, H. Peter Anvin wrote:
>> Arguably the exactness is available in the range...
>
> ... and the size too. FWIW, other region dumps don't even print size:
>
> [ 0.000000] e820: BIOS-provided physical RAM map:
> [ 0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009e7ff] usable
> [ 0.000000] BIOS-e820: [mem 0x000000000009e800-0x000000000009ffff] reserved
> [ 0.000000] BIOS-e820: [mem 0x00000000000e0000-0x00000000000fffff] reserved
> ...
>
> If we do that too for UEFI, we can certainly put an end to the
> bikeshedding... provided everyone is tired of it by now. :-P
Yes, please. (I'm tired and I barely got started ;)
--
~Randy