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

[edk2-devel] [PATCH v2 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.

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    | 144 ++++++++++++++--------
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |   5 +
 3 files changed, 99 insertions(+), 52 deletions(-)

-- 
2.43.0



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

[edk2-devel] [PATCH v2 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>
---
 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 35 ++++++++++++++++++++------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 7f4743b00399..54251633d0ee 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,27 @@ 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 & 0x7F) + NumBytes; i.e., the Offset inside
+  //    |         |     (or just past) the *double-word* such that Offset 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 +586,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 +622,7 @@ NorFlashWriteSingleBlock (
 
     Status = NorFlashWriteBuffer (
                Instance,
-               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+               BlockAddress + Start,
                P30_MAX_BUFFER_SIZE_IN_BYTES,
                Instance->ShadowBuffer
                );
@@ -609,12 +630,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 (#113833): https://edk2.groups.io/g/devel/message/113833
Mute This Topic: https://groups.io/mt/103741669/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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>
---
 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 54251633d0ee..67610d6920f7 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));
 
@@ -620,23 +621,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 (#113829): https://edk2.groups.io/g/devel/message/113829
Mute This Topic: https://groups.io/mt/103741663/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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>
---
 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 67610d6920f7..d80e9f0a2f3a 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
   //    [----------------|----------------]
@@ -577,7 +579,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 (#113830): https://edk2.groups.io/g/devel/message/113830
Mute This Topic: https://groups.io/mt/103741664/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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>
---
 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 (#113832): https://edk2.groups.io/g/devel/message/113832
Mute This Topic: https://groups.io/mt/103741666/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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, 51 insertions(+), 25 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index d80e9f0a2f3a..203bd64f2bdf 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -502,6 +502,37 @@ 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.
@@ -606,7 +637,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
+                   );
+        goto Exit;
       }
 
       OrigData[CurOffset] = Buffer[CurOffset];
@@ -635,33 +673,21 @@ NorFlashWriteSingleBlock (
         goto Exit;
       }
     }
+  } else {
+    Status = NorFlashWriteSingleBlockWithErase (
+               Instance,
+               Lba,
+               Offset,
+               NumBytes,
+               Buffer
+               );
+  }
 
 Exit:
-    // Put device back into Read Array mode
-    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
+  // Put device back into Read Array mode
+  SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
 
-    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;
-  }
-
-  // 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 (#113831): https://edk2.groups.io/g/devel/message/113831
Mute This Topic: https://groups.io/mt/103741665/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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/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 v2 2/6] OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer reads

[Laszlo Ersek]

On 1/15/24 16:59, Gerd Hoffmann wrote:
> 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>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 35 ++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 7f4743b00399..54251633d0ee 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,27 @@ 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 & 0x7F) + NumBytes; i.e., the Offset inside
> +  //    |         |     (or just past) the *double-word* such that Offset is
> +  //    |         |     the *exclusive* end of the (logical) update.

Obviously, when I proposed this diagram, I messed up this text.

Clearly, there's no better time for making a mistake in a comment than
when complaining about comments... :)

Two warts:

- 0x7F has not been replaced with BOUNDARY_OF_32_WORDS

- the uppercase "Offset" identifier (= proper noun), from my original
proposal, is misleading here. The common noun "offset" is what's need.

So I suggest refreshing it as:

  //    |         |     (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.

With that comment update:

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

Thanks!
Laszlo

> +  //    |         |
> +  //    |         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 +586,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 +622,7 @@ NorFlashWriteSingleBlock (
>
>      Status = NorFlashWriteBuffer (
>                 Instance,
> -               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
> +               BlockAddress + Start,
>                 P30_MAX_BUFFER_SIZE_IN_BYTES,
>                 Instance->ShadowBuffer
>                 );
> @@ -609,12 +630,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
>                   );



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113893): https://edk2.groups.io/g/devel/message/113893
Mute This Topic: https://groups.io/mt/103741669/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 v2 3/6] OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.

[Laszlo Ersek]

On 1/15/24 16:59, Gerd Hoffmann wrote:
> 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>
> ---
>  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 54251633d0ee..67610d6920f7 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));
>  
> @@ -620,23 +621,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:

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113894): https://edk2.groups.io/g/devel/message/113894
Mute This Topic: https://groups.io/mt/103741663/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 v2 4/6] OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase

[Laszlo Ersek]

On 1/15/24 16:59, Gerd Hoffmann wrote:
> 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>
> ---
>  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 67610d6920f7..d80e9f0a2f3a 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
>    //    [----------------|----------------]
> @@ -577,7 +579,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.

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113895): https://edk2.groups.io/g/devel/message/113895
Mute This Topic: https://groups.io/mt/103741664/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 v2 5/6] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too

