* [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations.
[not found] <20180227164740.4132-1-Marvin.Haeuser@outlook.com>
@ 2018-02-27 16:47 ` Marvin Häuser
2018-02-27 18:58 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Marvin Häuser @ 2018-02-27 16:47 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com
As per the C standard, bit-level operations on signed integers are
either undefined or implementation-defined. Hence, use the BIT
definitions from Base.h where applicable and mark the others as
unsigned.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 96 ++++++++++----------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index ee016b48ded3..16abf3fce95f 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -52,46 +52,46 @@ typedef enum {
//
// Bit definitions for EFI_TIME.Daylight
//
-#define EFI_TIME_ADJUST_DAYLIGHT 0x01
-#define EFI_TIME_IN_DAYLIGHT 0x02
+#define EFI_TIME_ADJUST_DAYLIGHT BIT0
+#define EFI_TIME_IN_DAYLIGHT BIT1
///
/// Value definition for EFI_TIME.TimeZone.
///
-#define EFI_UNSPECIFIED_TIMEZONE 0x07FF
+#define EFI_UNSPECIFIED_TIMEZONE 0x07FFU
//
// Memory cacheability attributes
//
-#define EFI_MEMORY_UC 0x0000000000000001ULL
-#define EFI_MEMORY_WC 0x0000000000000002ULL
-#define EFI_MEMORY_WT 0x0000000000000004ULL
-#define EFI_MEMORY_WB 0x0000000000000008ULL
-#define EFI_MEMORY_UCE 0x0000000000000010ULL
+#define EFI_MEMORY_UC BIT0
+#define EFI_MEMORY_WC BIT1
+#define EFI_MEMORY_WT BIT2
+#define EFI_MEMORY_WB BIT3
+#define EFI_MEMORY_UCE BIT4
//
// Physical memory protection attributes
//
// Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected physical memory
// protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
//
-#define EFI_MEMORY_WP 0x0000000000001000ULL
-#define EFI_MEMORY_RP 0x0000000000002000ULL
-#define EFI_MEMORY_XP 0x0000000000004000ULL
-#define EFI_MEMORY_RO 0x0000000000020000ULL
+#define EFI_MEMORY_WP BIT12
+#define EFI_MEMORY_RP BIT13
+#define EFI_MEMORY_XP BIT14
+#define EFI_MEMORY_RO BIT17
//
// Physical memory persistence attribute.
// The memory region supports byte-addressable non-volatility.
//
-#define EFI_MEMORY_NV 0x0000000000008000ULL
+#define EFI_MEMORY_NV BIT15
//
// The memory region provides higher reliability relative to other memory in the system.
// If all memory has the same reliability, then this bit is not used.
//
-#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000ULL
+#define EFI_MEMORY_MORE_RELIABLE BIT16
//
// Runtime memory attribute
//
-#define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
+#define EFI_MEMORY_RUNTIME BIT63
///
/// Memory descriptor version number.
@@ -363,7 +363,7 @@ EFI_STATUS
//
// ConvertPointer DebugDisposition type.
//
-#define EFI_OPTIONAL_PTR 0x00000001
+#define EFI_OPTIONAL_PTR 0x00000001U
/**
Determines the new virtual address that is to be used on subsequent memory accesses.
@@ -393,20 +393,20 @@ EFI_STATUS
// EVT_TIMER might be Ored with EVT_NOTIFY_WAIT or
// EVT_NOTIFY_SIGNAL.
//
-#define EVT_TIMER 0x80000000
-#define EVT_RUNTIME 0x40000000
-#define EVT_NOTIFY_WAIT 0x00000100
-#define EVT_NOTIFY_SIGNAL 0x00000200
+#define EVT_TIMER 0x80000000U
+#define EVT_RUNTIME 0x40000000U
+#define EVT_NOTIFY_WAIT 0x00000100U
+#define EVT_NOTIFY_SIGNAL 0x00000200U
-#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201
-#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
+#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201U
+#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202U
//
// The event's NotifyContext pointer points to a runtime memory
// address.
// The event is deprecated in UEFI2.0 and later specifications.
//
-#define EVT_RUNTIME_CONTEXT 0x20000000
+#define EVT_RUNTIME_CONTEXT 0x20000000U
/**
@@ -1279,12 +1279,12 @@ EFI_STATUS
OUT VOID **Interface
);
-#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL 0x00000001
-#define EFI_OPEN_PROTOCOL_GET_PROTOCOL 0x00000002
-#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL 0x00000004
-#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
-#define EFI_OPEN_PROTOCOL_BY_DRIVER 0x00000010
-#define EFI_OPEN_PROTOCOL_EXCLUSIVE 0x00000020
+#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL BIT0
+#define EFI_OPEN_PROTOCOL_GET_PROTOCOL BIT1
+#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL BIT2
+#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER BIT3
+#define EFI_OPEN_PROTOCOL_BY_DRIVER BIT4
+#define EFI_OPEN_PROTOCOL_EXCLUSIVE BIT5
/**
Queries a handle to determine if it supports a specified protocol. If the protocol is supported by the
@@ -1656,9 +1656,9 @@ typedef struct {
VOID* CapsulePtr[1];
} EFI_CAPSULE_TABLE;
-#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000
-#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000
-#define CAPSULE_FLAGS_INITIATE_RESET 0x00040000
+#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET BIT16
+#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE BIT17
+#define CAPSULE_FLAGS_INITIATE_RESET BIT18
/**
Passes capsules to the firmware with both virtual and physical mapping. Depending on the intended
@@ -1764,12 +1764,12 @@ EFI_STATUS
//
// Firmware should stop at a firmware user interface on next boot
//
-#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI 0x0000000000000001
-#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION 0x0000000000000002
-#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED 0x0000000000000004
-#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED 0x0000000000000008
-#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED 0x0000000000000010
-#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY 0x0000000000000040
+#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI BIT0
+#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION BIT1
+#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED BIT2
+#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED BIT3
+#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED BIT4
+#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY BIT6
//
// EFI Runtime Services Table
@@ -2093,18 +2093,18 @@ typedef struct _EFI_LOAD_OPTION {
//
// EFI Load Options Attributes
//
-#define LOAD_OPTION_ACTIVE 0x00000001
-#define LOAD_OPTION_FORCE_RECONNECT 0x00000002
-#define LOAD_OPTION_HIDDEN 0x00000008
-#define LOAD_OPTION_CATEGORY 0x00001F00
+#define LOAD_OPTION_ACTIVE BIT0
+#define LOAD_OPTION_FORCE_RECONNECT BIT1
+#define LOAD_OPTION_HIDDEN BIT3
+#define LOAD_OPTION_CATEGORY 0x00001F00U
-#define LOAD_OPTION_CATEGORY_BOOT 0x00000000
-#define LOAD_OPTION_CATEGORY_APP 0x00000100
+#define LOAD_OPTION_CATEGORY_BOOT 0x00000000U
+#define LOAD_OPTION_CATEGORY_APP 0x00000100U
-#define EFI_BOOT_OPTION_SUPPORT_KEY 0x00000001
-#define EFI_BOOT_OPTION_SUPPORT_APP 0x00000002
-#define EFI_BOOT_OPTION_SUPPORT_SYSPREP 0x00000010
-#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300
+#define EFI_BOOT_OPTION_SUPPORT_KEY BIT0
+#define EFI_BOOT_OPTION_SUPPORT_APP BIT1
+#define EFI_BOOT_OPTION_SUPPORT_SYSPREP BIT4
+#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300U
///
/// EFI Boot Key Data
--
2.16.0.windows.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations.
2018-02-27 16:47 ` [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations Marvin Häuser
@ 2018-02-27 18:58 ` Laszlo Ersek
2018-02-27 19:30 ` Marvin Häuser
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2018-02-27 18:58 UTC (permalink / raw)
To: Marvin Häuser, edk2-devel@lists.01.org
Cc: michael.d.kinney@intel.com, liming.gao@intel.com
Hi Marvin,
a request in advance: could you turn on threading (and *shallow*
threading at that) in your git-send-email config? The way you sent these
patches, they are not collected in a single thread; the patches are
scattered across the archive / my edk2-devel folder.
To the patch:
On 02/27/18 17:47, Marvin Häuser wrote:
> As per the C standard, bit-level operations on signed integers are
> either undefined or implementation-defined. Hence, use the BIT
> definitions from Base.h where applicable and mark the others as
> unsigned.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
> MdePkg/Include/Uefi/UefiSpec.h | 96 ++++++++++----------
> 1 file changed, 48 insertions(+), 48 deletions(-)
>
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> index ee016b48ded3..16abf3fce95f 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -52,46 +52,46 @@ typedef enum {
> //
> // Bit definitions for EFI_TIME.Daylight
> //
> -#define EFI_TIME_ADJUST_DAYLIGHT 0x01
> -#define EFI_TIME_IN_DAYLIGHT 0x02
> +#define EFI_TIME_ADJUST_DAYLIGHT BIT0
> +#define EFI_TIME_IN_DAYLIGHT BIT1
This might look nicer, but it makes no difference.
#define BIT0 0x00000001
#define BIT1 0x00000002
These constants have type "int" just the same.
(
Now, as a curiosity, BIT31 *does* have type "unsigned int":
#define BIT31 0x80000000
This is because the 0x prefix enables, but does not force, the language
to pick unsigned types for the constant, and because 0x80000000 does not
fit in an "int". So "unsigned int" is picked.
BIT32 and the rest have type "unsigned long long" (or UINT64), so I
agree the types that the BITxy macros cover are quite inconsistent.
)
What is the exact purpose here? Making the macro definitions look nicer,
or giving the constants "unsigned int" type?
>
> ///
> /// Value definition for EFI_TIME.TimeZone.
> ///
> -#define EFI_UNSPECIFIED_TIMEZONE 0x07FF
> +#define EFI_UNSPECIFIED_TIMEZONE 0x07FFU
>
I don't know where this constant is used, but, indeed, this change gives
it "unsigned int" type (or UINT32, if you will).
> //
> // Memory cacheability attributes
> //
> -#define EFI_MEMORY_UC 0x0000000000000001ULL
> -#define EFI_MEMORY_WC 0x0000000000000002ULL
> -#define EFI_MEMORY_WT 0x0000000000000004ULL
> -#define EFI_MEMORY_WB 0x0000000000000008ULL
> -#define EFI_MEMORY_UCE 0x0000000000000010ULL
> +#define EFI_MEMORY_UC BIT0
> +#define EFI_MEMORY_WC BIT1
> +#define EFI_MEMORY_WT BIT2
> +#define EFI_MEMORY_WB BIT3
> +#define EFI_MEMORY_UCE BIT4
No, this is a bad idea. Pre-patch, the constants have type "unsigned
long long", or UINT64. Post-patch, they'd have type "int" (or INT32).
Such changes can cause tricky regressions; they would require very
careful audit.
What is the exact purpose of this hunk?
> //
> // Physical memory protection attributes
> //
> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected physical memory
> // protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
> //
> -#define EFI_MEMORY_WP 0x0000000000001000ULL
> -#define EFI_MEMORY_RP 0x0000000000002000ULL
> -#define EFI_MEMORY_XP 0x0000000000004000ULL
> -#define EFI_MEMORY_RO 0x0000000000020000ULL
> +#define EFI_MEMORY_WP BIT12
> +#define EFI_MEMORY_RP BIT13
> +#define EFI_MEMORY_XP BIT14
> +#define EFI_MEMORY_RO BIT17
> //
> // Physical memory persistence attribute.
> // The memory region supports byte-addressable non-volatility.
> //
> -#define EFI_MEMORY_NV 0x0000000000008000ULL
> +#define EFI_MEMORY_NV BIT15
> //
> // The memory region provides higher reliability relative to other memory in the system.
> // If all memory has the same reliability, then this bit is not used.
> //
> -#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000ULL
> +#define EFI_MEMORY_MORE_RELIABLE BIT16
Same counter-argument as for the "Memory cacheability attributes".
> //
> // Runtime memory attribute
> //
> -#define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
> +#define EFI_MEMORY_RUNTIME BIT63
This change is valid (and a no-op, functionally).
>
> ///
> /// Memory descriptor version number.
> @@ -363,7 +363,7 @@ EFI_STATUS
> //
> // ConvertPointer DebugDisposition type.
> //
> -#define EFI_OPTIONAL_PTR 0x00000001
> +#define EFI_OPTIONAL_PTR 0x00000001U
>
> /**
This has the documented effect (the type changes from "int" to "unsigned
int", or INT32 -> UINT32). Did you audit all the locations where the
macro is used? (Even a signed -> unsigned change could cause unexpected
results.)
> Determines the new virtual address that is to be used on subsequent memory accesses.
> @@ -393,20 +393,20 @@ EFI_STATUS
> // EVT_TIMER might be Ored with EVT_NOTIFY_WAIT or
> // EVT_NOTIFY_SIGNAL.
> //
> -#define EVT_TIMER 0x80000000
> -#define EVT_RUNTIME 0x40000000
> -#define EVT_NOTIFY_WAIT 0x00000100
> -#define EVT_NOTIFY_SIGNAL 0x00000200
> +#define EVT_TIMER 0x80000000U
> +#define EVT_RUNTIME 0x40000000U
> +#define EVT_NOTIFY_WAIT 0x00000100U
> +#define EVT_NOTIFY_SIGNAL 0x00000200U
>
> -#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201
> -#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
> +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201U
> +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202U
>
> //
> // The event's NotifyContext pointer points to a runtime memory
> // address.
> // The event is deprecated in UEFI2.0 and later specifications.
> //
> -#define EVT_RUNTIME_CONTEXT 0x20000000
> +#define EVT_RUNTIME_CONTEXT 0x20000000U
>
>
> /**
In what contexts are these used so that they have to be, and can be,
unsigned? (In general I agree that all such macros should expand to
unsigned integer constants, but, as I mentioned earlier, changing them
*now* might require an audit of all usage sites.)
> @@ -1279,12 +1279,12 @@ EFI_STATUS
> OUT VOID **Interface
> );
>
> -#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL 0x00000001
> -#define EFI_OPEN_PROTOCOL_GET_PROTOCOL 0x00000002
> -#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL 0x00000004
> -#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
> -#define EFI_OPEN_PROTOCOL_BY_DRIVER 0x00000010
> -#define EFI_OPEN_PROTOCOL_EXCLUSIVE 0x00000020
> +#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL BIT0
> +#define EFI_OPEN_PROTOCOL_GET_PROTOCOL BIT1
> +#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL BIT2
> +#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER BIT3
> +#define EFI_OPEN_PROTOCOL_BY_DRIVER BIT4
> +#define EFI_OPEN_PROTOCOL_EXCLUSIVE BIT5
>
> /**
Right, these look nicer.
> Queries a handle to determine if it supports a specified protocol. If the protocol is supported by the
> @@ -1656,9 +1656,9 @@ typedef struct {
> VOID* CapsulePtr[1];
> } EFI_CAPSULE_TABLE;
>
> -#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000
> -#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000
> -#define CAPSULE_FLAGS_INITIATE_RESET 0x00040000
> +#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET BIT16
> +#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE BIT17
> +#define CAPSULE_FLAGS_INITIATE_RESET BIT18
>
> /**
OK.
> Passes capsules to the firmware with both virtual and physical mapping. Depending on the intended
> @@ -1764,12 +1764,12 @@ EFI_STATUS
> //
> // Firmware should stop at a firmware user interface on next boot
> //
> -#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI 0x0000000000000001
> -#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION 0x0000000000000002
> -#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED 0x0000000000000004
> -#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED 0x0000000000000008
> -#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED 0x0000000000000010
> -#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY 0x0000000000000040
> +#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI BIT0
> +#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION BIT1
> +#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED BIT2
> +#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED BIT3
> +#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED BIT4
> +#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY BIT6
>
> //
> // EFI Runtime Services Table
OK
> @@ -2093,18 +2093,18 @@ typedef struct _EFI_LOAD_OPTION {
> //
> // EFI Load Options Attributes
> //
> -#define LOAD_OPTION_ACTIVE 0x00000001
> -#define LOAD_OPTION_FORCE_RECONNECT 0x00000002
> -#define LOAD_OPTION_HIDDEN 0x00000008
> -#define LOAD_OPTION_CATEGORY 0x00001F00
> +#define LOAD_OPTION_ACTIVE BIT0
> +#define LOAD_OPTION_FORCE_RECONNECT BIT1
> +#define LOAD_OPTION_HIDDEN BIT3
> +#define LOAD_OPTION_CATEGORY 0x00001F00U
The first three changes are functionally no-ops, but the fourth change
is inconsistent (it changes the type from "int" to "unsigned int"). Is
that intentional?
>
> -#define LOAD_OPTION_CATEGORY_BOOT 0x00000000
> -#define LOAD_OPTION_CATEGORY_APP 0x00000100
> +#define LOAD_OPTION_CATEGORY_BOOT 0x00000000U
> +#define LOAD_OPTION_CATEGORY_APP 0x00000100U
>
> -#define EFI_BOOT_OPTION_SUPPORT_KEY 0x00000001
> -#define EFI_BOOT_OPTION_SUPPORT_APP 0x00000002
> -#define EFI_BOOT_OPTION_SUPPORT_SYSPREP 0x00000010
> -#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300
> +#define EFI_BOOT_OPTION_SUPPORT_KEY BIT0
> +#define EFI_BOOT_OPTION_SUPPORT_APP BIT1
> +#define EFI_BOOT_OPTION_SUPPORT_SYSPREP BIT4
> +#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300U
>
> ///
> /// EFI Boot Key Data
>
Similar comments as above.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations.
2018-02-27 18:58 ` Laszlo Ersek
@ 2018-02-27 19:30 ` Marvin Häuser
0 siblings, 0 replies; 3+ messages in thread
From: Marvin Häuser @ 2018-02-27 19:30 UTC (permalink / raw)
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek, michael.d.kinney@intel.com, liming.gao@intel.com
Hey Laszlo,
Thanks for your reply. Comments are inline.
Regards,
Marvin.
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, February 27, 2018 7:59 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: michael.d.kinney@intel.com; liming.gao@intel.com
> Subject: Re: [edk2] [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise
> operations.
>
> Hi Marvin,
>
> a request in advance: could you turn on threading (and *shallow* threading
> at that) in your git-send-email config? The way you sent these patches, they
> are not collected in a single thread; the patches are scattered across the
> archive / my edk2-devel folder.
Sorry, I will research that. :)
>
> To the patch:
>
> On 02/27/18 17:47, Marvin Häuser wrote:
> > As per the C standard, bit-level operations on signed integers are
> > either undefined or implementation-defined. Hence, use the BIT
> > definitions from Base.h where applicable and mark the others as
> > unsigned.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > ---
> > MdePkg/Include/Uefi/UefiSpec.h | 96 ++++++++++----------
> > 1 file changed, 48 insertions(+), 48 deletions(-)
> >
> > diff --git a/MdePkg/Include/Uefi/UefiSpec.h
> > b/MdePkg/Include/Uefi/UefiSpec.h index ee016b48ded3..16abf3fce95f
> > 100644
> > --- a/MdePkg/Include/Uefi/UefiSpec.h
> > +++ b/MdePkg/Include/Uefi/UefiSpec.h
> > @@ -52,46 +52,46 @@ typedef enum {
> > //
> > // Bit definitions for EFI_TIME.Daylight // -#define
> > EFI_TIME_ADJUST_DAYLIGHT 0x01
> > -#define EFI_TIME_IN_DAYLIGHT 0x02
> > +#define EFI_TIME_ADJUST_DAYLIGHT BIT0
> > +#define EFI_TIME_IN_DAYLIGHT BIT1
>
> This might look nicer, but it makes no difference.
This also makes the values unsigned as per the previous patch.
>
> #define BIT0 0x00000001
> #define BIT1 0x00000002
>
> These constants have type "int" just the same.
>
> (
>
> Now, as a curiosity, BIT31 *does* have type "unsigned int":
>
> #define BIT31 0x80000000
>
> This is because the 0x prefix enables, but does not force, the language to pick
> unsigned types for the constant, and because 0x80000000 does not fit in an
> "int". So "unsigned int" is picked.
>
> BIT32 and the rest have type "unsigned long long" (or UINT64), so I agree the
> types that the BITxy macros cover are quite inconsistent.
>
> )
>
> What is the exact purpose here? Making the macro definitions look nicer, or
> giving the constants "unsigned int" type?
Making them unsigned was the point, as it's relevant for bitwise operations as also discussed regarding the 'prevent truncation' patches.
>
> >
> > ///
> > /// Value definition for EFI_TIME.TimeZone.
> > ///
> > -#define EFI_UNSPECIFIED_TIMEZONE 0x07FF
> > +#define EFI_UNSPECIFIED_TIMEZONE 0x07FFU
> >
>
> I don't know where this constant is used, but, indeed, this change gives it
> "unsigned int" type (or UINT32, if you will).
>
> > //
> > // Memory cacheability attributes
> > //
> > -#define EFI_MEMORY_UC 0x0000000000000001ULL
> > -#define EFI_MEMORY_WC 0x0000000000000002ULL
> > -#define EFI_MEMORY_WT 0x0000000000000004ULL
> > -#define EFI_MEMORY_WB 0x0000000000000008ULL
> > -#define EFI_MEMORY_UCE 0x0000000000000010ULL
> > +#define EFI_MEMORY_UC BIT0
> > +#define EFI_MEMORY_WC BIT1
> > +#define EFI_MEMORY_WT BIT2
> > +#define EFI_MEMORY_WB BIT3
> > +#define EFI_MEMORY_UCE BIT4
>
> No, this is a bad idea. Pre-patch, the constants have type "unsigned long
> long", or UINT64. Post-patch, they'd have type "int" (or INT32).
> Such changes can cause tricky regressions; they would require very careful
> audit.
>
> What is the exact purpose of this hunk?
This was just supposed to give some continuity to the definitions. If you are afraid something could break, this can be dropped entirely.
>
> > //
> > // Physical memory protection attributes // // Note: UEFI spec 2.5
> > and following: use EFI_MEMORY_RO as write-protected physical memory
> > // protection attribute. Also, EFI_MEMORY_WP means cacheability
> attribute.
> > //
> > -#define EFI_MEMORY_WP 0x0000000000001000ULL
> > -#define EFI_MEMORY_RP 0x0000000000002000ULL
> > -#define EFI_MEMORY_XP 0x0000000000004000ULL
> > -#define EFI_MEMORY_RO 0x0000000000020000ULL
> > +#define EFI_MEMORY_WP BIT12
> > +#define EFI_MEMORY_RP BIT13
> > +#define EFI_MEMORY_XP BIT14
> > +#define EFI_MEMORY_RO BIT17
> > //
> > // Physical memory persistence attribute.
> > // The memory region supports byte-addressable non-volatility.
> > //
> > -#define EFI_MEMORY_NV 0x0000000000008000ULL
> > +#define EFI_MEMORY_NV BIT15
> > //
> > // The memory region provides higher reliability relative to other memory
> in the system.
> > // If all memory has the same reliability, then this bit is not used.
> > //
> > -#define EFI_MEMORY_MORE_RELIABLE 0x0000000000010000ULL
> > +#define EFI_MEMORY_MORE_RELIABLE BIT16
>
> Same counter-argument as for the "Memory cacheability attributes".
Same here too.
>
> > //
> > // Runtime memory attribute
> > //
> > -#define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
> > +#define EFI_MEMORY_RUNTIME BIT63
>
> This change is valid (and a no-op, functionally).
I would probably vote for 'all or none' and drop this as well.
>
> >
> > ///
> > /// Memory descriptor version number.
> > @@ -363,7 +363,7 @@ EFI_STATUS
> > //
> > // ConvertPointer DebugDisposition type.
> > //
> > -#define EFI_OPTIONAL_PTR 0x00000001
> > +#define EFI_OPTIONAL_PTR 0x00000001U
> >
> > /**
>
> This has the documented effect (the type changes from "int" to "unsigned
> int", or INT32 -> UINT32). Did you audit all the locations where the macro is
> used? (Even a signed -> unsigned change could cause unexpected
> results.)
>
The only locations I saw the definition used were drivers passing it unchanged and RuntimeDxe, checking with a binary AND for whether the bit is set. As this is a bit definitions supposed to be used with bit operations, I marked it as unsigned to prevent triggering implementation-defined behavior. Signed-specific behavior should be avoided in the first place. Actually, this should be BIT0 to be continuous with the other changes.
> > Determines the new virtual address that is to be used on subsequent
> memory accesses.
> > @@ -393,20 +393,20 @@ EFI_STATUS
> > // EVT_TIMER might be Ored with EVT_NOTIFY_WAIT or //
> > EVT_NOTIFY_SIGNAL.
> > //
> > -#define EVT_TIMER 0x80000000
> > -#define EVT_RUNTIME 0x40000000
> > -#define EVT_NOTIFY_WAIT 0x00000100
> > -#define EVT_NOTIFY_SIGNAL 0x00000200
> > +#define EVT_TIMER 0x80000000U
> > +#define EVT_RUNTIME 0x40000000U
> > +#define EVT_NOTIFY_WAIT 0x00000100U
> > +#define EVT_NOTIFY_SIGNAL 0x00000200U
To be continuous with my other changes, these should actually have used BITx.
> >
> > -#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201
> > -#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
> > +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201U
> > +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202U
> >
> > //
> > // The event's NotifyContext pointer points to a runtime memory //
> > address.
> > // The event is deprecated in UEFI2.0 and later specifications.
> > //
> > -#define EVT_RUNTIME_CONTEXT 0x20000000
> > +#define EVT_RUNTIME_CONTEXT 0x20000000U
> >
> >
> > /**
>
> In what contexts are these used so that they have to be, and can be,
> unsigned? (In general I agree that all such macros should expand to unsigned
> integer constants, but, as I mentioned earlier, changing them
> *now* might require an audit of all usage sites.)
This is actually not used anymore at all within the edk2 repo.
>
> > @@ -1279,12 +1279,12 @@ EFI_STATUS
> > OUT VOID **Interface
> > );
> >
> > -#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL 0x00000001
> > -#define EFI_OPEN_PROTOCOL_GET_PROTOCOL 0x00000002
> > -#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL 0x00000004
> > -#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
> > -#define EFI_OPEN_PROTOCOL_BY_DRIVER 0x00000010
> > -#define EFI_OPEN_PROTOCOL_EXCLUSIVE 0x00000020
> > +#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL BIT0
> > +#define EFI_OPEN_PROTOCOL_GET_PROTOCOL BIT1
> > +#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL BIT2
> > +#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER BIT3
> > +#define EFI_OPEN_PROTOCOL_BY_DRIVER BIT4
> > +#define EFI_OPEN_PROTOCOL_EXCLUSIVE BIT5
> >
> > /**
>
> Right, these look nicer.
>
> > Queries a handle to determine if it supports a specified protocol.
> > If the protocol is supported by the @@ -1656,9 +1656,9 @@ typedef struct
> {
> > VOID* CapsulePtr[1];
> > } EFI_CAPSULE_TABLE;
> >
> > -#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000
> > -#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000
> > -#define CAPSULE_FLAGS_INITIATE_RESET 0x00040000
> > +#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET BIT16
> > +#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE BIT17
> > +#define CAPSULE_FLAGS_INITIATE_RESET BIT18
> >
> > /**
>
> OK.
>
> > Passes capsules to the firmware with both virtual and physical
> > mapping. Depending on the intended @@ -1764,12 +1764,12 @@
> EFI_STATUS
> > // // Firmware should stop at a firmware user interface on next boot
> > //
> > -#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI
> 0x0000000000000001
> > -#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION
> 0x0000000000000002
> > -#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> 0x0000000000000004
> > -#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED
> 0x0000000000000008
> > -#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED
> 0x0000000000000010
> > -#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY
> 0x0000000000000040
> > +#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI BIT0
> > +#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION BIT1
> > +#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> BIT2
> > +#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED BIT3
> > +#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED
> BIT4
> > +#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY BIT6
> >
> > //
> > // EFI Runtime Services Table
>
> OK
>
> > @@ -2093,18 +2093,18 @@ typedef struct _EFI_LOAD_OPTION { // // EFI
> > Load Options Attributes //
> > -#define LOAD_OPTION_ACTIVE 0x00000001
> > -#define LOAD_OPTION_FORCE_RECONNECT 0x00000002
> > -#define LOAD_OPTION_HIDDEN 0x00000008
> > -#define LOAD_OPTION_CATEGORY 0x00001F00
> > +#define LOAD_OPTION_ACTIVE BIT0
> > +#define LOAD_OPTION_FORCE_RECONNECT BIT1
> > +#define LOAD_OPTION_HIDDEN BIT3
> > +#define LOAD_OPTION_CATEGORY 0x00001F00U
>
> The first three changes are functionally no-ops, but the fourth change is
> inconsistent (it changes the type from "int" to "unsigned int"). Is that
> intentional?
The first few are not no-ops as per the first patch, making them unsigned.
The inconsistency results from the amount of BIT definitions that would be required to be OR'd together to achieve this value.
>
> >
> > -#define LOAD_OPTION_CATEGORY_BOOT 0x00000000
> > -#define LOAD_OPTION_CATEGORY_APP 0x00000100
> > +#define LOAD_OPTION_CATEGORY_BOOT 0x00000000U
> > +#define LOAD_OPTION_CATEGORY_APP 0x00000100U
> >
> > -#define EFI_BOOT_OPTION_SUPPORT_KEY 0x00000001
> > -#define EFI_BOOT_OPTION_SUPPORT_APP 0x00000002
> > -#define EFI_BOOT_OPTION_SUPPORT_SYSPREP 0x00000010
> > -#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300
> > +#define EFI_BOOT_OPTION_SUPPORT_KEY BIT0
> > +#define EFI_BOOT_OPTION_SUPPORT_APP BIT1
> > +#define EFI_BOOT_OPTION_SUPPORT_SYSPREP BIT4
> > +#define EFI_BOOT_OPTION_SUPPORT_COUNT 0x00000300U
> >
> > ///
> > /// EFI Boot Key Data
> >
>
> Similar comments as above.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-27 19:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180227164740.4132-1-Marvin.Haeuser@outlook.com>
2018-02-27 16:47 ` [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations Marvin Häuser
2018-02-27 18:58 ` Laszlo Ersek
2018-02-27 19:30 ` Marvin Häuser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox