From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 545292034D8D3 for ; Tue, 27 Feb 2018 10:52:36 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 29AE5404084B; Tue, 27 Feb 2018 18:58:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-21.rdu2.redhat.com [10.10.120.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4412B23167; Tue, 27 Feb 2018 18:58:41 +0000 (UTC) To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" Cc: "michael.d.kinney@intel.com" , "liming.gao@intel.com" References: <20180227164740.4132-1-Marvin.Haeuser@outlook.com> From: Laszlo Ersek Message-ID: Date: Tue, 27 Feb 2018 19:58:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 27 Feb 2018 18:58:42 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 27 Feb 2018 18:58:42 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 18:52:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > --- > 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