[Laszlo Ersek]

On 1/15/24 16:59, Gerd Hoffmann wrote:
> 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>
> ---
>  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

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113896): https://edk2.groups.io/g/devel/message/113896
Mute This Topic: https://groups.io/mt/103741666/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 v2 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

[Laszlo Ersek]

On 1/15/24 16:59, 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, 51 insertions(+), 25 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index d80e9f0a2f3a..203bd64f2bdf 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -502,6 +502,37 @@ NorFlashRead (
>    return EFI_SUCCESS;
>  }
>
> +STATIC EFI_STATUS

(1) EFI_STATUS is not needed (and if it were needed, then we'd put it on
a separate line)

> +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;
> +}

Good; "git-show --color-moved=zebra" is really helpful here.

What changes across the code movement is the BufferSizeInBytes argument
for the NorFlashReadBlocks / NorFlashWriteBlocks calls. Previously, we'd
pass in the local variable BlockSize, with type UINTN. After, we pass in
Instance->BlockSize, a UINT32. However, this is all fine, given that the
BufferSizeInBytes param of both callees is UINTN, and even the local
variable is assigned from Instance->BlockSize, pre-patch. OK.

> +
>  /*
>    Write a full or portion of a block. It must not span block boundaries; that is,
>    Offset + *NumBytes <= Instance->BlockSize.
> @@ -606,7 +637,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
> +                   );
> +        goto Exit;
>        }
>
>        OrigData[CurOffset] = Buffer[CurOffset];
> @@ -635,33 +673,21 @@ NorFlashWriteSingleBlock (
>          goto Exit;
>        }
>      }
> +  } else {
> +    Status = NorFlashWriteSingleBlockWithErase (
> +               Instance,
> +               Lba,
> +               Offset,
> +               NumBytes,
> +               Buffer
> +               );
> +  }
>
>  Exit:
> -    // Put device back into Read Array mode
> -    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> +  // Put device back into Read Array mode
> +  SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>
> -    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;
> -  }
> -
> -  // 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) The extraction of the erase code path is fine, but how it is being
put to use is not identical to the pre-patch state.

The key observation is that, pre-patch, the Exit label is *semantically
local* to the "word-based writes" branch.

Of course this statement makes no sense at the C language level, because
labels are local to functions, not to blocks within functions. Either
way, the original logic is:

- if the logical update is too big (i.e., we don't run the word-based
optimization), just run DoErase. "Exit" is not reached, and we don't put
device back into Read Array mode.

- if the logical update is not too big, we verify the bit transitions.
If those prevent the word-based optimization, we just run DoErase again.
"Exit" is not reached, and we don't put the device back into Read Array
mode.

- Otherwise, we commit to the word-based optimization. We start out by
GET_NOR_BLOCK_ADDRESS(), after which we need to finish with "Exit",
i.e., with kicking the flash back to Read Array mode, regardless of
success vs. error.

The patch changes this, because now we reach "Exit" (and the Read Array
mode setting) in *all three* cases. That seems wrong (or at least an
unjustified change).

(2.1) The simplest fix (to be squashed) that I can imagine is just to
"return Status" right after each NorFlashWriteSingleBlockWithErase()
call returns:

>| diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| index 203bd64f2bdf..bd343562f4f0 100644
>| --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| @@ -638,19 +638,19 @@ NorFlashWriteSingleBlock (
>|      for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
>|        if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
>|          Status = NorFlashWriteSingleBlockWithErase (
>|                     Instance,
>|                     Lba,
>|                     Offset,
>|                     NumBytes,
>|                     Buffer
>|                     );
>| -        goto Exit;
>| +        return Status;
>|        }
>|
>|        OrigData[CurOffset] = Buffer[CurOffset];
>|      }
>|
>|      //
>|      // Write the updated buffer to NOR.
>|      //
>|      BlockAddress = GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress, Lba,
>|      BlockSize);
>| @@ -675,18 +675,19 @@ NorFlashWriteSingleBlock (
>|      }
>|    } else {
>|      Status = NorFlashWriteSingleBlockWithErase (
>|                 Instance,
>|                 Lba,
>|                 Offset,
>|                 NumBytes,
>|                 Buffer
>|                 );
>| +    return Status;
>|    }
>|
>|  Exit:
>|    // Put device back into Read Array mode
>|    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>|
>|    return Status;
>|  }
>|

(2.2) A more extensive reorganization would be the following (also to be
squashed). The benefit of this approach is that it decreses the total
nesting depth in NorFlashWriteSingleBlock(), using early exit points.

The diff is formatted with "git diff -b", in order to make the
un-indentation less confusing.

>| diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| index 203bd64f2bdf..ba043f851547 100644
>| --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>| @@ -596,106 +596,107 @@ NorFlashWriteSingleBlock (
>|    //    ^         ^             ^         ^
>|    //    |         |             |         |
>|    //    |         |             |        End, the next "word" boundary beyond
>|    //    |         |             |        the (logical) update
>|    //    |         |             |
>|    //    |         |     (Offset & 0x7F) + NumBytes; i.e., the Offset inside
>|    //    |         |     (or just past) the *double-word* such that Offset 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) <= (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
>| +  if ((End - Start) > (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
>| +    Status = NorFlashWriteSingleBlockWithErase (
>| +               Instance,
>| +               Lba,
>| +               Offset,
>| +               NumBytes,
>| +               Buffer
>| +               );
>| +    return Status;
>| +  }
>| +
>|    // 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.
>|    // If any byte requires us to erase we just give up and rewrite all of it.
>|
>|    // Read the old version of the data into the shadow buffer
>|    Status = NorFlashRead (
>|               Instance,
>|               Lba,
>|               Start,
>|               End - Start,
>|               Instance->ShadowBuffer
>|               );
>|    if (EFI_ERROR (Status)) {
>|      return EFI_DEVICE_ERROR;
>|    }
>|
>|    // Make OrigData point to the start of the old version of the data inside
>|    // the word aligned buffer
>|    OrigData = Instance->ShadowBuffer + (Offset & BOUNDARY_OF_32_WORDS);
>|
>|    // Update the buffer containing the old version of the data with the new
>|    // 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 (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
>|        Status = NorFlashWriteSingleBlockWithErase (
>|                   Instance,
>|                   Lba,
>|                   Offset,
>|                   NumBytes,
>|                   Buffer
>|                   );
>| -        goto Exit;
>| +      return Status;
>|      }
>|
>|      OrigData[CurOffset] = Buffer[CurOffset];
>|    }
>|
>|    //
>|    // Write the updated buffer to NOR.
>|    //
>|    BlockAddress = GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress, Lba, BlockSize);
>|
>|    // Unlock the block if we have to
>|    Status = NorFlashUnlockSingleBlockIfNecessary (Instance, BlockAddress);
>|    if (EFI_ERROR (Status)) {
>|      goto Exit;
>|    }
>|
>|    Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
>|    for (Index = 0; Index < Count; Index++) {
>|      Status = NorFlashWriteBuffer (
>|                 Instance,
>|                 BlockAddress + Start + Index * P30_MAX_BUFFER_SIZE_IN_BYTES,
>|                 P30_MAX_BUFFER_SIZE_IN_BYTES,
>|                 Instance->ShadowBuffer + Index * P30_MAX_BUFFER_SIZE_IN_BYTES
>|                 );
>|      if (EFI_ERROR (Status)) {
>|        goto Exit;
>|      }
>|    }
>| -  } else {
>| -    Status = NorFlashWriteSingleBlockWithErase (
>| -               Instance,
>| -               Lba,
>| -               Offset,
>| -               NumBytes,
>| -               Buffer
>| -               );
>| -  }
>|
>|  Exit:
>|    // Put device back into Read Array mode
>|    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>|
>|    return Status;
>|  }

I'd be OK with ether (2.1) or (2.2).

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113899): https://edk2.groups.io/g/devel/message/113899
Mute This Topic: https://groups.io/mt/103741665/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 v2 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function

[Laszlo Ersek]

On 1/16/24 14:44, Laszlo Ersek wrote:
> On 1/15/24 16:59, 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, 51 insertions(+), 25 deletions(-)
>>
>> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> index d80e9f0a2f3a..203bd64f2bdf 100644
>> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> @@ -502,6 +502,37 @@ NorFlashRead (
>>    return EFI_SUCCESS;
>>  }
>>
>> +STATIC EFI_STATUS
> 
> (1) EFI_STATUS is not needed (and if it were needed, then we'd put it on
> a separate line)
> 
>> +NorFlashWriteSingleBlockWithErase (
>> +  IN        NOR_FLASH_INSTANCE  *Instance,
>> +  IN        EFI_LBA             Lba,
>> +  IN        UINTN               Offset,
>> +  IN OUT    UINTN               *NumBytes,
>> +  IN        UINT8               *Buffer
>> +  )
>> +{

Sigh. In your patch, I mistook / misread "EFI_STATUS" for "EFIAPI". :/

So, please ignore this; we obviously need the EFI_STATUS return type.

... Perhaps consider breaking "EFI_STATUS" to its own line.

(Strange how the brain works; my "mental alarm" about having typed
something foolish went off approx. 90 minutes after hitting Send --
while I was running outside. I know that some people get the best
programming ideas while showering...)

Sorry!
Laszlo



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