[edk2-devel] [PATCH v3 0/6] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[edk2-devel] [PATCH v3 0/6] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Gerd Hoffmann]

This is a little series containing the flash corruption fix sent
yesterday with an slightly improved commit message and some small
improvements on top of this.

v3:
 - fix diagram
 - fix DoErase control flow
 - pick up reviewed-by tags
v2:
 - drop broken bugfix, fix the bug when introducing Start+End variables
   instead.
 - add patch with UINTN and UINT32 casts.
 - add patch splitting the DoErase code path into a new function.
 - add the diagram sent by Laszlo.

Gerd Hoffmann (6):
  OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
  OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer
    reads
  OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.
  OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
  OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
  OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h    |   2 +-
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 145 ++++++++++++++--------
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |   5 +
 3 files changed, 101 insertions(+), 51 deletions(-)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113915): https://edk2.groups.io/g/devel/message/113915
Mute This Topic: https://groups.io/mt/103766773/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

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>
Reviewed-by: Laszlo Ersek <lersek@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 (#113917): https://edk2.groups.io/g/devel/message/113917
Mute This Topic: https://groups.io/mt/103766775/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/6] OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer reads

[Gerd Hoffmann]

Introduce 'Start' and 'End' variables to make it easier to follow the
logic and code flow.  Also add a ascii art diagram (based on a
suggestion by Laszlo).

This also fixes the 'Size' calculation for the NorFlashRead() call.
Without this patch the code will read only one instead of two
P30_MAX_BUFFER_SIZE_IN_BYTES blocks in case '*NumBytes' is smaller than
P30_MAX_BUFFER_SIZE_IN_BYTES but 'Offset + *NumBytes' is not, i.e. the
update range crosses a P30_MAX_BUFFER_SIZE_IN_BYTES boundary.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 36 ++++++++++++++++++++------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 7f4743b00399..88a4d2c23fc0 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -520,6 +520,7 @@ NorFlashWriteSingleBlock (
   UINTN       BlockSize;
   UINTN       BlockAddress;
   UINT8       *OrigData;
+  UINTN       Start, End;
 
   DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes, Buffer));
 
