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

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-