[edk2-devel] [PATCH v2 1/6] OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32

Gerd Hoffmann posted 6 patches 2 years, 6 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 1/6] OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
Posted by Gerd Hoffmann 2 years, 6 months ago
This is needed to avoid bit operations being applied to signed integers.

Suggested-by: László Érsek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h | 2 +-
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
index b7f5d208b236..455eafacc2cf 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
@@ -61,7 +61,7 @@
 #define P30_MAX_BUFFER_SIZE_IN_BYTES  ((UINTN)128)
 #define P30_MAX_BUFFER_SIZE_IN_WORDS  (P30_MAX_BUFFER_SIZE_IN_BYTES/((UINTN)4))
 #define MAX_BUFFERED_PROG_ITERATIONS  10000000
-#define BOUNDARY_OF_32_WORDS          0x7F
+#define BOUNDARY_OF_32_WORDS          ((UINTN)0x7F)
 
 // CFI Addresses
 #define P30_CFI_ADDR_QUERY_UNIQUE_QRY  0x10
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 1afd60ce66eb..7f4743b00399 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -581,7 +581,7 @@ NorFlashWriteSingleBlock (
     // contents, while checking whether the old version had any bits cleared
     // that we want to set. In that case, we will need to erase the block first.
     for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
-      if (~OrigData[CurOffset] & Buffer[CurOffset]) {
+      if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
         goto DoErase;
       }
 
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113828): https://edk2.groups.io/g/devel/message/113828
Mute This Topic: https://groups.io/mt/103741662/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v2 1/6] OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
Posted by Laszlo Ersek 2 years, 6 months ago
On 1/15/24 16:59, Gerd Hoffmann wrote:
> This is needed to avoid bit operations being applied to signed integers.
> 
> Suggested-by: László Érsek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h | 2 +-
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
> index b7f5d208b236..455eafacc2cf 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
> @@ -61,7 +61,7 @@
>  #define P30_MAX_BUFFER_SIZE_IN_BYTES  ((UINTN)128)
>  #define P30_MAX_BUFFER_SIZE_IN_WORDS  (P30_MAX_BUFFER_SIZE_IN_BYTES/((UINTN)4))
>  #define MAX_BUFFERED_PROG_ITERATIONS  10000000
> -#define BOUNDARY_OF_32_WORDS          0x7F
> +#define BOUNDARY_OF_32_WORDS          ((UINTN)0x7F)
>  
>  // CFI Addresses
>  #define P30_CFI_ADDR_QUERY_UNIQUE_QRY  0x10

I've made an effort to audit all current (= pre-patch) uses of
BOUNDARY_OF_32_WORDS: the change looks safe.

> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 1afd60ce66eb..7f4743b00399 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -581,7 +581,7 @@ NorFlashWriteSingleBlock (
>      // contents, while checking whether the old version had any bits cleared
>      // that we want to set. In that case, we will need to erase the block first.
>      for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
> -      if (~OrigData[CurOffset] & Buffer[CurOffset]) {
> +      if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
>          goto DoErase;
>        }
>  

The explicit cast for the RHS is not strictly necessary (the same would
happen as a consequence of the cast being added to the LHS, through the
usual arithmetic conversions), *but* it definitely doesn't hurt, and
arguably improves readability.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I'll probably look at the rest of the patches tomorrow.)

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113846): https://edk2.groups.io/g/devel/message/113846
Mute This Topic: https://groups.io/mt/103741662/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-