Handling EFI errors (please see below)

[ghost]

Hello good sir! Long time spectator of your OS development series! After studying your source code and the EFI spec I've started to develop my own EFI API. Now the problem I have found here, and this was after extensive testing, is that the way you handle errors in EFI_ERROR isn't correct. The error codes that get returned I've noticed are not negative values so by checking for a negative values this is always going to succeed because there is none. So instead of checking for negative values we should instead test for those bits using bit masking. Here's an example of what I came up with (similar but fixes the EFI_ERROR issue)

#define EFI_TOP_BIT 0x8000000000000000
#define EFI_ENCODE_ERROR(x) (EFI_TOP_BIT | (x))
#define EFI_ERROR(x) (EFI_TOP_BIT & (x))

Again my apologies for the open issue good sir I thought I'd share this for everyone working on their own API 😊

Regards PC-Doctor

[queso-fuego]

I basically took the error macros from the EDK2 repo here https://github.com/tianocore/edk2/blob/af6e0e728f652f496a6fb1e659617c9662f774ac/BaseTools/Source/C/Include/Common/BaseTypes.h#L203,

although looking at that again I did have it slightly off with the parentheses. I have #define EFI_ERROR(x) ((INTN)((UINTN)(x)) < 0) but it should be #define EFI_ERROR(x) (((INTN)(UINTN)(x)) < 0), where x is casted to the full uint64 size and then to int64 before checking if less than 0.

define EFI_TOP_BIT 0x8000000000000000 is a negative value, when compared as a signed integer. It's equal to INT64_MIN. Casting to unsigned and then back to signed for a positive/negative comparison does work as when casted to a signed integer, any value >= INT64_MIN and < 0 will be negative.

Sample program:

#include <stdint.h>
#include <stdio.h>

#define TOP_BIT 0x8000000000000000
#define ENCODE_ERROR(x) (TOP_BIT | (x))
#define EFI_ERROR(x) (((INTN)(UINTN)(x)) < 0)

#define EFI_UNSUPPORTED  ENCODE_ERROR(3)

typedef int64_t  INTN;
typedef uint64_t UINTN;
typedef UINTN    EFI_STATUS;

int main(void) {
    printf("INT64_MAX: %#lx, INT64_MIN: %#lx\n"
           "As decimals: %ld, %ld\n", 
           INT64_MAX, INT64_MIN,
           INT64_MAX, INT64_MIN);

    UINTN test = EFI_UNSUPPORTED;
    printf("\nEFI_UNSUPPORTED: %#lx\n" 
           "(INTN)(UINTN) for EFI_UNSUPPORTED: %#lx (%ld)\n"
           "Is the Int value negative: %s\n"
           "EFI Error?: %s\n", 
           test, 
           (INTN)(UINTN)test, (INTN)(UINTN)test,
           (INTN)(UINTN)test < 0 ? "Yes" : "No",
           EFI_ERROR(test) ? "Yes" : "No");
}

Shows me output:

INT64_MAX: 0x7fffffffffffffff, INT64_MIN: 0x8000000000000000
As decimals: 9223372036854775807, -9223372036854775808

EFI_UNSUPPORTED: 0x8000000000000003
(INTN)(UINTN) for EFI_UNSUPPORTED: 0x8000000000000003 (-9223372036854775805)
Is the Int value negative: Yes
EFI Error?: Yes

So I'm not sure where it would be messing up, an example that I can test and reproduce would be good to double check.

However, your solution does also work for checking if that top bit is set, which is good!

[ghost]

I basically took the error macros from the EDK2 repo here https://github.com/tianocore/edk2/blob/af6e0e728f652f496a6fb1e659617c9662f774ac/BaseTools/Source/C/Include/Common/BaseTypes.h#L203,

although looking at that again I did have it slightly off with the parentheses. I have #define EFI_ERROR(x) ((INTN)((UINTN)(x)) < 0) but it should be #define EFI_ERROR(x) (((INTN)(UINTN)(x)) < 0), where x is casted to the full uint64 size and then to int64 before checking if less than 0.

define EFI_TOP_BIT 0x8000000000000000 is a negative value, when compared as a signed integer. It's equal to INT64_MIN. Casting to unsigned and then back to signed for a positive/negative comparison does work as when casted to a signed integer, any value >= INT64_MIN and < 0 will be negative.

Sample program:

#include <stdint.h>
#include <stdio.h>

#define TOP_BIT 0x8000000000000000
#define ENCODE_ERROR(x) (TOP_BIT | (x))
#define EFI_ERROR(x) (((INTN)(UINTN)(x)) < 0)

#define EFI_UNSUPPORTED  ENCODE_ERROR(3)

typedef int64_t  INTN;
typedef uint64_t UINTN;
typedef UINTN    EFI_STATUS;

int main(void) {
    printf("INT64_MAX: %#lx, INT64_MIN: %#lx\n"
           "As decimals: %ld, %ld\n", 
           INT64_MAX, INT64_MIN,
           INT64_MAX, INT64_MIN);

    UINTN test = EFI_UNSUPPORTED;
    printf("\nEFI_UNSUPPORTED: %#lx\n" 
           "(INTN)(UINTN) for EFI_UNSUPPORTED: %#lx (%ld)\n"
           "Is the Int value negative: %s\n"
           "EFI Error?: %s\n", 
           test, 
           (INTN)(UINTN)test, (INTN)(UINTN)test,
           (INTN)(UINTN)test < 0 ? "Yes" : "No",
           EFI_ERROR(test) ? "Yes" : "No");
}

Shows me output:

INT64_MAX: 0x7fffffffffffffff, INT64_MIN: 0x8000000000000000
As decimals: 9223372036854775807, -9223372036854775808

EFI_UNSUPPORTED: 0x8000000000000003
(INTN)(UINTN) for EFI_UNSUPPORTED: 0x8000000000000003 (-9223372036854775805)
Is the Int value negative: Yes
EFI Error?: Yes

So I'm not sure where it would be messing up, an example that I can test and reproduce would be good to double check.

However, your solution does also work for checking if that top bit is set, which is good!

I stand corrected! My apologies!