stand/efi: retrieving and printing the boot count

[iyan005_outlook.com]

Summary

Calling the GetNextHighMonotonicCount EFI Boot Service
and printing the actual boot count (nonvolatile high 32 bits) as
per https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-getnextmonotoniccount
Section 7.5.5

Test Plan

On UEFI devices, observe the value on boot, reboot, then
it should have incremented by 1.

[bms]

Comments inline. +10 points for joining us on Phabricator.

stand/efi/loader/bootinfo.c

141

Whitespace!

185

This is a good start, but the other 32 bits will need to be preserved to detect wraparound. I have observed this on Dell EDK2 derived firmware.

[iyan005_outlook.com]

implemented the suggestions of bms from the previous review
and improved error handling to use the correct macro

Thank you for the bonus points :)

P.S. I've just sent you by mail my (again) updated proposal with your suggestions, as per our last email exchange.

[bms]

Could use @imp eyes on this later but looks pretty straightforward.

stand/efi/loader/bootinfo.c

179

I guess C99 single-line comments are OK now, but I am used to C89 comments in C code.

[iyan005_outlook.com]

stand/efi/loader/bootinfo.c

179

wow, I didn't even know // comments is not a C89 thing.. You learn everyday. Got it for next time !

[imp]

stand/efi/loader/bootinfo.c

184

This never tests to see if the call succeeded.

[bms]

I concur with @imp 's point.

stand/efi/loader/bootinfo.c

179

I guess C99 single-line comments are OK now, but I am used to C89 comments in C code.

Yep I double checked style(9) and C99 // comments are now allowed.

However, it is best practice to follow the existing style of the file you are modifying, which, in this case, is using C89 /* */ comments.

It can be a little jarring to the eye to see the comment styles mixed and that was my initial reaction. I'm used to seeing // comments in C++98 and up code, but for C, C99 does allow them, and there are probably other places where new ("greenfield") development is using them.

It's a minor point but it's likely others may have the same reaction.

[iyan005_outlook.com]

Ok, i corrected the single line comments, and verified the return value
of the BS call, against EFI_ERROR.

However i have a question regarding the return value in case of errors.

The UEFI spec (https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-getnextmonotoniccount)
section 7.5.5 specifies only 2 possibles errors, either a device related error, or a null pointer is passed to the function (EFI_DEVICE_ERROR, EFI_INVALID_PARAMETER)

I've seen the function right above my own, efi_do_vmap(), that returns the EFI_* error, but another function bi_load_efi_data returns a errno based error (e.g. EINVAL, ENOMEM). So in my case, what would be the right thing to return upon encountering an error and why not the others :

• an errno error ?
• an EFI_* error ?
• a sentinel value, i.e. -1 ?

Thank you.

[imp]

In D56115#1284186, @iyan005_outlook.com wrote:

Ok, i corrected the single line comments, and verified the return value
of the BS call, against EFI_ERROR.

However i have a question regarding the return value in case of errors.

The UEFI spec (https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-getnextmonotoniccount)
section 7.5.5 specifies only 2 possibles errors, either a device related error, or a null pointer is passed to the function (EFI_DEVICE_ERROR, EFI_INVALID_PARAMETER)

So we've seen functions grow new errors over time, or some implementations return other errors for reasons that were consistent with the standard, but maybe not one of the ones listed.

I've seen the function right above my own, efi_do_vmap(), that returns the EFI_* error, but another function bi_load_efi_data returns a errno based error (e.g. EINVAL, ENOMEM). So in my case, what would be the right thing to return upon encountering an error and why not the others :

• an errno error ?
• an EFI_* error ?
• a sentinel value, i.e. -1 ?

So since we return uint64 that's the count, we really could only return -1 (with a suitable #define) so the caller knows the data is bogus.

Thank you.

[iyan005_outlook.com]

In D56115#1284430, @imp wrote:

In D56115#1284186, @iyan005_outlook.com wrote:

Ok, i corrected the single line comments, and verified the return value
of the BS call, against EFI_ERROR.

However i have a question regarding the return value in case of errors.

The UEFI spec (https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-getnextmonotoniccount)
section 7.5.5 specifies only 2 possibles errors, either a device related error, or a null pointer is passed to the function (EFI_DEVICE_ERROR, EFI_INVALID_PARAMETER)

So we've seen functions grow new errors over time, or some implementations return other errors for reasons that were consistent with the standard, but maybe not one of the ones listed.

Yeah that makes a lot of sense, best to be future proof.

I've seen the function right above my own, efi_do_vmap(), that returns the EFI_* error, but another function bi_load_efi_data returns a errno based error (e.g. EINVAL, ENOMEM). So in my case, what would be the right thing to return upon encountering an error and why not the others :

• an errno error ?
• an EFI_* error ?
• a sentinel value, i.e. -1 ?

So since we return uint64 that's the count, we really could only return -1 (with a suitable #define) so the caller knows the data is bogus.

Alright I'll implement that, and push it for this revision.

Thank you !

P.S. Should the error macro be defined in efierr.h ?

[bms]

In D56115#1284434, @iyan005_outlook.com wrote:

So we've seen functions grow new errors over time, or some implementations return other errors for reasons that were consistent with the standard, but maybe not one of the ones listed.

Yeah that makes a lot of sense, best to be future proof.

Warner knows far, far more about this than I do.

P.S. Should the error macro be defined in efierr.h ?

I only see efierr.h in older trees I have around. e.g. EFI_DEVICE_ERROR seems to be only defined in sys/contrib/edk2/Include/Uefi/UefiBaseType.h in -CURRENT.

Other than that, looking better all the time.

[iyan005_outlook.com]

In D56115#1284679, @bms wrote:

I only see efierr.h in older trees I have around. e.g. EFI_DEVICE_ERROR seems to be only defined in sys/contrib/edk2/Include/Uefi/UefiBaseType.h in -CURRENT.

Ow that's interesting, I'm on releng/15.0, and for me EFI_DEVICE_ERROR is in efierr.h , made me realize I should be on -CURRENT instead.

I'll be on it as soon as I submit my proposal, tomorrow is the last day.

Other than that, looking better all the time.

Thank you !

[imp]

Current adopted the upstream edk ii headers. Efierr is gone. I plan on mfcing that to 15, i think as the lessor evil.

[iyan005_outlook.com]

In D56115#1285041, @imp wrote:

Current adopted the upstream edk ii headers. Efierr is gone. I plan on mfcing that to 15, i think as the lessor evil.

Alright, well I just submitted the proposal, hopefully we get to discuss this topic again !

I'll be responsive to this thread of course.

Thank you !