Series comparison

In some cases (specifically when the flash update region is
small but crosses a multiple of P30_MAX_BUFFER_SIZE_IN_BYTES)
NorFlashWriteSingleBlock reads only one instead of two
P30_MAX_BUFFER_SIZE_IN_BYTES blocks into the shadow buffer.

That leads to random crap being written to the second block,
which in turn can corrupt both the variable store and the
FTW work space.  One observed corruption pattern is finding
0xaf (aka PcdDebugClearMemoryValue) right after the last
entry in the FTW log.  This should have been 0xff.

This patch fixes the calculation.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Introduce Start and End variables to make it easier to follow the
logic and code flow.  Also replace the two NorFlashWriteBuffer calls
with a loop containing one 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 | 31 +++++++++++++-------------
 1 file changed, 15 insertions(+), 16 deletions(-)

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

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

It is possible to find variable entries with State being 0xff,
i.e. not updated since flash block erase.   This indicates the
header write was not completed (and therefore State was not set
to VAR_HEADER_VALID_ONLY).  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 XXXXXXX..XXXXXXX 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -XXX,XX +XXX,XX @@ 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 (#113719): https://edk2.groups.io/g/devel/message/113719
Mute This Topic: https://groups.io/mt/103680936/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 XXXXXXX..XXXXXXX 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
@@ -XXX,XX +XXX,XX @@
 #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 XXXXXXX..XXXXXXX 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -XXX,XX +XXX,XX @@ 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 XXXXXXX..XXXXXXX 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -XXX,XX +XXX,XX @@ 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));
 
@@ -XXX,XX +XXX,XX @@ 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.
@@ -XXX,XX +XXX,XX @@ 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)) {
@@ -XXX,XX +XXX,XX @@ NorFlashWriteSingleBlock (
 
     Status = NorFlashWriteBuffer (
                Instance,
-               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+               BlockAddress + Start,
                P30_MAX_BUFFER_SIZE_IN_BYTES,
                Instance->ShadowBuffer
                );
@@ -XXX,XX +XXX,XX @@ 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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(-)

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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(-)

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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(+)

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 XXXXXXX..XXXXXXX 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -XXX,XX +XXX,XX @@ 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.
@@ -XXX,XX +XXX,XX @@ 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];
@@ -XXX,XX +XXX,XX @@ 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-