@@ -555,7 +556,28 @@ NorFlashWriteSingleBlock (
   // To avoid pathological cases were a 2 byte write is disregarded because it
   // occurs right at a 128 byte buffered write alignment boundary, permit up to
   // twice the max buffer size, and perform two writes if needed.
-  if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
+  //
+  //    0               128              256
+  //    [----------------|----------------]
+  //    ^         ^             ^         ^
+  //    |         |             |         |
+  //    |         |             |        End, the next "word" boundary beyond
+  //    |         |             |        the (logical) update
+  //    |         |             |
+  //    |         |     (Offset & BOUNDARY_OF_32_WORDS) + NumBytes;
+  //    |         |     i.e., the relative offset inside (or just past)
+  //    |         |     the *double-word* such that it is the
+  //    |         |     *exclusive* end of the (logical) update.
+  //    |         |
+  //    |         Offset & BOUNDARY_OF_32_WORDS; i.e., Offset within the "word";
+  //    |         this is where the (logical) update is supposed to start
+  //    |
+  //    Start = Offset & ~BOUNDARY_OF_32_WORDS; i.e., Offset truncated to "word" boundary
+
+  Start = Offset & ~BOUNDARY_OF_32_WORDS;
+  End   = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
+
+  if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
     // Check to see if we need to erase before programming the data into NOR.
     // If the destination bits are only changing from 1s to 0s we can just write.
     // After a block is erased all bits in the block is set to 1.
@@ -565,8 +587,8 @@ NorFlashWriteSingleBlock (
     Status = NorFlashRead (
                Instance,
                Lba,
-               Offset & ~BOUNDARY_OF_32_WORDS,
-               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
+               Start,
+               End - Start,
                Instance->ShadowBuffer
                );
     if (EFI_ERROR (Status)) {
@@ -601,7 +623,7 @@ NorFlashWriteSingleBlock (
 
     Status = NorFlashWriteBuffer (
                Instance,
-               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+               BlockAddress + Start,
                P30_MAX_BUFFER_SIZE_IN_BYTES,
                Instance->ShadowBuffer
                );
@@ -609,12 +631,10 @@ NorFlashWriteSingleBlock (
       goto Exit;
     }
 
-    if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
-      BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES;
-
+    if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
       Status = NorFlashWriteBuffer (
                  Instance,
-                 BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+                 BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
                  P30_MAX_BUFFER_SIZE_IN_BYTES,
                  Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
                  );
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113916): https://edk2.groups.io/g/devel/message/113916
Mute This Topic: https://groups.io/mt/103766774/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/6] OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.

[Gerd Hoffmann]

Replace the two NorFlashWriteBuffer() calls with a loop containing a
single NorFlashWriteBuffer() call.

With the changes in place the code is able to handle updates larger
than two P30_MAX_BUFFER_SIZE_IN_BYTES blocks, even though the patch
does not actually change the size limit.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 88a4d2c23fc0..3d1343b381bc 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -521,6 +521,7 @@ NorFlashWriteSingleBlock (
   UINTN       BlockAddress;
   UINT8       *OrigData;
   UINTN       Start, End;
+  UINT32      Index, Count;
 
   DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes, Buffer));
 
@@ -621,23 +622,17 @@ NorFlashWriteSingleBlock (
       goto Exit;
     }
 
-    Status = NorFlashWriteBuffer (
-               Instance,
-               BlockAddress + Start,
-               P30_MAX_BUFFER_SIZE_IN_BYTES,
-               Instance->ShadowBuffer
-               );
-    if (EFI_ERROR (Status)) {
-      goto Exit;
-    }
-
-    if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
+    Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
+    for (Index = 0; Index < Count; Index++) {
       Status = NorFlashWriteBuffer (
                  Instance,
-                 BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
+                 BlockAddress + Start + Index * P30_MAX_BUFFER_SIZE_IN_BYTES,
                  P30_MAX_BUFFER_SIZE_IN_BYTES,
-                 Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
+                 Instance->ShadowBuffer + Index * P30_MAX_BUFFER_SIZE_IN_BYTES
                  );
+      if (EFI_ERROR (Status)) {
+        goto Exit;
+      }
     }
 
 Exit:
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113919): https://edk2.groups.io/g/devel/message/113919
Mute This Topic: https://groups.io/mt/103766777/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/6] OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase

[Gerd Hoffmann]

Raise the limit for writes without block erase from two to four
P30_MAX_BUFFER_SIZE_IN_BYTES blocks.  With this in place almost all efi
variable updates are handled without block erase.  With the old limit
some variable updates (with device paths) took the block erase code
path.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 3d1343b381bc..3d1d20daa1e5 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -550,13 +550,15 @@ NorFlashWriteSingleBlock (
     return EFI_BAD_BUFFER_SIZE;
   }
 
-  // Pick P30_MAX_BUFFER_SIZE_IN_BYTES (== 128 bytes) as a good start for word
-  // operations as opposed to erasing the block and writing the data regardless
-  // if an erase is really needed.  It looks like most individual NV variable
-  // writes are smaller than 128 bytes.
-  // To avoid pathological cases were a 2 byte write is disregarded because it
-  // occurs right at a 128 byte buffered write alignment boundary, permit up to
-  // twice the max buffer size, and perform two writes if needed.
+  // Pick 4 * P30_MAX_BUFFER_SIZE_IN_BYTES (== 512 bytes) as a good
+  // start for word operations as opposed to erasing the block and
+  // writing the data regardless if an erase is really needed.
+  //
+  // Many NV variable updates are small enough for a a single
+  // P30_MAX_BUFFER_SIZE_IN_BYTES block write.  In case the update is
+  // larger than a single block, or the update crosses a
+  // P30_MAX_BUFFER_SIZE_IN_BYTES boundary (as shown in the diagram
+  // below), or both, we might have to write two or more blocks.
   //
   //    0               128              256
   //    [----------------|----------------]
@@ -578,7 +580,7 @@ NorFlashWriteSingleBlock (
   Start = Offset & ~BOUNDARY_OF_32_WORDS;
   End   = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
 
-  if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
+  if ((End - Start) <= (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
     // Check to see if we need to erase before programming the data into NOR.
     // If the destination bits are only changing from 1s to 0s we can just write.
     // After a block is erased all bits in the block is set to 1.
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113918): https://edk2.groups.io/g/devel/message/113918
Mute This Topic: https://groups.io/mt/103766776/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 5/6] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too

[Gerd Hoffmann]

It is possible to find variable entries with State being 0xff, i.e. not
updated since flash block erase.   This indicates the variable driver
could not complete the header write while appending a new entry, and
therefore State was not set to VAR_HEADER_VALID_ONLY.

This can only happen at the end of the variable list, so treat this as
additional "end of variable list" condition.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
index 8fcd999ac6df..c8b5e0be1379 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -302,6 +302,11 @@ ValidateFvHeader (
       break;
     }
 
+    if (VarHeader->State == 0xff) {
+      DEBUG ((DEBUG_INFO, "%a: end of var list (unwritten state)\n", __func__));
+      break;
+    }
+
     VarName = NULL;
     switch (VarHeader->State) {
       // usage: State = VAR_HEADER_VALID_ONLY
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113920): https://edk2.groups.io/g/devel/message/113920
Mute This Topic: https://groups.io/mt/103766778/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

[Gerd Hoffmann]

Move the DoErase code block into a separate function, call the function
instead of jumping around with goto.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 ++++++++++++++++++--------
 1 file changed, 52 insertions(+), 24 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 3d1d20daa1e5..e6aaed27ceba 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -502,6 +502,38 @@ NorFlashRead (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+NorFlashWriteSingleBlockWithErase (
+  IN        NOR_FLASH_INSTANCE  *Instance,
+  IN        EFI_LBA             Lba,
+  IN        UINTN               Offset,
+  IN OUT    UINTN               *NumBytes,
+  IN        UINT8               *Buffer
+  )
+{
+  EFI_STATUS  Status;
+
+  // Read NOR Flash data into shadow buffer
+  Status = NorFlashReadBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
+  if (EFI_ERROR (Status)) {
+    // Return one of the pre-approved error statuses
+    return EFI_DEVICE_ERROR;
+  }
+
+  // Put the data at the appropriate location inside the buffer area
+  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
+
+  // Write the modified buffer back to the NorFlash
+  Status = NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
+  if (EFI_ERROR (Status)) {
+    // Return one of the pre-approved error statuses
+    return EFI_DEVICE_ERROR;
+  }
+
+  return EFI_SUCCESS;
+}
+
 /*
   Write a full or portion of a block. It must not span block boundaries; that is,
   Offset + *NumBytes <= Instance->BlockSize.
@@ -607,7 +639,14 @@ NorFlashWriteSingleBlock (
     // that we want to set. In that case, we will need to erase the block first.
     for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
       if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
-        goto DoErase;
+        Status = NorFlashWriteSingleBlockWithErase (
+                   Instance,
+                   Lba,
+                   Offset,
+                   NumBytes,
+                   Buffer
+                   );
+        return Status;
       }
 
       OrigData[CurOffset] = Buffer[CurOffset];
@@ -636,33 +675,22 @@ NorFlashWriteSingleBlock (
         goto Exit;
       }
     }
-
-Exit:
-    // Put device back into Read Array mode
-    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
-
+  } else {
+    Status = NorFlashWriteSingleBlockWithErase (
+               Instance,
+               Lba,
+               Offset,
+               NumBytes,
+               Buffer
+               );
     return Status;
   }
 
-DoErase:
-  // Read NOR Flash data into shadow buffer
-  Status = NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
-  if (EFI_ERROR (Status)) {
-    // Return one of the pre-approved error statuses
-    return EFI_DEVICE_ERROR;
-  }
+Exit:
+  // Put device back into Read Array mode
+  SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
 
-  // Put the data at the appropriate location inside the buffer area
-  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
-
-  // Write the modified buffer back to the NorFlash
-  Status = NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
-  if (EFI_ERROR (Status)) {
-    // Return one of the pre-approved error statuses
-    return EFI_DEVICE_ERROR;
-  }
-
-  return EFI_SUCCESS;
+  return Status;
 }
 
 EFI_STATUS
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113921): https://edk2.groups.io/g/devel/message/113921
Mute This Topic: https://groups.io/mt/103766779/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

[Laszlo Ersek]

On 1/16/24 18:11, Gerd Hoffmann wrote:
> Move the DoErase code block into a separate function, call the function
> instead of jumping around with goto.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 ++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 24 deletions(-)
> 
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 3d1d20daa1e5..e6aaed27ceba 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -502,6 +502,38 @@ NorFlashRead (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +NorFlashWriteSingleBlockWithErase (
> +  IN        NOR_FLASH_INSTANCE  *Instance,
> +  IN        EFI_LBA             Lba,
> +  IN        UINTN               Offset,
> +  IN OUT    UINTN               *NumBytes,
> +  IN        UINT8               *Buffer
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  // Read NOR Flash data into shadow buffer
> +  Status = NorFlashReadBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
> +  if (EFI_ERROR (Status)) {
> +    // Return one of the pre-approved error statuses
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  // Put the data at the appropriate location inside the buffer area
> +  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
> +
> +  // Write the modified buffer back to the NorFlash
> +  Status = NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize, Instance->ShadowBuffer);
> +  if (EFI_ERROR (Status)) {
> +    // Return one of the pre-approved error statuses
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /*
>    Write a full or portion of a block. It must not span block boundaries; that is,
>    Offset + *NumBytes <= Instance->BlockSize.
> @@ -607,7 +639,14 @@ NorFlashWriteSingleBlock (
>      // that we want to set. In that case, we will need to erase the block first.
>      for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
>        if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
> -        goto DoErase;
> +        Status = NorFlashWriteSingleBlockWithErase (
> +                   Instance,
> +                   Lba,
> +                   Offset,
> +                   NumBytes,
> +                   Buffer
> +                   );
> +        return Status;
>        }
>  
>        OrigData[CurOffset] = Buffer[CurOffset];
> @@ -636,33 +675,22 @@ NorFlashWriteSingleBlock (
>          goto Exit;
>        }
>      }
> -
> -Exit:
> -    // Put device back into Read Array mode
> -    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> -
> +  } else {
> +    Status = NorFlashWriteSingleBlockWithErase (
> +               Instance,
> +               Lba,
> +               Offset,
> +               NumBytes,
> +               Buffer
> +               );
>      return Status;
>    }
>  
> -DoErase:
> -  // Read NOR Flash data into shadow buffer
> -  Status = NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
> -  if (EFI_ERROR (Status)) {
> -    // Return one of the pre-approved error statuses
> -    return EFI_DEVICE_ERROR;
> -  }
> +Exit:
> +  // Put device back into Read Array mode
> +  SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>  
> -  // Put the data at the appropriate location inside the buffer area
> -  CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumBytes);
> -
> -  // Write the modified buffer back to the NorFlash
> -  Status = NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->ShadowBuffer);
> -  if (EFI_ERROR (Status)) {
> -    // Return one of the pre-approved error statuses
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  EFI_STATUS

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

This series is now ready to be merged, but I'm unsure what the CI status
is, after the recent security fixes seem to have caused breakage with
multiple external definitions of the same symbol. For now I'll keep this
tagged, as a reminder for myself.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113944): https://edk2.groups.io/g/devel/message/113944
Mute This Topic: https://groups.io/mt/103766779/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Laszlo Ersek]

On 1/16/24 18:10, Gerd Hoffmann wrote:
> This is a little series containing the flash corruption fix sent
> yesterday with an slightly improved commit message and some small
> improvements on top of this.
> 
> v3:
>  - fix diagram
>  - fix DoErase control flow
>  - pick up reviewed-by tags
> v2:
>  - drop broken bugfix, fix the bug when introducing Start+End variables
>    instead.
>  - add patch with UINTN and UINT32 casts.
>  - add patch splitting the DoErase code path into a new function.
>  - add the diagram sent by Laszlo.
> 
> Gerd Hoffmann (6):
>   OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
>   OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer
>     reads
>   OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.
>   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
>   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
>   OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function
> 
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h    |   2 +-
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 145 ++++++++++++++--------
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |   5 +
>  3 files changed, 101 insertions(+), 51 deletions(-)
> 

Merged as commit range 59f024c76ee5..b481b00f593e, via
<https://github.com/tianocore/edk2/pull/5273>.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114010): https://edk2.groups.io/g/devel/message/114010
Mute This Topic: https://groups.io/mt/103766773/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Ard Biesheuvel]

On Thu, 18 Jan 2024 at 21:28, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/16/24 18:10, Gerd Hoffmann wrote:
> > This is a little series containing the flash corruption fix sent
> > yesterday with an slightly improved commit message and some small
> > improvements on top of this.
> >
> > v3:
> >  - fix diagram
> >  - fix DoErase control flow
> >  - pick up reviewed-by tags
> > v2:
> >  - drop broken bugfix, fix the bug when introducing Start+End variables
> >    instead.
> >  - add patch with UINTN and UINT32 casts.
> >  - add patch splitting the DoErase code path into a new function.
> >  - add the diagram sent by Laszlo.
> >
> > Gerd Hoffmann (6):
> >   OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
> >   OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer
> >     reads
> >   OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.
> >   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
> >   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
> >   OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function
> >
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h    |   2 +-
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 145 ++++++++++++++--------
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |   5 +
> >  3 files changed, 101 insertions(+), 51 deletions(-)
> >
>
> Merged as commit range 59f024c76ee5..b481b00f593e, via
> <https://github.com/tianocore/edk2/pull/5273>.
>

Much appreciated! Thanks to the both of you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114029): https://edk2.groups.io/g/devel/message/114029
Mute This Topic: https://groups.io/mt/103766773/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-